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

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

[Michael Haggerty]

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

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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

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

[Michael Haggerty]

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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 04/14] fetch_pack(): reindent function decl and defn

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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 05/14] Change fetch_pack() and friends to take string_list arguments

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 06/14] filter_refs(): do not check the same sought_pos twice

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 07/14] fetch_pack(): update sought->nr to reflect number of unique entries

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 08/14] filter_refs(): delete matched refs from sought list

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 09/14] filter_refs(): build refs list as we go

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 10/14] filter_refs(): simplify logic

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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 11/14] cmd_fetch_pack(): return early if finish_connect() fails

[Michael Haggerty]

This simplifies the logic without changing the behavior.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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 12/14] fetch-pack: report missing refs even if no existing refs were received

[Michael Haggerty]

This fixes a test in t5500.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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 13/14] cmd_fetch_pack(): simplify computation of return value

[Michael Haggerty]

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 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 14/14] fetch-pack: eliminate spurious error messages

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

Makes sense.  The only worry (without reading the series first) I
have is that the use of string list may make the responsiblity of
sorting the list fuzzier. I am guessing that we never sorted the
refs we asked to fetch (so that FETCH_HEAD comes out in an expected
order), so use of unsorted string list would be perfectly fine.

Queued and pushed out in 'pu' without reading, but will take a look
sometime tomorrow (eh, today, that is).

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

[Jeff King]

On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > This patch series depends on the "Add some string_list-related
> > functions" series that I just submitted.
> 
> Makes sense.  The only worry (without reading the series first) I
> have is that the use of string list may make the responsiblity of
> sorting the list fuzzier. I am guessing that we never sorted the
> refs we asked to fetch (so that FETCH_HEAD comes out in an expected
> order), so use of unsorted string list would be perfectly fine.

I haven't read the series yet, but both the list of heads from the user
and the list of heads from the remote should have been sorted by 4435968
and 9e8e704f, respectively.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote:
>
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>> > This patch series depends on the "Add some string_list-related
>> > functions" series that I just submitted.
>> 
>> Makes sense.  The only worry (without reading the series first) I
>> have is that the use of string list may make the responsiblity of
>> sorting the list fuzzier. I am guessing that we never sorted the
>> refs we asked to fetch (so that FETCH_HEAD comes out in an expected
>> order), so use of unsorted string list would be perfectly fine.
>
> I haven't read the series yet, but both the list of heads from the user
> and the list of heads from the remote should have been sorted by 4435968
> and 9e8e704f, respectively.

OK.  As long as the sort order matches the order string-list
internally uses for its bisection search, it won't be a problem,
then.

Thanks.

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

I guess the first one is because "all" already marks the fetched one
"used", and does not allow the explicit one to match any unused one
from the other side?  I wonder what happens when "--all" with an
explicit refspec that names non-existing ref is asked for (it should
notice that refs/heads/no-such-ref does not exist.  I do not know if
it is something that belongs to this set of new tests)?

It is funny that this only happens with "--depth" (I think I know
which part of the code introduces this bug, so it is not all that
interesting, but just funny).

Thanks for working on these glitches.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  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

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> 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 <mhagger@alum.mit.edu>
> ---

The earlier bikeshedding-fest on variable names seems to have
produced a winner ;-) I think "sought" captures what it is about
very well.

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

This is a tangent, but I _think_ our header files ignore the dogma
some other projects follow that insists on each header file to be
self sufficient, i.e.

	gcc fetch-pack.h

should pass.  Instead, our *.c files that include fetch-pack.h are
responsible for including everything the headers they include need.
So even though fetch-pack.h does not include run-command.h, it
declares a function that takes "struct child_process *" in its
arguments.  The new "struct string_list *" falls into the same camp.

Given that, I'd prefer to see the scope of this patch series shrunk
and have the caller include string-list.h, not here.

Updating the headers and sources so that each to be self sufficient
is a different topic, and I do not think there is a consensus yet if
we want to go that route.

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> 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.

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

Thanks for a pleasant read, nicely broken down into digestible
pieces.

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

[Michael Haggerty]

On 09/10/2012 10:46 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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.
> 
> I guess the first one is because "all" already marks the fetched one
> "used", and does not allow the explicit one to match any unused one
> from the other side?

Yes, more or less.  Spoiler: these failures are fixed later in the patch
series :-)

