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

Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list

From
Jeff King <peff@peff.net>
Date
Aug 23, 2012, 20:31 UTC
Message-ID
<20120823203152.GA1810@sigill.intra.peff.net>
In-Reply-To
<20120823195648.GB15268@sigill.intra.peff.net>
On Thu, Aug 23, 2012 at 03:56:48PM -0400, Jeff King wrote:
Show 18 quoted lines
> This code blames back to:
> 
>  commit 4bcb310c2539b66d535e87508d1b7a90fe29c083
>  Author: Alexandre Julliard <julliard@winehq.org>
>  Date:   Fri Nov 24 16:00:13 2006 +0100
> 
>     fetch-pack: Do not fetch tags for shallow clones.
> 
>     A better fix may be to only fetch tags that point to commits that we
>     are downloading, but git-clone doesn't have support for following
>     tags. This will happen automatically on the next git-fetch though.
> 
> So it is about making sure that "git clone --depth=1" does not
> accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,
> losing the purpose of using --depth in the first place. These days it is
> largely irrelevant, since this code path is not followed by clone, and
> clone will automatically restrict its list of fetched refs to a single
> branch if --depth is used.

I think part of the confusion of this code is that inside the loop over the refs it is sometimes checking aspects of the ref, and sometimes checking invariants of the loop (like args.fetch_all). Splitting it into separate loops makes it easier to see what is going on, like the patch below (on top of Michael's series).

I'm not sure if it ends up being more readable, since the generic "cut down this linked list" function has to operate through callbacks with a void pointer. On the other hand, that function could also be used elsewhere.

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 90683ca..877cf38 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,51 +521,80 @@ static void mark_recent_complete_commits(unsigned long cutoff)
 	}
 }
 
-static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
+static void filter_refs_callback(struct ref **refs,
+				 int (*want)(struct ref *, void *),
+				 void *data)
 {
-	struct ref *newlist = NULL;
-	struct ref **newtail = &newlist;
+	struct ref **tail = refs;
 	struct ref *ref, *next;
-	int match_pos = 0, unmatched = 0;
 
 	for (ref = *refs; ref; ref = next) {
-		int keep_ref = 0;
 		next = ref->next;
-		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_refname_format(ref->name, 0))
-			; /* trash */
-		else if (args.fetch_all &&
-			   (!args.depth || prefixcmp(ref->name, "refs/tags/")))
-			keep_ref = 1;
-		else
-			while (match_pos < *nr_heads) {
-				int cmp = strcmp(ref->name, heads[match_pos]);
-				if (cmp < 0) { /* definitely do not have it */
-					break;
-				} else if (cmp == 0) { /* definitely have it */
-					free(heads[match_pos++]);
-					keep_ref = 1;
-					break;
-				} else { /* might have it; keep looking */
-					heads[unmatched++] = heads[match_pos++];
-				}
-			}
-
-		if (keep_ref) {
-			*newtail = ref;
-			ref->next = NULL;
-			newtail = &ref->next;
-		} else {
+		if (want(ref, data))
+			tail = &ref->next;
+		else {
 			free(ref);
+			*tail = next;
 		}
 	}
+}
 
-	/* copy any remaining unmatched heads: */
-	while (match_pos < *nr_heads)
-		heads[unmatched++] = heads[match_pos++];
-	*nr_heads = unmatched;
+static int ref_name_is_ok(struct ref *ref, void *data)
+{
+	return memcmp(ref->name, "refs/", 5) ||
+		!check_refname_format(ref->name, 0);
+}
+
+static int ref_ok_for_shallow(struct ref *ref, void *data)
+{
+	return prefixcmp(ref->name, "refs/tags/");
+}
 
