revparse: support open-ended ranges#4231
Conversation
|
Look at those beautiful tests! 😍 Thanks for jumping in to fix this. This is great, and I appreciate the pull request! |
pks-t
left a comment
There was a problem hiding this comment.
Looks good besides missing tests for the special cases of having completely unlimited ranges. Thanks!
| &revspec->to, | ||
| repo, | ||
| *rstr == '\0' ? "HEAD" : rstr); | ||
| } |
There was a problem hiding this comment.
Note that this also supports the ranges ".." and "...", that is no limiter at all. While these seem nonsensical, it even seems to be the right thing as git.git also accepts these ranges:
$ git rev-parse ..
..
$ git rev-parse ...
a8c90ef62281db933118aa84489eb0e1e9cc347c
a8c90ef62281db933118aa84489eb0e1e9cc347c
^a8c90ef62281db933118aa84489eb0e1e9cc347c
While the first result stumps me, there at least is no error here.
Could you maybe add another two tests for these special-cases?
There was a problem hiding this comment.
I hadn't thought about it much because man gitrevisions seems to explicitly say that the empty ranges are allowed. ("Note that .. would mean HEAD..HEAD which is an empty range that is both reachable and unreachable from HEAD.") However, on inspection, git.git treats .. as a special case on purpose so that it's always handled as a path for potentially ambiguous commands like git log ... If the argument is disambiguated, git rev-parse gives an error:
$ git rev-parse .. --
fatal: bad revision '..'
I think it makes sense to reject .., since trying to parse an argument as a revision and then as a path is a pretty common git CLI idiom. Continuing to allow ... seems a bit weird, but it should probably be accepted since git.git supports it.
There was a problem hiding this comment.
Oh, that's what's going on here. The paragraph you probably mean from gitrevisions(7):
In these two shorthand notations, you can omit one end and let it default to HEAD.
For example, origin.. is a shorthand for origin..HEAD and asks "What did I do
since I forked from the origin branch?" Similarly, ..origin is a shorthand for
HEAD..origin and asks "What did the origin do since I forked from them?" Note that
.. would mean HEAD..HEAD which is an empty range that is both reachable and
unreachable from HEAD.
So it explicitly says that one can omit one end, only. So I think your reasoning about us rejecting when both sides are empty is correct.
| test_id("...HEAD~3", | ||
| "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", | ||
| "4a202b346bb0fb0db7eff3cffeb3c70babbd2045", | ||
| GIT_REVPARSE_RANGE | GIT_REVPARSE_MERGE_BASE); |
There was a problem hiding this comment.
Could you add two additional tests here to test the two special-cases of ".." and "...", please?
Support '..' and '...' ranges where one side is not specified. The unspecified side defaults to HEAD. Closes libgit2#4223
|
I've updated to match git.git's behavior for the special cases '..' and '...'. |
|
Looks good to me now. Thanks again! We might hold this off until after v0.26.0. @ethomson? |
Support '..' and '...' ranges where one or both sides are not
specified. The unspecified side defaults to HEAD.
Closes #4223