> I wonder what happens when "--all" with an
> explicit refspec that names non-existing ref is asked for (it should
> notice that refs/heads/no-such-ref does not exist.  I do not know if
> it is something that belongs to this set of new tests)?

Good idea; I just wrote such tests.  They appear to pass regardless of
this patch series.  I will submit them after I am sure that I understand
them.

Michael

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

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

[Michael Haggerty]

On 09/09/2012 08:15 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote:
>>
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>
>>>> This patch series depends on the "Add some string_list-related
>>>> functions" series that I just submitted.
>>>
>>> Makes sense.  The only worry (without reading the series first) I
>>> have is that the use of string list may make the responsiblity of
>>> sorting the list fuzzier. I am guessing that we never sorted the
>>> refs we asked to fetch (so that FETCH_HEAD comes out in an expected
>>> order), so use of unsorted string list would be perfectly fine.
>>
>> I haven't read the series yet, but both the list of heads from the user
>> and the list of heads from the remote should have been sorted by 4435968
>> and 9e8e704f, respectively.
> 
> OK.  As long as the sort order matches the order string-list
> internally uses for its bisection search, it won't be a problem,
> then.

The sorting is crucial but there is no bisection involved.  The sorted
linked-list of references available from the remote and the sorted
string_list of requested references are iterated through in parallel.

Michael

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

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> OK.  As long as the sort order matches the order string-list
>> internally uses for its bisection search, it won't be a problem,
>> then.
>
> The sorting is crucial but there is no bisection involved.  The sorted
> linked-list of references available from the remote and the sorted
> string_list of requested references are iterated through in parallel.

What I meant was that the order used by string-list is pretty much
internal to string-list implementation for its "quickly locate where
to insert" bisection.  It happens to be the byte value order, I
think, but the point is whatever order it is, that has to match the
order we keep references in the other data source you walk in
parallel to match (i.e. the linked list of references).

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

[Michael Haggerty]

On 09/10/2012 10:56 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 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);
> 
> This is a tangent, but I _think_ our header files ignore the dogma
> some other projects follow that insists on each header file to be
> self sufficient, i.e.
> 
> 	gcc fetch-pack.h
> 
> should pass.  Instead, our *.c files that include fetch-pack.h are
> responsible for including everything the headers they include need.
> So even though fetch-pack.h does not include run-command.h, it
> declares a function that takes "struct child_process *" in its
> arguments.  The new "struct string_list *" falls into the same camp.
> 
> Given that, I'd prefer to see the scope of this patch series shrunk
> and have the caller include string-list.h, not here.
> 
> Updating the headers and sources so that each to be self sufficient
> is a different topic, and I do not think there is a consensus yet if
> we want to go that route.

The include line can just be deleted, because the three files that
include it already get string-list.h from somewhere:

* builtin/clone.c, builtin/fetch-pack.c
  includes builtin.h
  includes notes.h
  includes string-list.h

* transport.c
  includes string-list.h

Patch forthcoming.

But how far should this policy be taken?  It seems to me that strict
adherence to the policy would dictate that *.h files should *never*
include other git project files.

For example, while working on this, I noticed that notes.h includes
string-list.h and also contains a forward declaration for "struct
string_list".  Obviously, the latter could be deleted.  Alternatively,
both could probably be deleted by ensuring that the relevant *.c files
include string-list.h before including notes.h.

Michael

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

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

[Michael Haggerty]

On 09/11/2012 12:10 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> OK.  As long as the sort order matches the order string-list
>>> internally uses for its bisection search, it won't be a problem,
>>> then.
>>
>> The sorting is crucial but there is no bisection involved.  The sorted
>> linked-list of references available from the remote and the sorted
>> string_list of requested references are iterated through in parallel.
> 
> What I meant was that the order used by string-list is pretty much
> internal to string-list implementation for its "quickly locate where
> to insert" bisection.  It happens to be the byte value order, I
> think, but the point is whatever order it is, that has to match the
> order we keep references in the other data source you walk in
> parallel to match (i.e. the linked list of references).

Yes, your point is good that the two sort orders have to agree.
Currently, they both use strcmp() order, so the situation is OK.

It is interesting that you consider the sort order of string_list to be
somewhat of an internal implementation detail.  I had thought of its
current behavior as being the obvious thing and considered it part of
the API's contract.  For example, the current sort order is already
observable via the API through iteration or by calling
print_string_list().  Therefore I think that we should document the
strcmp() sort order as part of the string_list contract.  Patch coming soon.

