Junio C Hamano <gitster@xxxxxxxxx> writes: > Gabriel Souza Franco <gabrielfrancosouza@xxxxxxxxx> 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@xxxxxxxxx> >> --- > > 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; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html