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

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

[mhagger]

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

Re-roll, incorporating Jeff's suggestions.  Some commit messages have
also been improved, but the only interdiff is that match_pos is
renamed to head_pos in filter_refs().

This patch series applies to the merge between master and
jc/maint-push-refs-all, though the dependency on the latter is only
textual.

Michael Haggerty (17):
  t5500: add tests of error output for missing refs
  Rename static function fetch_pack() to http_fetch_pack()
  Fix formatting.
  Name local variables more consistently
  Do not check the same head_pos twice
  Let fetch_pack() inform caller about number of unique heads
  Pass nr_heads to do_pack_ref() by reference
  Pass nr_heads to everything_local() by reference
  Pass nr_heads to filter_refs() by reference
  Remove ineffective optimization
  filter_refs(): do not leave gaps in return_refs
  filter_refs(): compress unmatched refs in heads array
  cmd_fetch_pack: return early if finish_connect() returns an error
  Report missing refs even if no existing refs were received
  cmd_fetch_pack(): simplify computation of return value
  fetch_pack(): free matching heads
  filter_refs(): simplify logic

 builtin/fetch-pack.c  | 128 ++++++++++++++++++++------------------------------
 fetch-pack.h          |  19 +++++---
 http-walker.c         |   4 +-
 t/t5500-fetch-pack.sh |  32 ++++++++++++-
 transport.c           |   8 ++--
 5 files changed, 101 insertions(+), 90 deletions(-)

-- 
1.7.11.3

[PATCH v2 01/17] t5500: add tests of error output for missing refs

[mhagger]

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

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..3cc3346 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 v2 02/17] Rename static function fetch_pack() to http_fetch_pack()

[mhagger]

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

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 v2 03/17] Fix formatting.

[mhagger]

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


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 7912d2b..cc21047 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1062,10 +1062,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 v2 04/17] Name local variables more consistently

[mhagger]

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

Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cc21047..76288a2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,27 +521,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, int nr_heads, char **heads)
 {
 	struct ref **return_refs;
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
 	struct ref *fastarray[32];
-	int match_pos;
+	int head_pos;
 
-	if (nr_match && !args.fetch_all) {
-		if (ARRAY_SIZE(fastarray) < nr_match)
-			return_refs = xcalloc(nr_match, sizeof(struct ref *));
+	if (nr_heads && !args.fetch_all) {
+		if (ARRAY_SIZE(fastarray) < nr_heads)
+			return_refs = xcalloc(nr_heads, sizeof(struct ref *));
 		else {
 			return_refs = fastarray;
-			memset(return_refs, 0, sizeof(struct ref *) * nr_match);
+			memset(return_refs, 0, sizeof(struct ref *) * nr_heads);
 		}
 	}
 	else
 		return_refs = NULL;
 
-	match_pos = 0;
+	head_pos = 0;
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
@@ -556,17 +556,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 (head_pos < nr_heads) {
+				cmp = strcmp(ref->name, heads[head_pos]);
 				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;
+					heads[head_pos][0] = '\0';
+					return_refs[head_pos] = ref;
 					break;
 				}
 				else /* might have it; keep looking */
-					match_pos++;
+					head_pos++;
 			}
 			if (!cmp)
 				continue; /* we will link it later */
@@ -576,7 +576,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 < nr_heads; i++) {
 			ref = return_refs[i];
 			if (ref) {
 				*newtail = ref;
@@ -595,7 +595,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, int nr_heads, char **heads)
 {
 	struct ref *ref;
 	int retval;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
 		}
 	}
 
-	filter_refs(refs, nr_match, match);
+	filter_refs(refs, nr_heads, heads);
 
 	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
 		const unsigned char *remote = ref->old_sha1;
