iterator/diff: allow trailing / on start/end paths to match submodules - #3724
Merged
carlosmn merged 2 commits intoApr 3, 2016
Merged
Conversation
added 2 commits
April 2, 2016 13:02
Test that submodules are found when the are included in a pathspec but have a trailing slash.
Allow callers to specify a start path with a trailing slash to match a submodule, instead of just a directory. This is for some legacy behavior that's sort of dumb, but there it is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the great iterator refactor of 2016, we made clear that submodules should not have trailing slashes for the
startandendpaths. This regressed some expected behavior.Unfortunately, this does not quite jive with diff's pathspec matching, which will happily accept
foo/iffoois a submodule. This is problematic because diff will set the iterator'sstartandendpaths to the longest common substring of the pathspecs. (Which may be nothing, but it may be something, and if it is that would speed up the diff.)Therefore, if you are asking about a bunch of paths, and you've foolishly added a trailing slash to a submodule, the behavior is different than if you ask about just that one path:
If you diff with
pathspecs = { "a", "b", "submod/", "z" }, then there is no common substring and so the diff cannot provide a start and end prefix to the iterator. Thus, diff looks at all the paths coming back from the iterator and so its logic about whethersubmod/should match a submodule namedsubmodis used. (And that does match, according to diff.)However, if you diff the same entries with
pathspecs = { "submod/" }then now diff notices that it can use a start and end prefix - and it does, ofsubmod/. So now, the iterators must treat this the same way that diff did for consistency. Sosubmod/must match a submodule namedsubmodin iterator, so that diff can do its thing.This change is really quite minor, but it's a bit annoying, since I was very careful to treat submodules differently than directories and thought I was actually righting the behavior here. :)