From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 21 Mar 2008 23:59:06 GMT
Subject: Re: [PATCH] remote.c: Fix overtight refspec validation
Message-ID: <7vy78bmxx1.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LNX.1.00.0803211840480.19665@iabervon.org>

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.

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

Ahh...  do you mean:

	(*rs[i].src) === (is lhs non empty?) === !!llen

I guess using "llen" there is more consistent and is moderately cleaner.

Perhaps squash this as a clean-up?

diff --git a/remote.c b/remote.c
index 4117bfc..86113b7 100644
--- a/remote.c
+++ b/remote.c
@@ -453,7 +453,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		 * "empty" for removal) in LHS, and we cannot check
 		 * for error until it actually gets used.
 		 */
-		if (fetch ? (*rs[i].src) : (!rhs || is_glob)) {
+		if (fetch ? llen : (!rhs || is_glob)) {
 			st = check_ref_format(rs[i].src);
 			if (st && st != CHECK_REF_FORMAT_ONELEVEL)
 				goto invalid;