Re: Bug in fetch-pack.c, please confirm

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> ...
> And there we stop. We don't pass the "refs" list out of that function
> (which, as an aside, is probably a leak). Instead, we depend on the list
> of heads we already knew in the "to_fetch" array. That comes from
> processing the earlier list of refs returned from get_refs_via_connect.
> ...
> That doesn't mean there isn't an additional bug lurking. That is,
> _should_ somebody be caring about that value? I don't think so. The
> old/new pairs in a "struct ref" are meaningful as "I proposed to update
> to X, and we are at Y". But this list of refs does not have anything to
> do with the update of local refs. It is only "what is the value of the
> ref on the other side". The local refs are taken care of in a separate
> list.

Correct.  When we want to insert some function to allow users/hooks
to tweak the result of update, we might want to make sure that we
are passing the final list of refs to that function, but the purpose
of this function is not to make such a modification.

Just to make sure we do not keep this hanging forever and eventually
forget it, I'm planning to queue this.

Thanks.

-- >8 --
From: Jeff King <peff@peff.next>
Date: Sun, 15 Mar 2015 21:13:43 -0400
Subject: [PATCH] fetch-pack: remove dead assignment to ref->new_sha1

In everything_local(), we used to assign the current ref's value
found in ref->old_sha1 to ref->new_sha1 when we already have all the
necessary objects to complete the history leading to that commit.
This copying was broken at 49bb805e (Do not ask for objects known to
be complete., 2005-10-19) and ever since we instead stuffed a random
bytes in ref->new_sha1 here.  No code complained or failed due to this
breakage.

It turns out that no codepath that comes after this assignment even
looks at ref->new_sha1 at all.

 - The only caller of everything_local(), do_fetch_pack(), returns
   this list of ref, whose element has bogus new_sha1 values, to its
   caller.  It does not look at the elements and acts on them.

 - The only caller of do_fetch_pack(), fetch_pack(), returns this
   list to its caller.  It does not look at the elements and acts on
   them.

 - One of the two callers of fetch_pack() is cmd_fetch_pack(), the
   top-level that implements "git fetch-pack".  The only thing it
   looks at in the elements of the returned ref list is the old_sha1
   and name fields.

 - The other caller of fetch_pack() is fetch_refs_via_pack() in the
   transport layer, which is a helper that implements "git fetch".
   It only cares about whether the returned list is empty (i.e.
   failed to fetch anything).

Just drop the bogus assignment, that is not even necessary.  The
remote-tracking refs are updated based on a different list and not
using the ref list being manipulated by this codepath; the caller
do_fetch_pack() created a copy of that real ref list and passed the
copy down to this function, and modifying the elements here does not
affect anything.

Noticed-by: Kyle J. McKay <mackyle@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 fetch-pack.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 655ee64..6f0c0e1 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -625,7 +625,6 @@ static int everything_local(struct fetch_pack_args *args,
 
 	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
 		const unsigned char *remote = ref->old_sha1;
-		unsigned char local[20];
 		struct object *o;
 
 		o = lookup_object(remote);
@@ -638,8 +637,6 @@ static int everything_local(struct fetch_pack_args *args,
 				ref->name);
 			continue;
 		}
-
-		hashcpy(ref->new_sha1, local);
 		if (!args->verbose)
 			continue;
 		fprintf(stderr,
-- 
2.3.3-377-gdf43604