Re: [PATCH] Permit refspec source side to parse as a sha1

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2008, 05:30 UTC
Message-ID: <7vmyosskyu.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LNX.1.00.0803210014100.19665@iabervon.org>

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;

 (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;

 (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.

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?