git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/4] filter_ref: make a copy of extra "sought" entries

From
Jeff King <peff@peff.net>
Date
Mar 19, 2015, 20:37 UTC
Message-ID
<20150319203708.GB7666@peff.net>
In-Reply-To
<20150319203126.GA31663@peff.net>

If the server supports allow_tip_sha1_in_want, we add any unmatched raw-sha1 entries in our "sought" list of refs to the list of refs we will ask the other side for. We do so by inserting the original "struct ref" directly into our list, rather than making a copy. This has several problems.

The most minor problem is that one cannot ever free the resulting list; it contains structs that are copies of the remote refs (made earlier by fetch_pack) along with sought refs that are referenced elsewhere.

But more importantly that we set the ref->next pointer to NULL, chopping off the remainder of any existing list that the ref was a part of. We get the set of "sought" refs in an array rather than a linked list, but that array is often in turn generated from a list. The test modification in t5516 demonstrates this. Rather than fetching just an exact sha1, we fetch that sha1 plus another ref:

  - we build a linked list of refs to fetch when do_fetch
    calls get_ref_map; the exact sha1 is first, followed by
    the named ref ("refs/heads/extra" in this case).
  - we pass that linked list to transport_fetch_ref, which
    squashes it into an array of pointers
  - that array goes to fetch_pack, which calls filter_ref.
    There we generate the want list from a mix of what the
    remote side has advertised, and the "sought" entry for
    the exact sha1. We set the sought entry's "next" pointer
    to NULL.
  - after we return from transport_fetch_refs, we then try
    to update the refs by following the linked list. But our
    list is now truncated, and we do not update
    refs/heads/extra at all.

We can fix this by making a copy of the ref. There's nothing that fetch_pack does to it that must be reflected in the original "sought" list (and indeed, if that were the case we would have a serious bug, because it is only exact-sha1 entries which are treated this way).

Signed-off-by: Jeff King <peff@peff.net>
---
I always feel a little dirty modifying an existing test rather than
writing a new one that more clearly demonstrates what we are testing.
But the setup involved in the exact-sha1 fetch is a little complicated,
so this seemed easiest (and anybody blaming the change can hopefully
rely on the explanation above to understand what is going on).
 fetch-pack.c          |  5 ++---
 t/t5516-fetch-push.sh | 13 ++++++++++---
 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 058c258..84d5a66 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -555,9 +555,8 @@ static void filter_refs(struct fetch_pack_args *args,
 				continue;
 
 			ref->matched = 1;
-			*newtail = ref;
-			ref->next = NULL;
-			newtail = &ref->next;
+			*newtail = copy_ref(ref);
+			newtail = &(*newtail)->next;
 		}
 	}
 	*refs = newlist;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 630885d..5e04d64 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1107,9 +1107,16 @@ test_expect_success 'fetch exact SHA1' '
 			git config uploadpack.allowtipsha1inwant true
 		) &&
 
-		git fetch -v ../testrepo $the_commit:refs/heads/copy &&
-		result=$(git rev-parse --verify refs/heads/copy) &&
-		test "$the_commit" = "$result"
+		git fetch -v ../testrepo $the_commit:refs/heads/copy master:refs/heads/extra &&
+		cat >expect <<-EOF &&
+		$the_commit
+		$the_first_commit
+		EOF
+		{
+			git rev-parse --verify refs/heads/copy &&
+			git rev-parse --verify refs/heads/extra
+		} >actual &&
+		test_cmp expect actual
 	)
 '
 
-- 
2.3.3.520.g3cfbb5d
Previous: Junio C HamanoNext: Jeff King
Message 12 of 14 in “Bug in fetch-pack.c, please confirm”
  1. Kyle J. McKayMar 15, 2015
  2. Junio C HamanoMar 15, 2015
  3. Junio C HamanoMar 15, 2015
  4. Kyle J. McKayMar 15, 2015
  5. Jeff KingMar 16, 2015
  6. Junio C HamanoMar 19, 2015
  7. Jeff KingMar 19, 2015
  8. Junio C HamanoMar 19, 2015
  9. Jeff KingMar 19, 2015
  10. 1/4 filter_ref: avoid overwriting ref->old_sha1 with garbageJeff King, Mar 19, 2015
  11. Junio C HamanoMar 19, 2015
  12. 2/4 filter_ref: make a copy of extra "sought" entriesJeff King, Mar 19, 2015
  13. 3/4 fetch_refs_via_pack: free extra copy of refsJeff King, Mar 19, 2015
  14. 4/4 fetch-pack: remove dead assignment to ref->new_sha1Jeff King, Mar 19, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.