Re: [PATCH] fetch-pack: fix unadvertised requests validation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 27, 2016, 18:32 UTC
- Message-ID
- <xmqq60xagg1w.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <xmqqd1riggd7.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes: > >> 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?
Nah, I think you just misspelt hashcpy() as hashcmp(). The original wanted to get the binary representation of the hex in old_sha1 when it continued, and you wanted to delay the touching of old_sha1 until we make sure that the input is valid 40-hex, so something like this is what Gabriel wants to do (which I agree with), isn't it?
fetch-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c index 058c258..bb5237f 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -551,7 +551,7 @@ static void filter_refs(struct fetch_pack_args *args, continue; if (get_sha1_hex(ref->name, sha1) || ref->name[40] != '\0' || - hashcmp(sha1, ref->old_sha1)) + hashcpy(ref->old_sha1, sha1)) continue; ref->matched = 1;