{"thread":{"id":"31329","subject":"[PATCH 01/17] t5500: add tests of error output for missing refs","startedAt":"2012-08-23T08:10:25Z","lastAt":"2012-09-02T07:02:17Z","messageCount":40,"participants":["mhagger@alum.mit.edu","Jeff King","Philip Oakley","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":17},"messages":[{"id":"197683","messageId":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":null,"subject":"[PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:25Z","receivedAt":"2012-08-23T08:10:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nThere were various confusing things (and a couple of bugs) in the way\nthat fetch_pack() handled the list (nr_heads, heads) of references to\nbe sought from the remote:\n\n* Different names were used for the list in different functions for no\n special reason.\n\n* fetch_pack() modified the list in-place:\n\n * It moved entries around\n\n * It sometimes shrunk the list without informing the caller (which\n could lead to spurious error messages)\n\n * It overwrote the first byte of matching entries with NUL, leaving\n a sparse list that the caller had to interpret\n\n * Its interaction with the list was documented\n\n* No error was reported if *all* requested references were missing\n from the remote.\n\nI'm still suspicious about the logic related to args.fetch_all and\nargs.depth, but I don't think I've made anything worse.\n\nThis patch series applies to the merge between master and\njc/maint-push-refs-all, though the dependency on the latter is only\ntextual.\n\nMichael Haggerty (17):\n t5500: add tests of error output for missing refs\n Rename static function fetch_pack() to http_fetch_pack()\n Fix formatting.\n Name local variables more consistently\n Do not check the same match_pos twice\n Let fetch_pack() inform caller about number of unique heads\n Pass nr_heads to do_pack_ref() by reference\n Pass nr_heads to everything_local() by reference\n Pass nr_heads to filter_refs() by reference\n Remove ineffective optimization\n filter_refs(): do not leave gaps in return_refs\n filter_refs(): compress unmatched refs in heads array\n cmd_fetch_pack: return early if finish_connect() returns an error\n Report missing refs even if no existing refs were received\n cmd_fetch_pack(): simplify computation of return value\n fetch_pack(): free matching heads\n fetch_refs(): simplify logic\n\n builtin/fetch-pack.c | 128 ++++++++++++++++++++------------------------------\n fetch-pack.h | 19 +++++---\n http-walker.c | 4 +-\n t/t5500-fetch-pack.sh | 32 ++++++++++++-\n transport.c | 8 ++--\n 5 files changed, 101 insertions(+), 90 deletions(-)\n\n-- \n1.7.11.3\n"},{"id":"197681","messageId":"1345709442-16046-2-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/17] t5500: add tests of error output for missing refs","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:26Z","receivedAt":"2012-08-23T08:10:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nIf \"git fetch-pack\" is called with reference names that do not exist\non the remote, then it should emit an error message\n\n error: no such remote ref refs/heads/xyzzy\n\nThis is currently broken if *only* missing references are passed to\n\"git fetch-pack\".\n\nSigned-off-by: Michael Haggerty \n---\n t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex e80a2af..3cc3346 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -397,4 +397,34 @@ test_expect_success 'test duplicate refs from stdin' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'set up tests of missing reference' '\n+\tcat >expect-error <<-\\EOF\n+\terror: no such remote ref refs/heads/xyzzy\n+\tEOF\n+'\n+\n+test_expect_failure 'test lonely missing ref' '\n+\t(\n+\tcd client &&\n+\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy\n+\t) >/dev/null 2>error-m &&\n+\ttest_cmp expect-error error-m\n+'\n+\n+test_expect_success 'test missing ref after existing' '\n+\t(\n+\tcd client &&\n+\ttest_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy\n+\t) >/dev/null 2>error-em &&\n+\ttest_cmp expect-error error-em\n+'\n+\n+test_expect_success 'test missing ref before existing' '\n+\t(\n+\tcd client &&\n+\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A\n+\t) >/dev/null 2>error-me &&\n+\ttest_cmp expect-error error-me\n+'\n+\n test_done\n-- \n1.7.11.3\n"},{"id":"197684","messageId":"1345709442-16046-3-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/17] Rename static function fetch_pack() to http_fetch_pack()","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:27Z","receivedAt":"2012-08-23T08:10:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nAvoid confusion with the non-static function of the same name from\nfetch-pack.h.\n\nSigned-off-by: Michael Haggerty \n---\n http-walker.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 51a906e..1516c5e 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -396,7 +396,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)\n \treturn ret;\n }\n \n-static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1)\n+static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1)\n {\n \tstruct packed_git *target;\n \tint ret;\n@@ -524,7 +524,7 @@ static int fetch(struct walker *walker, unsigned char *sha1)\n \tif (!fetch_object(walker, altbase, sha1))\n \t\treturn 0;\n \twhile (altbase) {\n-\t\tif (!fetch_pack(walker, altbase, sha1))\n+\t\tif (!http_fetch_pack(walker, altbase, sha1))\n \t\t\treturn 0;\n \t\tfetch_alternates(walker, data->alt->base);\n \t\taltbase = altbase->next;\n-- \n1.7.11.3\n"},{"id":"197685","messageId":"1345709442-16046-4-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/17] Fix formatting.","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:28Z","receivedAt":"2012-08-23T08:10:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 8 ++++----\n fetch-pack.h | 12 ++++++------\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 7912d2b..cc21047 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1062,10 +1062,10 @@ static int compare_heads(const void *a, const void *b)\n struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t int fd[], struct child_process *conn,\n \t\t const struct ref *ref,\n-\t\tconst char *dest,\n-\t\tint nr_heads,\n-\t\tchar **heads,\n-\t\tchar **pack_lockfile)\n+\t\t const char *dest,\n+\t\t int nr_heads,\n+\t\t char **heads,\n+\t\t char **pack_lockfile)\n {\n \tstruct stat st;\n \tstruct ref *ref_cpy;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 7c2069c..1dbe90f 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -18,11 +18,11 @@ struct fetch_pack_args {\n };\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-\t\tint fd[], struct child_process *conn,\n-\t\tconst struct ref *ref,\n-\t\tconst char *dest,\n-\t\tint nr_heads,\n-\t\tchar **heads,\n-\t\tchar **pack_lockfile);\n+\t\t int fd[], struct child_process *conn,\n+\t\t const struct ref *ref,\n+\t\t const char *dest,\n+\t\t int nr_heads,\n+\t\t char **heads,\n+\t\t char **pack_lockfile);\n \n #endif\n-- \n1.7.11.3\n"},{"id":"197689","messageId":"1345709442-16046-5-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/17] Name local variables more consistently","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:29Z","receivedAt":"2012-08-23T08:10:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nUse the names (nr_heads, heads) consistently across functions, instead\nof sometimes naming the same values (nr_match, match).\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex cc21047..f11545e 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n \t}\n }\n \n-static void filter_refs(struct ref **refs, int nr_match, char **match)\n+static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n {\n \tstruct ref **return_refs;\n \tstruct ref *newlist = NULL;\n@@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n \tstruct ref *fastarray[32];\n \tint match_pos;\n \n-\tif (nr_match && !args.fetch_all) {\n-\t\tif (ARRAY_SIZE(fastarray) < nr_match)\n-\t\t\treturn_refs = xcalloc(nr_match, sizeof(struct ref *));\n+\tif (nr_heads && !args.fetch_all) {\n+\t\tif (ARRAY_SIZE(fastarray) < nr_heads)\n+\t\t\treturn_refs = xcalloc(nr_heads, sizeof(struct ref *));\n \t\telse {\n \t\t\treturn_refs = fastarray;\n-\t\t\tmemset(return_refs, 0, sizeof(struct ref *) * nr_match);\n+\t\t\tmemset(return_refs, 0, sizeof(struct ref *) * nr_heads);\n \t\t}\n \t}\n \telse\n@@ -556,12 +556,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n \t\t}\n \t\telse {\n \t\t\tint cmp = -1;\n-\t\t\twhile (match_pos < nr_match) {\n-\t\t\t\tcmp = strcmp(ref->name, match[match_pos]);\n+\t\t\twhile (match_pos < nr_heads) {\n+\t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n \t\t\t\tif (cmp < 0) /* definitely do not have it */\n \t\t\t\t\tbreak;\n \t\t\t\telse if (cmp == 0) { /* definitely have it */\n-\t\t\t\t\tmatch[match_pos][0] = '\\0';\n+\t\t\t\t\theads[match_pos][0] = '\\0';\n \t\t\t\t\treturn_refs[match_pos] = ref;\n \t\t\t\t\tbreak;\n \t\t\t\t}\n@@ -576,7 +576,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n \n \tif (!args.fetch_all) {\n \t\tint i;\n-\t\tfor (i = 0; i < nr_match; i++) {\n+\t\tfor (i = 0; i < nr_heads; i++) {\n \t\t\tref = return_refs[i];\n \t\t\tif (ref) {\n \t\t\t\t*newtail = ref;\n@@ -595,7 +595,7 @@ static void mark_alternate_complete(const struct ref *ref, void *unused)\n \tmark_complete(NULL, ref->old_sha1, 0, NULL);\n }\n \n-static int everything_local(struct ref **refs, int nr_match, char **match)\n+static int everything_local(struct ref **refs, int nr_heads, char **heads)\n {\n \tstruct ref *ref;\n \tint retval;\n@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)\n \t\t}\n \t}\n \n-\tfilter_refs(refs, nr_match, match);\n+\tfilter_refs(refs, nr_heads, heads);\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n@@ -777,8 +777,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \n static struct ref *do_fetch_pack(int fd[2],\n \t\tconst struct ref *orig_ref,\n-\t\tint nr_match,\n-\t\tchar **match,\n+\t\tint nr_heads, char **heads,\n \t\tchar **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -819,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (everything_local(&ref, nr_match, match)) {\n+\tif (everything_local(&ref, nr_heads, heads)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n-- \n1.7.11.3\n"},{"id":"197686","messageId":"1345709442-16046-6-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/17] Do not check the same match_pos twice","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:30Z","receivedAt":"2012-08-23T08:10:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nOnce a match has been found at match_pos, the entry is zeroed and no\nfuture attempts will match that entry. So increment match_pos to\navoid checking against the zeroed-out entry during the next iteration.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex f11545e..a6406e7 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -561,8 +561,8 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n \t\t\t\tif (cmp < 0) /* definitely do not have it */\n \t\t\t\t\tbreak;\n \t\t\t\telse if (cmp == 0) { /* definitely have it */\n-\t\t\t\t\theads[match_pos][0] = '\\0';\n \t\t\t\t\treturn_refs[match_pos] = ref;\n+\t\t\t\t\theads[match_pos++][0] = '\\0';\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\telse /* might have it; keep looking */\n-- \n1.7.11.3\n"},{"id":"197687","messageId":"1345709442-16046-7-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/17] Let fetch_pack() inform caller about number of unique heads","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:31Z","receivedAt":"2012-08-23T08:10:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nfetch_pack() remotes duplicates from the list (nr_heads, heads),\nthereby shrinking the list. But previously, the caller was not\ninformed about the shrinkage. This would cause a spurious error\nmessage to be emitted by cmd_fetch_pack() if \"git fetch-pack\" is\ncalled with duplicate refnames.\n\nSo change the signature of fetch_pack() to accept nr_heads by\nreference, and if any duplicates were removed then modify it to\nreflect the number of remaining references.\n\nThe last test of t5500 inexplicably *required* \"git fetch-pack\" to\nfail when fetching a list of references that contains duplicates;\ni.e., it insisted on the buggy behavior. So change the test to expect\nthe correct behavior.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 12 ++++++------\n fetch-pack.h | 2 +-\n t/t5500-fetch-pack.sh | 2 +-\n transport.c | 2 +-\n 4 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex a6406e7..3c93ec4 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1021,7 +1021,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tget_remote_heads(fd[0], &ref, 0, NULL);\n \n \tref = fetch_pack(&args, fd, conn, ref, dest,\n-\t\tnr_heads, heads, pack_lockfile_ptr);\n+\t\t&nr_heads, heads, pack_lockfile_ptr);\n \tif (pack_lockfile) {\n \t\tprintf(\"lock %s\\n\", pack_lockfile);\n \t\tfflush(stdout);\n@@ -1062,7 +1062,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t int fd[], struct child_process *conn,\n \t\t const struct ref *ref,\n \t\t const char *dest,\n-\t\t int nr_heads,\n+\t\t int *nr_heads,\n \t\t char **heads,\n \t\t char **pack_lockfile)\n {\n@@ -1077,16 +1077,16 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t\tst.st_mtime = 0;\n \t}\n \n-\tif (heads && nr_heads) {\n-\t\tqsort(heads, nr_heads, sizeof(*heads), compare_heads);\n-\t\tnr_heads = remove_duplicates(nr_heads, heads);\n+\tif (heads && *nr_heads) {\n+\t\tqsort(heads, *nr_heads, sizeof(*heads), compare_heads);\n+\t\t*nr_heads = remove_duplicates(*nr_heads, heads);\n \t}\n \n \tif (!ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(\"no matching remote head\");\n \t}\n-\tref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);\n+\tref_cpy = do_fetch_pack(fd, ref, *nr_heads, heads, pack_lockfile);\n \n \tif (args.depth > 0) {\n \t\tstruct cache_time mtime;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 1dbe90f..a9d505b 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -21,7 +21,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t int fd[], struct child_process *conn,\n \t\t const struct ref *ref,\n \t\t const char *dest,\n-\t\t int nr_heads,\n+\t\t int *nr_heads,\n \t\t char **heads,\n \t\t char **pack_lockfile);\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 3cc3346..0d4edcb 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' '\n test_expect_success 'test duplicate refs from stdin' '\n \t(\n \tcd client &&\n-\ttest_must_fail git fetch-pack --stdin --no-progress .. <../input.dup\n+\tgit fetch-pack --stdin --no-progress .. <../input.dup\n \t) >output &&\n \tcut -d \" \" -f 2 actual &&\n \ttest_cmp expect actual\ndiff --git a/transport.c b/transport.c\nindex 1811b50..f7bbf58 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -548,7 +548,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \trefs = fetch_pack(&args, data->fd, data->conn,\n \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t dest, nr_heads, heads, &transport->pack_lockfile);\n+\t\t\t dest, &nr_heads, heads, &transport->pack_lockfile);\n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n \tif (finish_connect(data->conn))\n-- \n1.7.11.3\n"},{"id":"197691","messageId":"1345709442-16046-8-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/17] Pass nr_heads to do_pack_ref() by reference","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:32Z","receivedAt":"2012-08-23T08:10:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nThis is the first of a few baby steps towards changing filter_refs()\nto compress matched refs out of the list rather than overwriting the\nfirst character of such references with '\\0'.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 3c93ec4..d3ff166 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -777,7 +777,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \n static struct ref *do_fetch_pack(int fd[2],\n \t\tconst struct ref *orig_ref,\n-\t\tint nr_heads, char **heads,\n+\t\tint *nr_heads, char **heads,\n \t\tchar **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -818,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (everything_local(&ref, nr_heads, heads)) {\n+\tif (everything_local(&ref, *nr_heads, heads)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -1086,7 +1086,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\tpacket_flush(fd[1]);\n \t\tdie(\"no matching remote head\");\n \t}\n-\tref_cpy = do_fetch_pack(fd, ref, *nr_heads, heads, pack_lockfile);\n+\tref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);\n \n \tif (args.depth > 0) {\n \t\tstruct cache_time mtime;\n-- \n1.7.11.3\n"},{"id":"197695","messageId":"1345709442-16046-9-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/17] Pass nr_heads to everything_local() by reference","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:33Z","receivedAt":"2012-08-23T08:10:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex d3ff166..5905dae 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -595,7 +595,7 @@ static void mark_alternate_complete(const struct ref *ref, void *unused)\n \tmark_complete(NULL, ref->old_sha1, 0, NULL);\n }\n \n-static int everything_local(struct ref **refs, int nr_heads, char **heads)\n+static int everything_local(struct ref **refs, int *nr_heads, char **heads)\n {\n \tstruct ref *ref;\n \tint retval;\n@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int nr_heads, char **heads)\n \t\t}\n \t}\n \n-\tfilter_refs(refs, nr_heads, heads);\n+\tfilter_refs(refs, *nr_heads, heads);\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n@@ -818,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (everything_local(&ref, *nr_heads, heads)) {\n+\tif (everything_local(&ref, nr_heads, heads)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n-- \n1.7.11.3\n"},{"id":"197693","messageId":"1345709442-16046-10-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/17] Pass nr_heads to filter_refs() by reference","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:34Z","receivedAt":"2012-08-23T08:10:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 5905dae..0426cf4 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n \t}\n }\n \n-static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n+static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n {\n \tstruct ref **return_refs;\n \tstruct ref *newlist = NULL;\n@@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n \tstruct ref *fastarray[32];\n \tint match_pos;\n \n-\tif (nr_heads && !args.fetch_all) {\n-\t\tif (ARRAY_SIZE(fastarray) < nr_heads)\n-\t\t\treturn_refs = xcalloc(nr_heads, sizeof(struct ref *));\n+\tif (*nr_heads && !args.fetch_all) {\n+\t\tif (ARRAY_SIZE(fastarray) < *nr_heads)\n+\t\t\treturn_refs = xcalloc(*nr_heads, sizeof(struct ref *));\n \t\telse {\n \t\t\treturn_refs = fastarray;\n-\t\t\tmemset(return_refs, 0, sizeof(struct ref *) * nr_heads);\n+\t\t\tmemset(return_refs, 0, sizeof(struct ref *) * *nr_heads);\n \t\t}\n \t}\n \telse\n@@ -556,7 +556,7 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n \t\t}\n \t\telse {\n \t\t\tint cmp = -1;\n-\t\t\twhile (match_pos < nr_heads) {\n+\t\t\twhile (match_pos < *nr_heads) {\n \t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n \t\t\t\tif (cmp < 0) /* definitely do not have it */\n \t\t\t\t\tbreak;\n@@ -576,7 +576,7 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n \n \tif (!args.fetch_all) {\n \t\tint i;\n-\t\tfor (i = 0; i < nr_heads; i++) {\n+\t\tfor (i = 0; i < *nr_heads; i++) {\n \t\t\tref = return_refs[i];\n \t\t\tif (ref) {\n \t\t\t\t*newtail = ref;\n@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int *nr_heads, char **heads)\n \t\t}\n \t}\n \n-\tfilter_refs(refs, *nr_heads, heads);\n+\tfilter_refs(refs, nr_heads, heads);\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n-- \n1.7.11.3\n"},{"id":"197690","messageId":"1345709442-16046-11-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/17] Remove ineffective optimization","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:35Z","receivedAt":"2012-08-23T08:10:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nI cannot find a scenario in which this function is called any\nsignificant number of times, so simplify the code by always allocating\nan array for return_refs rather than trying to use a stack-allocated\narray for small lists.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 0426cf4..9398059 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -527,17 +527,10 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n \tstruct ref *ref, *next;\n-\tstruct ref *fastarray[32];\n \tint match_pos;\n \n-\tif (*nr_heads && !args.fetch_all) {\n-\t\tif (ARRAY_SIZE(fastarray) < *nr_heads)\n-\t\t\treturn_refs = xcalloc(*nr_heads, sizeof(struct ref *));\n-\t\telse {\n-\t\t\treturn_refs = fastarray;\n-\t\t\tmemset(return_refs, 0, sizeof(struct ref *) * *nr_heads);\n-\t\t}\n-\t}\n+\tif (*nr_heads && !args.fetch_all)\n+\t\treturn_refs = xcalloc(*nr_heads, sizeof(struct ref *));\n \telse\n \t\treturn_refs = NULL;\n \n@@ -584,8 +577,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \t\t\t\tnewtail = &ref->next;\n \t\t\t}\n \t\t}\n-\t\tif (return_refs != fastarray)\n-\t\t\tfree(return_refs);\n+\t\tfree(return_refs);\n \t}\n \t*refs = newlist;\n }\n-- \n1.7.11.3\n"},{"id":"197692","messageId":"1345709442-16046-12-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/17] filter_refs(): do not leave gaps in return_refs","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:36Z","receivedAt":"2012-08-23T08:10:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nIt used to be that this function processed refnames in some arbitrary\norder but wanted to return them in the order that they were requested,\nnot the order that they were processed. Now, the refnames are\nprocessed in sorted order, so there is no reason to go to the extra\neffort.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 9398059..8366012 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -527,14 +527,13 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n \tstruct ref *ref, *next;\n-\tint match_pos;\n+\tint match_pos = 0, matched = 0;\n \n \tif (*nr_heads && !args.fetch_all)\n \t\treturn_refs = xcalloc(*nr_heads, sizeof(struct ref *));\n \telse\n \t\treturn_refs = NULL;\n \n-\tmatch_pos = 0;\n \tfor (ref = *refs; ref; ref = next) {\n \t\tnext = ref->next;\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n@@ -554,7 +553,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \t\t\t\tif (cmp < 0) /* definitely do not have it */\n \t\t\t\t\tbreak;\n \t\t\t\telse if (cmp == 0) { /* definitely have it */\n-\t\t\t\t\treturn_refs[match_pos] = ref;\n+\t\t\t\t\treturn_refs[matched++] = ref;\n \t\t\t\t\theads[match_pos++][0] = '\\0';\n \t\t\t\t\tbreak;\n \t\t\t\t}\n@@ -569,13 +568,11 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \n \tif (!args.fetch_all) {\n \t\tint i;\n-\t\tfor (i = 0; i < *nr_heads; i++) {\n+\t\tfor (i = 0; i < matched; i++) {\n \t\t\tref = return_refs[i];\n-\t\t\tif (ref) {\n-\t\t\t\t*newtail = ref;\n-\t\t\t\tref->next = NULL;\n-\t\t\t\tnewtail = &ref->next;\n-\t\t\t}\n+\t\t\t*newtail = ref;\n+\t\t\tref->next = NULL;\n+\t\t\tnewtail = &ref->next;\n \t\t}\n \t\tfree(return_refs);\n \t}\n-- \n1.7.11.3\n"},{"id":"197697","messageId":"1345709442-16046-13-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/17] filter_refs(): compress unmatched refs in heads array","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:37Z","receivedAt":"2012-08-23T08:10:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nRemove any references that were received from the remote from the list\n(*nr_heads, heads) of requested references by squeezing them out of\nthe list (rather than overwriting their names with NUL characters, as\nbefore). On exit, *nr_heads is the number of requested references\nthat were not received.\n\nDocument this aspect of fetch_pack() in a comment in the header file.\n(More documentation is obviously still needed.)\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 21 +++++++++++++--------\n fetch-pack.h | 6 ++++++\n transport.c | 3 ++-\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 8366012..cc9af80 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -527,7 +527,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n \tstruct ref *ref, *next;\n-\tint match_pos = 0, matched = 0;\n+\tint match_pos = 0, matched = 0, unmatched = 0;\n \n \tif (*nr_heads && !args.fetch_all)\n \t\treturn_refs = xcalloc(*nr_heads, sizeof(struct ref *));\n@@ -554,11 +554,12 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \t\t\t\t\tbreak;\n \t\t\t\telse if (cmp == 0) { /* definitely have it */\n \t\t\t\t\treturn_refs[matched++] = ref;\n-\t\t\t\t\theads[match_pos++][0] = '\\0';\n+\t\t\t\t\tmatch_pos++;\n \t\t\t\t\tbreak;\n \t\t\t\t}\n-\t\t\t\telse /* might have it; keep looking */\n-\t\t\t\t\tmatch_pos++;\n+\t\t\t\telse { /* might have it; keep looking */\n+\t\t\t\t\theads[unmatched++] = heads[match_pos++];\n+\t\t\t\t}\n \t\t\t}\n \t\t\tif (!cmp)\n \t\t\t\tcontinue; /* we will link it later */\n@@ -568,6 +569,12 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \n \tif (!args.fetch_all) {\n \t\tint i;\n+\n+\t\t/* copy remaining unmatched heads: */\n+\t\twhile (match_pos < *nr_heads)\n+\t\t\theads[unmatched++] = heads[match_pos++];\n+\t\t*nr_heads = unmatched;\n+\n \t\tfor (i = 0; i < matched; i++) {\n \t\t\tref = return_refs[i];\n \t\t\t*newtail = ref;\n@@ -1028,10 +1035,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t * silently succeed without issuing an error.\n \t\t */\n \t\tfor (i = 0; i < nr_heads; i++)\n-\t\t\tif (heads[i] && heads[i][0]) {\n-\t\t\t\terror(\"no such remote ref %s\", heads[i]);\n-\t\t\t\tret = 1;\n-\t\t\t}\n+\t\t\terror(\"no such remote ref %s\", heads[i]);\n+\t\tret = 1;\n \t}\n \twhile (ref) {\n \t\tprintf(\"%s %s\\n\",\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex a9d505b..915858e 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -17,6 +17,12 @@ struct fetch_pack_args {\n \t\tstateless_rpc:1;\n };\n \n+/*\n+ * (*nr_heads, heads) is an array of pointers to the full names of\n+ * remote references that should be updated from. On return, both\n+ * will have been changed to list only the names that were not found\n+ * on the remote.\n+ */\n struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t int fd[], struct child_process *conn,\n \t\t const struct ref *ref,\ndiff --git a/transport.c b/transport.c\nindex f7bbf58..3c38d44 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -520,6 +520,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tstruct git_transport_data *data = transport->data;\n \tchar **heads = xmalloc(nr_heads * sizeof(*heads));\n \tchar **origh = xmalloc(nr_heads * sizeof(*origh));\n+\tint orig_nr_heads = nr_heads;\n \tconst struct ref *refs;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n@@ -558,7 +559,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tfree_refs(refs_tmp);\n \n-\tfor (i = 0; i < nr_heads; i++)\n+\tfor (i = 0; i < orig_nr_heads; i++)\n \t\tfree(origh[i]);\n \tfree(origh);\n \tfree(heads);\n-- \n1.7.11.3\n"},{"id":"197688","messageId":"1345709442-16046-14-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/17] cmd_fetch_pack: return early if finish_connect() returns an error","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:38Z","receivedAt":"2012-08-23T08:10:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nThis simplifies the logic without changing the behavior.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex cc9af80..4794839 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1025,10 +1025,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tclose(fd[0]);\n \tclose(fd[1]);\n \tif (finish_connect(conn))\n-\t\tref = NULL;\n-\tret = !ref;\n+\t\treturn 1;\n \n-\tif (!ret && nr_heads) {\n+\tret = !ref;\n+\tif (ref && nr_heads) {\n \t\t/* If the heads to pull were given, we should have\n \t\t * consumed all of them by matching the remote.\n \t\t * Otherwise, 'git fetch remote no-such-ref' would\n-- \n1.7.11.3\n"},{"id":"197698","messageId":"1345709442-16046-15-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/17] Report missing refs even if no existing refs were received","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:39Z","receivedAt":"2012-08-23T08:10:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nThis fixes a test in t5500.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 2 +-\n t/t5500-fetch-pack.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4794839..5b1e603 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1028,7 +1028,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\treturn 1;\n \n \tret = !ref;\n-\tif (ref && nr_heads) {\n+\tif (nr_heads) {\n \t\t/* If the heads to pull were given, we should have\n \t\t * consumed all of them by matching the remote.\n \t\t * Otherwise, 'git fetch remote no-such-ref' would\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 0d4edcb..f78a118 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -403,7 +403,7 @@ test_expect_success 'set up tests of missing reference' '\n \tEOF\n '\n \n-test_expect_failure 'test lonely missing ref' '\n+test_expect_success 'test lonely missing ref' '\n \t(\n \tcd client &&\n \ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy\n-- \n1.7.11.3\n"},{"id":"197682","messageId":"1345709442-16046-16-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 15/17] cmd_fetch_pack(): simplify computation of return value","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:40Z","receivedAt":"2012-08-23T08:10:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nSet the final value at initialization rather than initializing it then\nsometimes changing it.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 5b1e603..6c1a48e 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1027,17 +1027,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tif (finish_connect(conn))\n \t\treturn 1;\n \n-\tret = !ref;\n-\tif (nr_heads) {\n-\t\t/* If the heads to pull were given, we should have\n-\t\t * consumed all of them by matching the remote.\n-\t\t * Otherwise, 'git fetch remote no-such-ref' would\n-\t\t * silently succeed without issuing an error.\n-\t\t */\n-\t\tfor (i = 0; i < nr_heads; i++)\n-\t\t\terror(\"no such remote ref %s\", heads[i]);\n-\t\tret = 1;\n-\t}\n+\tret = !ref || nr_heads;\n+\n+\t/*\n+\t * If the heads to pull were given, we should have consumed\n+\t * all of them by matching the remote. Otherwise, 'git fetch\n+\t * remote no-such-ref' would silently succeed without issuing\n+\t * an error.\n+\t */\n+\tfor (i = 0; i < nr_heads; i++)\n+\t\terror(\"no such remote ref %s\", heads[i]);\n \twhile (ref) {\n \t\tprintf(\"%s %s\\n\",\n \t\t sha1_to_hex(ref->old_sha1), ref->name);\n-- \n1.7.11.3\n"},{"id":"197696","messageId":"1345709442-16046-17-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 16/17] fetch_pack(): free matching heads","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:41Z","receivedAt":"2012-08-23T08:10:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nfetch_pack() used to delete entries from the input list (*nr_heads,\nheads) and drop them on the floor. (Even earlier versions dropped\nsome names on the floor and modified others.) This forced\nfetch_refs_via_pack() to create a separate copy of the original list\nso that it could free the all of the names.\n\nInstead, teach fetch_pack() to free any names that it discards from\nthe list, and change fetch_refs_via_pack() to free only the remaining\n(unmatched) names.\n\nDocument the change in the function comment in the header file.\n\nSigned-off-by: Michael Haggerty \n---\n\nThis change forces callers to allocate names on the heap. But the two\nexisting callers did so already, and the function already modified the\nlist, so I think the new style is no more intrusive than the old.\n\n builtin/fetch-pack.c | 4 +++-\n fetch-pack.h | 7 ++++---\n transport.c | 9 +++------\n 3 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 6c1a48e..9650d04 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -554,7 +554,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n \t\t\t\t\tbreak;\n \t\t\t\telse if (cmp == 0) { /* definitely have it */\n \t\t\t\t\treturn_refs[matched++] = ref;\n-\t\t\t\t\tmatch_pos++;\n+\t\t\t\t\tfree(heads[match_pos++]);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\telse { /* might have it; keep looking */\n@@ -844,6 +844,8 @@ static int remove_duplicates(int nr_heads, char **heads)\n \tfor (src = dst = 1; src < nr_heads; src++)\n \t\tif (strcmp(heads[src], heads[dst-1]))\n \t\t\theads[dst++] = heads[src];\n+\t\telse\n+\t\t\tfree(heads[src]);\n \treturn dst;\n }\n \ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 915858e..d1860eb 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -19,9 +19,10 @@ struct fetch_pack_args {\n \n /*\n * (*nr_heads, heads) is an array of pointers to the full names of\n- * remote references that should be updated from. On return, both\n- * will have been changed to list only the names that were not found\n- * on the remote.\n+ * remote references that should be updated from. The names must have\n+ * been allocated from the heap. The names that are found in the\n+ * remote will be removed from the list and freed; the others will be\n+ * left in the front of the array with *nr_heads adjusted accordingly.\n */\n struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t int fd[], struct child_process *conn,\ndiff --git a/transport.c b/transport.c\nindex 3c38d44..f94b753 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -519,8 +519,6 @@ static int fetch_refs_via_pack(struct transport *transport,\n {\n \tstruct git_transport_data *data = transport->data;\n \tchar **heads = xmalloc(nr_heads * sizeof(*heads));\n-\tchar **origh = xmalloc(nr_heads * sizeof(*origh));\n-\tint orig_nr_heads = nr_heads;\n \tconst struct ref *refs;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n@@ -539,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.depth = data->options.depth;\n \n \tfor (i = 0; i < nr_heads; i++)\n-\t\torigh[i] = heads[i] = xstrdup(to_fetch[i]->name);\n+\t\theads[i] = xstrdup(to_fetch[i]->name);\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n@@ -559,9 +557,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tfree_refs(refs_tmp);\n \n-\tfor (i = 0; i < orig_nr_heads; i++)\n-\t\tfree(origh[i]);\n-\tfree(origh);\n+\tfor (i = 0; i < nr_heads; i++)\n+\t\tfree(heads[i]);\n \tfree(heads);\n \tfree(dest);\n \treturn (refs ? 0 : -1);\n-- \n1.7.11.3\n"},{"id":"197694","messageId":"1345709442-16046-18-git-send-email-mhagger@alum.mit.edu","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 17/17] fetch_refs(): simplify logic","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-23T08:10:42Z","receivedAt":"2012-08-23T08:10:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\n* Build linked list of return values as we go rather than recording\n them in a temporary array and linking them up later.\n\n* Handle ref in a single if...else statement in the main loop, to make\n it clear that each ref has exactly two possible destinies.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 56 ++++++++++++++++++----------------------------------\n 1 file changed, 19 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 9650d04..90683ca 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -523,66 +523,48 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n \n static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n {\n-\tstruct ref **return_refs;\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n \tstruct ref *ref, *next;\n-\tint match_pos = 0, matched = 0, unmatched = 0;\n-\n-\tif (*nr_heads && !args.fetch_all)\n-\t\treturn_refs = xcalloc(*nr_heads, sizeof(struct ref *));\n-\telse\n-\t\treturn_refs = NULL;\n+\tint match_pos = 0, unmatched = 0;\n \n \tfor (ref = *refs; ref; ref = next) {\n+\t\tint keep_ref = 0;\n \t\tnext = ref->next;\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n \t\t check_refname_format(ref->name, 0))\n \t\t\t; /* trash */\n \t\telse if (args.fetch_all &&\n-\t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\n-\t\t\t*newtail = ref;\n-\t\t\tref->next = NULL;\n-\t\t\tnewtail = &ref->next;\n-\t\t\tcontinue;\n-\t\t}\n-\t\telse {\n-\t\t\tint cmp = -1;\n+\t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\")))\n+\t\t\tkeep_ref = 1;\n+\t\telse\n \t\t\twhile (match_pos < *nr_heads) {\n-\t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n-\t\t\t\tif (cmp < 0) /* definitely do not have it */\n+\t\t\t\tint cmp = strcmp(ref->name, heads[match_pos]);\n+\t\t\t\tif (cmp < 0) { /* definitely do not have it */\n \t\t\t\t\tbreak;\n-\t\t\t\telse if (cmp == 0) { /* definitely have it */\n-\t\t\t\t\treturn_refs[matched++] = ref;\n+\t\t\t\t} else if (cmp == 0) { /* definitely have it */\n \t\t\t\t\tfree(heads[match_pos++]);\n+\t\t\t\t\tkeep_ref = 1;\n \t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t\telse { /* might have it; keep looking */\n+\t\t\t\t} else { /* might have it; keep looking */\n \t\t\t\t\theads[unmatched++] = heads[match_pos++];\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (!cmp)\n-\t\t\t\tcontinue; /* we will link it later */\n-\t\t}\n-\t\tfree(ref);\n-\t}\n-\n-\tif (!args.fetch_all) {\n-\t\tint i;\n \n-\t\t/* copy remaining unmatched heads: */\n-\t\twhile (match_pos < *nr_heads)\n-\t\t\theads[unmatched++] = heads[match_pos++];\n-\t\t*nr_heads = unmatched;\n-\n-\t\tfor (i = 0; i < matched; i++) {\n-\t\t\tref = return_refs[i];\n+\t\tif (keep_ref) {\n \t\t\t*newtail = ref;\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n+\t\t} else {\n+\t\t\tfree(ref);\n \t\t}\n-\t\tfree(return_refs);\n \t}\n+\n+\t/* copy any remaining unmatched heads: */\n+\twhile (match_pos < *nr_heads)\n+\t\theads[unmatched++] = heads[match_pos++];\n+\t*nr_heads = unmatched;\n+\n \t*refs = newlist;\n }\n \n-- \n1.7.11.3\n"},{"id":"197701","messageId":"20120823083918.GB6963@sigill.intra.peff.net","threadId":"31329","inReplyTo":"1345709442-16046-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T08:39:18Z","receivedAt":"2012-08-23T08:39:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:\n\n> From: Michael Haggerty \n> \n> Use the names (nr_heads, heads) consistently across functions, instead\n> of sometimes naming the same values (nr_match, match).\n\nI think this is fine, although:\n\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n> \t}\n> }\n> \n> -static void filter_refs(struct ref **refs, int nr_match, char **match)\n> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n> {\n> \tstruct ref **return_refs;\n> \tstruct ref *newlist = NULL;\n> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n> \tstruct ref *fastarray[32];\n> \tint match_pos;\n\nThis match_pos is an index into the \"match\" array, which becomes \"head\".\nShould it become head_pos?\n\nAnd then bits like this:\n\n> -\t\t\twhile (match_pos < nr_match) {\n> -\t\t\t\tcmp = strcmp(ref->name, match[match_pos]);\n> +\t\t\twhile (match_pos < nr_heads) {\n> +\t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n\nWould be:\n\n while (head_pos < nr_heads)\n\nwhich makes more sense to me.\n\n-Peff\n"},{"id":"197702","messageId":"20120823084213.GC6963@sigill.intra.peff.net","threadId":"31329","inReplyTo":"1345709442-16046-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/17] Do not check the same match_pos twice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T08:42:15Z","receivedAt":"2012-08-23T08:42:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 10:10:30AM +0200, mhagger@alum.mit.edu wrote:\n\n> From: Michael Haggerty \n> \n> Once a match has been found at match_pos, the entry is zeroed and no\n> future attempts will match that entry. So increment match_pos to\n> avoid checking against the zeroed-out entry during the next iteration.\n\nGood catch.\n\nA subtle side effect of this zero-ing (not introduced by your patch, but\nsomething I noticed while re-reading the code) is that we implicitly\neliminate duplicate entries from the list of remote refs. There\nshouldn't generally be any duplicates, of course, but I think skipping\nthem is probably sane.\n\n-Peff\n"},{"id":"197703","messageId":"20120823085409.GD6963@sigill.intra.peff.net","threadId":"31329","inReplyTo":"1345709442-16046-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T08:54:10Z","receivedAt":"2012-08-23T08:54:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhagger@alum.mit.edu wrote:\n\n> From: Michael Haggerty \n> \n> fetch_pack() remotes duplicates from the list (nr_heads, heads),\n> thereby shrinking the list. But previously, the caller was not\n> informed about the shrinkage. This would cause a spurious error\n> message to be emitted by cmd_fetch_pack() if \"git fetch-pack\" is\n> called with duplicate refnames.\n> \n> So change the signature of fetch_pack() to accept nr_heads by\n> reference, and if any duplicates were removed then modify it to\n> reflect the number of remaining references.\n> \n> The last test of t5500 inexplicably *required* \"git fetch-pack\" to\n> fail when fetching a list of references that contains duplicates;\n> i.e., it insisted on the buggy behavior. So change the test to expect\n> the correct behavior.\n\nEek, yeah, the current behavior is obviously wrong. The\nremove_duplicates code comes from 310b86d (fetch-pack: do not barf when\nduplicate re patterns are given, 2006-11-25) and clearly meant for\nfetch-pack to handle this case gracefully.\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 3cc3346..0d4edcb 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' '\n> test_expect_success 'test duplicate refs from stdin' '\n> \t(\n> \tcd client &&\n> -\ttest_must_fail git fetch-pack --stdin --no-progress .. <../input.dup\n> +\tgit fetch-pack --stdin --no-progress .. <../input.dup\n> \t) >output &&\n> \tcut -d \" \" -f 2 actual &&\n> \ttest_cmp expect actual\n\nIt's interesting that the output was the same before and after the fix.\nI guess that is because the error comes at the very end, when we are\nmaking sure all of the provided heads have been consumed.\n\n-Peff\n"},{"id":"197704","messageId":"20120823090439.GE6963@sigill.intra.peff.net","threadId":"31329","inReplyTo":"1345709442-16046-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 16/17] fetch_pack(): free matching heads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T09:04:39Z","receivedAt":"2012-08-23T09:04:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 10:10:41AM +0200, mhagger@alum.mit.edu wrote:\n\n> From: Michael Haggerty \n> \n> fetch_pack() used to delete entries from the input list (*nr_heads,\n> heads) and drop them on the floor. (Even earlier versions dropped\n> some names on the floor and modified others.) This forced\n> fetch_refs_via_pack() to create a separate copy of the original list\n> so that it could free the all of the names.\n\nMinor typo: s/the all/all/.\n\n-Peff\n"},{"id":"197705","messageId":"20120823090740.GF6963@sigill.intra.peff.net","threadId":"31329","inReplyTo":"1345709442-16046-18-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 17/17] fetch_refs(): simplify logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T09:07:40Z","receivedAt":"2012-08-23T09:07:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 10:10:42AM +0200, mhagger@alum.mit.edu wrote:\n\n> Subject: Re: [PATCH 17/17] fetch_refs(): simplify logic\n> [...]\n> static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n\nThe subject should be \"filter_refs\", no?\n\n-Peff\n"},{"id":"197707","messageId":"20120823092624.GG6963@sigill.intra.peff.net","threadId":"31329","inReplyTo":"1345709442-16046-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T09:26:24Z","receivedAt":"2012-08-23T09:26:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 10:10:25AM +0200, mhagger@alum.mit.edu wrote:\n\n> There were various confusing things (and a couple of bugs) in the way\n> that fetch_pack() handled the list (nr_heads, heads) of references to\n> be sought from the remote:\n\nAside from the minor comments I made to individual patches, this all\nlooks good. As usual, thanks for breaking it down; I wish all series\nwere as easy to review as this.\n\n> I'm still suspicious about the logic related to args.fetch_all and\n> args.depth, but I don't think I've made anything worse.\n\nI think the point of that is that when doing \"git fetch-pack --all\n--depth=1\", the meaning of \"--all\" is changed from \"all refs\" to\n\"everything but tags\".\n\nWhich I kind of see the point of, because you don't want to grab ancient\ntags that will be expensive. But wouldn't it make more sense to limit it\nonly to the contents of refs/heads in that case? Surely you wouldn't\nwant refs/notes, refs/remotes, or other hierarchies.\n\nI suspect this code is never even run at all these days. All of the\ncallers inside git should actually provide a real list of refs, not\n\"--all\". So it is really historical cruft for anybody who calls the\nfetch-pack plumbing (I wonder if any third-party callers even exist;\nthis is such a deep part of the network infrastructure that any sane\nscripts would probably just be calling fetch).\n\n-Peff\n"},{"id":"197730","messageId":"49DEA56853C64B9B900D3C7F886D4FA6@PhilipOakley","threadId":"31329","inReplyTo":"20120823092624.GG6963@sigill.intra.peff.net","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2012-08-23T19:13:29Z","receivedAt":"2012-08-23T19:13:29Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \nSent: Thursday, August 23, 2012 10:26 AM\n> On Thu, Aug 23, 2012 at 10:10:25AM +0200, mhagger@alum.mit.edu wrote:\n>\n>> There were various confusing things (and a couple of bugs) in the way\n>> that fetch_pack() handled the list (nr_heads, heads) of references to\n>> be sought from the remote:\n>\n> Aside from the minor comments I made to individual patches, this all\n> looks good. As usual, thanks for breaking it down; I wish all series\n> were as easy to review as this.\n>\n>> I'm still suspicious about the logic related to args.fetch_all and\n>> args.depth, but I don't think I've made anything worse.\n>\n> I think the point of that is that when doing \"git fetch-pack --all\n> --depth=1\", the meaning of \"--all\" is changed from \"all refs\" to\n> \"everything but tags\".\n>\n\nThere is a comment in \\git\\Documentation\\technical\\shallow.txt that\n\"- If you deepen a history, you'd want to get the tags of the\n newly stored (but older!) commits. This does not work right now.\"\nwhich may be the source of this restriction. That is, how is the depth\nof the tag fetching to be restricted to the requested depth count?\n[assuming I've undestood the problem correctly]\n\nIt may be (?) that it is a good time to think about a 'datedepth' \ncapability to bypass the current counted-depth shallow fetch that can \ncause so much trouble. With a date limited depth the relevant tags could \nalso be fetched.\n\n> Which I kind of see the point of, because you don't want to grab \n> ancient\n> tags that will be expensive. But wouldn't it make more sense to limit \n> it\n> only to the contents of refs/heads in that case? Surely you wouldn't\n> want refs/notes, refs/remotes, or other hierarchies.\n>\n> I suspect this code is never even run at all these days. All of the\n> callers inside git should actually provide a real list of refs, not\n> \"--all\". So it is really historical cruft for anybody who calls the\n> fetch-pack plumbing (I wonder if any third-party callers even exist;\n> this is such a deep part of the network infrastructure that any sane\n> scripts would probably just be calling fetch).\n>\n> -Peff\n> --\n> \n"},{"id":"197731","messageId":"20120823195648.GB15268@sigill.intra.peff.net","threadId":"31329","inReplyTo":"49DEA56853C64B9B900D3C7F886D4FA6@PhilipOakley","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T19:56:48Z","receivedAt":"2012-08-23T19:56:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 08:13:29PM +0100, Philip Oakley wrote:\n\n> >>I'm still suspicious about the logic related to args.fetch_all and\n> >>args.depth, but I don't think I've made anything worse.\n> >\n> >I think the point of that is that when doing \"git fetch-pack --all\n> >--depth=1\", the meaning of \"--all\" is changed from \"all refs\" to\n> >\"everything but tags\".\n> >\n> \n> There is a comment in \\git\\Documentation\\technical\\shallow.txt that\n> \"- If you deepen a history, you'd want to get the tags of the\n> newly stored (but older!) commits. This does not work right now.\"\n> which may be the source of this restriction. That is, how is the depth\n> of the tag fetching to be restricted to the requested depth count?\n> [assuming I've undestood the problem correctly]\n\nI don't think this is about deepening, but rather about making sure we\nremain shallow for the initial fetch. Remember that this is on the\n\"fetch-pack --all\" code path, which used to be used by \"git clone\" when\nit was a shell script (these days, clone is a C builtin and will\nactually feed the list of refs to fetch-pack).\n\nThis code blames back to:\n\n commit 4bcb310c2539b66d535e87508d1b7a90fe29c083\n Author: Alexandre Julliard \n Date: Fri Nov 24 16:00:13 2006 +0100\n\n fetch-pack: Do not fetch tags for shallow clones.\n\n A better fix may be to only fetch tags that point to commits that we\n are downloading, but git-clone doesn't have support for following\n tags. This will happen automatically on the next git-fetch though.\n\nSo it is about making sure that \"git clone --depth=1\" does not\naccidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,\nlosing the purpose of using --depth in the first place. These days it is\nlargely irrelevant, since this code path is not followed by clone, and\nclone will automatically restrict its list of fetched refs to a single\nbranch if --depth is used.\n\nThe bug that shallow.txt talks about (and which is mentioned in that\ncommit message) is that we will not properly auto-follow tags during\nsuch a clone (i.e., when we fetch a tag because it is pointing to a\ncommit that we already have or are already pulling). I'm not sure if\nthat is still the case or not. But assuming your workflow is something\nlike:\n\n [make an initial, cheap clone]\n git clone --depth=1 $repo\n\n [the next day, you do a regular fetch, which will just get new stuff\n on top of what you already have]\n git fetch\n\nThen that second fetch will auto-follow the tags, anyway. And that is\nwhat the commit message is pointing: it's a bug, but one you can work\naround.\n\n> It may be (?) that it is a good time to think about a 'datedepth'\n> capability to bypass the current counted-depth shallow fetch that can\n> cause so much trouble. With a date limited depth the relevant tags\n> could also be fetched.\n\nI don't have anything against such an idea, but I think it is orthogonal\nto the issue being discussed here.\n\n-Peff\n"},{"id":"197732","messageId":"20120823203152.GA1810@sigill.intra.peff.net","threadId":"31329","inReplyTo":"20120823195648.GB15268@sigill.intra.peff.net","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-23T20:31:53Z","receivedAt":"2012-08-23T20:31:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2012 at 03:56:48PM -0400, Jeff King wrote:\n\n> This code blames back to:\n> \n> commit 4bcb310c2539b66d535e87508d1b7a90fe29c083\n> Author: Alexandre Julliard \n> Date: Fri Nov 24 16:00:13 2006 +0100\n> \n> fetch-pack: Do not fetch tags for shallow clones.\n> \n> A better fix may be to only fetch tags that point to commits that we\n> are downloading, but git-clone doesn't have support for following\n> tags. This will happen automatically on the next git-fetch though.\n> \n> So it is about making sure that \"git clone --depth=1\" does not\n> accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,\n> losing the purpose of using --depth in the first place. These days it is\n> largely irrelevant, since this code path is not followed by clone, and\n> clone will automatically restrict its list of fetched refs to a single\n> branch if --depth is used.\n\nI think part of the confusion of this code is that inside the loop over\nthe refs it is sometimes checking aspects of the ref, and sometimes\nchecking invariants of the loop (like args.fetch_all). Splitting it into\nseparate loops makes it easier to see what is going on, like the patch\nbelow (on top of Michael's series).\n\nI'm not sure if it ends up being more readable, since the generic \"cut\ndown this linked list\" function has to operate through callbacks with a\nvoid pointer. On the other hand, that function could also be used\nelsewhere.\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 90683ca..877cf38 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -521,51 +521,80 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n \t}\n }\n \n-static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n+static void filter_refs_callback(struct ref **refs,\n+\t\t\t\t int (*want)(struct ref *, void *),\n+\t\t\t\t void *data)\n {\n-\tstruct ref *newlist = NULL;\n-\tstruct ref **newtail = &newlist;\n+\tstruct ref **tail = refs;\n \tstruct ref *ref, *next;\n-\tint match_pos = 0, unmatched = 0;\n \n \tfor (ref = *refs; ref; ref = next) {\n-\t\tint keep_ref = 0;\n \t\tnext = ref->next;\n-\t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n-\t\t check_refname_format(ref->name, 0))\n-\t\t\t; /* trash */\n-\t\telse if (args.fetch_all &&\n-\t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\")))\n-\t\t\tkeep_ref = 1;\n-\t\telse\n-\t\t\twhile (match_pos < *nr_heads) {\n-\t\t\t\tint cmp = strcmp(ref->name, heads[match_pos]);\n-\t\t\t\tif (cmp < 0) { /* definitely do not have it */\n-\t\t\t\t\tbreak;\n-\t\t\t\t} else if (cmp == 0) { /* definitely have it */\n-\t\t\t\t\tfree(heads[match_pos++]);\n-\t\t\t\t\tkeep_ref = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t} else { /* might have it; keep looking */\n-\t\t\t\t\theads[unmatched++] = heads[match_pos++];\n-\t\t\t\t}\n-\t\t\t}\n-\n-\t\tif (keep_ref) {\n-\t\t\t*newtail = ref;\n-\t\t\tref->next = NULL;\n-\t\t\tnewtail = &ref->next;\n-\t\t} else {\n+\t\tif (want(ref, data))\n+\t\t\ttail = &ref->next;\n+\t\telse {\n \t\t\tfree(ref);\n+\t\t\t*tail = next;\n \t\t}\n \t}\n+}\n \n-\t/* copy any remaining unmatched heads: */\n-\twhile (match_pos < *nr_heads)\n-\t\theads[unmatched++] = heads[match_pos++];\n-\t*nr_heads = unmatched;\n+static int ref_name_is_ok(struct ref *ref, void *data)\n+{\n+\treturn memcmp(ref->name, \"refs/\", 5) ||\n+\t\t!check_refname_format(ref->name, 0);\n+}\n+\n+static int ref_ok_for_shallow(struct ref *ref, void *data)\n+{\n+\treturn prefixcmp(ref->name, \"refs/tags/\");\n+}\n \n-\t*refs = newlist;\n+struct filter_by_name_data {\n+\tchar **heads;\n+\tint nr_heads;\n+\tint match_pos;\n+\tint unmatched;\n+};\n+\n+static int want_ref_name(struct ref *ref, void *data)\n+{\n+\tstruct filter_by_name_data *f = data;\n+\n+\twhile (f->match_pos < f->nr_heads) {\n+\t\tint cmp = strcmp(ref->name, f->heads[f->match_pos]);\n+\t\tif (cmp < 0) /* definitely do not have it */\n+\t\t\treturn 0;\n+\t\telse if (cmp == 0) { /* definitely have it */\n+\t\t\tfree(f->heads[f->match_pos++]);\n+\t\t\treturn 1;\n+\t\t} else /* might have it; keep looking */\n+\t\t\tf->heads[f->unmatched++] = f->heads[f->match_pos++];\n+\t}\n+\treturn 0;\n+}\n+\n+static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n+{\n+\tstruct filter_by_name_data f;\n+\n+\tfilter_refs_callback(refs, ref_name_is_ok, NULL);\n+\n+\tif (args.fetch_all) {\n+\t\tif (args.depth)\n+\t\t\tfilter_refs_callback(refs, ref_ok_for_shallow, NULL);\n+\t\treturn;\n+\t}\n+\n+\tmemset(&f, 0, sizeof(f));\n+\tf.heads = heads;\n+\tf.nr_heads = *nr_heads;\n+\tfilter_refs_callback(refs, want_ref_name, &f);\n+\n+\t/* copy any remaining unmatched heads: */\n+\twhile (f.match_pos < f.nr_heads)\n+\t\theads[f.unmatched++] = heads[f.match_pos++];\n+\t*nr_heads = f.unmatched;\n }\n \n static void mark_alternate_complete(const struct ref *ref, void *unused)\n"},{"id":"197740","messageId":"40BA082AE16542A8B5E1F83167322A1B@PhilipOakley","threadId":"31329","inReplyTo":"20120823195648.GB15268@sigill.intra.peff.net","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2012-08-23T22:09:27Z","receivedAt":"2012-08-23T22:09:27Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \nSent: Thursday, August 23, 2012 8:56 PM\n> On Thu, Aug 23, 2012 at 08:13:29PM +0100, Philip Oakley wrote:\n>\n>> >>I'm still suspicious about the logic related to args.fetch_all and\n>> >>args.depth, but I don't think I've made anything worse.\n>> >\n>> >I think the point of that is that when doing \"git fetch-pack --all\n>> >--depth=1\", the meaning of \"--all\" is changed from \"all refs\" to\n>> >\"everything but tags\".\n>> >\n>>\n>> There is a comment in \\git\\Documentation\\technical\\shallow.txt that\n>> \"- If you deepen a history, you'd want to get the tags of the\n>> newly stored (but older!) commits. This does not work right now.\"\n>> which may be the source of this restriction. That is, how is the \n>> depth\n>> of the tag fetching to be restricted to the requested depth count?\n>> [assuming I've undestood the problem correctly]\n>\n> I don't think this is about deepening, but rather about making sure we\n> remain shallow for the initial fetch. Remember that this is on the\n> \"fetch-pack --all\" code path, which used to be used by \"git clone\" \n> when\n> it was a shell script (these days, clone is a C builtin and will\n> actually feed the list of refs to fetch-pack).\n\nOK\n\n>\n> This code blames back to:\n>\n> commit 4bcb310c2539b66d535e87508d1b7a90fe29c083\n> Author: Alexandre Julliard \n> Date: Fri Nov 24 16:00:13 2006 +0100\n>\n> fetch-pack: Do not fetch tags for shallow clones.\n>\n> A better fix may be to only fetch tags that point to commits that \n> we\n> are downloading, but git-clone doesn't have support for following\n> tags. This will happen automatically on the next git-fetch though.\n>\n> So it is about making sure that \"git clone --depth=1\" does not\n> accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,\n> losing the purpose of using --depth in the first place. These days it \n> is\n> largely irrelevant, since this code path is not followed by clone, and\n> clone will automatically restrict its list of fetched refs to a single\n> branch if --depth is used.\n>\n> The bug that shallow.txt talks about (and which is mentioned in that\n> commit message) is that we will not properly auto-follow tags during\n> such a clone (i.e., when we fetch a tag because it is pointing to a\n> commit that we already have or are already pulling). I'm not sure if\n> that is still the case or not. But assuming your workflow is something\n> like:\n>\n> [make an initial, cheap clone]\n> git clone --depth=1 $repo\n>\n> [the next day, you do a regular fetch, which will just get new stuff\n> on top of what you already have]\n> git fetch\n>\n> Then that second fetch will auto-follow the tags, anyway. And that is\n> what the commit message is pointing: it's a bug, but one you can work\n> around.\n\nI hadn't appreciated that the fetch would limit itself to the original \nshallow\ndepth. I'd gained the impression that one need to use the --depth to\ncontrol what was being fetched.\n\n>\n>> It may be (?) that it is a good time to think about a 'datedepth'\n>> capability to bypass the current counted-depth shallow fetch that can\n>> cause so much trouble. With a date limited depth the relevant tags\n>> could also be fetched.\n>\n> I don't have anything against such an idea, but I think it is \n> orthogonal\n> to the issue being discussed here.\nOK. I'll stick it on my slow-burner pile ;-)\n\n>\n> -Peff\n"},{"id":"197743","messageId":"7vd32g3ol6.fsf@alter.siamese.dyndns.org","threadId":"31329","inReplyTo":"20120823195648.GB15268@sigill.intra.peff.net","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-24T04:23:33Z","receivedAt":"2012-08-24T04:23:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> It may be (?) that it is a good time to think about a 'datedepth'\n>> capability to bypass the current counted-depth shallow fetch that can\n>> cause so much trouble. With a date limited depth the relevant tags\n>> could also be fetched.\n>\n> I don't have anything against such an idea, but I think it is orthogonal\n> to the issue being discussed here.\n\nCorrect.\n\nThe biggest problem with the \"shallow\" hack is that the deepening\nfetch counts from the tip of the refs at the time of deepening when\ndeepening the history (i.e. \"clone --depth\", followed by number of\n\"fetch\", followed by \"fetch --depth\"). If you start from a shallow\nclone of depth 1, repeated fetch to keep up while the history grew\nby 100, you would still have a connected history down to the initial\ncauterization point, and \"fetch --depth=200\" would give you a\nhistory that is deeper than your original clone by 100 commits. But\nif you start from the same shallow clone of depth 1, did not do\nanything while the history grew by 100, and then decide to fetch\nagain with \"fetch --depth=20\", it does not grow. It just makes\n20-commit deep history from the updated tip, and leave the commit\nfrom your original clone dangling.\n\nThe problem with \"depth\" does not have anything to do with how it is\nspecified at the UI level. The end-user input is used to find out\nat what set of commits the history is cauterized, and once they are\ncomputed, the \"shallow\" logic solely works on \"is the history before\nthese cauterization points, or after, in topological terms.\" (and it\nhas to be that way to make sure we get reproducible results). Even\nif a new way to specify these cauterization points in terms of date\nis introduced, it does not change anything and does not solve the\nfundamental problem the code has when deepening.\n"},{"id":"197752","messageId":"503727A8.3040600@alum.mit.edu","threadId":"31329","inReplyTo":"20120823083918.GB6963@sigill.intra.peff.net","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-24T07:05:12Z","receivedAt":"2012-08-24T07:05:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/23/2012 10:39 AM, Jeff King wrote:\n> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:\n> \n>> From: Michael Haggerty \n>>\n>> Use the names (nr_heads, heads) consistently across functions, instead\n>> of sometimes naming the same values (nr_match, match).\n> \n> I think this is fine, although:\n> \n>> --- a/builtin/fetch-pack.c\n>> +++ b/builtin/fetch-pack.c\n>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n>> \t}\n>> }\n>> \n>> -static void filter_refs(struct ref **refs, int nr_match, char **match)\n>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n>> {\n>> \tstruct ref **return_refs;\n>> \tstruct ref *newlist = NULL;\n>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n>> \tstruct ref *fastarray[32];\n>> \tint match_pos;\n> \n> This match_pos is an index into the \"match\" array, which becomes \"head\".\n> Should it become head_pos?\n> \n> And then bits like this:\n> \n>> -\t\t\twhile (match_pos < nr_match) {\n>> -\t\t\t\tcmp = strcmp(ref->name, match[match_pos]);\n>> +\t\t\twhile (match_pos < nr_heads) {\n>> +\t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n> \n> Would be:\n> \n> while (head_pos < nr_heads)\n> \n> which makes more sense to me.\n\nI was up in the air about this, because match_pos *is* the position at\nwhich a match is attempted. But since the name also strikes you as\nwrong, I will change it in the next version.\n\nThanks for this and all of your other comments!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197769","messageId":"DA390375EB8D4F21ADE618B88F1F0C4C@PhilipOakley","threadId":"31329","inReplyTo":"7vd32g3ol6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2012-08-24T12:46:18Z","receivedAt":"2012-08-24T12:46:18Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \nSent: Friday, August 24, 2012 5:23 AM\n> Jeff King writes:\n>\n>>> It may be (?) that it is a good time to think about a 'datedepth'\n>>> capability to bypass the current counted-depth shallow fetch that \n>>> can\n>>> cause so much trouble. With a date limited depth the relevant tags\n>>> could also be fetched.\n>>\n>> I don't have anything against such an idea, but I think it is \n>> orthogonal\n>> to the issue being discussed here.\n>\n> Correct.\n>\n> The biggest problem with the \"shallow\" hack is that the deepening\n> fetch counts from the tip of the refs at the time of deepening when\n> deepening the history (i.e. \"clone --depth\", followed by number of\n> \"fetch\", followed by \"fetch --depth\"). If you start from a shallow\n> clone of depth 1, repeated fetch to keep up while the history grew\n> by 100, you would still have a connected history down to the initial\n> cauterization point, and \"fetch --depth=200\" would give you a\n> history that is deeper than your original clone by 100 commits. But\n> if you start from the same shallow clone of depth 1, did not do\n> anything while the history grew by 100, and then decide to fetch\n> again with \"fetch --depth=20\", it does not grow. It just makes\n> 20-commit deep history from the updated tip, and leave the commit\n> from your original clone dangling.\n>\n> The problem with \"depth\" does not have anything to do with how it is\n> specified at the UI level.\n\nThat is, correct me if I'm wrong, the server currently lacks a \ncapability to provide anything other than the counted depth shallow \nresponse (if available). Hence my comment about needing an additional \nserver side capability, though that would need a well thought out set of \nuse cases to make it useful.\n\n> The end-user input is used to \n> find out\n> at what set of commits the history is cauterized, and once they are\n> computed, the \"shallow\" logic solely works on \"is the history before\n> these cauterization points, or after, in topological terms.\" (and it\n> has to be that way to make sure we get reproducible results). Even\n> if a new way to specify these cauterization points in terms of date\n> is introduced, it does not change anything and does not solve the\n> fundamental problem the code has when deepening.\n\nfundamental problem = no server capability other than counted depth.\n>\n"},{"id":"197808","messageId":"50385D1B.8010404@alum.mit.edu","threadId":"31329","inReplyTo":"20120823085409.GD6963@sigill.intra.peff.net","subject":"Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-25T05:05:31Z","receivedAt":"2012-08-25T05:05:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/23/2012 10:54 AM, Jeff King wrote:\n> On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhagger@alum.mit.edu wrote:\n> \n>> From: Michael Haggerty \n>>\n>> fetch_pack() remotes duplicates from the list (nr_heads, heads),\n>> thereby shrinking the list. But previously, the caller was not\n>> informed about the shrinkage. This would cause a spurious error\n>> message to be emitted by cmd_fetch_pack() if \"git fetch-pack\" is\n>> called with duplicate refnames.\n>>\n>> So change the signature of fetch_pack() to accept nr_heads by\n>> reference, and if any duplicates were removed then modify it to\n>> reflect the number of remaining references.\n>>\n>> The last test of t5500 inexplicably *required* \"git fetch-pack\" to\n>> fail when fetching a list of references that contains duplicates;\n>> i.e., it insisted on the buggy behavior. So change the test to expect\n>> the correct behavior.\n> \n> Eek, yeah, the current behavior is obviously wrong. The\n> remove_duplicates code comes from 310b86d (fetch-pack: do not barf when\n> duplicate re patterns are given, 2006-11-25) and clearly meant for\n> fetch-pack to handle this case gracefully.\n> \n>> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n>> index 3cc3346..0d4edcb 100755\n>> --- a/t/t5500-fetch-pack.sh\n>> +++ b/t/t5500-fetch-pack.sh\n>> @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' '\n>> test_expect_success 'test duplicate refs from stdin' '\n>> \t(\n>> \tcd client &&\n>> -\ttest_must_fail git fetch-pack --stdin --no-progress .. <../input.dup\n>> +\tgit fetch-pack --stdin --no-progress .. <../input.dup\n>> \t) >output &&\n>> \tcut -d \" \" -f 2 actual &&\n>> \ttest_cmp expect actual\n> \n> It's interesting that the output was the same before and after the fix.\n> I guess that is because the error comes at the very end, when we are\n> making sure all of the provided heads have been consumed.\n\n\"git fetch-pack\" emits information about successfully-received\nreferences regardless of whether some requested references were not\nreceived. The \"no such remote ref %s\" output goes to stderr. So the\nonly difference between before/after fix should be what is written to\nstderr, whereas the test only looks at stdout.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197810","messageId":"5038729B.3000501@alum.mit.edu","threadId":"31329","inReplyTo":"20120823090740.GF6963@sigill.intra.peff.net","subject":"Re: [PATCH 17/17] fetch_refs(): simplify logic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-25T06:37:15Z","receivedAt":"2012-08-25T06:37:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/23/2012 11:07 AM, Jeff King wrote:\n> On Thu, Aug 23, 2012 at 10:10:42AM +0200, mhagger@alum.mit.edu wrote:\n> \n>> Subject: Re: [PATCH 17/17] fetch_refs(): simplify logic\n>> [...]\n>> static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n> \n> The subject should be \"filter_refs\", no?\n\nYes, thanks. Also thanks for the typo correction in [PATCH 16/17], and\nin general for your review of the patch series. Re-roll to follow shortly.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197829","messageId":"50387936.1000205@alum.mit.edu","threadId":"31329","inReplyTo":"20120823203152.GA1810@sigill.intra.peff.net","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-25T07:05:26Z","receivedAt":"2012-08-25T07:05:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/23/2012 10:31 PM, Jeff King wrote:\n> I think part of the confusion of this code is that inside the loop over\n> the refs it is sometimes checking aspects of the ref, and sometimes\n> checking invariants of the loop (like args.fetch_all). Splitting it into\n> separate loops makes it easier to see what is going on, like the patch\n> below (on top of Michael's series).\n> \n> I'm not sure if it ends up being more readable, since the generic \"cut\n> down this linked list\" function has to operate through callbacks with a\n> void pointer. On the other hand, that function could also be used\n> elsewhere.\n> [...]\n\nDespite requiring a bit more boilerplate, I think that your change makes\nthe logic clearer.\n\n*If* we want to switch to using callbacks, then we can get even more\nbang for the buck, as in the attached patch (which applies on top of my\npatch v2). Beyond your suggestion, this patch:\n\n* Inlines your filter_refs() into everything_local(), because (a) it's\nshort and (b) the policy work implemented there logically belongs\nhigher-up in the call chain.\n\n* Renames your filter_refs_callback() to filter_refs().\n\n* Moves the initialization of the filter_by_name_data structure\n(including sorting and de-duping) all the way up to fetch_pack(), and\npasses a filter_by_name_data* (rather than (nr_heads, heads)) down to\nthe callees.\n\nIf you like this change, let me know and I'll massage it into a\ndigestible patch series.\n\nMichael\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex db77ee6..d1dabcf 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -521,51 +521,91 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n \t}\n }\n \n-static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n+static void filter_refs(struct ref **refs,\n+\t\t\tint (*want)(struct ref *, void *),\n+\t\t\tvoid *data)\n {\n-\tstruct ref *newlist = NULL;\n-\tstruct ref **newtail = &newlist;\n+\tstruct ref **tail = refs;\n \tstruct ref *ref, *next;\n-\tint head_pos = 0, unmatched = 0;\n \n \tfor (ref = *refs; ref; ref = next) {\n-\t\tint keep_ref = 0;\n \t\tnext = ref->next;\n-\t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n-\t\t check_refname_format(ref->name, 0))\n-\t\t\t; /* trash */\n-\t\telse if (args.fetch_all &&\n-\t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\")))\n-\t\t\tkeep_ref = 1;\n-\t\telse\n-\t\t\twhile (head_pos < *nr_heads) {\n-\t\t\t\tint cmp = strcmp(ref->name, heads[head_pos]);\n-\t\t\t\tif (cmp < 0) { /* definitely do not have it */\n-\t\t\t\t\tbreak;\n-\t\t\t\t} else if (cmp == 0) { /* definitely have it */\n-\t\t\t\t\tfree(heads[head_pos++]);\n-\t\t\t\t\tkeep_ref = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t} else { /* might have it; keep looking */\n-\t\t\t\t\theads[unmatched++] = heads[head_pos++];\n-\t\t\t\t}\n-\t\t\t}\n-\n-\t\tif (keep_ref) {\n-\t\t\t*newtail = ref;\n-\t\t\tref->next = NULL;\n-\t\t\tnewtail = &ref->next;\n-\t\t} else {\n+\t\tif (want(ref, data))\n+\t\t\ttail = &ref->next;\n+\t\telse {\n \t\t\tfree(ref);\n+\t\t\t*tail = next;\n \t\t}\n \t}\n+}\n \n-\t/* copy any remaining unmatched heads: */\n-\twhile (head_pos < *nr_heads)\n-\t\theads[unmatched++] = heads[head_pos++];\n-\t*nr_heads = unmatched;\n+static int ref_name_is_ok(struct ref *ref, void *data)\n+{\n+\treturn memcmp(ref->name, \"refs/\", 5) ||\n+\t\t!check_refname_format(ref->name, 0);\n+}\n+\n+static int ref_ok_for_shallow(struct ref *ref, void *data)\n+{\n+\treturn prefixcmp(ref->name, \"refs/tags/\");\n+}\n+\n+static int compare_heads(const void *a, const void *b)\n+{\n+\treturn strcmp(*(const char **)a, *(const char **)b);\n+}\n+\n+static int remove_duplicates(int nr_heads, char **heads)\n+{\n+\tint src, dst;\n+\tfor (src = dst = 1; src < nr_heads; src++)\n+\t\tif (strcmp(heads[src], heads[dst-1]))\n+\t\t\theads[dst++] = heads[src];\n+\t\telse\n+\t\t\tfree(heads[src]);\n+\treturn dst;\n+}\n \n-\t*refs = newlist;\n+struct filter_by_name_data {\n+\tchar **heads;\n+\tint *nr_heads;\n+\tint head_pos;\n+\tint unmatched;\n+};\n+\n+static void filter_by_name_init(struct filter_by_name_data *f,\n+\t\t\t int *nr_heads, char **heads)\n+{\n+\tmemset(f, 0, sizeof(*f));\n+\tf->heads = heads;\n+\tf->nr_heads = nr_heads;\n+\tqsort(f->heads, *f->nr_heads, sizeof(*f->heads), compare_heads);\n+\t*f->nr_heads = remove_duplicates(*f->nr_heads, f->heads);\n+}\n+\n+static int filter_by_name_want(struct ref *ref, void *data)\n+{\n+\tstruct filter_by_name_data *f = data;\n+\n+\twhile (f->head_pos < *f->nr_heads) {\n+\t\tint cmp = strcmp(ref->name, f->heads[f->head_pos]);\n+\t\tif (cmp < 0) /* definitely do not have it */\n+\t\t\treturn 0;\n+\t\telse if (cmp == 0) { /* definitely have it */\n+\t\t\tfree(f->heads[f->head_pos++]);\n+\t\t\treturn 1;\n+\t\t} else /* might have it; keep looking */\n+\t\t\tf->heads[f->unmatched++] = f->heads[f->head_pos++];\n+\t}\n+\treturn 0;\n+}\n+\n+static void filter_by_name_finish(struct filter_by_name_data *f)\n+{\n+\t/* copy any remaining unmatched heads: */\n+\twhile (f->head_pos < *f->nr_heads)\n+\t\tf->heads[f->unmatched++] = f->heads[f->head_pos++];\n+\t*f->nr_heads = f->unmatched;\n }\n \n static void mark_alternate_complete(const struct ref *ref, void *unused)\n@@ -573,7 +613,8 @@ static void mark_alternate_complete(const struct ref *ref, void *unused)\n \tmark_complete(NULL, ref->old_sha1, 0, NULL);\n }\n \n-static int everything_local(struct ref **refs, int *nr_heads, char **heads)\n+static int everything_local(struct ref **refs,\n+\t\t\t struct filter_by_name_data *filter_by_name_data)\n {\n \tstruct ref *ref;\n \tint retval;\n@@ -624,7 +665,14 @@ static int everything_local(struct ref **refs, int *nr_heads, char **heads)\n \t\t}\n \t}\n \n-\tfilter_refs(refs, nr_heads, heads);\n+\tfilter_refs(refs, ref_name_is_ok, NULL);\n+\n+\tif (args.fetch_all) {\n+\t\tif (args.depth)\n+\t\t\tfilter_refs(refs, ref_ok_for_shallow, NULL);\n+\t} else if (filter_by_name_data) {\n+\t\tfilter_refs(refs, filter_by_name_want, filter_by_name_data);\n+\t}\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n@@ -754,9 +802,9 @@ static int get_pack(int xd[2], char **pack_lockfile)\n }\n \n static struct ref *do_fetch_pack(int fd[2],\n-\t\tconst struct ref *orig_ref,\n-\t\tint *nr_heads, char **heads,\n-\t\tchar **pack_lockfile)\n+\t\t\t\t const struct ref *orig_ref,\n+\t\t\t\t struct filter_by_name_data *filter_by_name_data,\n+\t\t\t\t char **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tunsigned char sha1[20];\n@@ -796,7 +844,7 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (everything_local(&ref, nr_heads, heads)) {\n+\tif (everything_local(&ref, filter_by_name_data)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -816,21 +864,6 @@ static struct ref *do_fetch_pack(int fd[2],\n \treturn ref;\n }\n \n-static int remove_duplicates(int nr_heads, char **heads)\n-{\n-\tint src, dst;\n-\n-\tif (!nr_heads)\n-\t\treturn 0;\n-\n-\tfor (src = dst = 1; src < nr_heads; src++)\n-\t\tif (strcmp(heads[src], heads[dst-1]))\n-\t\t\theads[dst++] = heads[src];\n-\t\telse\n-\t\t\tfree(heads[src]);\n-\treturn dst;\n-}\n-\n static int fetch_pack_config(const char *var, const char *value, void *cb)\n {\n \tif (strcmp(var, \"fetch.unpacklimit\") == 0) {\n@@ -1030,11 +1063,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \treturn ret;\n }\n \n-static int compare_heads(const void *a, const void *b)\n-{\n-\treturn strcmp(*(const char **)a, *(const char **)b);\n-}\n-\n struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t int fd[], struct child_process *conn,\n \t\t const struct ref *ref,\n@@ -1045,6 +1073,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n {\n \tstruct stat st;\n \tstruct ref *ref_cpy;\n+\tstruct filter_by_name_data name_data, *filter_by_name_data;\n \n \tfetch_pack_setup();\n \tif (&args != my_args)\n@@ -1054,16 +1083,20 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t\tst.st_mtime = 0;\n \t}\n \n-\tif (heads && *nr_heads) {\n-\t\tqsort(heads, *nr_heads, sizeof(*heads), compare_heads);\n-\t\t*nr_heads = remove_duplicates(*nr_heads, heads);\n-\t}\n-\n \tif (!ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(\"no matching remote head\");\n \t}\n-\tref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);\n+\tif (heads && *nr_heads) {\n+\t\tfilter_by_name_data = &name_data;\n+\t\tfilter_by_name_init(filter_by_name_data, nr_heads, heads);\n+\t} else\n+\t\tfilter_by_name_data = NULL;\n+\n+\tref_cpy = do_fetch_pack(fd, ref, filter_by_name_data, pack_lockfile);\n+\n+\tif (filter_by_name_data)\n+\t\tfilter_by_name_finish(filter_by_name_data);\n \n \tif (args.depth > 0) {\n \t\tstruct cache_time mtime;\n"},{"id":"197863","messageId":"7vvcg5v9hh.fsf@alter.siamese.dyndns.org","threadId":"31329","inReplyTo":"20120823083918.GB6963@sigill.intra.peff.net","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-26T17:39:06Z","receivedAt":"2012-08-26T17:39:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:\n>\n>> From: Michael Haggerty \n>> \n>> Use the names (nr_heads, heads) consistently across functions, instead\n>> of sometimes naming the same values (nr_match, match).\n>\n> I think this is fine, although:\n>\n>> --- a/builtin/fetch-pack.c\n>> +++ b/builtin/fetch-pack.c\n>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n>> \t}\n>> }\n>> \n>> -static void filter_refs(struct ref **refs, int nr_match, char **match)\n>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n>> {\n>> \tstruct ref **return_refs;\n>> \tstruct ref *newlist = NULL;\n>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n>> \tstruct ref *fastarray[32];\n>> \tint match_pos;\n>\n> This match_pos is an index into the \"match\" array, which becomes \"head\".\n> Should it become head_pos?\n>\n> And then bits like this:\n>\n>> -\t\t\twhile (match_pos < nr_match) {\n>> -\t\t\t\tcmp = strcmp(ref->name, match[match_pos]);\n>> +\t\t\twhile (match_pos < nr_heads) {\n>> +\t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n>\n> Would be:\n>\n> while (head_pos < nr_heads)\n>\n> which makes more sense to me.\n\nUsing one name is better, but I wonder \"heads\" is the better one\nbetween the two.\n\nAfter all, this codepath is not limited to branches these days as\nthe word \"head\" implies. Rather, has what we\nasked for, and has what the other sides have, and we are\ntrying to make sure we haven't asked what they do not have in this\nfunction.\n\nAnd the way we do so is to match the \"thing\"s with what are in\n\"refs\" to find unmatched ones.\n\nSo between the two, I would have chosen \"match\" instead of \"heads\"\nto call the \"thing\".\n"},{"id":"197891","messageId":"503B3C5C.1020109@alum.mit.edu","threadId":"31329","inReplyTo":"7vvcg5v9hh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-27T09:22:36Z","receivedAt":"2012-08-27T09:22:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/26/2012 07:39 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:\n>>\n>>> From: Michael Haggerty \n>>>\n>>> Use the names (nr_heads, heads) consistently across functions, instead\n>>> of sometimes naming the same values (nr_match, match).\n>>\n>> I think this is fine, although:\n>>\n>>> --- a/builtin/fetch-pack.c\n>>> +++ b/builtin/fetch-pack.c\n>>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n>>> \t}\n>>> }\n>>> \n>>> -static void filter_refs(struct ref **refs, int nr_match, char **match)\n>>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)\n>>> {\n>>> \tstruct ref **return_refs;\n>>> \tstruct ref *newlist = NULL;\n>>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n>>> \tstruct ref *fastarray[32];\n>>> \tint match_pos;\n>>\n>> This match_pos is an index into the \"match\" array, which becomes \"head\".\n>> Should it become head_pos?\n>>\n>> And then bits like this:\n>>\n>>> -\t\t\twhile (match_pos < nr_match) {\n>>> -\t\t\t\tcmp = strcmp(ref->name, match[match_pos]);\n>>> +\t\t\twhile (match_pos < nr_heads) {\n>>> +\t\t\t\tcmp = strcmp(ref->name, heads[match_pos]);\n>>\n>> Would be:\n>>\n>> while (head_pos < nr_heads)\n>>\n>> which makes more sense to me.\n> \n> Using one name is better, but I wonder \"heads\" is the better one\n> between the two.\n> \n> After all, this codepath is not limited to branches these days as\n> the word \"head\" implies. Rather, has what we\n> asked for, and has what the other sides have, and we are\n> trying to make sure we haven't asked what they do not have in this\n> function.\n> \n> And the way we do so is to match the \"thing\"s with what are in\n> \"refs\" to find unmatched ones.\n> \n> So between the two, I would have chosen \"match\" instead of \"heads\"\n> to call the \"thing\".\n\nWhen I decided between \"heads\" and \"match\", my main consideration was\nthat \"match\" sounds like something that has already been matched, not\nsomething that is being matched against. The word \"match\" also implies\nto me that some nontrivial matching process is going on, like glob\nexpansion.\n\nBut I agree with you that \"heads\" also has disadvantages.\n\nWhat about a third option: \"refnames\"? This name makes it clear that we\nare talking about simple names as opposed to \"struct ref\" or some kind\nof refname glob patterns and also makes it clear that they are not\nnecessarily all branches. It would also be distinct from the \"refs\"\nlinked list that is often used in the same functions.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197892","messageId":"20120827092502.GA2428@sigill.intra.peff.net","threadId":"31329","inReplyTo":"503B3C5C.1020109@alum.mit.edu","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T09:25:02Z","receivedAt":"2012-08-27T09:25:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 27, 2012 at 11:22:36AM +0200, Michael Haggerty wrote:\n\n> > Using one name is better, but I wonder \"heads\" is the better one\n> > between the two.\n> > \n> > After all, this codepath is not limited to branches these days as\n> > the word \"head\" implies. Rather, has what we\n> > asked for, and has what the other sides have, and we are\n> > trying to make sure we haven't asked what they do not have in this\n> > function.\n> > \n> > And the way we do so is to match the \"thing\"s with what are in\n> > \"refs\" to find unmatched ones.\n> > \n> > So between the two, I would have chosen \"match\" instead of \"heads\"\n> > to call the \"thing\".\n> \n> When I decided between \"heads\" and \"match\", my main consideration was\n> that \"match\" sounds like something that has already been matched, not\n> something that is being matched against. The word \"match\" also implies\n> to me that some nontrivial matching process is going on, like glob\n> expansion.\n> \n> But I agree with you that \"heads\" also has disadvantages.\n> \n> What about a third option: \"refnames\"? This name makes it clear that we\n> are talking about simple names as opposed to \"struct ref\" or some kind\n> of refname glob patterns and also makes it clear that they are not\n> necessarily all branches. It would also be distinct from the \"refs\"\n> linked list that is often used in the same functions.\n\nYeah, I agree that \"refnames\" would be better. I think something like\n\"spec\" or \"refspec\" would indicate better that they are to be matched\nagainst, but then you run afoul of confusing that with colon-delimited\nrefspecs (which I do not think fetch-pack understands at all).\n\n-Peff\n"},{"id":"197917","messageId":"7vsjb8s2hd.fsf@alter.siamese.dyndns.org","threadId":"31329","inReplyTo":"503B3C5C.1020109@alum.mit.edu","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T16:50:54Z","receivedAt":"2012-08-27T16:50:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 08/26/2012 07:39 PM, Junio C Hamano wrote:\n> ...\n>> After all, this codepath is not limited to branches these days as\n>> the word \"head\" implies. Rather, has what we\n>> asked for, and has what the other sides have, and we are\n>> trying to make sure we haven't asked what they do not have in this\n>> function.\n>> \n>> And the way we do so is to match the \"thing\"s with what are in\n>> \"refs\" to find unmatched ones.\n>> \n>> So between the two, I would have chosen \"match\" instead of \"heads\"\n>> to call the \"thing\".\n>\n> When I decided between \"heads\" and \"match\", my main consideration was\n> that \"match\" sounds like something that has already been matched, not\n> something that is being matched against. The word \"match\" also implies\n> to me that some nontrivial matching process is going on, like glob\n> expansion.\n>\n> But I agree with you that \"heads\" also has disadvantages.\n>\n> What about a third option: \"refnames\"? This name makes it clear that we\n> are talking about simple names as opposed to \"struct ref\" or some kind\n> of refname glob patterns and also makes it clear that they are not\n> necessarily all branches. It would also be distinct from the \"refs\"\n> linked list that is often used in the same functions.\n\n\"refnames\" has a similar issue as \"match\", as you have pointed out,\nand more. Was it the remote or us who populated the \"refnames\"\n(answer: these are \"our\" refnames, and the other one \"refs\" is what\nthey have)? What do we have that \"refnames\" for (answer: these\nspecify what we are going to pick among what they have)?\n\nPerhaps \"asked\"? At the beginning of the processing, we have all\nthat were asked for, and at the end, we leave what were asked for\nbut were not fulfilled.\n"},{"id":"197918","messageId":"7voblws299.fsf@alter.siamese.dyndns.org","threadId":"31329","inReplyTo":"20120827092502.GA2428@sigill.intra.peff.net","subject":"Re: [PATCH 04/17] Name local variables more consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T16:55:46Z","receivedAt":"2012-08-27T16:55:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I agree that \"refnames\" would be better. I think something like\n> \"spec\" or \"refspec\" would indicate better that they are to be matched\n> against, but then you run afoul of confusing that with colon-delimited\n> refspecs (which I do not think fetch-pack understands at all).\n\nCorrect. It only takes the LHS of the refspecs, following the \"one\ntool does one specific thing\" philosophy. The arrangement was that\nthe calling script interpreted refspecs, split them into LHS and\nRHS, fed the LHS of the refspecs to fetch-pack, and then read the\noutput to learn which local refs (i.e. RHS of the refspecs) to\nupdate with what value.\n"},{"id":"198222","messageId":"50430479.3060006@alum.mit.edu","threadId":"31329","inReplyTo":"20120823203152.GA1810@sigill.intra.peff.net","subject":"Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-02T07:02:17Z","receivedAt":"2012-09-02T07:02:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/23/2012 10:31 PM, Jeff King wrote:\n> On Thu, Aug 23, 2012 at 03:56:48PM -0400, Jeff King wrote:\n> \n>> This code blames back to:\n>>\n>> commit 4bcb310c2539b66d535e87508d1b7a90fe29c083\n>> Author: Alexandre Julliard \n>> Date: Fri Nov 24 16:00:13 2006 +0100\n>>\n>> fetch-pack: Do not fetch tags for shallow clones.\n>>\n>> A better fix may be to only fetch tags that point to commits that we\n>> are downloading, but git-clone doesn't have support for following\n>> tags. This will happen automatically on the next git-fetch though.\n>>\n>> So it is about making sure that \"git clone --depth=1\" does not\n>> accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,\n>> losing the purpose of using --depth in the first place. These days it is\n>> largely irrelevant, since this code path is not followed by clone, and\n>> clone will automatically restrict its list of fetched refs to a single\n>> branch if --depth is used.\n> \n> I think part of the confusion of this code is that inside the loop over\n> the refs it is sometimes checking aspects of the ref, and sometimes\n> checking invariants of the loop (like args.fetch_all). Splitting it into\n> separate loops makes it easier to see what is going on, like the patch\n> below (on top of Michael's series).\n> \n> I'm not sure if it ends up being more readable, since the generic \"cut\n> down this linked list\" function has to operate through callbacks with a\n> void pointer. On the other hand, that function could also be used\n> elsewhere.\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 90683ca..877cf38 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -521,51 +521,80 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n> \t}\n> }\n> \n> -static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n> +static void filter_refs_callback(struct ref **refs,\n> +\t\t\t\t int (*want)(struct ref *, void *),\n> +\t\t\t\t void *data)\n> {\n> -\tstruct ref *newlist = NULL;\n> -\tstruct ref **newtail = &newlist;\n> +\tstruct ref **tail = refs;\n> \tstruct ref *ref, *next;\n> -\tint match_pos = 0, unmatched = 0;\n> \n> \tfor (ref = *refs; ref; ref = next) {\n> -\t\tint keep_ref = 0;\n> \t\tnext = ref->next;\n> -\t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n> -\t\t check_refname_format(ref->name, 0))\n> -\t\t\t; /* trash */\n> -\t\telse if (args.fetch_all &&\n> -\t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\")))\n> -\t\t\tkeep_ref = 1;\n> -\t\telse\n> -\t\t\twhile (match_pos < *nr_heads) {\n> -\t\t\t\tint cmp = strcmp(ref->name, heads[match_pos]);\n> -\t\t\t\tif (cmp < 0) { /* definitely do not have it */\n> -\t\t\t\t\tbreak;\n> -\t\t\t\t} else if (cmp == 0) { /* definitely have it */\n> -\t\t\t\t\tfree(heads[match_pos++]);\n> -\t\t\t\t\tkeep_ref = 1;\n> -\t\t\t\t\tbreak;\n> -\t\t\t\t} else { /* might have it; keep looking */\n> -\t\t\t\t\theads[unmatched++] = heads[match_pos++];\n> -\t\t\t\t}\n> -\t\t\t}\n> -\n> -\t\tif (keep_ref) {\n> -\t\t\t*newtail = ref;\n> -\t\t\tref->next = NULL;\n> -\t\t\tnewtail = &ref->next;\n> -\t\t} else {\n> +\t\tif (want(ref, data))\n> +\t\t\ttail = &ref->next;\n> +\t\telse {\n> \t\t\tfree(ref);\n> +\t\t\t*tail = next;\n> \t\t}\n> \t}\n> +}\n> \n> -\t/* copy any remaining unmatched heads: */\n> -\twhile (match_pos < *nr_heads)\n> -\t\theads[unmatched++] = heads[match_pos++];\n> -\t*nr_heads = unmatched;\n> +static int ref_name_is_ok(struct ref *ref, void *data)\n> +{\n> +\treturn memcmp(ref->name, \"refs/\", 5) ||\n> +\t\t!check_refname_format(ref->name, 0);\n> +}\n> +\n> +static int ref_ok_for_shallow(struct ref *ref, void *data)\n> +{\n> +\treturn prefixcmp(ref->name, \"refs/tags/\");\n> +}\n> \n> -\t*refs = newlist;\n> +struct filter_by_name_data {\n> +\tchar **heads;\n> +\tint nr_heads;\n> +\tint match_pos;\n> +\tint unmatched;\n> +};\n> +\n> +static int want_ref_name(struct ref *ref, void *data)\n> +{\n> +\tstruct filter_by_name_data *f = data;\n> +\n> +\twhile (f->match_pos < f->nr_heads) {\n> +\t\tint cmp = strcmp(ref->name, f->heads[f->match_pos]);\n> +\t\tif (cmp < 0) /* definitely do not have it */\n> +\t\t\treturn 0;\n> +\t\telse if (cmp == 0) { /* definitely have it */\n> +\t\t\tfree(f->heads[f->match_pos++]);\n> +\t\t\treturn 1;\n> +\t\t} else /* might have it; keep looking */\n> +\t\t\tf->heads[f->unmatched++] = f->heads[f->match_pos++];\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static void filter_refs(struct ref **refs, int *nr_heads, char **heads)\n> +{\n> +\tstruct filter_by_name_data f;\n> +\n> +\tfilter_refs_callback(refs, ref_name_is_ok, NULL);\n> +\n> +\tif (args.fetch_all) {\n> +\t\tif (args.depth)\n> +\t\t\tfilter_refs_callback(refs, ref_ok_for_shallow, NULL);\n> +\t\treturn;\n> +\t}\n> +\n> +\tmemset(&f, 0, sizeof(f));\n> +\tf.heads = heads;\n> +\tf.nr_heads = *nr_heads;\n> +\tfilter_refs_callback(refs, want_ref_name, &f);\n> +\n> +\t/* copy any remaining unmatched heads: */\n> +\twhile (f.match_pos < f.nr_heads)\n> +\t\theads[f.unmatched++] = heads[f.match_pos++];\n> +\t*nr_heads = f.unmatched;\n> }\n> \n> static void mark_alternate_complete(const struct ref *ref, void *unused)\n> \n\nI need a sanity check here. I don't see that it is forbidden to call\n\"git fetch-pack --all --depth=N\" and also specify some explicit\nreferences. (This usage would make it possible for the user to do a\nshallow clone while also retrieving some specific tags.)\n\nHowever, if I try to do this I get (before your change)\n\n $ git fetch-pack -v --all --depth=1 $URL refs/heads/master\n Server supports multi_ack_detailed\n Server supports side-band-64k\n Server supports ofs-delta\n want 9c9f2f4453a7260d0d60926c73811f025be98ded (HEAD)\n want 9c9f2f4453a7260d0d60926c73811f025be98ded (refs/heads/master)\n done\n remote: Counting objects: 6, done.\n remote: Compressing objects: 100% (4/4), done.\n remote: Total 6 (delta 0), reused 0 (delta 0)\n Unpacking objects: 100% (6/6), done.\n error: no such remote ref refs/heads/master\n 9c9f2f4453a7260d0d60926c73811f025be98ded HEAD\n 9c9f2f4453a7260d0d60926c73811f025be98ded refs/heads/master\n\nNote the error message \"no such remote ref refs/heads/master\", which is\nincorrect. This happens because when the (fetch_all && (!depth ||\nref_non_tag(ref))) branch of the if statement succeeds, then the\nlook-in-heads-list branch is never executed, and therefore the requested\nreference is never marked as having been found among the remote references.\n\nOn the other hand, if I try the same with an (existing) tag, before your\nchange I get\n\n $ git fetch-pack -v --all --depth=1 $URL refs/tags/c\n Server supports multi_ack_detailed\n Server supports side-band-64k\n Server supports ofs-delta\n fatal: The remote end hung up unexpectedly\n\nNow this is obviously broken in git-fetch-pack, because when fetch_all\nis specified, return_refs is never initialized, but the\nlook-in-heads-list branch of the if is executed nevertheless.\n\nIt is not hard to fix these problems in the old code: (1)\nunconditionally initialize return_refs, and (2) exchange the\nlook-in-heads-list handling and the fetch_all handling.\n\n\nAfter your change the first problem (spurious error when asking for a\nhead) remains unchanged. The second problem is changed from a crash to\na misbehavior:\n\n $ git fetch-pack -v --all --depth=1 $URL refs/tags/c\n Initialized empty Git repository in /home/mhagger/tmp/cl/c/.git/\n Server supports multi_ack_detailed\n Server supports side-band-64k\n Server supports ofs-delta\n want 9c9f2f4453a7260d0d60926c73811f025be98ded (HEAD)\n want 9c9f2f4453a7260d0d60926c73811f025be98ded (refs/heads/master)\n done\n remote: Counting objects: 6, done.\n remote: Compressing objects: 100% (4/4), done.\n remote: Total 6 (delta 0), reused 0 (delta 0)\n Unpacking objects: 100% (6/6), done.\n error: no such remote ref refs/tags/c\n 9c9f2f4453a7260d0d60926c73811f025be98ded HEAD\n 9c9f2f4453a7260d0d60926c73811f025be98ded refs/heads/master\n\nNote that \"refs/tags/c\" is not obtained from the remote even though it\nwas specifically requested. The reason that your patch makes the crash\ndisappear is that when fetch_all is true, then the look-in-heads-list\nbranch is not executed at all -- but this is also why \"refs/tags/c\" is\nnot obtained.\n\nSo, assuming that we want to support\n\n git fetch-pack --all --depth=N $URL refs/tags/TAG\n\ncorrectly, then your filter_refs_callback() refactoring won't work, at\nleast not the way that you have written it. I will continue working on\nthis.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}