@@ -777,8 +777,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,
+		int nr_heads, char **heads,
 		char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -819,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (everything_local(&ref, nr_match, match)) {
+	if (everything_local(&ref, nr_heads, heads)) {
 		packet_flush(fd[1]);
 		goto all_done;
 	}
-- 
1.7.11.3

[PATCH v2 05/17] Do not check the same head_pos twice

[mhagger]

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

Once a match has been found at head_pos, the entry is zeroed and no
future attempts will match that entry.  So increment head_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 76288a2..bda3c0c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -561,8 +561,8 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)
 				if (cmp < 0) /* definitely do not have it */
 					break;
 				else if (cmp == 0) { /* definitely have it */
-					heads[head_pos][0] = '\0';
 					return_refs[head_pos] = ref;
+					heads[head_pos++][0] = '\0';
 					break;
 				}
 				else /* might have it; keep looking */
-- 
1.7.11.3

[PATCH v2 06/17] Let fetch_pack() inform caller about number of unique heads

[mhagger]

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

fetch_pack() removes duplicates from the list (nr_heads, heads),
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.

So change the signature of fetch_pack() to accept nr_heads by
reference, and if any duplicates were removed then modify it to
reflect the number of remaining references.

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  | 12 ++++++------
 fetch-pack.h          |  2 +-
 t/t5500-fetch-pack.sh |  2 +-
 transport.c           |  2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bda3c0c..cf65998 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)
 	get_remote_heads(fd[0], &ref, 0, NULL);
 
 	ref = fetch_pack(&args, fd, conn, ref, dest,
-		nr_heads, heads, pack_lockfile_ptr);
+		&nr_heads, heads, pack_lockfile_ptr);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
 		fflush(stdout);
@@ -1062,7 +1062,7 @@ 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,
+		       int *nr_heads,
 		       char **heads,
 		       char **pack_lockfile)
 {
@@ -1077,16 +1077,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 (heads && *nr_heads) {
+		qsort(heads, *nr_heads, sizeof(*heads), compare_heads);
+		*nr_heads = remove_duplicates(*nr_heads, heads);
 	}
 
 	if (!ref) {
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
+	ref_cpy = do_fetch_pack(fd, ref, *nr_heads, heads, pack_lockfile);
 
 	if (args.depth > 0) {
 		struct cache_time mtime;
diff --git a/fetch-pack.h b/fetch-pack.h
index 1dbe90f..a9d505b 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -21,7 +21,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,
+		       int *nr_heads,
 		       char **heads,
 		       char **pack_lockfile);
 
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 3cc3346..0d4edcb 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
diff --git a/transport.c b/transport.c
index 1811b50..f7bbf58 100644
--- a/transport.c
+++ b/transport.c
@@ -548,7 +548,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, &nr_heads, heads, &transport->pack_lockfile);
 	close(data->fd[0]);
 	close(data->fd[1]);
 	if (finish_connect(data->conn))
-- 
1.7.11.3

[PATCH v2 07/17] Pass nr_heads to do_pack_ref() by reference

[mhagger]

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

This is the first of a few baby steps towards changing filter_refs()
to compress matched refs out of the list rather than overwriting the
first character of such references with '\0'.

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 cf65998..fae4f7c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -777,7 +777,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_heads, char **heads,
+		int *nr_heads, char **heads,
 		char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -818,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (everything_local(&ref, nr_heads, heads)) {
+	if (everything_local(&ref, *nr_heads, heads)) {
 		packet_flush(fd[1]);
 		goto all_done;
 	}
@@ -1086,7 +1086,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 		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, nr_heads, heads, pack_lockfile);
 
 	if (args.depth > 0) {
 		struct cache_time mtime;
-- 
1.7.11.3

[PATCH v2 08/17] Pass nr_heads to everything_local() by reference

[mhagger]

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


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 fae4f7c..a4bb0ff 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -595,7 +595,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_heads, char **heads)
+static int everything_local(struct ref **refs, int *nr_heads, char **heads)
 {
 	struct ref *ref;
 	int retval;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int nr_heads, char **heads)
 		}
 	}
 
-	filter_refs(refs, nr_heads, heads);
+	filter_refs(refs, *nr_heads, heads);
 
 	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
 		const unsigned char *remote = ref->old_sha1;
