Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle future commit datetimes in the future without panicking #1306

Open
1 task done
MalteT opened this issue Apr 11, 2024 · 7 comments
Open
1 task done

Handle future commit datetimes in the future without panicking #1306

MalteT opened this issue Apr 11, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@MalteT
Copy link

MalteT commented Apr 11, 2024

Duplicates

  • I have searched the existing issues

Current behavior 馃槸

If the commit time is in the future, onefetch crashes.

Expected behavior 馃

It should possibly handle the case or fail gracefully. The crash report blames this on me, but my system time is okay and I don't have much influence over my colleague's system time.

Steps to reproduce 馃暪

Have the most recent git commit in the future and run onefetch

Additional context/Screenshots 馃敠

name = "onefetch"
operating_system = "NixOS 24.5.0 [64-bit]"
crate_version = "2.20.0"
explanation = """
Panic occurred in file 'src/info/utils/mod.rs' at line 35
"""
cause = "Achievement unlocked: time travel! Check your system clock and commit dates."
method = "Panic"
backtrace = """

   0: 0x56049cc6bb63 - core::option::expect_failed::hcbf5a9b7e22cab9f
   1: 0x56049cc93285 - onefetch::info::utils::format_time::he784b32eb1d439c7
   2: 0x56049ccec879 - onefetch::info::InfoBuilder::last_change::h5aac04e9a53655fa
   3: 0x56049ccea471 - onefetch::info::build_info::h6232c8d9e8572b77
   4: 0x56049cc7dded - onefetch::main::hfc8e895e84fac4d0
   5: 0x56049cc85363 - std::sys_common::backtrace::__rust_begin_short_backtrace::he043df6535079d0d
   6: 0x56049cc86c9d - std::rt::lang_start::{{closure}}::h5f878ceb72151ea0
   7: 0x56049d5883d4 - std::panicking::try::h45c8379f4866ecc7
   8: 0x56049d581eb5 - std::rt::lang_start_internal::hf358ddc7f05da9a0
   9: 0x56049cc7e5f5 - main
  10: 0x7f25f061c0ce - __libc_start_call_main
  11: 0x7f25f061c189 - __libc_start_main@GLIBC_2.2.5
  12: 0x56049cc6be25 - _start
  13:        0x0 - <unresolved>"""

Possible Solution 馃挕

No response

Thanks for your awesome work!

@MalteT MalteT added the bug Something isn't working label Apr 11, 2024
@OsiPog
Copy link

OsiPog commented Apr 11, 2024

I'm that colleague

@spenserblack
Copy link
Collaborator

spenserblack commented Apr 11, 2024

But I like that "time travel" achievement 馃槩 馃槅 But you're right that we could handle this more gracefully.

I think that most people don't pay attention to the commit time, so even if it was an unreasonable value like in the year 3000 it could still get merged in. And if that happens the repo is basically unusable by onefetch unless a maintainer rewrites the history to fix the commit date. So overall we should change this from a panic to a warning message so that at least onefetch can display the other stats, and one unexpected time getting accidentally merged in doesn't "break" the whole repo for onefetch.

Besides the panic, I'm also worried that we're dropping the timezone offset somewhere, which can cause this issue. Do both of you (@OsiPog and @MalteT) live in different timezones, and can you confirm that your system times are correct?

@OsiPog
Copy link

OsiPog commented Apr 11, 2024

Actually, we live in the same time zone and I can confirm that my system time was in fact incorrect. So I just caused this issue by commiting with my system time being in the future.

@spenserblack
Copy link
Collaborator

I think we did have a timezone issue, anway 馃槅 2a3707e

Regardless of who or what caused the issue, I don't think anybody wants to rebase the commit history to fix the commit times just to get onefetch to stop panicking. So I think the solution is to print something like could not calculate last change: last commit datetime is in the future and then continue running.

@spenserblack spenserblack changed the title onefetch Crash Report Handle future commit datetimes in the future without panicking Apr 11, 2024
@spenserblack spenserblack added enhancement New feature or request and removed bug Something isn't working labels Apr 11, 2024
@spenserblack
Copy link
Collaborator

Labeling this as an enhancement instead of a bug since the panic was intended behavior.

@MalteT
Copy link
Author

MalteT commented Apr 11, 2024

So I think the solution is to print something like could not calculate last change: last commit datetime is in the future and then continue running.

Or time traveler detected, omitting last change calculation to prevent temporal paradox :D

I'm not familiar with the code base, but if the change is not too involved I could assemble a PR :)

@spenserblack
Copy link
Collaborator

Oh, I like that better, actually! All you need to do is filter out commits where the commit time is greater than now.

But to do that some refactoring might be needed to avoid recalculating "now" in multiple places. Ideally SystemTime::now should only be called once per execution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants