On Thu, 20 Mar 2008, Junio C Hamano wrote: > Daniel Barkalow <barkalow@xxxxxxxxxxxx> 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* -- 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