@@ -818,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (everything_local(&ref, *nr_heads, heads)) {
+	if (everything_local(&ref, nr_heads, heads)) {
 		packet_flush(fd[1]);
 		goto all_done;
 	}
-- 
1.7.11.3

[PATCH v2 09/17] Pass nr_heads to filter_refs() by reference

[mhagger]

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


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index a4bb0ff..c47090d 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)
 	}
 }
 
-static void filter_refs(struct ref **refs, int nr_heads, char **heads)
+static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 {
 	struct ref **return_refs;
 	struct ref *newlist = NULL;
@@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)
 	struct ref *fastarray[32];
 	int head_pos;
 
-	if (nr_heads && !args.fetch_all) {
-		if (ARRAY_SIZE(fastarray) < nr_heads)
-			return_refs = xcalloc(nr_heads, sizeof(struct ref *));
+	if (*nr_heads && !args.fetch_all) {
+		if (ARRAY_SIZE(fastarray) < *nr_heads)
+			return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
 		else {
 			return_refs = fastarray;
-			memset(return_refs, 0, sizeof(struct ref *) * nr_heads);
+			memset(return_refs, 0, sizeof(struct ref *) * *nr_heads);
 		}
 	}
 	else
@@ -556,7 +556,7 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)
 		}
 		else {
 			int cmp = -1;
-			while (head_pos < nr_heads) {
+			while (head_pos < *nr_heads) {
 				cmp = strcmp(ref->name, heads[head_pos]);
 				if (cmp < 0) /* definitely do not have it */
 					break;
@@ -576,7 +576,7 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)
 
 	if (!args.fetch_all) {
 		int i;
-		for (i = 0; i < nr_heads; i++) {
+		for (i = 0; i < *nr_heads; i++) {
 			ref = return_refs[i];
 			if (ref) {
 				*newtail = ref;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int *nr_heads, char **heads)
 		}
 	}
 
-	filter_refs(refs, *nr_heads, heads);
+	filter_refs(refs, nr_heads, heads);
 
 	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
 		const unsigned char *remote = ref->old_sha1;
-- 
1.7.11.3

[PATCH v2 10/17] Remove ineffective optimization

[mhagger]

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

I cannot find a scenario in which this function is called any
significant number of times, so simplify the code by always allocating
an array for return_refs rather than trying to use a stack-allocated
array for small lists.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c47090d..ca1ddd9 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -527,17 +527,10 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
-	struct ref *fastarray[32];
 	int head_pos;
 
-	if (*nr_heads && !args.fetch_all) {
-		if (ARRAY_SIZE(fastarray) < *nr_heads)
-			return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
-		else {
-			return_refs = fastarray;
-			memset(return_refs, 0, sizeof(struct ref *) * *nr_heads);
-		}
-	}
+	if (*nr_heads && !args.fetch_all)
+		return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
 	else
 		return_refs = NULL;
 
@@ -584,8 +577,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 				newtail = &ref->next;
 			}
 		}
-		if (return_refs != fastarray)
-			free(return_refs);
+		free(return_refs);
 	}
 	*refs = newlist;
 }
-- 
1.7.11.3

[PATCH v2 11/17] filter_refs(): do not leave gaps in return_refs

[mhagger]

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

It used to be that this function processed refnames in some arbitrary
order but wanted to return them in the order that they were requested,
not the order that they were processed.  Now, the refnames are
processed in sorted order, so there is no reason to go to the extra
effort.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index ca1ddd9..a995357 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -527,14 +527,13 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
-	int head_pos;
+	int head_pos = 0, matched = 0;
 
 	if (*nr_heads && !args.fetch_all)
 		return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
 	else
 		return_refs = NULL;
 
-	head_pos = 0;
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
@@ -554,7 +553,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 				if (cmp < 0) /* definitely do not have it */
 					break;
 				else if (cmp == 0) { /* definitely have it */
-					return_refs[head_pos] = ref;
+					return_refs[matched++] = ref;
 					heads[head_pos++][0] = '\0';
 					break;
 				}
