Re: [PATCH] fetch-pack: fix unadvertised requests validation

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2016, 18:25 UTC
Message-ID: <xmqqd1riggd7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1456577034-6494-1-git-send-email-gabrielfrancosouza@gmail.com>

Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:

Show 6 quoted lines

> Check was introduced in b791642 (filter_ref: avoid overwriting
> ref->old_sha1 with garbage, 2015-03-19), but was always false because
> ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.
>
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---

Peff, that commit points me at your direction. And I can see the original patch avoids overwriting old_sha1 by saving the result from get_sha1_hex() in a temporary, it is true that old_sha1 is not updated from the temporary.

The original code before b791642 wanted to say "if ref->name is not 40-hex, continue, and otherwise, do the ref->matched thing" and an implementation of b791642 that is more faithful to the original would indeed have been the result of applying this patch from Gabriel, but I am scratching my head why we have hashcmp() there.

Was it to avoid adding the same thing twice to the resulting list, or something?

Show 21 quoted lines

>  fetch-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 01e34b6..83b937b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,
>  			if (ref->matched)
>  				continue;
>  			if (get_sha1_hex(ref->name, sha1) ||
> -			    ref->name[40] != '\0' ||
> -			    hashcmp(sha1, ref->old_oid.hash))
> +			    ref->name[40] != '\0')
>  				continue;
>  
>  			ref->matched = 1;
> +			hashcpy(ref->old_oid.hash, sha1);
>  			*newtail = copy_ref(ref);
>  			newtail = &(*newtail)->next;
>  		}