[PATCH v3 00/14] Clean up how fetch_pack() handles the heads list

STALE3716d

Revision v3 of 3 in this series.

16 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH v3 00/14] Clean up how fetch_pack() handles the heads list

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

This patch series depends on the "Add some string_list-related
functions" series that I just submitted.

This series is a significant rewrite of v2 based on the realization
that the <nr_heads,heads> pair that is passed around in these
functions is better expressed as a string_list.  I hope you will find
that this version is shorter and clearer than its predecessors, even
though its basic logic is mostly the same.

Sorry for the false starts in v1 and v2 and the extra reviewing work
that this will cost.

Michael Haggerty (14):
  t5500: add tests of error output for missing refs
  t5500: add tests of fetch-pack --all --depth=N $URL $REF
  Rename static function fetch_pack() to http_fetch_pack()
  fetch_pack(): reindent function decl and defn
  Change fetch_pack() and friends to take string_list arguments
  filter_refs(): do not check the same sought_pos twice
  fetch_pack(): update sought->nr to reflect number of unique entries
  filter_refs(): delete matched refs from sought list
  filter_refs(): build refs list as we go
  filter_refs(): simplify logic
  cmd_fetch_pack(): return early if finish_connect() fails
  fetch-pack: report missing refs even if no existing refs were
    received
  cmd_fetch_pack(): simplify computation of return value
  fetch-pack: eliminate spurious error messages

 builtin/fetch-pack.c  | 169 +++++++++++++++++++-------------------------------
 fetch-pack.h          |  20 ++++--
 http-walker.c         |   4 +-
 t/t5500-fetch-pack.sh |  47 +++++++++++++-
 transport.c           |  12 ++--
 5 files changed, 130 insertions(+), 122 deletions(-)

-- 
1.7.11.3

[PATCH v3 01/14] t5500: add tests of error output for missing refs

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

If "git fetch-pack" is called with reference names that do not exist
on the remote, then it should emit an error message

    error: no such remote ref refs/heads/xyzzy

This is currently broken if *only* missing references are passed to
"git fetch-pack".

Signed-off-by: Michael Haggerty <redacted>
---
 t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index e80a2af..6fa1cef 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -397,4 +397,34 @@ test_expect_success 'test duplicate refs from stdin' '
 	test_cmp expect actual
 '
 
+test_expect_success 'set up tests of missing reference' '
+	cat >expect-error <<-\EOF
+	error: no such remote ref refs/heads/xyzzy
+	EOF
+'
+
+test_expect_failure 'test lonely missing ref' '
+	(
+		cd client &&
+		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy
+	) >/dev/null 2>error-m &&
+	test_cmp expect-error error-m
+'
+
+test_expect_success 'test missing ref after existing' '
+	(
+		cd client &&
+		test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy
+	) >/dev/null 2>error-em &&
+	test_cmp expect-error error-em
+'
+
+test_expect_success 'test missing ref before existing' '
+	(
+		cd client &&
+		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A
+	) >/dev/null 2>error-me &&
+	test_cmp expect-error error-me
+'
+
 test_done
-- 
1.7.11.3

[PATCH v3 03/14] Rename static function fetch_pack() to http_fetch_pack()

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Avoid confusion with the non-static function of the same name from
fetch-pack.h.

Signed-off-by: Michael Haggerty <redacted>
---
 http-walker.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 51a906e..1516c5e 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -396,7 +396,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)
 	return ret;
 }
 
