Re: [PATCH] remote.c: Fix overtight refspec validation

[Junio C Hamano <gitster@pobox.com>]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Fri, 21 Mar 2008, Junio C Hamano wrote:
> ...
>> Do you mean you want the callers of this internal implementation to also
>> loop over the input set of refs?  I think that would be more complex code
>> but I do not see much gain by doing so.
>
> I think it's more breadth but less depth. It would make the internal 
> implementation not depend on fetch, and put the checks that only apply to 
> fetch out of the push code path and vice versa.
>
> Or just have a section
>
> 	if (fetch) {
> 		// checks for fetch LHS
> 		// checks for fetch RHS
> 	} else {
> 		// checks for push LHS
> 		// checks for push RHS
> 	}
>
> The body of the condition is only four lines, after all.

There are two commits on jc/refspec-fix branch merged to 'pu'.  The
earlier one is my version, and the one on top is based on the above
suggestion.  I do not know which one is clearer, more readable and
maintainable.

>> Ahh...  do you mean:
>> 
>> 	(*rs[i].src) === (is lhs non empty?) === !!llen
>> 
>> I guess using "llen" there is more consistent and is moderately cleaner.
>
> I'd go the other way, but having them all from the same set makes more 
> sense than one from one set and two from the other, regardless of which 
> way. If you go this way, you should probably also include the the rhs 
> checks, and the argument to check_ref_format().

It is very much sensible to look at either the local variables it used
during the parsing and the tentative result it produced while deciding
what to check and how, and it also is very sensible to validate what it
gives back to the user and/or what it knows is equal to what it gives back
to the user.  When they are equivalent, it is mostly a matter of taste
which to use for deciding and which to use for validating.  But it makes
more sense to prefer its local variables for logic to decide what to do
and how, and validate what we are actually going to give back.

If you mean to suggest to change parameter given to check_ref_format()
from rs[i].{src,dst} to something else based on the local variables we
used, that's backwards.

But we have already agreed that our brains are wired differently when it
comes to taste; I'd prefer not pursuing bikeshedding any further.

Unless your suggestion _isn't_ bikeshedding, that is.

What I did so far was to spend time to respond with code that fixes
existing breakages, while what you did so far was to kibitz instead of
showing code.  Showing code might make me realize that the way you may
want to go is more than bikeshedding and actual improvements to
readability and maintainability, but honestly speaking I am tired of
looking at this part of the code for now, so...