From: Daniel Barkalow <barkalow@iabervon.org>
Date: Fri, 21 Mar 2008 05:57:52 GMT
Subject: Re: [PATCH] Permit refspec source side to parse as a sha1
Message-ID: <alpine.LNX.1.00.0803210134070.19665@iabervon.org>
In-Reply-To: <7vmyosskyu.fsf@gitster.siamese.dyndns.org>

On Thu, 20 Mar 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > So we should call the same code to parse the string, have that code do no 
> > validation at all, and have the caller validate the return as appropriate. 
> > The parsing doesn't depend at all on whether it's for fetching or pushing.
> 
> Obviously you did not read my patch before responding.
> 
> While I would agree that removing the check from parsing and add necessary
> checks to all callers would be another way to catch the same kind of
> breakages,
> 
>  (1) it would be more code to change, and I do not see a clear advantage,
>      in that approach, especially given the discussion that lead to
>      $gmane/77413;

Surely it's no more code to change all of the callers of parse_ref_spec to 
also call a function to validate it as appropriate than it is to change 
all of the callers of parse_ref_spec to call a different function instead.
 
>  (2) the error reporting by the callers that use the parsed result will
>      not be able to say "Invalid refspec '%s'" on the source string,
>      simply because the source string is not available to them anymore;

That's easy enough to keep, and likely even worthwhile; it would probably 
be nice, for example, to have "git fetch origin typo/*:something/*" give 
an error when the pattern doesn't match anything, and that error would 
need the refspec string.

>  (3) worse yet, the older code even discarded part of the user input, so
>      the error reporting that uses the parsed src/dst pair may not even be
>      similar to the problematic input the user gave us to begin with,
>      making it harder for the user to spot what we did not like and
>      correct it.

That was a bug that was fixed in the intermediate version.

> In any case, don't you agree that the patch you responded to is much
> easier to understand what we are (and we are not) checking than the
> original code?

No, I think it's much more complicated. It mixes the semantics of what an 
empty side means for a particular use of refspecs into the parsing of the 
string. At the very least, the checks should be outside of _internal() in 
the functions for particular uses.

	-Daniel
*This .sig left intentionally blank*