Loosen restriction on wildcard "*" refspecs - #5060
Conversation
In commit cd377f45c9 (refs: loosen restriction on wildcard "*" refspecs, 2015-07-22) in git.git, the restrictions on wildcard "*" refspecs has been loosened. While wildcards were previously only allowed if the component is a single "*", this was changed to also accept other patterns as part of the component. We never adapted to that change and still reject any wildcard patterns that aren't a single "*" only. Update our tests to reflect the upstream change and adjust our own code accordingly.
When we transform a refspec with a component containing a glob, then we simply copy over the component until the next separator from the matching ref. E.g. if we have a ref "refs/heads/foo/bar" and a refspec "refs/heads/*/bar:refs/remotes/origin/*/bar", we: 1. Copy over everything until hitting the glob from the <dst> part: "refs/remotes/origin/". 2. Strip the common prefix of ref and <src> part until the glob, which is "refs/heads/". This leaves us with a ref of "foo/bar". 3. Copy from the ref until the next "/" separator, resulting in "refs/remotes/origin/foo". 4. Copy over the remaining part of the <dst> spec, which is "bar": "refs/remotes/origin/foo/bar". This worked just fine in a world where globs in refspecs were restricted such that a globbing component may only contain a single "*", only. But this restriction has been lifted, so that a glob component may be nested between other characters, causing the above algorithm to fail. Most notably the third step, where we copy until hitting the next "/" separator, might result in a wrong transformation. Given e.g. a ref "refs/gbranchg/head" and a refspec "refs/g*g/head:refs/remotes/origin/*", we'd also be copying the "g" between "branch" and "/" and end up with the wrong transformed ref "refs/remotes/origin/branchg". Instead of copying until the next component separator, we should copy until we hit the pattern after the "*". So in the above example, we'd copy until hitting the string "g/head".
|
|
||
| if (*current == '*') { | ||
| if (!may_contain_glob) | ||
| return -1; |
There was a problem hiding this comment.
I realize that you didn't introduce this problem, but reviewing your change shows that we might have one with error reporting. We return -1 out of this function w/o setting an error message. When called by git_reference__normalize_name, it will set an error message iff the error code is GIT_EINVALIDSPEC. So it looks to me like we'll return an error without setting a message. I think we want to return GIT_EINVALIDSPEC from this function on any error?
There was a problem hiding this comment.
Nope, in fact we're good, even though it's non-obvious. First note that we do not set any error code via ensure_segment_validity at all, but only the segment length. But we do initialize error = GIT_EINVALIDSPEC in the calling function and make sure that it doesn't get set to a different value throughout the function.
There was a problem hiding this comment.
Cool. Thanks for the explanation.
Loosen restriction on wildcard "*" refspecs
Upstream has changed the restrictions on wildcard "" refspecs. Previously, a wildcard in a refspec was only allowed if it was the only character in a component. This restriction has been lifted to allow wildcards to be nested between other characters in that component with cd377f45c9 (refs: loosen restriction on wildcard "" refspecs, 2015-07-22).
More details in the commits.
Fixes #5058