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

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Mar 21, 2008, 16:08 UTC
Message-ID: <alpine.LNX.1.00.0803211148120.19665@iabervon.org>
In-Reply-To: <7v3aqksic6.fsf@gitster.siamese.dyndns.org>

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

Show 16 quoted lines

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Thu, 20 Mar 2008, Junio C Hamano wrote:
> > ...
> >> 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.
> 
> The thing is, the syntax is the same between fetch and push only to a
> degree.  They are both <LHS> ':' <RHS>.  What is allowed on LHS and RHS
> are quite different even at the syntactic level.

There's also the optional + at the beginning and the way of forming patterns, and the need to distinguish not having a colon with not having anything on one or the other side of the colon.

> I already know our criteria when judging if a particular code is clean or
> complex are _vastly_ different, from the experience working with you in
> other parts of the system (namely, read-tree 3-way rules and
> unpack_trees() rewrite that happened quite a long time ago).

Agreed.

And I do think that the rewrite of the part of the function before the semantic checks is clearer than the corresponding original (although the patch text made it hard to see; we really need some magic to make diff not bother to save 4 identifier-free lines and just show it as a complete replacement).

	-Daniel
*This .sig left intentionally blank*