-	*refs = newlist;
+struct filter_by_name_data {
+	char **heads;
+	int nr_heads;
+	int match_pos;
+	int unmatched;
+};
+
+static int want_ref_name(struct ref *ref, void *data)
+{
+	struct filter_by_name_data *f = data;
+
+	while (f->match_pos < f->nr_heads) {
+		int cmp = strcmp(ref->name, f->heads[f->match_pos]);
+		if (cmp < 0) /* definitely do not have it */
+			return 0;
+		else if (cmp == 0) { /* definitely have it */
+			free(f->heads[f->match_pos++]);
+			return 1;
+		} else /* might have it; keep looking */
+			f->heads[f->unmatched++] = f->heads[f->match_pos++];
+	}
+	return 0;
+}
+
+static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
+{
+	struct filter_by_name_data f;
+
+	filter_refs_callback(refs, ref_name_is_ok, NULL);
+
+	if (args.fetch_all) {
+		if (args.depth)
+			filter_refs_callback(refs, ref_ok_for_shallow, NULL);
+		return;
+	}
+
+	memset(&f, 0, sizeof(f));
+	f.heads = heads;
+	f.nr_heads = *nr_heads;
+	filter_refs_callback(refs, want_ref_name, &f);
+
+	/* copy any remaining unmatched heads: */
+	while (f.match_pos < f.nr_heads)
+		heads[f.unmatched++] = heads[f.match_pos++];
+	*nr_heads = f.unmatched;
 }
 
 static void mark_alternate_complete(const struct ref *ref, void *unused)
Previous: Jeff KingNext: Michael Haggerty
Message 35 of 40 in “Clean up how fetch_pack() handles the heads list”
  1. 00/17 Clean up how fetch_pack() handles the heads listmhagger@alum.mit.edu, Aug 23, 2012
  2. 01/17 t5500: add tests of error output for missing refsmhagger@alum.mit.edu, Aug 23, 2012
  3. 02/17 Rename static function fetch_pack() to http_fetch_pack()mhagger@alum.mit.edu, Aug 23, 2012
  4. 03/17 Fix formatting.mhagger@alum.mit.edu, Aug 23, 2012
  5. 04/17 Name local variables more consistentlymhagger@alum.mit.edu, Aug 23, 2012
  6. Jeff KingAug 23, 2012
  7. Michael HaggertyAug 24, 2012
  8. Junio C HamanoAug 26, 2012
  9. Michael HaggertyAug 27, 2012
  10. Jeff KingAug 27, 2012
  11. Junio C HamanoAug 27, 2012
  12. Junio C HamanoAug 27, 2012
  13. 05/17 Do not check the same match_pos twicemhagger@alum.mit.edu, Aug 23, 2012
  14. Jeff KingAug 23, 2012
  15. 06/17 Let fetch_pack() inform caller about number of unique headsmhagger@alum.mit.edu, Aug 23, 2012
  16. Jeff KingAug 23, 2012
  17. Michael HaggertyAug 25, 2012
  18. 07/17 Pass nr_heads to do_pack_ref() by referencemhagger@alum.mit.edu, Aug 23, 2012
  19. 08/17 Pass nr_heads to everything_local() by referencemhagger@alum.mit.edu, Aug 23, 2012
  20. 09/17 Pass nr_heads to filter_refs() by referencemhagger@alum.mit.edu, Aug 23, 2012
  21. 10/17 Remove ineffective optimizationmhagger@alum.mit.edu, Aug 23, 2012
  22. 11/17 filter_refs(): do not leave gaps in return_refsmhagger@alum.mit.edu, Aug 23, 2012
  23. 12/17 filter_refs(): compress unmatched refs in heads arraymhagger@alum.mit.edu, Aug 23, 2012
  24. 13/17 cmd_fetch_pack: return early if finish_connect() returns an errormhagger@alum.mit.edu, Aug 23, 2012
  25. 14/17 Report missing refs even if no existing refs were receivedmhagger@alum.mit.edu, Aug 23, 2012
  26. 15/17 cmd_fetch_pack(): simplify computation of return valuemhagger@alum.mit.edu, Aug 23, 2012
  27. 16/17 fetch_pack(): free matching headsmhagger@alum.mit.edu, Aug 23, 2012
  28. Jeff KingAug 23, 2012
  29. 17/17 fetch_refs(): simplify logicmhagger@alum.mit.edu, Aug 23, 2012
  30. Jeff KingAug 23, 2012
  31. Michael HaggertyAug 25, 2012
  32. Jeff KingAug 23, 2012
  33. Philip OakleyAug 23, 2012
  34. Jeff KingAug 23, 2012
  35. Jeff KingAug 23, 2012
  36. Michael HaggertyAug 25, 2012
  37. Michael HaggertySep 2, 2012
  38. Philip OakleyAug 23, 2012
  39. Junio C HamanoAug 24, 2012
  40. Philip OakleyAug 24, 2012

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.