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

Jeff King [off-list ref] 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 <redacted>
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 [off-list ref]
Signed-off-by: Jeff King <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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

On Thu, Mar 19, 2015 at 10:41:50AM -0700, Junio C Hamano wrote:

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

Thanks for following up. A few minor nits (and maybe a more serious
problem) on the explanation in the commit message:

be complete., 2005-10-19) and ever since we instead stuffed a random
bytes in ref->new_sha1 here.

s/a random/random/

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.

s/list of ref/list of refs/ ? Or did you mean "the list we store in the
'ref' variable"?

I'm not sure what the final sentence means. Should it be "It does not
look at the elements nor act on them"?

do_fetch_pack actually does pass them down to find_common. But the
latter does not look at ref->new_sha1, so we're OK.

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

Ditto on the wording in the final sentence here (but it is correct in
this case that we do not touch the list at all).

 - 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).

So I thought I would follow this up by adding a free_refs() call in
fetch_refs_via_pack. After all, we just leak that list, right?

If only it were so simple. It turns out that the list returned from
fetch_pack() is _mostly_ a copy of the incoming refs list we give it.
But in filter_refs(), if allow_tip_sha1_in_want is set, we actually add
the unmatched "sought" refs here, too.

Which means we may be setting new_sha1 in one of these "sought" refs,
and that broadens the scope of code that might be affected by the patch
we have been discussing. However, I think the filter_refs code is wrong,
and should be making a copy of the sought refs to put in the new list.

I'm working up a few patches in that area, which I'll send out in a few
minutes. Once that is done, then I think the explanation you give above
would be correct.

-Peff