@@ -569,13 +568,11 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 
 	if (!args.fetch_all) {
 		int i;
-		for (i = 0; i < *nr_heads; i++) {
+		for (i = 0; i < matched; i++) {
 			ref = return_refs[i];
-			if (ref) {
-				*newtail = ref;
-				ref->next = NULL;
-				newtail = &ref->next;
-			}
+			*newtail = ref;
+			ref->next = NULL;
+			newtail = &ref->next;
 		}
 		free(return_refs);
 	}
-- 
1.7.11.3

[PATCH v2 12/17] filter_refs(): compress unmatched refs in heads array

[mhagger]

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

Remove any references that were received from the remote from the list
(*nr_heads, heads) of requested references by squeezing them out of
the list (rather than overwriting their names with NUL characters, as
before).  On exit, *nr_heads is the number of requested references
that were not received.

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 | 21 +++++++++++++--------
 fetch-pack.h         |  6 ++++++
 transport.c          |  3 ++-
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index a995357..5ba1cef 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -527,7 +527,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
-	int head_pos = 0, matched = 0;
+	int head_pos = 0, matched = 0, unmatched = 0;
 
 	if (*nr_heads && !args.fetch_all)
 		return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
@@ -554,11 +554,12 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 					break;
 				else if (cmp == 0) { /* definitely have it */
 					return_refs[matched++] = ref;
-					heads[head_pos++][0] = '\0';
+					head_pos++;
 					break;
 				}
-				else /* might have it; keep looking */
-					head_pos++;
+				else { /* might have it; keep looking */
+					heads[unmatched++] = heads[head_pos++];
+				}
 			}
 			if (!cmp)
 				continue; /* we will link it later */