-static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
+static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
 {
 	struct packed_git *target;
 	int ret;
@@ -524,7 +524,7 @@ static int fetch(struct walker *walker, unsigned char *sha1)
 	if (!fetch_object(walker, altbase, sha1))
 		return 0;
 	while (altbase) {
-		if (!fetch_pack(walker, altbase, sha1))
+		if (!http_fetch_pack(walker, altbase, sha1))
 			return 0;
 		fetch_alternates(walker, data->alt->base);
 		altbase = altbase->next;
-- 
1.7.11.3

[PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Instead of juggling <nr_heads,heads> (sometimes called
<nr_match,match>), pass around the list of references to be sought in
a single string_list variable called "sought".  Future commits will
make more use of string_list functionality.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 88 +++++++++++++++++++++++-----------------------------
 fetch-pack.h         |  5 +--
 transport.c          | 18 +++++------
 3 files changed, 51 insertions(+), 60 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 30990c0..df81995 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -525,27 +525,27 @@ static void mark_recent_complete_commits(unsigned long cutoff)
 	}
 }
 
-static void filter_refs(struct ref **refs, int nr_match, char **match)
+static void filter_refs(struct ref **refs, struct string_list *sought)
 {
 	struct ref **return_refs;
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
 	struct ref *fastarray[32];
-	int match_pos;
+	int sought_pos;
 
-	if (nr_match && !args.fetch_all) {
-		if (ARRAY_SIZE(fastarray) < nr_match)
-			return_refs = xcalloc(nr_match, sizeof(struct ref *));
+	if (sought->nr && !args.fetch_all) {
+		if (ARRAY_SIZE(fastarray) < sought->nr)
+			return_refs = xcalloc(sought->nr, sizeof(struct ref *));
 		else {
 			return_refs = fastarray;
-			memset(return_refs, 0, sizeof(struct ref *) * nr_match);
+			memset(return_refs, 0, sizeof(struct ref *) * sought->nr);
 		}
 	}
 	else
 		return_refs = NULL;
 
-	match_pos = 0;
+	sought_pos = 0;
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
@@ -560,17 +560,17 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 		}
 		else {
 			int cmp = -1;
-			while (match_pos < nr_match) {
-				cmp = strcmp(ref->name, match[match_pos]);
+			while (sought_pos < sought->nr) {
+				cmp = strcmp(ref->name, sought->items[sought_pos].string);
 				if (cmp < 0) /* definitely do not have it */
 					break;
 				else if (cmp == 0) { /* definitely have it */
-					match[match_pos][0] = '\0';
-					return_refs[match_pos] = ref;
+					sought->items[sought_pos].string[0] = '\0';
+					return_refs[sought_pos] = ref;
 					break;
 				}
 				else /* might have it; keep looking */
-					match_pos++;
+					sought_pos++;
 			}
 			if (!cmp)
 				continue; /* we will link it later */
@@ -580,7 +580,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 
 	if (!args.fetch_all) {
 		int i;
-		for (i = 0; i < nr_match; i++) {
+		for (i = 0; i < sought->nr; i++) {
 			ref = return_refs[i];
 			if (ref) {
 				*newtail = ref;
@@ -599,7 +599,7 @@ 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_match, char **match)
+static int everything_local(struct ref **refs, struct string_list *sought)
 {
 	struct ref *ref;
 	int retval;
@@ -650,7 +650,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
 		}
 	}
 
-	filter_refs(refs, nr_match, match);
+	filter_refs(refs, sought);
 
 	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
 		const unsigned char *remote = ref->old_sha1;
@@ -781,8 +781,7 @@ 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_match,
-		char **match,
+		struct string_list *sought,
 		char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -839,7 +838,7 @@ static struct ref *do_fetch_pack(int fd[2],
 				agent_len, agent_feature);
 	}
 
-	if (everything_local(&ref, nr_match, match)) {
+	if (everything_local(&ref, sought)) {
 		packet_flush(fd[1]);
 		goto all_done;
 	}
@@ -859,16 +858,16 @@ static struct ref *do_fetch_pack(int fd[2],
 	return ref;
 }
 
-static int remove_duplicates(int nr_heads, char **heads)
+static int remove_duplicates(struct string_list *sought)
 {
 	int src, dst;
 
-	if (!nr_heads)
+	if (!sought->nr)
 		return 0;
 
-	for (src = dst = 1; src < nr_heads; src++)
-		if (strcmp(heads[src], heads[dst-1]))
-			heads[dst++] = heads[src];
+	for (src = dst = 1; src < sought->nr; src++)
+		if (strcmp(sought->items[src].string, sought->items[dst-1].string))
+			sought->items[dst++] = sought->items[src];
 	return dst;
 }
 
@@ -922,8 +921,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	int i, ret;
 	struct ref *ref = NULL;
 	const char *dest = NULL;
-	int alloc_heads = 0, nr_heads = 0;
-	char **heads = NULL;
+	struct string_list sought = STRING_LIST_INIT_DUP;
 	int fd[2];
 	char *pack_lockfile = NULL;
 	char **pack_lockfile_ptr = NULL;
@@ -1000,9 +998,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	 * Copy refs from cmdline to growable list, then append any
 	 * refs from the standard input:
 	 */
-	ALLOC_GROW(heads, argc - i, alloc_heads);
 	for (; i < argc; i++)
-		heads[nr_heads++] = xstrdup(argv[i]);
+		string_list_append(&sought, xstrdup(argv[i]));
 	if (args.stdin_refs) {
 		if (args.stateless_rpc) {
 			/* in stateless RPC mode we use pkt-line to read
@@ -1015,17 +1012,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 					break;
 				if (line[n-1] == '\n')
 					n--;
-				ALLOC_GROW(heads, nr_heads + 1, alloc_heads);
-				heads[nr_heads++] = xmemdupz(line, n);
+				string_list_append(&sought, xmemdupz(line, n));
 			}
 		}
 		else {
 			/* read from stdin one ref per line, until EOF */
 			struct strbuf line = STRBUF_INIT;
-			while (strbuf_getline(&line, stdin, '\n') != EOF) {
-				ALLOC_GROW(heads, nr_heads + 1, alloc_heads);
-				heads[nr_heads++] = strbuf_detach(&line, NULL);
-			}
+			while (strbuf_getline(&line, stdin, '\n') != EOF)
+				string_list_append(&sought, strbuf_detach(&line, NULL));
 			strbuf_release(&line);
 		}
 	}
@@ -1042,7 +1036,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	get_remote_heads(fd[0], &ref, 0, NULL);
 
 	ref = fetch_pack(&args, fd, conn, ref, dest,
-		nr_heads, heads, pack_lockfile_ptr);
+			 &sought, pack_lockfile_ptr);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
 		fflush(stdout);
@@ -1053,17 +1047,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		ref = NULL;
 	ret = !ref;
 
-	if (!ret && nr_heads) {
+	if (!ret && sought.nr) {
 		/* If the heads to pull were given, we should have
 		 * consumed all of them by matching the remote.
 		 * Otherwise, 'git fetch remote no-such-ref' would
 		 * silently succeed without issuing an error.
 		 */
-		for (i = 0; i < nr_heads; i++)
-			if (heads[i] && heads[i][0]) {
-				error("no such remote ref %s", heads[i]);
+		for (i = 0; i < sought.nr; i++) {
+			char *s = sought.items[i].string;
+			if (s && s[0]) {
+				error("no such remote ref %s", s);
 				ret = 1;
 			}
+		}
 	}
 	while (ref) {
 		printf("%s %s\n",
@@ -1074,17 +1070,11 @@ 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,
 		       const char *dest,
-		       int nr_heads,
-		       char **heads,
+		       struct string_list *sought,
 		       char **pack_lockfile)
 {
 	struct stat st;
@@ -1098,16 +1088,16 @@ 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 (sought->nr) {
+		sort_string_list(sought);
+		remove_duplicates(sought);
 	}
 
 	if (!ref) {
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
+	ref_cpy = do_fetch_pack(fd, ref, sought, pack_lockfile);
 
 	if (args.depth > 0) {
 		struct cache_time mtime;
diff --git a/fetch-pack.h b/fetch-pack.h
index 1dbe90f..a6a8a73 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -1,6 +1,8 @@
 #ifndef FETCH_PACK_H
 #define FETCH_PACK_H
 
+#include "string-list.h"
+
 struct fetch_pack_args {
 	const char *uploadpack;
 	int unpacklimit;
@@ -21,8 +23,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 		       int fd[], struct child_process *conn,
 		       const struct ref *ref,
 		       const char *dest,
-		       int nr_heads,
-		       char **heads,
+		       struct string_list *sought,
 		       char **pack_lockfile);
 
 #endif
diff --git a/transport.c b/transport.c
index 1811b50..a847bf3 100644
--- a/transport.c
+++ b/transport.c
@@ -518,8 +518,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct git_transport_data *data = transport->data;
-	char **heads = xmalloc(nr_heads * sizeof(*heads));
-	char **origh = xmalloc(nr_heads * sizeof(*origh));
+	struct string_list orig_sought = STRING_LIST_INIT_DUP;
+	struct string_list sought = STRING_LIST_INIT_NODUP;
 	const struct ref *refs;
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
@@ -537,8 +537,10 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
 
-	for (i = 0; i < nr_heads; i++)
-		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
+	for (i = 0; i < nr_heads; i++) {
+		string_list_append(&orig_sought, to_fetch[i]->name);
+		string_list_append(&sought, orig_sought.items[orig_sought.nr - 1].string);
+	}
 
 	if (!data->got_remote_heads) {
 		connect_setup(transport, 0, 0);
@@ -548,7 +550,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	refs = fetch_pack(&args, data->fd, data->conn,
 			  refs_tmp ? refs_tmp : transport->remote_refs,
-			  dest, nr_heads, heads, &transport->pack_lockfile);
+			  dest, &sought, &transport->pack_lockfile);
 	close(data->fd[0]);
 	close(data->fd[1]);
 	if (finish_connect(data->conn))
@@ -558,10 +560,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	free_refs(refs_tmp);
 
-	for (i = 0; i < nr_heads; i++)
-		free(origh[i]);
-	free(origh);
-	free(heads);
+	string_list_clear(&sought, 0);
+	string_list_clear(&orig_sought, 0);
 	free(dest);
 	return (refs ? 0 : -1);
 }
-- 
1.7.11.3

[PATCH v3 09/14] filter_refs(): build refs list as we go

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Instead of temporarily storing matched refs to temporary array
"return_refs", simply append them to newlist as we go.  This changes
the order of references in newlist to strictly sorted if "--all" and
"--depth" and named references are all specified, but that usage is
broken anyway (see the last two tests in t5500).

This changes the last test in t5500 from segfaulting into just
emitting a spurious error (this will be fixed in a moment).

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 12ba009..4e94aa4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -537,24 +537,11 @@ static int non_matching_ref(struct string_list_item *item, void *unused)
 
 static void filter_refs(struct ref **refs, struct string_list *sought)
 {
-	struct ref **return_refs;
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
-	struct ref *fastarray[32];
 	int sought_pos;
 
-	if (sought->nr && !args.fetch_all) {
-		if (ARRAY_SIZE(fastarray) < sought->nr)
-			return_refs = xcalloc(sought->nr, sizeof(struct ref *));
-		else {
-			return_refs = fastarray;
-			memset(return_refs, 0, sizeof(struct ref *) * sought->nr);
-		}
-	}
-	else
-		return_refs = NULL;
-
 	sought_pos = 0;
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
@@ -575,8 +562,10 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 				if (cmp < 0) /* definitely do not have it */
 					break;
 				else if (cmp == 0) { /* definitely have it */
-					return_refs[sought_pos] = ref;
 					sought->items[sought_pos++].util = "matched";
+					*newtail = ref;
+					ref->next = NULL;
+					newtail = &ref->next;
 					break;
 				}
 				else /* might have it; keep looking */
@@ -588,20 +577,8 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 		free(ref);
 	}
 
-	if (!args.fetch_all) {
-		int i;
-		for (i = 0; i < sought->nr; i++) {
-			ref = return_refs[i];
-			if (ref) {
-				*newtail = ref;
-				ref->next = NULL;
-				newtail = &ref->next;
-			}
-		}
-		if (return_refs != fastarray)
-			free(return_refs);
+	if (!args.fetch_all)
 		filter_string_list(sought, 0, non_matching_ref, NULL);
-	}
 	*refs = newlist;
 }
 
-- 
1.7.11.3

[PATCH v3 13/14] cmd_fetch_pack(): simplify computation of return value

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Set the final value at initialization rather than initializing it then
sometimes changing it.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 3d388b5..42078e5 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1020,17 +1020,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	if (finish_connect(conn))
 		return 1;
 
-	ret = !ref;
-	if (sought.nr) {
-		/* If the heads to pull were given, we should have
-		 * consumed all of them by matching the remote.
-		 * Otherwise, 'git fetch remote no-such-ref' would
-		 * silently succeed without issuing an error.
-		 */
-		for (i = 0; i < sought.nr; i++)
-			error("no such remote ref %s", sought.items[i].string);
-		ret = 1;
-	}
+	ret = !ref || sought.nr;
+
+	/*
+	 * If the heads to pull were given, we should have consumed
+	 * all of them by matching the remote.  Otherwise, 'git fetch
+	 * remote no-such-ref' would silently succeed without issuing
+	 * an error.
+	 */
+	for (i = 0; i < sought.nr; i++)
+		error("no such remote ref %s", sought.items[i].string);
 	while (ref) {
 		printf("%s %s\n",
 		       sha1_to_hex(ref->old_sha1), ref->name);
-- 
1.7.11.3

[PATCH v3 12/14] fetch-pack: report missing refs even if no existing refs were received

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

This fixes a test in t5500.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c  | 2 +-
 t/t5500-fetch-pack.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index fb2a423..3d388b5 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1021,7 +1021,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	ret = !ref;
-	if (ref && sought.nr) {
+	if (sought.nr) {
 		/* If the heads to pull were given, we should have
 		 * consumed all of them by matching the remote.
 		 * Otherwise, 'git fetch remote no-such-ref' would
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index acd41e8..894d945 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -403,7 +403,7 @@ test_expect_success 'set up tests of missing reference' '
 	EOF
 '
 
-test_expect_failure 'test lonely missing ref' '
+test_expect_success 'test lonely missing ref' '
 	(
 		cd client &&
 		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy
-- 
1.7.11.3

[PATCH v3 11/14] cmd_fetch_pack(): return early if finish_connect() fails

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

This simplifies the logic without changing the behavior.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 056ccb8..fb2a423 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1018,10 +1018,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	close(fd[0]);
 	close(fd[1]);
 	if (finish_connect(conn))
-		ref = NULL;
-	ret = !ref;
+		return 1;
 
-	if (!ret && sought.nr) {
+	ret = !ref;
+	if (ref && sought.nr) {
 		/* If the heads to pull were given, we should have
 		 * consumed all of them by matching the remote.
 		 * Otherwise, 'git fetch remote no-such-ref' would
-- 
1.7.11.3

[PATCH v3 14/14] fetch-pack: eliminate spurious error messages

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

It used to be that if "--all", "--depth", and also explicit references
were sought, then the explicit references were not handled correctly
in filter_refs() because the "--all --depth" code took precedence over
the explicit reference handling, and the explicit references were
never noted as having been found.  So check for explicitly sought
references before proceeding to the "--all --depth" logic.

This fixes two test cases in t5500.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c  | 10 +++++-----
 t/t5500-fetch-pack.sh |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 42078e5..e644398 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -549,9 +549,6 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 		if (!memcmp(ref->name, "refs/", 5) &&
 		    check_refname_format(ref->name + 5, 0))
 			; /* trash */
-		else if (args.fetch_all &&
-			 (!args.depth || prefixcmp(ref->name, "refs/tags/")))
-			keep = 1;
 		else {
 			while (sought_pos < sought->nr) {
 				int cmp = strcmp(ref->name, sought->items[sought_pos].string);
@@ -567,6 +564,10 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 			}
 		}
 
+		if (! keep && args.fetch_all &&
+		    (!args.depth || prefixcmp(ref->name, "refs/tags/")))
+			keep = 1;
+
 		if (keep) {
 			*newtail = ref;
 			ref->next = NULL;
@@ -576,8 +577,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 		}
 	}
 
-	if (!args.fetch_all)
-		filter_string_list(sought, 0, non_matching_ref, NULL);
+	filter_string_list(sought, 0, non_matching_ref, NULL);
 	*refs = newlist;
 }
 
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 894d945..6322e8a 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -427,14 +427,14 @@ test_expect_success 'test missing ref before existing' '
 	test_cmp expect-error error-me
 '
 
-test_expect_failure 'test --all, --depth, and explicit head' '
+test_expect_success 'test --all, --depth, and explicit head' '
 	(
 		cd client &&
 		git fetch-pack --no-progress --all --depth=1 .. refs/heads/A
 	) >out-adh 2>error-adh
 '
 
-test_expect_failure 'test --all, --depth, and explicit tag' '
+test_expect_success 'test --all, --depth, and explicit tag' '
 	git tag OLDTAG refs/heads/B~5 &&
 	(
 		cd client &&
-- 
1.7.11.3

[PATCH v3 10/14] filter_refs(): simplify logic

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Simplify flow within loop: first decide whether to keep the reference,
then keep/free it.  This makes it clearer that each ref has exactly
two possible destinies, and removes duplication of the code for
appending the reference to the linked list.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4e94aa4..056ccb8 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -544,37 +544,36 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 
 	sought_pos = 0;
 	for (ref = *refs; ref; ref = next) {
+		int keep = 0;
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
 		    check_refname_format(ref->name + 5, 0))
 			; /* trash */
 		else if (args.fetch_all &&
-			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
-			*newtail = ref;
-			ref->next = NULL;
-			newtail = &ref->next;
-			continue;
-		}
+			 (!args.depth || prefixcmp(ref->name, "refs/tags/")))
+			keep = 1;
 		else {
-			int cmp = -1;
 			while (sought_pos < sought->nr) {
-				cmp = strcmp(ref->name, sought->items[sought_pos].string);
-				if (cmp < 0) /* definitely do not have it */
-					break;
-				else if (cmp == 0) { /* definitely have it */
+				int cmp = strcmp(ref->name, sought->items[sought_pos].string);
+				if (cmp < 0)
+					break; /* definitely do not have it */
+				else if (cmp == 0) {
+					keep = 1; /* definitely have it */
 					sought->items[sought_pos++].util = "matched";
-					*newtail = ref;
-					ref->next = NULL;
-					newtail = &ref->next;
 					break;
 				}
-				else /* might have it; keep looking */
-					sought_pos++;
+				else
+					sought_pos++; /* might have it; keep looking */
 			}
-			if (!cmp)
-				continue; /* we will link it later */
 		}
-		free(ref);
+
+		if (keep) {
+			*newtail = ref;
+			ref->next = NULL;
+			newtail = &ref->next;
+		} else {
+			free(ref);
+		}
 	}
 
 	if (!args.fetch_all)
-- 
1.7.11.3

[PATCH v3 04/14] fetch_pack(): reindent function decl and defn

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c |  8 ++++----
 fetch-pack.h         | 12 ++++++------
 2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index fdda36f..30990c0 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1082,10 +1082,10 @@ static int compare_heads(const void *a, const void *b)
 struct ref *fetch_pack(struct fetch_pack_args *my_args,
 		       int fd[], struct child_process *conn,
 		       const struct ref *ref,
-		const char *dest,
-		int nr_heads,
-		char **heads,
-		char **pack_lockfile)
+		       const char *dest,
+		       int nr_heads,
+		       char **heads,
+		       char **pack_lockfile)
 {
 	struct stat st;
 	struct ref *ref_cpy;
diff --git a/fetch-pack.h b/fetch-pack.h
index 7c2069c..1dbe90f 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -18,11 +18,11 @@ struct fetch_pack_args {
 };
 
 struct ref *fetch_pack(struct fetch_pack_args *args,
-		int fd[], struct child_process *conn,
-		const struct ref *ref,
-		const char *dest,
-		int nr_heads,
-		char **heads,
-		char **pack_lockfile);
+		       int fd[], struct child_process *conn,
+		       const struct ref *ref,
+		       const char *dest,
+		       int nr_heads,
+		       char **heads,
+		       char **pack_lockfile);
 
 #endif
-- 
1.7.11.3

[PATCH v3 08/14] filter_refs(): delete matched refs from sought list

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Remove any references that are available from the remote from the
sought list (rather than overwriting their names with NUL characters,
as previously).  Mark matching entries by writing a non-NULL pointer
to string_list_item::util during the iteration, then use
filter_string_list() later to filter out the entries that have been
marked.

Document this aspect of fetch_pack() in a comment in the header file.
(More documentation is obviously still needed.)

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 23 +++++++++++++++--------
 fetch-pack.h         |  7 +++++++
 transport.c          | 10 +++-------
 3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6cd734a..12ba009 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -525,6 +525,16 @@ static void mark_recent_complete_commits(unsigned long cutoff)
 	}
 }
 
+static int non_matching_ref(struct string_list_item *item, void *unused)
+{
+	if (item->util) {
+		item->util = NULL;
+		return 0;
+	}
+	else
+		return 1;
+}
+
 static void filter_refs(struct ref **refs, struct string_list *sought)
 {
 	struct ref **return_refs;
@@ -566,7 +576,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 					break;
 				else if (cmp == 0) { /* definitely have it */
 					return_refs[sought_pos] = ref;
-					sought->items[sought_pos++].string[0] = '\0';
+					sought->items[sought_pos++].util = "matched";
 					break;
 				}
 				else /* might have it; keep looking */
@@ -590,6 +600,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 		}
 		if (return_refs != fastarray)
 			free(return_refs);
+		filter_string_list(sought, 0, non_matching_ref, NULL);
 	}
 	*refs = newlist;
 }
@@ -1040,13 +1051,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		 * Otherwise, 'git fetch remote no-such-ref' would
 		 * silently succeed without issuing an error.
 		 */
-		for (i = 0; i < sought.nr; i++) {
-			char *s = sought.items[i].string;
-			if (s && s[0]) {
-				error("no such remote ref %s", s);
-				ret = 1;
-			}
-		}
+		for (i = 0; i < sought.nr; i++)
+			error("no such remote ref %s", sought.items[i].string);
+		ret = 1;
 	}
 	while (ref) {
 		printf("%s %s\n",
diff --git a/fetch-pack.h b/fetch-pack.h
index a6a8a73..cb14871 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -19,6 +19,13 @@ struct fetch_pack_args {
 		stateless_rpc:1;
 };
 
+/*
+ * sought contains the full names of remote references that should be
+ * updated from.  On return, the names that were found on the remote
+ * will have been removed from the list.  The util members of the
+ * string_list_items are used internally; they must be NULL on entry
+ * (and will be NULL on exit).
+ */
 struct ref *fetch_pack(struct fetch_pack_args *args,
 		       int fd[], struct child_process *conn,
 		       const struct ref *ref,
diff --git a/transport.c b/transport.c
index a847bf3..9932f40 100644
--- a/transport.c
+++ b/transport.c
@@ -518,8 +518,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct git_transport_data *data = transport->data;
-	struct string_list orig_sought = STRING_LIST_INIT_DUP;
-	struct string_list sought = STRING_LIST_INIT_NODUP;
+	struct string_list sought = STRING_LIST_INIT_DUP;
 	const struct ref *refs;
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
@@ -537,10 +536,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
 
-	for (i = 0; i < nr_heads; i++) {
-		string_list_append(&orig_sought, to_fetch[i]->name);
-		string_list_append(&sought, orig_sought.items[orig_sought.nr - 1].string);
-	}
+	for (i = 0; i < nr_heads; i++)
+		string_list_append(&sought, to_fetch[i]->name);
 
 	if (!data->got_remote_heads) {
 		connect_setup(transport, 0, 0);
@@ -561,7 +558,6 @@ static int fetch_refs_via_pack(struct transport *transport,
 	free_refs(refs_tmp);
 
 	string_list_clear(&sought, 0);
-	string_list_clear(&orig_sought, 0);
 	free(dest);
 	return (refs ? 0 : -1);
 }
-- 
1.7.11.3

[PATCH v3 07/14] fetch_pack(): update sought->nr to reflect number of unique entries

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

fetch_pack() removes duplicates from the "sought" list, thereby
shrinking the list.  But previously, the caller was not informed about
the shrinkage.  This would cause a spurious error message to be
emitted by cmd_fetch_pack() if "git fetch-pack" is called with
duplicate refnames.

Instead, remove duplicates using string_list_remove_duplicates(),
which adjusts sought->nr to reflect the new length of the list.

The last test of t5500 inexplicably *required* "git fetch-pack" to
fail when fetching a list of references that contains duplicates;
i.e., it insisted on the buggy behavior.  So change the test to expect
the correct behavior.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c  | 15 +--------------
 t/t5500-fetch-pack.sh |  2 +-
 2 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63d455f..6cd734a 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -858,19 +858,6 @@ static struct ref *do_fetch_pack(int fd[2],
 	return ref;
 }
 
-static int remove_duplicates(struct string_list *sought)
-{
-	int src, dst;
-
-	if (!sought->nr)
-		return 0;
-
-	for (src = dst = 1; src < sought->nr; src++)
-		if (strcmp(sought->items[src].string, sought->items[dst-1].string))
-			sought->items[dst++] = sought->items[src];
-	return dst;
-}
-
 static int fetch_pack_config(const char *var, const char *value, void *cb)
 {
 	if (strcmp(var, "fetch.unpacklimit") == 0) {
@@ -1090,7 +1077,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 
 	if (sought->nr) {
 		sort_string_list(sought);
-		remove_duplicates(sought);
+		string_list_remove_duplicates(sought, 0);
 	}
 
 	if (!ref) {
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 15d277f..acd41e8 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' '
 test_expect_success 'test duplicate refs from stdin' '
 	(
 	cd client &&
-	test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup
+	git fetch-pack --stdin --no-progress .. <../input.dup
 	) >output &&
 	cut -d " " -f 2 <output | sort >actual &&
 	test_cmp expect actual
-- 
1.7.11.3

[PATCH v3 06/14] filter_refs(): do not check the same sought_pos twice

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Once a match has been found at sought_pos, the entry is zeroed and no
future attempts will match that entry.  So increment sought_pos to
avoid checking against the zeroed-out entry during the next iteration.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index df81995..63d455f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -565,8 +565,8 @@ static void filter_refs(struct ref **refs, struct string_list *sought)
 				if (cmp < 0) /* definitely do not have it */
 					break;
 				else if (cmp == 0) { /* definitely have it */
-					sought->items[sought_pos].string[0] = '\0';
 					return_refs[sought_pos] = ref;
+					sought->items[sought_pos++].string[0] = '\0';
 					break;
 				}
 				else /* might have it; keep looking */
-- 
1.7.11.3

[PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:54:42

Document some bugs in "git fetch-pack":

1. If "git fetch-pack" is called with "--all", "--depth", and an
explicit existing non-tag reference to fetch, then it falsely reports
that the reference was not found, even though it was fetched
correctly.

2. If "git fetch-pack" is called with "--all", "--depth", and an
explicit existing tag reference to fetch, then it segfaults in
filter_refs() because return_refs is used without having been
initialized.

Signed-off-by: Michael Haggerty <redacted>
---
 t/t5500-fetch-pack.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 6fa1cef..15d277f 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -427,4 +427,19 @@ test_expect_success 'test missing ref before existing' '
 	test_cmp expect-error error-me
 '
 
+test_expect_failure 'test --all, --depth, and explicit head' '
+	(
+		cd client &&
+		git fetch-pack --no-progress --all --depth=1 .. refs/heads/A
+	) >out-adh 2>error-adh
+'
+
+test_expect_failure 'test --all, --depth, and explicit tag' '
+	git tag OLDTAG refs/heads/B~5 &&
+	(
+		cd client &&
+		git fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG
+	) >out-adt 2>error-adt
+'
+
 test_done
-- 
1.7.11.3

Re: [PATCH v3 09/14] filter_refs(): build refs list as we go

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:43

Michael Haggerty [off-list ref] writes:
Instead of temporarily storing matched refs to temporary array
"return_refs", simply append them to newlist as we go.  This changes
the order of references in newlist to strictly sorted if "--all" and
"--depth" and named references are all specified, but that usage is
broken anyway (see the last two tests in t5500).
Removes two warts (the temporary array in general, and the
fastarray[] special case) with one patch.  Nicely done.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help