If, at some future time, somebody wants a string_list that is sorted by
a different criterion, then the order should be determined via a
callback function specified by the user.  The callback function could
even be stored in the string_list header, to allow such lists to be used
in combination with the "functions for sorted lists only".

Michael

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

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It is interesting that you consider the sort order of string_list to be
> somewhat of an internal implementation detail.  I had thought of its
> current behavior as being the obvious thing and considered it part of
> the API's contract.  For example, the current sort order is already
> observable via the API through iteration or by calling
> print_string_list().

You can prepare an unsorted string list that holds items in the
order you desire, and call print_string_list() on it.  I do not
think we ever promised to keep using the same sort order when you
allow the string list to use sorted insertion by calling the
"sorted" API functions.

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> But how far should this policy be taken?  It seems to me that strict
> adherence to the policy would dictate that *.h files should *never*
> include other git project files.

I wouldn't call that a "policy".  It's something we think about when
adding a new "#include" to see if it is worth adding and/or if it is
the right place to add it to reduce code churn.

As I said, what policy to pick and stick to is open to discussion,
and I wanted to leave it outside the scope of this series.  As it
has been cooking in 'next', I do not think it is worth reverting the
inclusion of "string-list.h" to delay this topic.  It is something
that can and should be cleaned up when we decide to pick the
inclusion policy and enforce it.  If we choose to go in the other
direction, we would end up adding it back, so let's keep it as-is
for now.

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

[Jeff King]

On Mon, Sep 17, 2012 at 03:10:07PM -0700, Junio C Hamano wrote:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > But how far should this policy be taken?  It seems to me that strict
> > adherence to the policy would dictate that *.h files should *never*
> > include other git project files.
> 
> I wouldn't call that a "policy".  It's something we think about when
> adding a new "#include" to see if it is worth adding and/or if it is
> the right place to add it to reduce code churn.
> 
> As I said, what policy to pick and stick to is open to discussion,
> and I wanted to leave it outside the scope of this series.  As it
> has been cooking in 'next', I do not think it is worth reverting the
> inclusion of "string-list.h" to delay this topic.  It is something
> that can and should be cleaned up when we decide to pick the
> inclusion policy and enforce it.  If we choose to go in the other
> direction, we would end up adding it back, so let's keep it as-is
> for now.

I will admit that I usually follow the opposite policy of what you have
suggested, and include dependent headers in the .h files. Mostly just
because it makes things simpler for the user of the header file, and
there aren't really downsides (yes, you can have weird dependency-order
issues, but in practice those are rare, and they generate very obvious
compile-time errors).

What I think would be much more productive is breaking apart gigantic
includes like cache.h into more reasonable modules, which would mean
less frequent recompilation when an uninteresting part of the header
changes. But git is reasonably fast to compile as it is, so I have never
quite decided that it is worth the human effort to go in that direction.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> What I think would be much more productive is breaking apart gigantic
> includes like cache.h into more reasonable modules, which would mean
> less frequent recompilation when an uninteresting part of the header
> changes.

Ideally cache.h should cover what read-cache.c, sha1_file.c and
sha1_name.c do and need.  The parts related to sha1_name.c may
deserve to have its own header file, as its scope is fairly wide and
spans from low-level object name get_sha1_hex() to high level
parsing like dwim_ref().  Right now, the header also has pieces that
are needed only by connect.c and config.c (not even commented as
such), pager, base85, alloc, trace, add, piece of diff, etc.

Also we may want to move helper functions that are generally useful
and do not depend on specific command to more appropriate files.

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

[Philip Oakley]

From: "Michael Haggerty" <mhagger@alum.mit.edu>
Sent: Monday, September 10, 2012 10:53 PM
> On 09/10/2012 10:46 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> 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.
>>

This probably isn't the right place in the series but I've just see a
question on SO
http://stackoverflow.com/questions/12475210/git-shallow-clone-along-with-branch
which gives a user case that may or may not be covered by the
pack-fetch.

The user was looking for 'git clone --depth 1 -b master' and comparing
it to a plain 'git clone --depth 1' and to 'git clone -b master' with
the follow up question "That is, why when passing -b it seems like it's
changing behaviour of --depth ?"

Perhaps something to test out.

Philip