"Shawn O. Pearce" <spearce@xxxxxxxxxxx> writes:
> Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>> +valid_ref_normalized 'heads/foo' 'heads/foo'
>> +valid_ref_normalized 'refs///heads/foo' 'refs/heads/foo'
>> +invalid_ref_normalized 'foo'
>> +invalid_ref_normalized 'heads/foo/../bar'
>> +invalid_ref_normalized 'heads/./foo'
>> +invalid_ref_normalized 'heads\foo'
>
> What about '/refs/heads/foo'? Shouldn't that drop the leading /?
>
> I actually had someone enter that into Gerrit Code Review once,
> exposing a bug I have yet to fix that permits that as a valid
> branch name. *sigh*
>
> FWIW, I think this is heading in the right direction. Rather
> than teaching the UIs how clean up a name, give us a tool to
> do the normalization and validation, and let us call it when
> we get user input.
I understand that you prefer the latter between "there is no tool; the
caller is responsibile to make sure it feeds us canonical representation"
and "there is a tool that makes a slightly malformed string into canonical
form for the callers to use before calling us." And that would be my
preference between these two as well.
But that is based on the current behaviour of accepting slightly malformed
and silently making it canonical. If we throw a third alternative,
Jonathan's original patch, that did "we reject malformed string", in the
mix, what would be your preference?
I moderately favour the "tool to canonicalize is given, and it would be a
bug for the caller not to use it" approach this series takes primarily
because that approach won't break scripts that do something like this to
make a new branch, optionally grouped by the owner (e.g. 'sp/smart-http'
or 'next'):
owner=$1 topic=$2
branch=$owner/$topic
git branch "$branch"
This currently works as expected as long as it does not later try to
compare for-each-ref output with "refs/heads/$branch" and expects a match.
With the third approach, the optional username grouping becomes mandatory
for such a script, as 'git branch' will error out. To keep the grouping
still optinal, such a script needs to be written perhaps like:
if test -z "$2"
then
branch="$1"
else
branch="$1/$2"
fi
and that would be a hassle for the scripters, but this _could_ be a kind
of backward incompatible tightening we might want to consider for 1.7.0,
as somebody suggested earlier.
But now I have spelled this out, I do not see much upside for rejecting,
and more importantly, I think it would be an independent issue. We can
reject or just keep normalizing silently, and a tool to show the
normalized name would be useful and necessary regardless of that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html