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

Junio C Hamano [off-list ref] writes:

Gabriel Souza Franco [off-list ref] writes:

quoted

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 <redacted>
---

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;