{"thread":{"id":"31482","subject":"[PATCH v3 01/14] t5500: add tests of error output for missing refs","startedAt":"2012-09-09T06:19:35Z","lastAt":"2012-09-18T23:37:20Z","messageCount":32,"participants":["Michael Haggerty","Junio C Hamano","Jeff King","Philip Oakley"],"isPatch":true,"patchVersion":3,"patchTotal":14},"messages":[{"id":"198603","messageId":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":null,"subject":"[PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:35Z","receivedAt":"2012-09-09T06:19:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series depends on the \"Add some string_list-related\nfunctions\" series that I just submitted.\n\nThis series is a significant rewrite of v2 based on the realization\nthat the <nr_heads,heads> pair that is passed around in these\nfunctions is better expressed as a string_list.  I hope you will find\nthat this version is shorter and clearer than its predecessors, even\nthough its basic logic is mostly the same.\n\nSorry for the false starts in v1 and v2 and the extra reviewing work\nthat this will cost.\n\nMichael Haggerty (14):\n  t5500: add tests of error output for missing refs\n  t5500: add tests of fetch-pack --all --depth=N $URL $REF\n  Rename static function fetch_pack() to http_fetch_pack()\n  fetch_pack(): reindent function decl and defn\n  Change fetch_pack() and friends to take string_list arguments\n  filter_refs(): do not check the same sought_pos twice\n  fetch_pack(): update sought->nr to reflect number of unique entries\n  filter_refs(): delete matched refs from sought list\n  filter_refs(): build refs list as we go\n  filter_refs(): simplify logic\n  cmd_fetch_pack(): return early if finish_connect() fails\n  fetch-pack: report missing refs even if no existing refs were\n    received\n  cmd_fetch_pack(): simplify computation of return value\n  fetch-pack: eliminate spurious error messages\n\n builtin/fetch-pack.c  | 169 +++++++++++++++++++-------------------------------\n fetch-pack.h          |  20 ++++--\n http-walker.c         |   4 +-\n t/t5500-fetch-pack.sh |  47 +++++++++++++-\n transport.c           |  12 ++--\n 5 files changed, 130 insertions(+), 122 deletions(-)\n\n-- \n1.7.11.3\n"},{"id":"198602","messageId":"1347171589-13327-2-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 01/14] t5500: add tests of error output for missing refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:36Z","receivedAt":"2012-09-09T06:19:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If \"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 <mhagger@alum.mit.edu>\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..6fa1cef 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+\t\tcd client &&\n+\t\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+\t\tcd client &&\n+\t\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+\t\tcd client &&\n+\t\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":"198616","messageId":"1347171589-13327-3-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:37Z","receivedAt":"2012-09-09T06:19:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document some bugs in \"git fetch-pack\":\n\n1. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\nexplicit existing non-tag reference to fetch, then it falsely reports\nthat the reference was not found, even though it was fetched\ncorrectly.\n\n2. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\nexplicit existing tag reference to fetch, then it segfaults in\nfilter_refs() because return_refs is used without having been\ninitialized.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t5500-fetch-pack.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 6fa1cef..15d277f 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -427,4 +427,19 @@ test_expect_success 'test missing ref before existing' '\n \ttest_cmp expect-error error-me\n '\n \n+test_expect_failure 'test --all, --depth, and explicit head' '\n+\t(\n+\t\tcd client &&\n+\t\tgit fetch-pack --no-progress --all --depth=1 .. refs/heads/A\n+\t) >out-adh 2>error-adh\n+'\n+\n+test_expect_failure 'test --all, --depth, and explicit tag' '\n+\tgit tag OLDTAG refs/heads/B~5 &&\n+\t(\n+\t\tcd client &&\n+\t\tgit fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG\n+\t) >out-adt 2>error-adt\n+'\n+\n test_done\n-- \n1.7.11.3\n"},{"id":"198604","messageId":"1347171589-13327-4-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 03/14] Rename static function fetch_pack() to http_fetch_pack()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:38Z","receivedAt":"2012-09-09T06:19:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Avoid confusion with the non-static function of the same name from\nfetch-pack.h.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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":"198612","messageId":"1347171589-13327-5-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 04/14] fetch_pack(): reindent function decl and defn","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:39Z","receivedAt":"2012-09-09T06:19:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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 fdda36f..30990c0 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1082,10 +1082,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":"198605","messageId":"1347171589-13327-6-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:40Z","receivedAt":"2012-09-09T06:19:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of juggling <nr_heads,heads> (sometimes called\n<nr_match,match>), pass around the list of references to be sought in\na single string_list variable called \"sought\".  Future commits will\nmake more use of string_list functionality.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c | 88 +++++++++++++++++++++++-----------------------------\n fetch-pack.h         |  5 +--\n transport.c          | 18 +++++------\n 3 files changed, 51 insertions(+), 60 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 30990c0..df81995 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -525,27 +525,27 @@ 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, struct string_list *sought)\n {\n \tstruct ref **return_refs;\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+\tint sought_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 (sought->nr && !args.fetch_all) {\n+\t\tif (ARRAY_SIZE(fastarray) < sought->nr)\n+\t\t\treturn_refs = xcalloc(sought->nr, 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 *) * sought->nr);\n \t\t}\n \t}\n \telse\n \t\treturn_refs = NULL;\n \n-\tmatch_pos = 0;\n+\tsought_pos = 0;\n \tfor (ref = *refs; ref; ref = next) {\n \t\tnext = ref->next;\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n@@ -560,17 +560,17 @@ 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 (sought_pos < sought->nr) {\n+\t\t\t\tcmp = strcmp(ref->name, sought->items[sought_pos].string);\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\treturn_refs[match_pos] = ref;\n+\t\t\t\t\tsought->items[sought_pos].string[0] = '\\0';\n+\t\t\t\t\treturn_refs[sought_pos] = ref;\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\t\tsought_pos++;\n \t\t\t}\n \t\t\tif (!cmp)\n \t\t\t\tcontinue; /* we will link it later */\n@@ -580,7 +580,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 < sought->nr; i++) {\n \t\t\tref = return_refs[i];\n \t\t\tif (ref) {\n \t\t\t\t*newtail = ref;\n@@ -599,7 +599,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, struct string_list *sought)\n {\n \tstruct ref *ref;\n \tint retval;\n@@ -650,7 +650,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, sought);\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n@@ -781,8 +781,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\tstruct string_list *sought,\n \t\tchar **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -839,7 +838,7 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\t\tagent_len, agent_feature);\n \t}\n \n-\tif (everything_local(&ref, nr_match, match)) {\n+\tif (everything_local(&ref, sought)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -859,16 +858,16 @@ 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+static int remove_duplicates(struct string_list *sought)\n {\n \tint src, dst;\n \n-\tif (!nr_heads)\n+\tif (!sought->nr)\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+\tfor (src = dst = 1; src < sought->nr; src++)\n+\t\tif (strcmp(sought->items[src].string, sought->items[dst-1].string))\n+\t\t\tsought->items[dst++] = sought->items[src];\n \treturn dst;\n }\n \n@@ -922,8 +921,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tint i, ret;\n \tstruct ref *ref = NULL;\n \tconst char *dest = NULL;\n-\tint alloc_heads = 0, nr_heads = 0;\n-\tchar **heads = NULL;\n+\tstruct string_list sought = STRING_LIST_INIT_DUP;\n \tint fd[2];\n \tchar *pack_lockfile = NULL;\n \tchar **pack_lockfile_ptr = NULL;\n@@ -1000,9 +998,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t * Copy refs from cmdline to growable list, then append any\n \t * refs from the standard input:\n \t */\n-\tALLOC_GROW(heads, argc - i, alloc_heads);\n \tfor (; i < argc; i++)\n-\t\theads[nr_heads++] = xstrdup(argv[i]);\n+\t\tstring_list_append(&sought, xstrdup(argv[i]));\n \tif (args.stdin_refs) {\n \t\tif (args.stateless_rpc) {\n \t\t\t/* in stateless RPC mode we use pkt-line to read\n@@ -1015,17 +1012,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tbreak;\n \t\t\t\tif (line[n-1] == '\\n')\n \t\t\t\t\tn--;\n-\t\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n-\t\t\t\theads[nr_heads++] = xmemdupz(line, n);\n+\t\t\t\tstring_list_append(&sought, xmemdupz(line, n));\n \t\t\t}\n \t\t}\n \t\telse {\n \t\t\t/* read from stdin one ref per line, until EOF */\n \t\t\tstruct strbuf line = STRBUF_INIT;\n-\t\t\twhile (strbuf_getline(&line, stdin, '\\n') != EOF) {\n-\t\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n-\t\t\t\theads[nr_heads++] = strbuf_detach(&line, NULL);\n-\t\t\t}\n+\t\t\twhile (strbuf_getline(&line, stdin, '\\n') != EOF)\n+\t\t\t\tstring_list_append(&sought, strbuf_detach(&line, NULL));\n \t\t\tstrbuf_release(&line);\n \t\t}\n \t}\n@@ -1042,7 +1036,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\t &sought, pack_lockfile_ptr);\n \tif (pack_lockfile) {\n \t\tprintf(\"lock %s\\n\", pack_lockfile);\n \t\tfflush(stdout);\n@@ -1053,17 +1047,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tref = NULL;\n \tret = !ref;\n \n-\tif (!ret && nr_heads) {\n+\tif (!ret && sought.nr) {\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\tif (heads[i] && heads[i][0]) {\n-\t\t\t\terror(\"no such remote ref %s\", heads[i]);\n+\t\tfor (i = 0; i < sought.nr; i++) {\n+\t\t\tchar *s = sought.items[i].string;\n+\t\t\tif (s && s[0]) {\n+\t\t\t\terror(\"no such remote ref %s\", s);\n \t\t\t\tret = 1;\n \t\t\t}\n+\t\t}\n \t}\n \twhile (ref) {\n \t\tprintf(\"%s %s\\n\",\n@@ -1074,17 +1070,11 @@ 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 \t\t       const char *dest,\n-\t\t       int nr_heads,\n-\t\t       char **heads,\n+\t\t       struct string_list *sought,\n \t\t       char **pack_lockfile)\n {\n \tstruct stat st;\n@@ -1098,16 +1088,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 (sought->nr) {\n+\t\tsort_string_list(sought);\n+\t\tremove_duplicates(sought);\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, sought, 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..a6a8a73 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -1,6 +1,8 @@\n #ifndef FETCH_PACK_H\n #define FETCH_PACK_H\n \n+#include \"string-list.h\"\n+\n struct fetch_pack_args {\n \tconst char *uploadpack;\n \tint unpacklimit;\n@@ -21,8 +23,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       char **heads,\n+\t\t       struct string_list *sought,\n \t\t       char **pack_lockfile);\n \n #endif\ndiff --git a/transport.c b/transport.c\nindex 1811b50..a847bf3 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -518,8 +518,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t       int nr_heads, struct ref **to_fetch)\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+\tstruct string_list orig_sought = STRING_LIST_INIT_DUP;\n+\tstruct string_list sought = STRING_LIST_INIT_NODUP;\n \tconst struct ref *refs;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n@@ -537,8 +537,10 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.no_progress = !transport->progress;\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+\tfor (i = 0; i < nr_heads; i++) {\n+\t\tstring_list_append(&orig_sought, to_fetch[i]->name);\n+\t\tstring_list_append(&sought, orig_sought.items[orig_sought.nr - 1].string);\n+\t}\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n@@ -548,7 +550,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, &sought, &transport->pack_lockfile);\n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n \tif (finish_connect(data->conn))\n@@ -558,10 +560,8 @@ 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-\t\tfree(origh[i]);\n-\tfree(origh);\n-\tfree(heads);\n+\tstring_list_clear(&sought, 0);\n+\tstring_list_clear(&orig_sought, 0);\n \tfree(dest);\n \treturn (refs ? 0 : -1);\n }\n-- \n1.7.11.3\n"},{"id":"198615","messageId":"1347171589-13327-7-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 06/14] filter_refs(): do not check the same sought_pos twice","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:41Z","receivedAt":"2012-09-09T06:19:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Once a match has been found at sought_pos, the entry is zeroed and no\nfuture attempts will match that entry.  So increment sought_pos to\navoid checking against the zeroed-out entry during the next iteration.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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 df81995..63d455f 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -565,8 +565,8 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\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\tsought->items[sought_pos].string[0] = '\\0';\n \t\t\t\t\treturn_refs[sought_pos] = ref;\n+\t\t\t\t\tsought->items[sought_pos++].string[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":"198614","messageId":"1347171589-13327-8-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 07/14] fetch_pack(): update sought->nr to reflect number of unique entries","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:42Z","receivedAt":"2012-09-09T06:19:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"fetch_pack() removes duplicates from the \"sought\" list, thereby\nshrinking the list.  But previously, the caller was not informed about\nthe shrinkage.  This would cause a spurious error message to be\nemitted by cmd_fetch_pack() if \"git fetch-pack\" is called with\nduplicate refnames.\n\nInstead, remove duplicates using string_list_remove_duplicates(),\nwhich adjusts sought->nr to reflect the new length of the list.\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 <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c  | 15 +--------------\n t/t5500-fetch-pack.sh |  2 +-\n 2 files changed, 2 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 63d455f..6cd734a 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -858,19 +858,6 @@ static struct ref *do_fetch_pack(int fd[2],\n \treturn ref;\n }\n \n-static int remove_duplicates(struct string_list *sought)\n-{\n-\tint src, dst;\n-\n-\tif (!sought->nr)\n-\t\treturn 0;\n-\n-\tfor (src = dst = 1; src < sought->nr; src++)\n-\t\tif (strcmp(sought->items[src].string, sought->items[dst-1].string))\n-\t\t\tsought->items[dst++] = sought->items[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@@ -1090,7 +1077,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \n \tif (sought->nr) {\n \t\tsort_string_list(sought);\n-\t\tremove_duplicates(sought);\n+\t\tstring_list_remove_duplicates(sought, 0);\n \t}\n \n \tif (!ref) {\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 15d277f..acd41e8 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 <output | sort >actual &&\n \ttest_cmp expect actual\n-- \n1.7.11.3\n"},{"id":"198613","messageId":"1347171589-13327-9-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 08/14] filter_refs(): delete matched refs from sought list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:43Z","receivedAt":"2012-09-09T06:19:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Remove any references that are available from the remote from the\nsought list (rather than overwriting their names with NUL characters,\nas previously).  Mark matching entries by writing a non-NULL pointer\nto string_list_item::util during the iteration, then use\nfilter_string_list() later to filter out the entries that have been\nmarked.\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 <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c | 23 +++++++++++++++--------\n fetch-pack.h         |  7 +++++++\n transport.c          | 10 +++-------\n 3 files changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 6cd734a..12ba009 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -525,6 +525,16 @@ static void mark_recent_complete_commits(unsigned long cutoff)\n \t}\n }\n \n+static int non_matching_ref(struct string_list_item *item, void *unused)\n+{\n+\tif (item->util) {\n+\t\titem->util = NULL;\n+\t\treturn 0;\n+\t}\n+\telse\n+\t\treturn 1;\n+}\n+\n static void filter_refs(struct ref **refs, struct string_list *sought)\n {\n \tstruct ref **return_refs;\n@@ -566,7 +576,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \t\t\t\t\tbreak;\n \t\t\t\telse if (cmp == 0) { /* definitely have it */\n \t\t\t\t\treturn_refs[sought_pos] = ref;\n-\t\t\t\t\tsought->items[sought_pos++].string[0] = '\\0';\n+\t\t\t\t\tsought->items[sought_pos++].util = \"matched\";\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\telse /* might have it; keep looking */\n@@ -590,6 +600,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \t\t}\n \t\tif (return_refs != fastarray)\n \t\t\tfree(return_refs);\n+\t\tfilter_string_list(sought, 0, non_matching_ref, NULL);\n \t}\n \t*refs = newlist;\n }\n@@ -1040,13 +1051,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\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 < sought.nr; i++) {\n-\t\t\tchar *s = sought.items[i].string;\n-\t\t\tif (s && s[0]) {\n-\t\t\t\terror(\"no such remote ref %s\", s);\n-\t\t\t\tret = 1;\n-\t\t\t}\n-\t\t}\n+\t\tfor (i = 0; i < sought.nr; i++)\n+\t\t\terror(\"no such remote ref %s\", sought.items[i].string);\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 a6a8a73..cb14871 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -19,6 +19,13 @@ struct fetch_pack_args {\n \t\tstateless_rpc:1;\n };\n \n+/*\n+ * sought contains the full names of remote references that should be\n+ * updated from.  On return, the names that were found on the remote\n+ * will have been removed from the list.  The util members of the\n+ * string_list_items are used internally; they must be NULL on entry\n+ * (and will be NULL on exit).\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 a847bf3..9932f40 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -518,8 +518,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t       int nr_heads, struct ref **to_fetch)\n {\n \tstruct git_transport_data *data = transport->data;\n-\tstruct string_list orig_sought = STRING_LIST_INIT_DUP;\n-\tstruct string_list sought = STRING_LIST_INIT_NODUP;\n+\tstruct string_list sought = STRING_LIST_INIT_DUP;\n \tconst struct ref *refs;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n@@ -537,10 +536,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.no_progress = !transport->progress;\n \targs.depth = data->options.depth;\n \n-\tfor (i = 0; i < nr_heads; i++) {\n-\t\tstring_list_append(&orig_sought, to_fetch[i]->name);\n-\t\tstring_list_append(&sought, orig_sought.items[orig_sought.nr - 1].string);\n-\t}\n+\tfor (i = 0; i < nr_heads; i++)\n+\t\tstring_list_append(&sought, to_fetch[i]->name);\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n@@ -561,7 +558,6 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tfree_refs(refs_tmp);\n \n \tstring_list_clear(&sought, 0);\n-\tstring_list_clear(&orig_sought, 0);\n \tfree(dest);\n \treturn (refs ? 0 : -1);\n }\n-- \n1.7.11.3\n"},{"id":"198606","messageId":"1347171589-13327-10-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 09/14] filter_refs(): build refs list as we go","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:44Z","receivedAt":"2012-09-09T06:19:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of temporarily storing matched refs to temporary array\n\"return_refs\", simply append them to newlist as we go.  This changes\nthe order of references in newlist to strictly sorted if \"--all\" and\n\"--depth\" and named references are all specified, but that usage is\nbroken anyway (see the last two tests in t5500).\n\nThis changes the last test in t5500 from segfaulting into just\nemitting a spurious error (this will be fixed in a moment).\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c | 31 ++++---------------------------\n 1 file changed, 4 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 12ba009..4e94aa4 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -537,24 +537,11 @@ static int non_matching_ref(struct string_list_item *item, void *unused)\n \n static void filter_refs(struct ref **refs, struct string_list *sought)\n {\n-\tstruct ref **return_refs;\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n \tstruct ref *ref, *next;\n-\tstruct ref *fastarray[32];\n \tint sought_pos;\n \n-\tif (sought->nr && !args.fetch_all) {\n-\t\tif (ARRAY_SIZE(fastarray) < sought->nr)\n-\t\t\treturn_refs = xcalloc(sought->nr, sizeof(struct ref *));\n-\t\telse {\n-\t\t\treturn_refs = fastarray;\n-\t\t\tmemset(return_refs, 0, sizeof(struct ref *) * sought->nr);\n-\t\t}\n-\t}\n-\telse\n-\t\treturn_refs = NULL;\n-\n \tsought_pos = 0;\n \tfor (ref = *refs; ref; ref = next) {\n \t\tnext = ref->next;\n@@ -575,8 +562,10 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\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[sought_pos] = ref;\n \t\t\t\t\tsought->items[sought_pos++].util = \"matched\";\n+\t\t\t\t\t*newtail = ref;\n+\t\t\t\t\tref->next = NULL;\n+\t\t\t\t\tnewtail = &ref->next;\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\telse /* might have it; keep looking */\n@@ -588,20 +577,8 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \t\tfree(ref);\n \t}\n \n-\tif (!args.fetch_all) {\n-\t\tint i;\n-\t\tfor (i = 0; i < sought->nr; 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}\n-\t\tif (return_refs != fastarray)\n-\t\t\tfree(return_refs);\n+\tif (!args.fetch_all)\n \t\tfilter_string_list(sought, 0, non_matching_ref, NULL);\n-\t}\n \t*refs = newlist;\n }\n \n-- \n1.7.11.3\n"},{"id":"198611","messageId":"1347171589-13327-11-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 10/14] filter_refs(): simplify logic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:45Z","receivedAt":"2012-09-09T06:19:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Simplify flow within loop: first decide whether to keep the reference,\nthen keep/free it.  This makes it clearer that each ref has exactly\ntwo possible destinies, and removes duplication of the code for\nappending the reference to the linked list.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c | 37 ++++++++++++++++++-------------------\n 1 file changed, 18 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4e94aa4..056ccb8 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -544,37 +544,36 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \n \tsought_pos = 0;\n \tfor (ref = *refs; ref; ref = next) {\n+\t\tint keep = 0;\n \t\tnext = ref->next;\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n \t\t    check_refname_format(ref->name + 5, 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\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\")))\n+\t\t\tkeep = 1;\n \t\telse {\n-\t\t\tint cmp = -1;\n \t\t\twhile (sought_pos < sought->nr) {\n-\t\t\t\tcmp = strcmp(ref->name, sought->items[sought_pos].string);\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\tint cmp = strcmp(ref->name, sought->items[sought_pos].string);\n+\t\t\t\tif (cmp < 0)\n+\t\t\t\t\tbreak; /* definitely do not have it */\n+\t\t\t\telse if (cmp == 0) {\n+\t\t\t\t\tkeep = 1; /* definitely have it */\n \t\t\t\t\tsought->items[sought_pos++].util = \"matched\";\n-\t\t\t\t\t*newtail = ref;\n-\t\t\t\t\tref->next = NULL;\n-\t\t\t\t\tnewtail = &ref->next;\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\tsought_pos++;\n+\t\t\t\telse\n+\t\t\t\t\tsought_pos++; /* might have it; keep looking */\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+\n+\t\tif (keep) {\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}\n \n \tif (!args.fetch_all)\n-- \n1.7.11.3\n"},{"id":"198609","messageId":"1347171589-13327-12-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 11/14] cmd_fetch_pack(): return early if finish_connect() fails","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:46Z","receivedAt":"2012-09-09T06:19:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This simplifies the logic without changing the behavior.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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 056ccb8..fb2a423 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1018,10 +1018,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 && sought.nr) {\n+\tret = !ref;\n+\tif (ref && sought.nr) {\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":"198608","messageId":"1347171589-13327-13-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 12/14] fetch-pack: report missing refs even if no existing refs were received","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:47Z","receivedAt":"2012-09-09T06:19:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This fixes a test in t5500.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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 fb2a423..3d388b5 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 \t\treturn 1;\n \n \tret = !ref;\n-\tif (ref && sought.nr) {\n+\tif (sought.nr) {\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 acd41e8..894d945 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 \t\tcd client &&\n \t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy\n-- \n1.7.11.3\n"},{"id":"198607","messageId":"1347171589-13327-14-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 13/14] cmd_fetch_pack(): simplify computation of return value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:48Z","receivedAt":"2012-09-09T06:19:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Set the final value at initialization rather than initializing it then\nsometimes changing it.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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 3d388b5..42078e5 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1020,17 +1020,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 (sought.nr) {\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 < sought.nr; i++)\n-\t\t\terror(\"no such remote ref %s\", sought.items[i].string);\n-\t\tret = 1;\n-\t}\n+\tret = !ref || sought.nr;\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 < sought.nr; i++)\n+\t\terror(\"no such remote ref %s\", sought.items[i].string);\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":"198610","messageId":"1347171589-13327-15-git-send-email-mhagger@alum.mit.edu","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 14/14] fetch-pack: eliminate spurious error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T06:19:49Z","receivedAt":"2012-09-09T06:19:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It used to be that if \"--all\", \"--depth\", and also explicit references\nwere sought, then the explicit references were not handled correctly\nin filter_refs() because the \"--all --depth\" code took precedence over\nthe explicit reference handling, and the explicit references were\nnever noted as having been found.  So check for explicitly sought\nreferences before proceeding to the \"--all --depth\" logic.\n\nThis fixes two test cases in t5500.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c  | 10 +++++-----\n t/t5500-fetch-pack.sh |  4 ++--\n 2 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 42078e5..e644398 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -549,9 +549,6 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n \t\t    check_refname_format(ref->name + 5, 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 = 1;\n \t\telse {\n \t\t\twhile (sought_pos < sought->nr) {\n \t\t\t\tint cmp = strcmp(ref->name, sought->items[sought_pos].string);\n@@ -567,6 +564,10 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \t\t\t}\n \t\t}\n \n+\t\tif (! keep && args.fetch_all &&\n+\t\t    (!args.depth || prefixcmp(ref->name, \"refs/tags/\")))\n+\t\t\tkeep = 1;\n+\n \t\tif (keep) {\n \t\t\t*newtail = ref;\n \t\t\tref->next = NULL;\n@@ -576,8 +577,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought)\n \t\t}\n \t}\n \n-\tif (!args.fetch_all)\n-\t\tfilter_string_list(sought, 0, non_matching_ref, NULL);\n+\tfilter_string_list(sought, 0, non_matching_ref, NULL);\n \t*refs = newlist;\n }\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 894d945..6322e8a 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -427,14 +427,14 @@ test_expect_success 'test missing ref before existing' '\n \ttest_cmp expect-error error-me\n '\n \n-test_expect_failure 'test --all, --depth, and explicit head' '\n+test_expect_success 'test --all, --depth, and explicit head' '\n \t(\n \t\tcd client &&\n \t\tgit fetch-pack --no-progress --all --depth=1 .. refs/heads/A\n \t) >out-adh 2>error-adh\n '\n \n-test_expect_failure 'test --all, --depth, and explicit tag' '\n+test_expect_success 'test --all, --depth, and explicit tag' '\n \tgit tag OLDTAG refs/heads/B~5 &&\n \t(\n \t\tcd client &&\n-- \n1.7.11.3\n"},{"id":"198625","messageId":"7v7gs3sdjx.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T10:20:18Z","receivedAt":"2012-09-09T10:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> This patch series depends on the \"Add some string_list-related\n> functions\" series that I just submitted.\n\nMakes sense.  The only worry (without reading the series first) I\nhave is that the use of string list may make the responsiblity of\nsorting the list fuzzier. I am guessing that we never sorted the\nrefs we asked to fetch (so that FETCH_HEAD comes out in an expected\norder), so use of unsorted string list would be perfectly fine.\n\nQueued and pushed out in 'pu' without reading, but will take a look\nsometime tomorrow (eh, today, that is).\n"},{"id":"198626","messageId":"20120909130532.GA27754@sigill.intra.peff.net","threadId":"31482","inReplyTo":"7v7gs3sdjx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-09T13:05:33Z","receivedAt":"2012-09-09T13:05:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n> > This patch series depends on the \"Add some string_list-related\n> > functions\" series that I just submitted.\n> \n> Makes sense.  The only worry (without reading the series first) I\n> have is that the use of string list may make the responsiblity of\n> sorting the list fuzzier. I am guessing that we never sorted the\n> refs we asked to fetch (so that FETCH_HEAD comes out in an expected\n> order), so use of unsorted string list would be perfectly fine.\n\nI haven't read the series yet, but both the list of heads from the user\nand the list of heads from the remote should have been sorted by 4435968\nand 9e8e704f, respectively.\n\n-Peff\n"},{"id":"198634","messageId":"7vhar7qd07.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"20120909130532.GA27754@sigill.intra.peff.net","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T18:15:04Z","receivedAt":"2012-09-09T18:15:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote:\n>\n>> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> \n>> > This patch series depends on the \"Add some string_list-related\n>> > functions\" series that I just submitted.\n>> \n>> Makes sense.  The only worry (without reading the series first) I\n>> have is that the use of string list may make the responsiblity of\n>> sorting the list fuzzier. I am guessing that we never sorted the\n>> refs we asked to fetch (so that FETCH_HEAD comes out in an expected\n>> order), so use of unsorted string list would be perfectly fine.\n>\n> I haven't read the series yet, but both the list of heads from the user\n> and the list of heads from the remote should have been sorted by 4435968\n> and 9e8e704f, respectively.\n\nOK.  As long as the sort order matches the order string-list\ninternally uses for its bisection search, it won't be a problem,\nthen.\n\nThanks.\n"},{"id":"198726","messageId":"7vmx0xmwr9.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"1347171589-13327-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T20:46:34Z","receivedAt":"2012-09-10T20:46:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Document some bugs in \"git fetch-pack\":\n>\n> 1. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\n> explicit existing non-tag reference to fetch, then it falsely reports\n> that the reference was not found, even though it was fetched\n> correctly.\n>\n> 2. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\n> explicit existing tag reference to fetch, then it segfaults in\n> filter_refs() because return_refs is used without having been\n> initialized.\n\nI guess the first one is because \"all\" already marks the fetched one\n\"used\", and does not allow the explicit one to match any unused one\nfrom the other side?  I wonder what happens when \"--all\" with an\nexplicit refspec that names non-existing ref is asked for (it should\nnotice that refs/heads/no-such-ref does not exist.  I do not know if\nit is something that belongs to this set of new tests)?\n\nIt is funny that this only happens with \"--depth\" (I think I know\nwhich part of the code introduces this bug, so it is not all that\ninteresting, but just funny).\n\nThanks for working on these glitches.\n\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  t/t5500-fetch-pack.sh | 15 +++++++++++++++\n>  1 file changed, 15 insertions(+)\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 6fa1cef..15d277f 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -427,4 +427,19 @@ test_expect_success 'test missing ref before existing' '\n>  \ttest_cmp expect-error error-me\n>  '\n>  \n> +test_expect_failure 'test --all, --depth, and explicit head' '\n> +\t(\n> +\t\tcd client &&\n> +\t\tgit fetch-pack --no-progress --all --depth=1 .. refs/heads/A\n> +\t) >out-adh 2>error-adh\n> +'\n> +\n> +test_expect_failure 'test --all, --depth, and explicit tag' '\n> +\tgit tag OLDTAG refs/heads/B~5 &&\n> +\t(\n> +\t\tcd client &&\n> +\t\tgit fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG\n> +\t) >out-adt 2>error-adt\n> +'\n> +\n>  test_done\n"},{"id":"198727","messageId":"7vipblmwaq.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"1347171589-13327-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T20:56:29Z","receivedAt":"2012-09-10T20:56:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Instead of juggling <nr_heads,heads> (sometimes called\n> <nr_match,match>), pass around the list of references to be sought in\n> a single string_list variable called \"sought\".  Future commits will\n> make more use of string_list functionality.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n\nThe earlier bikeshedding-fest on variable names seems to have\nproduced a winner ;-) I think \"sought\" captures what it is about\nvery well.\n\n> diff --git a/fetch-pack.h b/fetch-pack.h\n> index 1dbe90f..a6a8a73 100644\n> --- a/fetch-pack.h\n> +++ b/fetch-pack.h\n> @@ -1,6 +1,8 @@\n>  #ifndef FETCH_PACK_H\n>  #define FETCH_PACK_H\n>  \n> +#include \"string-list.h\"\n> +\n>  struct fetch_pack_args {\n>  \tconst char *uploadpack;\n>  \tint unpacklimit;\n> @@ -21,8 +23,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       char **heads,\n> +\t\t       struct string_list *sought,\n>  \t\t       char **pack_lockfile);\n\nThis is a tangent, but I _think_ our header files ignore the dogma\nsome other projects follow that insists on each header file to be\nself sufficient, i.e.\n\n\tgcc fetch-pack.h\n\nshould pass.  Instead, our *.c files that include fetch-pack.h are\nresponsible for including everything the headers they include need.\nSo even though fetch-pack.h does not include run-command.h, it\ndeclares a function that takes \"struct child_process *\" in its\narguments.  The new \"struct string_list *\" falls into the same camp.\n\nGiven that, I'd prefer to see the scope of this patch series shrunk\nand have the caller include string-list.h, not here.\n\nUpdating the headers and sources so that each to be self sufficient\nis a different topic, and I do not think there is a consensus yet if\nwe want to go that route.\n"},{"id":"198730","messageId":"7v392pmv9f.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"1347171589-13327-10-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 09/14] filter_refs(): build refs list as we go","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T21:18:52Z","receivedAt":"2012-09-10T21:18:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Instead of temporarily storing matched refs to temporary array\n> \"return_refs\", simply append them to newlist as we go.  This changes\n> the order of references in newlist to strictly sorted if \"--all\" and\n> \"--depth\" and named references are all specified, but that usage is\n> broken anyway (see the last two tests in t5500).\n\nRemoves two warts (the temporary array in general, and the\nfastarray[] special case) with one patch.  Nicely done.\n"},{"id":"198739","messageId":"7vy5khlgk6.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"1347171589-13327-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T21:21:45Z","receivedAt":"2012-09-10T21:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> This patch series depends on the \"Add some string_list-related\n> functions\" series that I just submitted.\n>\n> This series is a significant rewrite of v2 based on the realization\n> that the <nr_heads,heads> pair that is passed around in these\n> functions is better expressed as a string_list.  I hope you will find\n> that this version is shorter and clearer than its predecessors, even\n> though its basic logic is mostly the same.\n>\n> Sorry for the false starts in v1 and v2 and the extra reviewing work\n> that this will cost.\n\nThanks for a pleasant read, nicely broken down into digestible\npieces.\n"},{"id":"198743","messageId":"504E6141.1080306@alum.mit.edu","threadId":"31482","inReplyTo":"7vmx0xmwr9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T21:53:05Z","receivedAt":"2012-09-10T21:53:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2012 10:46 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Document some bugs in \"git fetch-pack\":\n>>\n>> 1. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\n>> explicit existing non-tag reference to fetch, then it falsely reports\n>> that the reference was not found, even though it was fetched\n>> correctly.\n>>\n>> 2. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\n>> explicit existing tag reference to fetch, then it segfaults in\n>> filter_refs() because return_refs is used without having been\n>> initialized.\n> \n> I guess the first one is because \"all\" already marks the fetched one\n> \"used\", and does not allow the explicit one to match any unused one\n> from the other side?\n\nYes, more or less.  Spoiler: these failures are fixed later in the patch\nseries :-)\n\n> I wonder what happens when \"--all\" with an\n> explicit refspec that names non-existing ref is asked for (it should\n> notice that refs/heads/no-such-ref does not exist.  I do not know if\n> it is something that belongs to this set of new tests)?\n\nGood idea; I just wrote such tests.  They appear to pass regardless of\nthis patch series.  I will submit them after I am sure that I understand\nthem.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198746","messageId":"504E62DF.5030408@alum.mit.edu","threadId":"31482","inReplyTo":"7vhar7qd07.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-10T21:59:59Z","receivedAt":"2012-09-10T21:59:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2012 08:15 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Sun, Sep 09, 2012 at 03:20:18AM -0700, Junio C Hamano wrote:\n>>\n>>> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>>>\n>>>> This patch series depends on the \"Add some string_list-related\n>>>> functions\" series that I just submitted.\n>>>\n>>> Makes sense.  The only worry (without reading the series first) I\n>>> have is that the use of string list may make the responsiblity of\n>>> sorting the list fuzzier. I am guessing that we never sorted the\n>>> refs we asked to fetch (so that FETCH_HEAD comes out in an expected\n>>> order), so use of unsorted string list would be perfectly fine.\n>>\n>> I haven't read the series yet, but both the list of heads from the user\n>> and the list of heads from the remote should have been sorted by 4435968\n>> and 9e8e704f, respectively.\n> \n> OK.  As long as the sort order matches the order string-list\n> internally uses for its bisection search, it won't be a problem,\n> then.\n\nThe sorting is crucial but there is no bisection involved.  The sorted\nlinked-list of references available from the remote and the sorted\nstring_list of requested references are iterated through in parallel.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198750","messageId":"7vhar5leal.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"504E62DF.5030408@alum.mit.edu","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T22:10:42Z","receivedAt":"2012-09-10T22:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> OK.  As long as the sort order matches the order string-list\n>> internally uses for its bisection search, it won't be a problem,\n>> then.\n>\n> The sorting is crucial but there is no bisection involved.  The sorted\n> linked-list of references available from the remote and the sorted\n> string_list of requested references are iterated through in parallel.\n\nWhat I meant was that the order used by string-list is pretty much\ninternal to string-list implementation for its \"quickly locate where\nto insert\" bisection.  It happens to be the byte value order, I\nthink, but the point is whatever order it is, that has to match the\norder we keep references in the other data source you walk in\nparallel to match (i.e. the linked list of references).\n"},{"id":"199220","messageId":"5057167A.6040403@alum.mit.edu","threadId":"31482","inReplyTo":"7vipblmwaq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-17T12:24:26Z","receivedAt":"2012-09-17T12:24:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2012 10:56 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> diff --git a/fetch-pack.h b/fetch-pack.h\n>> index 1dbe90f..a6a8a73 100644\n>> --- a/fetch-pack.h\n>> +++ b/fetch-pack.h\n>> @@ -1,6 +1,8 @@\n>>  #ifndef FETCH_PACK_H\n>>  #define FETCH_PACK_H\n>>  \n>> +#include \"string-list.h\"\n>> +\n>>  struct fetch_pack_args {\n>>  \tconst char *uploadpack;\n>>  \tint unpacklimit;\n>> @@ -21,8 +23,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       char **heads,\n>> +\t\t       struct string_list *sought,\n>>  \t\t       char **pack_lockfile);\n> \n> This is a tangent, but I _think_ our header files ignore the dogma\n> some other projects follow that insists on each header file to be\n> self sufficient, i.e.\n> \n> \tgcc fetch-pack.h\n> \n> should pass.  Instead, our *.c files that include fetch-pack.h are\n> responsible for including everything the headers they include need.\n> So even though fetch-pack.h does not include run-command.h, it\n> declares a function that takes \"struct child_process *\" in its\n> arguments.  The new \"struct string_list *\" falls into the same camp.\n> \n> Given that, I'd prefer to see the scope of this patch series shrunk\n> and have the caller include string-list.h, not here.\n> \n> Updating the headers and sources so that each to be self sufficient\n> is a different topic, and I do not think there is a consensus yet if\n> we want to go that route.\n\nThe include line can just be deleted, because the three files that\ninclude it already get string-list.h from somewhere:\n\n* builtin/clone.c, builtin/fetch-pack.c\n  includes builtin.h\n  includes notes.h\n  includes string-list.h\n\n* transport.c\n  includes string-list.h\n\nPatch forthcoming.\n\nBut how far should this policy be taken?  It seems to me that strict\nadherence to the policy would dictate that *.h files should *never*\ninclude other git project files.\n\nFor example, while working on this, I noticed that notes.h includes\nstring-list.h and also contains a forward declaration for \"struct\nstring_list\".  Obviously, the latter could be deleted.  Alternatively,\nboth could probably be deleted by ensuring that the relevant *.c files\ninclude string-list.h before including notes.h.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"199224","messageId":"50571DC9.2030602@alum.mit.edu","threadId":"31482","inReplyTo":"7vhar5leal.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-17T12:55:37Z","receivedAt":"2012-09-17T12:55:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/11/2012 12:10 AM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>>> OK.  As long as the sort order matches the order string-list\n>>> internally uses for its bisection search, it won't be a problem,\n>>> then.\n>>\n>> The sorting is crucial but there is no bisection involved.  The sorted\n>> linked-list of references available from the remote and the sorted\n>> string_list of requested references are iterated through in parallel.\n> \n> What I meant was that the order used by string-list is pretty much\n> internal to string-list implementation for its \"quickly locate where\n> to insert\" bisection.  It happens to be the byte value order, I\n> think, but the point is whatever order it is, that has to match the\n> order we keep references in the other data source you walk in\n> parallel to match (i.e. the linked list of references).\n\nYes, your point is good that the two sort orders have to agree.\nCurrently, they both use strcmp() order, so the situation is OK.\n\nIt is interesting that you consider the sort order of string_list to be\nsomewhat of an internal implementation detail.  I had thought of its\ncurrent behavior as being the obvious thing and considered it part of\nthe API's contract.  For example, the current sort order is already\nobservable via the API through iteration or by calling\nprint_string_list().  Therefore I think that we should document the\nstrcmp() sort order as part of the string_list contract.  Patch coming soon.\n\nIf, at some future time, somebody wants a string_list that is sorted by\na different criterion, then the order should be determined via a\ncallback function specified by the user.  The callback function could\neven be stored in the string_list header, to allow such lists to be used\nin combination with the \"functions for sorted lists only\".\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"199282","messageId":"7vr4q0tmd3.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"50571DC9.2030602@alum.mit.edu","subject":"Re: [PATCH v3 00/14] Clean up how fetch_pack() handles the heads list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-17T20:39:36Z","receivedAt":"2012-09-17T20:39:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> It is interesting that you consider the sort order of string_list to be\n> somewhat of an internal implementation detail.  I had thought of its\n> current behavior as being the obvious thing and considered it part of\n> the API's contract.  For example, the current sort order is already\n> observable via the API through iteration or by calling\n> print_string_list().\n\nYou can prepare an unsorted string list that holds items in the\norder you desire, and call print_string_list() on it.  I do not\nthink we ever promised to keep using the same sort order when you\nallow the string list to use sorted insertion by calling the\n\"sorted\" API functions.\n"},{"id":"199300","messageId":"7vd31ks3ls.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"5057167A.6040403@alum.mit.edu","subject":"Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-17T22:10:07Z","receivedAt":"2012-09-17T22:10:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> But how far should this policy be taken?  It seems to me that strict\n> adherence to the policy would dictate that *.h files should *never*\n> include other git project files.\n\nI wouldn't call that a \"policy\".  It's something we think about when\nadding a new \"#include\" to see if it is worth adding and/or if it is\nthe right place to add it to reduce code churn.\n\nAs I said, what policy to pick and stick to is open to discussion,\nand I wanted to leave it outside the scope of this series.  As it\nhas been cooking in 'next', I do not think it is worth reverting the\ninclusion of \"string-list.h\" to delay this topic.  It is something\nthat can and should be cleaned up when we decide to pick the\ninclusion policy and enforce it.  If we choose to go in the other\ndirection, we would end up adding it back, so let's keep it as-is\nfor now.\n"},{"id":"199301","messageId":"20120917221710.GA555@sigill.intra.peff.net","threadId":"31482","inReplyTo":"7vd31ks3ls.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-17T22:17:10Z","receivedAt":"2012-09-17T22:17:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2012 at 03:10:07PM -0700, Junio C Hamano wrote:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n> > But how far should this policy be taken?  It seems to me that strict\n> > adherence to the policy would dictate that *.h files should *never*\n> > include other git project files.\n> \n> I wouldn't call that a \"policy\".  It's something we think about when\n> adding a new \"#include\" to see if it is worth adding and/or if it is\n> the right place to add it to reduce code churn.\n> \n> As I said, what policy to pick and stick to is open to discussion,\n> and I wanted to leave it outside the scope of this series.  As it\n> has been cooking in 'next', I do not think it is worth reverting the\n> inclusion of \"string-list.h\" to delay this topic.  It is something\n> that can and should be cleaned up when we decide to pick the\n> inclusion policy and enforce it.  If we choose to go in the other\n> direction, we would end up adding it back, so let's keep it as-is\n> for now.\n\nI will admit that I usually follow the opposite policy of what you have\nsuggested, and include dependent headers in the .h files. Mostly just\nbecause it makes things simpler for the user of the header file, and\nthere aren't really downsides (yes, you can have weird dependency-order\nissues, but in practice those are rare, and they generate very obvious\ncompile-time errors).\n\nWhat I think would be much more productive is breaking apart gigantic\nincludes like cache.h into more reasonable modules, which would mean\nless frequent recompilation when an uninteresting part of the header\nchanges. But git is reasonably fast to compile as it is, so I have never\nquite decided that it is worth the human effort to go in that direction.\n\n-Peff\n"},{"id":"199409","messageId":"7vehlzkqdr.fsf@alter.siamese.dyndns.org","threadId":"31482","inReplyTo":"20120917221710.GA555@sigill.intra.peff.net","subject":"Re: [PATCH v3 05/14] Change fetch_pack() and friends to take string_list arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-18T20:49:52Z","receivedAt":"2012-09-18T20:49:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> What I think would be much more productive is breaking apart gigantic\n> includes like cache.h into more reasonable modules, which would mean\n> less frequent recompilation when an uninteresting part of the header\n> changes.\n\nIdeally cache.h should cover what read-cache.c, sha1_file.c and\nsha1_name.c do and need.  The parts related to sha1_name.c may\ndeserve to have its own header file, as its scope is fairly wide and\nspans from low-level object name get_sha1_hex() to high level\nparsing like dwim_ref().  Right now, the header also has pieces that\nare needed only by connect.c and config.c (not even commented as\nsuch), pager, base85, alloc, trace, add, piece of diff, etc.\n\nAlso we may want to move helper functions that are generally useful\nand do not depend on specific command to more appropriate files.\n"},{"id":"199433","messageId":"012B3B00ED9F48E5B0A8A4E21B944754@PhilipOakley","threadId":"31482","inReplyTo":"504E6141.1080306@alum.mit.edu","subject":"Re: [PATCH v3 02/14] t5500: add tests of fetch-pack --all --depth=N $URL $REF","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2012-09-18T23:37:20Z","receivedAt":"2012-09-18T23:37:20Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Michael Haggerty\" <mhagger@alum.mit.edu>\nSent: Monday, September 10, 2012 10:53 PM\n> On 09/10/2012 10:46 PM, Junio C Hamano wrote:\n>> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>>\n>>> Document some bugs in \"git fetch-pack\":\n>>>\n>>> 1. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\n>>> explicit existing non-tag reference to fetch, then it falsely\n>>> reports\n>>> that the reference was not found, even though it was fetched\n>>> correctly.\n>>>\n>>> 2. If \"git fetch-pack\" is called with \"--all\", \"--depth\", and an\n>>> explicit existing tag reference to fetch, then it segfaults in\n>>> filter_refs() because return_refs is used without having been\n>>> initialized.\n>>\n\nThis probably isn't the right place in the series but I've just see a\nquestion on SO\nhttp://stackoverflow.com/questions/12475210/git-shallow-clone-along-with-branch\nwhich gives a user case that may or may not be covered by the\npack-fetch.\n\nThe user was looking for 'git clone --depth 1 -b master' and comparing\nit to a plain 'git clone --depth 1' and to 'git clone -b master' with\nthe follow up question \"That is, why when passing -b it seems like it's\nchanging behaviour of --depth ?\"\n\nPerhaps something to test out.\n\nPhilip\n"}]}