Closed MalteT closed 3 weeks ago
I'm that colleague
But I like that "time travel" achievement :cry: :laughing: 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?
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.
I think we did have a timezone issue, anway :laughing: https://github.com/o2sh/onefetch/pull/1307/commits/2a3707e0a5d1ef76dcc1c14dbe89773cbe7d237f
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.
Labeling this as an enhancement instead of a bug since the panic was intended behavior.
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 :)
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.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Coming back to this, I think it should be pretty easy to implement. I'm just making a quick comment without checking the codebase, but I think that all that needs to happen is:
eprintln!
.Option<T>
(or Result<T, E>
) so that it can be None
(or Err(E)
), and then hide the stat if it couldn't be calculated.[^1][^1]: Honestly, as an alternative to changing the value's type, we could probably get away with the easier solution of just falling back to SystemTime::now
if the latest commit date is too high.
Duplicates
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 π¦
Possible Solution π‘
No response
Thanks for your awesome work!