Skip to content

Fix interaction between limited flag and sorting over resets#4688

Merged
ethomson merged 1 commit intolibgit2:masterfrom
mystor:sorted_revwalk_reset
Jun 27, 2018
Merged

Fix interaction between limited flag and sorting over resets#4688
ethomson merged 1 commit intolibgit2:masterfrom
mystor:sorted_revwalk_reset

Conversation

@mystor
Copy link
Copy Markdown
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
@mystor
Update to the libgit2 master branch to use mailmap
2b2b834 
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.
@mystor mystor mentioned this pull request Jun 19, 2018
Merged
@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 19, 2018

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

@carlosmn
Copy link
Copy Markdown
Member

carlosmn commented Jun 19, 2018

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
Copy Markdown
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
Copy Markdown
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.

@mystor
Clear revwalk sorting when resetting
4fd81c5 
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 mystor force-pushed the sorted_revwalk_reset branch from 05b86f9 to 4fd81c5 Compare June 23, 2018 15:00
@mystor
Copy link
Copy Markdown
Contributor Author

mystor commented Jun 23, 2018

Swapped the behaviour change over

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 27, 2018

Thanks, @mystor!

@ethomson ethomson merged commit 967da2c into libgit2:master Jun 27, 2018
@snyk-bot snyk-bot mentioned this pull request Feb 23, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2020
Open
@snyk-bot snyk-bot mentioned this pull request May 5, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mystor @ethomson @carlosmn @pks-t