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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix interaction between limited flag and sorting over resets #4688

Merged
merged 1 commit into from
Jun 27, 2018

Conversation

mystor
Copy link
Contributor

@mystor mystor commented Jun 18, 2018

Sorted revwalks have their limited flag set because lazy revwalks, which are used when limited is not set since #4606, currently only handle GIT_SORT_NONE.

Revwalks persist their sorting over calls to git_revwalk_reset. However, the limited flag was unconditionally cleared during git_revwalk_reset, which would cause breakage.

This patch sets the limited flag for non-GIT_SORT_NONE revwalks in reset, and adds a test for the failing case.

I ran into this bug while making a version of pygit2 with the mailmap changes from #4586, as pygit2 tests started failing.

mystor added a commit to mystor/pygit2 that referenced this pull request Jun 19, 2018
This patch shouldn't be merged until libgit2 0.28 is released, as the
mailmap support is not present outside of the master branch currently.
This patch sets us up to test against the master branch for now.

NOTE: A test fails until libgit2/libgit2#4688 is
landed on libgit2 master.
@ethomson
Copy link
Member

Thanks, @mystor! @carlosmn can you review this quickly?

@carlosmn
Copy link
Member

Where we do we get the idea that the sorting should persist? That sort of worked many years ago but the git_revwalk_reset function is there to re-use the walk structures and caching and should reset all of the state and you need to provide it with all of the information as though you had created a new one.

@mystor
Copy link
Contributor Author

mystor commented Jun 19, 2018

Not sure. I was surprised that it persisted, but that appears to match the current behavior and the behavior tested by pygit2 in a test. It would be perfectly reasonable to change to not persisting as well.

@pks-t
Copy link
Member

pks-t commented Jun 22, 2018

I agree with @carlosmn. The documentation of git_revwalk_reset states that:

This will clear all the pushed and hidden commits, and leave the walker in a blank state (just like at creation) ready to receive new commit pushes and start a new walk.

So there really shouldn't be any difference between a newly allocated revwalk via git_revwalk_new and a resetted one. So I guess the right fix would be to set walk->sorting = GIT_SORT_NONE in git_revwalk_reset, as well.

Currently we fail to clear the sorting flag for revwalks when resetting.
This caused a poor interaction with the limited flag during a recent
patch. This patch clears the revwalk sorting flag and causes it to no
longer persist over resets.
@mystor
Copy link
Contributor Author

mystor commented Jun 23, 2018

Swapped the behaviour change over

@ethomson
Copy link
Member

Thanks, @mystor!

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

Successfully merging this pull request may close these issues.

None yet

4 participants