Re: [PATCH/RFC 3/4] git check-ref-format --print

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2009, 21:06 UTC
Message-ID: <7veip8e302.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20091012143922.GL9261@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 18 quoted lines

> Jonathan Nieder <jrnieder@gmail.com> 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.