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

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Mar 22, 2008, 00:36 UTC
Message-ID: <alpine.LNX.1.00.0803212021190.19665@iabervon.org>
In-Reply-To: <7vy78bmxx1.fsf@gitster.siamese.dyndns.org>

On Fri, 21 Mar 2008, Junio C Hamano wrote:

Show 15 quoted lines

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> >> +		/*
> >> +		 * Do we want to validate LHS?
> >> + ...
> >> +		 * Hence we check non-empty LHS for fetch, and
> >> +		 * colonless or glob LHS for push here.
> >> +		 */
> >
> > Wouldn't this be clearer and not meaningfully harder in 
> > parse_fetch_refspec and parse_push_refspec?
> 
> 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.

Show 12 quoted lines

> >> +		if (fetch ? (*rs[i].src) : (!rhs || is_glob)) {
> >
> > This is an odd combination of locals and struct members.
> >                                        : (!rs[i].dst || rs[i].pattern) {
> 
> Sorry, I do not understand what's wrong about it.
> 
> 	!!rhs === (did we see a colon) === !!rs[].dst
>         is_glob === (did they both end with "/*") === rs[].pattern
> 
> They are equivalent, and local variables are primarily what the logic
> works on and bases its decisions to store what in rs[] structures.

Considering that it's already stored values into the struct, it might as well use those, and those are presumably more familiar to the average reader, because all of the git code that uses refspecs other than this function uses them.

Show 5 quoted lines

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

	-Daniel
*This .sig left intentionally blank*