@@ -568,6 +569,12 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 
 	if (!args.fetch_all) {
 		int i;
+
+		/* copy remaining unmatched heads: */
+		while (head_pos < *nr_heads)
+			heads[unmatched++] = heads[head_pos++];
+		*nr_heads = unmatched;
+
 		for (i = 0; i < matched; i++) {
 			ref = return_refs[i];
 			*newtail = ref;
@@ -1028,10 +1035,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		 * 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]);
-				ret = 1;
-			}
+			error("no such remote ref %s", heads[i]);
+		ret = 1;
 	}
 	while (ref) {
 		printf("%s %s\n",
diff --git a/fetch-pack.h b/fetch-pack.h
index a9d505b..915858e 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -17,6 +17,12 @@ struct fetch_pack_args {
 		stateless_rpc:1;
 };
 
+/*
+ * (*nr_heads, heads) is an array of pointers to the full names of
+ * remote references that should be updated from.  On return, both
+ * will have been changed to list only the names that were not found
+ * on the remote.
+ */
 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 f7bbf58..3c38d44 100644
--- a/transport.c
+++ b/transport.c
@@ -520,6 +520,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	struct git_transport_data *data = transport->data;
 	char **heads = xmalloc(nr_heads * sizeof(*heads));
 	char **origh = xmalloc(nr_heads * sizeof(*origh));
+	int orig_nr_heads = nr_heads;
 	const struct ref *refs;
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
@@ -558,7 +559,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	free_refs(refs_tmp);
 
-	for (i = 0; i < nr_heads; i++)
+	for (i = 0; i < orig_nr_heads; i++)
 		free(origh[i]);
 	free(origh);
 	free(heads);
-- 
1.7.11.3

[PATCH v2 13/17] cmd_fetch_pack: return early if finish_connect() returns an error

[mhagger]

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

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 5ba1cef..f04fd59 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1025,10 +1025,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 && nr_heads) {
+	ret = !ref;
+	if (ref && nr_heads) {
 		/* 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 v2 14/17] Report missing refs even if no existing refs were received

[mhagger]

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

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 f04fd59..00ac3b1 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1028,7 +1028,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	ret = !ref;
-	if (ref && nr_heads) {
+	if (nr_heads) {
 		/* 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 0d4edcb..f78a118 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 v2 15/17] cmd_fetch_pack(): simplify computation of return value

[mhagger]

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

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 00ac3b1..c49dadf 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1027,17 +1027,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	if (finish_connect(conn))
 		return 1;
 
-	ret = !ref;
-	if (nr_heads) {
-		/* 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++)
-			error("no such remote ref %s", heads[i]);
-		ret = 1;
-	}
+	ret = !ref || nr_heads;
+
+	/*
+	 * 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++)
+		error("no such remote ref %s", heads[i]);
 	while (ref) {
 		printf("%s %s\n",
 		       sha1_to_hex(ref->old_sha1), ref->name);
-- 
1.7.11.3

[PATCH v2 16/17] fetch_pack(): free matching heads

[mhagger]

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

fetch_pack() used to delete entries from the input list (*nr_heads,
heads) and drop them on the floor.  (Even earlier versions dropped
some names on the floor and modified others.)  This forced
fetch_refs_via_pack() to create a separate copy of the original list
so that it could free all of the names.

Instead, teach fetch_pack() to free any names that it discards from
the list, and change fetch_refs_via_pack() to free only the remaining
(unmatched) names.

Document the change in the function comment in the header file.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c | 4 +++-
 fetch-pack.h         | 7 ++++---
 transport.c          | 9 +++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c49dadf..1bc4599 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -554,7 +554,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 					break;
 				else if (cmp == 0) { /* definitely have it */
 					return_refs[matched++] = ref;
-					head_pos++;
+					free(heads[head_pos++]);
 					break;
 				}
 				else { /* might have it; keep looking */
@@ -844,6 +844,8 @@ static int remove_duplicates(int nr_heads, char **heads)
 	for (src = dst = 1; src < nr_heads; src++)
 		if (strcmp(heads[src], heads[dst-1]))
 			heads[dst++] = heads[src];
+		else
+			free(heads[src]);
 	return dst;
 }
 
diff --git a/fetch-pack.h b/fetch-pack.h
index 915858e..d1860eb 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -19,9 +19,10 @@ struct fetch_pack_args {
 
 /*
  * (*nr_heads, heads) is an array of pointers to the full names of
- * remote references that should be updated from.  On return, both
- * will have been changed to list only the names that were not found
- * on the remote.
+ * remote references that should be updated from.  The names must have
+ * been allocated from the heap.  The names that are found in the
+ * remote will be removed from the list and freed; the others will be
+ * left in the front of the array with *nr_heads adjusted accordingly.
  */
 struct ref *fetch_pack(struct fetch_pack_args *args,
 		       int fd[], struct child_process *conn,
diff --git a/transport.c b/transport.c
index 3c38d44..f94b753 100644
--- a/transport.c
+++ b/transport.c
@@ -519,8 +519,6 @@ static int fetch_refs_via_pack(struct transport *transport,
 {
 	struct git_transport_data *data = transport->data;
 	char **heads = xmalloc(nr_heads * sizeof(*heads));
-	char **origh = xmalloc(nr_heads * sizeof(*origh));
-	int orig_nr_heads = nr_heads;
 	const struct ref *refs;
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
@@ -539,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.depth = data->options.depth;
 
 	for (i = 0; i < nr_heads; i++)
-		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
+		heads[i] = xstrdup(to_fetch[i]->name);
 
 	if (!data->got_remote_heads) {
 		connect_setup(transport, 0, 0);
@@ -559,9 +557,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	free_refs(refs_tmp);
 
-	for (i = 0; i < orig_nr_heads; i++)
-		free(origh[i]);
-	free(origh);
+	for (i = 0; i < nr_heads; i++)
+		free(heads[i]);
 	free(heads);
 	free(dest);
 	return (refs ? 0 : -1);
-- 
1.7.11.3

[PATCH v2 17/17] filter_refs(): simplify logic

[mhagger]

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

* Build linked list of return values as we go rather than recording
  them in a temporary array and linking them up later.

* Handle ref in a single if...else statement in the main loop, to make
  it clear that each ref has exactly two possible destinies.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c | 56 ++++++++++++++++++----------------------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 1bc4599..db77ee6 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -523,66 +523,48 @@ static void mark_recent_complete_commits(unsigned long cutoff)
 
 static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
 {
-	struct ref **return_refs;
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	struct ref *ref, *next;
-	int head_pos = 0, matched = 0, unmatched = 0;
-
-	if (*nr_heads && !args.fetch_all)
-		return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
-	else
-		return_refs = NULL;
+	int head_pos = 0, unmatched = 0;
 
 	for (ref = *refs; ref; ref = next) {
+		int keep_ref = 0;
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
 		    check_refname_format(ref->name, 0))
 			; /* trash */
 		else if (args.fetch_all &&
-			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
-			*newtail = ref;
-			ref->next = NULL;
-			newtail = &ref->next;
-			continue;
-		}
-		else {
-			int cmp = -1;
+			   (!args.depth || prefixcmp(ref->name, "refs/tags/")))
+			keep_ref = 1;
+		else
 			while (head_pos < *nr_heads) {
-				cmp = strcmp(ref->name, heads[head_pos]);
-				if (cmp < 0) /* definitely do not have it */
+				int cmp = strcmp(ref->name, heads[head_pos]);
+				if (cmp < 0) { /* definitely do not have it */
 					break;
-				else if (cmp == 0) { /* definitely have it */
-					return_refs[matched++] = ref;
+				} else if (cmp == 0) { /* definitely have it */
 					free(heads[head_pos++]);
+					keep_ref = 1;
 					break;
-				}
-				else { /* might have it; keep looking */
+				} else { /* might have it; keep looking */
 					heads[unmatched++] = heads[head_pos++];
 				}
 			}
-			if (!cmp)
-				continue; /* we will link it later */
-		}
-		free(ref);
-	}
-
-	if (!args.fetch_all) {
-		int i;
 
-		/* copy remaining unmatched heads: */
-		while (head_pos < *nr_heads)
-			heads[unmatched++] = heads[head_pos++];
-		*nr_heads = unmatched;
-
-		for (i = 0; i < matched; i++) {
-			ref = return_refs[i];
+		if (keep_ref) {
 			*newtail = ref;
 			ref->next = NULL;
 			newtail = &ref->next;
+		} else {
+			free(ref);
 		}
-		free(return_refs);
 	}
+
+	/* copy any remaining unmatched heads: */
+	while (head_pos < *nr_heads)
+		heads[unmatched++] = heads[head_pos++];
+	*nr_heads = unmatched;
+
 	*refs = newlist;
 }
 
-- 
1.7.11.3

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

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> Re-roll, incorporating Jeff's suggestions.  Some commit messages have
> also been improved, but the only interdiff is that match_pos is
> renamed to head_pos in filter_refs().
>
> This patch series applies to the merge between master and
> jc/maint-push-refs-all, though the dependency on the latter is only
> textual.

I've read this twice and saw nothing questionable in the series
(other than that the titles of a few weren't clear enough when
placed in a short-log like below).  Very nicely done.

Thanks.  Will queue.

> Michael Haggerty (17):
>   t5500: add tests of error output for missing refs
>   Rename static function fetch_pack() to http_fetch_pack()
>   Fix formatting.
>   Name local variables more consistently
>   Do not check the same head_pos twice
>   Let fetch_pack() inform caller about number of unique heads
>   Pass nr_heads to do_pack_ref() by reference
>   Pass nr_heads to everything_local() by reference
>   Pass nr_heads to filter_refs() by reference
>   Remove ineffective optimization
>   filter_refs(): do not leave gaps in return_refs
>   filter_refs(): compress unmatched refs in heads array
>   cmd_fetch_pack: return early if finish_connect() returns an error
>   Report missing refs even if no existing refs were received
>   cmd_fetch_pack(): simplify computation of return value
>   fetch_pack(): free matching heads
>   filter_refs(): simplify logic
>
>  builtin/fetch-pack.c  | 128 ++++++++++++++++++++------------------------------
>  fetch-pack.h          |  19 +++++---
>  http-walker.c         |   4 +-
>  t/t5500-fetch-pack.sh |  32 ++++++++++++-
>  transport.c           |   8 ++--
>  5 files changed, 101 insertions(+), 90 deletions(-)