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
Michael Haggerty <mhagger@alum.mit.edu>
Date
Aug 25, 2012, 07:05 UTC
Message-ID
<50387936.1000205@alum.mit.edu>
In-Reply-To
<20120823203152.GA1810@sigill.intra.peff.net>
On 08/23/2012 10:31 PM, Jeff King wrote:
Show 11 quoted lines
> 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.
> [...]

Despite requiring a bit more boilerplate, I think that your change makes the logic clearer.

*If* we want to switch to using callbacks, then we can get even more bang for the buck, as in the attached patch (which applies on top of my patch v2). Beyond your suggestion, this patch:

* Inlines your filter_refs() into everything_local(), because (a) it's
short and (b) the policy work implemented there logically belongs
higher-up in the call chain.
* Renames your filter_refs_callback() to filter_refs().
* Moves the initialization of the filter_by_name_data structure
(including sorting and de-duping) all the way up to fetch_pack(), and
passes a filter_by_name_data* (rather than (nr_heads, heads)) down to
the callees.

If you like this change, let me know and I'll massage it into a digestible patch series.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/


diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index db77ee6..d1dabcf 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,51 +521,91 @@ 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(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 head_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 (head_pos < *nr_heads) {
-				int cmp = strcmp(ref->name, heads[head_pos]);
-				if (cmp < 0) { /* definitely do not have it */
-					break;
-				} else if (cmp == 0) { /* definitely have it */
-					free(heads[head_pos++]);
-					keep_ref = 1;
-					break;
-				} else { /* might have it; keep looking */
-					heads[unmatched++] = heads[head_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 (head_pos < *nr_heads)
-		heads[unmatched++] = heads[head_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/");
+}
+
+static int compare_heads(const void *a, const void *b)
+{
+	return strcmp(*(const char **)a, *(const char **)b);
+}
+
+static int remove_duplicates(int nr_heads, char **heads)
+{
+	int src, dst;
+	for (src = dst = 1; src < nr_heads; src++)
+		if (strcmp(heads[src], heads[dst-1]))
+			heads[dst++] = heads[src];
+		else
+			free(heads[src]);
+	return dst;
+}
 
-	*refs = newlist;
+struct filter_by_name_data {
+	char **heads;
+	int *nr_heads;
+	int head_pos;
+	int unmatched;
+};
+
+static void filter_by_name_init(struct filter_by_name_data *f,
+			 int *nr_heads, char **heads)
+{
+	memset(f, 0, sizeof(*f));
+	f->heads = heads;
+	f->nr_heads = nr_heads;
+	qsort(f->heads, *f->nr_heads, sizeof(*f->heads), compare_heads);
+	*f->nr_heads = remove_duplicates(*f->nr_heads, f->heads);
+}
+
+static int filter_by_name_want(struct ref *ref, void *data)
+{
+	struct filter_by_name_data *f = data;
+
+	while (f->head_pos < *f->nr_heads) {
+		int cmp = strcmp(ref->name, f->heads[f->head_pos]);
+		if (cmp < 0) /* definitely do not have it */
+			return 0;
+		else if (cmp == 0) { /* definitely have it */
+			free(f->heads[f->head_pos++]);
+			return 1;
+		} else /* might have it; keep looking */
+			f->heads[f->unmatched++] = f->heads[f->head_pos++];
+	}
+	return 0;
+}
+
+static void filter_by_name_finish(struct filter_by_name_data *f)
+{
+	/* copy any remaining unmatched heads: */
+	while (f->head_pos < *f->nr_heads)
+		f->heads[f->unmatched++] = f->heads[f->head_pos++];
+	*f->nr_heads = f->unmatched;
 }
 
 static void mark_alternate_complete(const struct ref *ref, void *unused)
@@ -573,7 +613,8 @@ static void mark_alternate_complete(const struct ref *ref, void *unused)
 	mark_complete(NULL, ref->old_sha1, 0, NULL);
 }
 
-static int everything_local(struct ref **refs, int *nr_heads, char **heads)
+static int everything_local(struct ref **refs,
+			    struct filter_by_name_data *filter_by_name_data)
 {
 	struct ref *ref;
 	int retval;
@@ -624,7 +665,14 @@ static int everything_local(struct ref **refs, int *nr_heads, char **heads)
 		}
 	}
 
-	filter_refs(refs, nr_heads, heads);
+	filter_refs(refs, ref_name_is_ok, NULL);
+
+	if (args.fetch_all) {
+		if (args.depth)
+			filter_refs(refs, ref_ok_for_shallow, NULL);
+	} else if (filter_by_name_data) {
+		filter_refs(refs, filter_by_name_want, filter_by_name_data);
+	}
 
 	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
 		const unsigned char *remote = ref->old_sha1;
@@ -754,9 +802,9 @@ static int get_pack(int xd[2], char **pack_lockfile)
 }
 
 static struct ref *do_fetch_pack(int fd[2],
-		const struct ref *orig_ref,
-		int *nr_heads, char **heads,
-		char **pack_lockfile)
+				 const struct ref *orig_ref,
+				 struct filter_by_name_data *filter_by_name_data,
+				 char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
 	unsigned char sha1[20];
@@ -796,7 +844,7 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (everything_local(&ref, nr_heads, heads)) {
+	if (everything_local(&ref, filter_by_name_data)) {
 		packet_flush(fd[1]);
 		goto all_done;
 	}
@@ -816,21 +864,6 @@ static struct ref *do_fetch_pack(int fd[2],
 	return ref;
 }
 
-static int remove_duplicates(int nr_heads, char **heads)
-{
-	int src, dst;
-
-	if (!nr_heads)
-		return 0;
-
-	for (src = dst = 1; src < nr_heads; src++)
-		if (strcmp(heads[src], heads[dst-1]))
-			heads[dst++] = heads[src];
-		else
-			free(heads[src]);
-	return dst;
-}
-
 static int fetch_pack_config(const char *var, const char *value, void *cb)
 {
 	if (strcmp(var, "fetch.unpacklimit") == 0) {
@@ -1030,11 +1063,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-static int compare_heads(const void *a, const void *b)
-{
-	return strcmp(*(const char **)a, *(const char **)b);
-}
-
 struct ref *fetch_pack(struct fetch_pack_args *my_args,
 		       int fd[], struct child_process *conn,
 		       const struct ref *ref,
@@ -1045,6 +1073,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 {
 	struct stat st;
 	struct ref *ref_cpy;
+	struct filter_by_name_data name_data, *filter_by_name_data;
 
 	fetch_pack_setup();
 	if (&args != my_args)
@@ -1054,16 +1083,20 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 			st.st_mtime = 0;
 	}
 
-	if (heads && *nr_heads) {
-		qsort(heads, *nr_heads, sizeof(*heads), compare_heads);
-		*nr_heads = remove_duplicates(*nr_heads, heads);
-	}
-
 	if (!ref) {
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
+	if (heads && *nr_heads) {
+		filter_by_name_data = &name_data;
+		filter_by_name_init(filter_by_name_data, nr_heads, heads);
+	} else
+		filter_by_name_data = NULL;
+
+	ref_cpy = do_fetch_pack(fd, ref, filter_by_name_data, pack_lockfile);
+
+	if (filter_by_name_data)
+		filter_by_name_finish(filter_by_name_data);
 
 	if (args.depth > 0) {
 		struct cache_time mtime;
Previous: Jeff KingNext: Michael Haggerty
Message 36 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.