-
Notifications
You must be signed in to change notification settings - Fork 283
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conclude gitoxide
to git2
transition
#1285
Conversation
b6feeb6
to
a942922
Compare
a942922
to
68b0e42
Compare
src/info/pending.rs
Outdated
Summary::Removed => (added, deleted + 1, modified), | ||
Summary::Added => (added + 1, deleted, modified), | ||
Summary::Modified | Summary::TypeChange => (added, deleted, modified + 1), | ||
Summary::Renamed | Summary::Copied => (added + 1, deleted + 1, modified), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watching the conclusion of switching from git2
to gix
is awesome!
Should deleted
really be incremented when the status is Copied
? AFAIK a rename is always a delete + add, while a copy just means a new file was created with extremely similar contents to an existing file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great catch right there! Copied
should only count as added file.
This Summary
was added just for onefetch
and it was interesting to see that the possible states indeed are best represented by an enum, even though libgit2
tries to convince you otherwise.
68b0e42
to
dfc0831
Compare
While I prepare the
|
Summary::Removed => (added, deleted + 1, modified), | ||
Summary::Added | Summary::Copied => (added + 1, deleted, modified), | ||
Summary::Modified | Summary::TypeChange => (added, deleted, modified + 1), | ||
Summary::Renamed => (added + 1, deleted + 1, modified), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, it won't run into Renamed
and Copied
as rename tracking isn't activated by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on that, what are the implications ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean the code could call unreachable!()
in case of Copied
and Renamed
and be right about it. The implementation/settings have the same effect as git2
previously.
Does that make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you refering to git config --get diff.renames
? Whether it's enable doesn't change the output for onefetch, right? a rename is always shown as an addition and an deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point to bring up! I decided not to let it affect renames between index and worktree as Git itself doesn't even expose that functionality to the user (only git2
seems to have it). Everything that is affected by configuration, really only emit_untracked
at this time, is overridden though.
dfc0831
to
9c15105
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, onefetch is fully oxidized 🎉
The missing piece finally came through.
Just tested it and #978 seems to be resolved
Can the PR be merged?
Great to hear that another issue is fixed :). This PR is good to go from my side. |
Fixes #628
I had to wait a long time for this PR to become possible, and finally here it is :)!
Tasks
git2
dependencygix
Review Notes
onefetch
truly read-only.If there is disagreement, the index could be updated and written back - it will be valid, but won't write back all extensions that might
have been present before.
git2
most certainly also didn't do it in a way that is 100% faithful to what Git would do.renames_head_to_index()
was never functional, as the status was only obtained between the index and the worktree. If this must be presentwith the switch, then I have to hold off with this update as
gix status
doesn't yet supportHEAD->index
status.Performance
In short,
onefetch
got a little bit faster, and speeds up with the size of the repositories measured by the amount of files in their worktrees.WebKit
Here the faster
status
implementation is definitely visible.Linux
For linux, the commit-aggregation is the most stressful part, and there is much less of a difference as
status
is just a small part of the overall runtime.Git
Git is interesting as it doesn't have too many files in the worktree, yet it seems to greatly benefit from the new
status
implementation. Unfortunately, at ~200ms, the difference isn't really perceivable by a user.