{"thread":{"id":"33931","subject":"[PATCH v2 00/25] Remove assumptions about each_ref_fn arg lifetimes","startedAt":"2013-05-25T09:07:59Z","lastAt":"2013-06-03T15:31:33Z","messageCount":40,"participants":["Michael Haggerty","Thomas Rast","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":2,"patchTotal":25},"messages":[{"id":"218508","messageId":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":null,"subject":"[PATCH v2 00/25] Remove assumptions about each_ref_fn arg lifetimes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:07:59Z","receivedAt":"2013-05-25T09:07:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is version two of the patch series.  Aside from addressing\nJunio's comments about the first version, it goes significantly\nfurther than v1:\n\nI did a manual audit of the 50 (!) functions that are used as an\neach_ref_fn callback to the for_each_ref()-style functions.  (I hope I\nhaven't missed any.)  I checked that they do not make the assumption\nthat the lifetimes of the refname and sha1 arguments extend past the\nduration of the callback invocation.  There were a number of callers\nthat got this wrong; I believe I have fixed them all.\n\nI also changed how object_array_entry manages its name field.  Like\nthe RFC in the first version of this patch series, I change\nadd_object_array_with_mode() to make a copy of the name before storing\nit in its field.  But (at Peff's suggestion) I also make an\noptimization like that of strbuf of not copying the name if it is the\nempty string, but rather using a pointer to a static empty string.\n\nAfter this patch series, the test suite runs without errors under\nvalgrind even when I change refs.c:do_one_ref() to pass temporary\ncopies of refname and sha1 to the callback functions then free them\nimmediately.  (Before this patch series, such a test failed in many\nplaces.)  But it is still not enough to make Peff's\njk/packed-refs-race series work correctly under the \"hyperactive\nrepository\" stress-test in which the packed-refs file is made to\nalways look stale,\n\n    refs.c:get_packed_refs():\n    -\tif (refs->packed &&\n    -\t    !stat_validity_check(&refs->packed_validity, packed_refs_file))\n    +\tif (refs->packed /* &&\n    +\t    !stat_validity_check(&refs->packed_validity, packed_refs_file) */)\n\n\nMichael Haggerty (25):\n  describe: make own copy of refname\n  fetch: make own copies of refnames\n  add_rev_cmdline(): make a copy of the name argument\n  builtin_diff_tree(): make it obvious that function wants two entries\n  cmd_diff(): use an object_array for holding trees\n  cmd_diff(): rename local variable \"list\" -> \"entry\"\n  cmd_diff(): make it obvious which cases are exclusive of each other\n  revision: split some overly-long lines\n  object_array: add function object_array_filter()\n  revision: use object_array_filter() in implementation of gc_boundary()\n  object_array_remove_duplicates(): rewrite to reduce copying\n  fsck: don't put a void*-shaped peg in a char*-shaped hole\n  find_first_merges(): initialize merges variable using initializer\n  find_first_merges(): remove unnecessary code\n  object_array_entry: fix memory handling of the name field\n  do_fetch(): reduce scope of peer_item\n  do_fetch(): clean up existing_refs before exiting\n  add_existing(): do not retain a reference to sha1\n  show_head_ref(): do not shadow name of argument\n  show_head_ref(): rename first parameter to \"refname\"\n  string_list_add_one_ref(): rename first parameter to \"refname\"\n  string_list_add_refs_by_glob(): add a comment about memory management\n  exclude_existing(): set existing_refs.strdup_strings\n  register_ref(): make a copy of the bad reference SHA-1\n  refs: document the lifetime of the args passed to each_ref_fn\n\n bisect.c           |  5 ++--\n builtin/describe.c |  6 +++--\n builtin/diff.c     | 69 ++++++++++++++++++++++++++---------------------------\n builtin/fetch.c    | 29 ++++++++++++----------\n builtin/fsck.c     |  2 +-\n builtin/show-ref.c |  2 +-\n bundle.c           |  2 +-\n http-backend.c     |  6 ++---\n notes.c            |  9 ++++---\n object.c           | 70 ++++++++++++++++++++++++++++++++++++++++++++----------\n object.h           | 25 +++++++++++++++++--\n refs.h             | 22 ++++++++++++-----\n revision.c         | 64 +++++++++++++++++++++++++++++--------------------\n revision.h         | 32 ++++++++++++++++---------\n submodule.c        |  6 ++---\n 15 files changed, 228 insertions(+), 121 deletions(-)\n\n-- \n1.8.2.3\n"},{"id":"218533","messageId":"1369472904-12875-2-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 01/25] describe: make own copy of refname","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:00Z","receivedAt":"2013-05-25T09:08:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Do not retain a reference to the refname passed to the each_ref_fn\ncallback get_name(), because there is no guarantee of the lifetimes of\nthese names.  Instead, make a local copy when needed.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/describe.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 6636a68..3dc09eb 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -42,7 +42,7 @@ struct commit_name {\n \tunsigned prio:2; /* annotated tag = 2, tag = 1, head = 0 */\n \tunsigned name_checked:1;\n \tunsigned char sha1[20];\n-\tconst char *path;\n+\tchar *path;\n };\n static const char *prio_names[] = {\n \t\"head\", \"lightweight\", \"annotated\",\n@@ -126,12 +126,14 @@ static void add_to_known_names(const char *path,\n \t\t\t} else {\n \t\t\t\te->next = NULL;\n \t\t\t}\n+\t\t\te->path = NULL;\n \t\t}\n \t\te->tag = tag;\n \t\te->prio = prio;\n \t\te->name_checked = 0;\n \t\thashcpy(e->sha1, sha1);\n-\t\te->path = path;\n+\t\tfree(e->path);\n+\t\te->path = xstrdup(path);\n \t}\n }\n \n-- \n1.8.2.3\n"},{"id":"218530","messageId":"1369472904-12875-3-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 02/25] fetch: make own copies of refnames","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:01Z","receivedAt":"2013-05-25T09:08:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Do not retain references to refnames passed to the each_ref_fn\ncallback add_existing(), because their lifetime is not guaranteed.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4b6b1df..f949115 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -590,7 +590,7 @@ static void find_non_local_tags(struct transport *transport,\n \t\t\tstruct ref **head,\n \t\t\tstruct ref ***tail)\n {\n-\tstruct string_list existing_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n \tstruct string_list remote_refs = STRING_LIST_INIT_NODUP;\n \tconst struct ref *ref;\n \tstruct string_list_item *item = NULL;\n@@ -693,7 +693,7 @@ static int truncate_fetch_head(void)\n static int do_fetch(struct transport *transport,\n \t\t    struct refspec *refs, int ref_count)\n {\n-\tstruct string_list existing_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *peer_item = NULL;\n \tstruct ref *ref_map;\n \tstruct ref *rm;\n-- \n1.8.2.3\n"},{"id":"218526","messageId":"1369472904-12875-4-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 03/25] add_rev_cmdline(): make a copy of the name argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:02Z","receivedAt":"2013-05-25T09:08:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of assuming that the memory pointed to by the name argument\nwill live forever, make a local copy of it before storing it in the\nref_cmdline_info.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n revision.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex a67b615..25e424c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -898,6 +898,10 @@ static int limit_list(struct rev_info *revs)\n \treturn 0;\n }\n \n+/*\n+ * Add an entry to refs->cmdline with the specified information.\n+ * *name is copied.\n+ */\n static void add_rev_cmdline(struct rev_info *revs,\n \t\t\t    struct object *item,\n \t\t\t    const char *name,\n@@ -909,7 +913,7 @@ static void add_rev_cmdline(struct rev_info *revs,\n \n \tALLOC_GROW(info->rev, nr + 1, info->alloc);\n \tinfo->rev[nr].item = item;\n-\tinfo->rev[nr].name = name;\n+\tinfo->rev[nr].name = xstrdup(name);\n \tinfo->rev[nr].whence = whence;\n \tinfo->rev[nr].flags = flags;\n \tinfo->nr++;\n-- \n1.8.2.3\n"},{"id":"218509","messageId":"1369472904-12875-5-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 04/25] builtin_diff_tree(): make it obvious that function wants two entries","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:03Z","receivedAt":"2013-05-25T09:08:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of accepting an array and using exactly two elements from the\narray, take two single (struct object_array_entry *) arguments.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/diff.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8c2af6c..abdd613 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -153,7 +153,8 @@ static int builtin_diff_index(struct rev_info *revs,\n \n static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t     int argc, const char **argv,\n-\t\t\t     struct object_array_entry *ent)\n+\t\t\t     struct object_array_entry *ent0,\n+\t\t\t     struct object_array_entry *ent1)\n {\n \tconst unsigned char *(sha1[2]);\n \tint swap = 0;\n@@ -161,13 +162,14 @@ static int builtin_diff_tree(struct rev_info *revs,\n \tif (argc > 1)\n \t\tusage(builtin_diff_usage);\n \n-\t/* We saw two trees, ent[0] and ent[1].\n-\t * if ent[1] is uninteresting, they are swapped\n+\t/*\n+\t * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,\n+\t * swap them.\n \t */\n-\tif (ent[1].item->flags & UNINTERESTING)\n+\tif (ent1->item->flags & UNINTERESTING)\n \t\tswap = 1;\n-\tsha1[swap] = ent[0].item->sha1;\n-\tsha1[1-swap] = ent[1].item->sha1;\n+\tsha1[swap] = ent0->item->sha1;\n+\tsha1[1-swap] = ent1->item->sha1;\n \tdiff_tree_sha1(sha1[0], sha1[1], \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n \treturn 0;\n@@ -403,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \telse if (ents == 1)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n \telse if (ents == 2)\n-\t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n+\t\tresult = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[1]);\n \telse if (ent[0].item->flags & UNINTERESTING) {\n \t\t/*\n \t\t * diff A...B where there is at least one merge base\n@@ -412,8 +414,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t * between the base and B.  Note that we pick one\n \t\t * merge base at random if there are more than one.\n \t\t */\n-\t\tent[1] = ent[ents-1];\n-\t\tresult = builtin_diff_tree(&rev, argc, argv, ent);\n+\t\tresult = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[ents-1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t       ent, ents);\n-- \n1.8.2.3\n"},{"id":"218511","messageId":"1369472904-12875-6-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 05/25] cmd_diff(): use an object_array for holding trees","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:04Z","receivedAt":"2013-05-25T09:08:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change cmd_diff() to use a (struct object_array) for holding the trees\nthat it accumulates, rather than rolling its own equivalent.\n\nIncidentally, this change removes a hard-coded limit of 100 trees in\ncombined diff, not that it matters in practice.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/diff.c | 37 ++++++++++++++++++-------------------\n 1 file changed, 18 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex abdd613..661fdde 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -253,8 +253,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n \tstruct rev_info rev;\n-\tstruct object_array_entry ent[100];\n-\tint ents = 0, blobs = 0, paths = 0;\n+\tstruct object_array ent = OBJECT_ARRAY_INIT;\n+\tint blobs = 0, paths = 0;\n \tconst char *path = NULL;\n \tstruct blobinfo blob[2];\n \tint nongit;\n@@ -351,13 +351,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tif (obj->type == OBJ_COMMIT)\n \t\t\tobj = &((struct commit *)obj)->tree->object;\n \t\tif (obj->type == OBJ_TREE) {\n-\t\t\tif (ARRAY_SIZE(ent) <= ents)\n-\t\t\t\tdie(_(\"more than %d trees given: '%s'\"),\n-\t\t\t\t    (int) ARRAY_SIZE(ent), name);\n \t\t\tobj->flags |= flags;\n-\t\t\tent[ents].item = obj;\n-\t\t\tent[ents].name = name;\n-\t\t\tents++;\n+\t\t\tadd_object_array(obj, name, &ent);\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\n@@ -381,7 +376,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t/*\n \t * Now, do the arguments look reasonable?\n \t */\n-\tif (!ents) {\n+\tif (!ent.nr) {\n \t\tswitch (blobs) {\n \t\tcase 0:\n \t\t\tresult = builtin_diff_files(&rev, argc, argv);\n@@ -402,22 +397,26 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \telse if (blobs)\n \t\tusage(builtin_diff_usage);\n-\telse if (ents == 1)\n+\telse if (ent.nr == 1)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n-\telse if (ents == 2)\n-\t\tresult = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[1]);\n-\telse if (ent[0].item->flags & UNINTERESTING) {\n+\telse if (ent.nr == 2)\n+\t\tresult = builtin_diff_tree(&rev, argc, argv,\n+\t\t\t\t\t   &ent.objects[0], &ent.objects[1]);\n+\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n \t\t/*\n \t\t * diff A...B where there is at least one merge base\n-\t\t * between A and B.  We have ent[0] == merge-base,\n-\t\t * ent[ents-2] == A, and ent[ents-1] == B.  Show diff\n-\t\t * between the base and B.  Note that we pick one\n-\t\t * merge base at random if there are more than one.\n+\t\t * between A and B.  We have ent.objects[0] ==\n+\t\t * merge-base, ent.objects[ents-2] == A, and\n+\t\t * ent.objects[ents-1] == B.  Show diff between the\n+\t\t * base and B.  Note that we pick one merge base at\n+\t\t * random if there are more than one.\n \t\t */\n-\t\tresult = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[ents-1]);\n+\t\tresult = builtin_diff_tree(&rev, argc, argv,\n+\t\t\t\t\t   &ent.objects[0],\n+\t\t\t\t\t   &ent.objects[ent.nr-1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n-\t\t\t\t\t       ent, ents);\n+\t\t\t\t\t       ent.objects, ent.nr);\n \tresult = diff_result_code(&rev.diffopt, result);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\n-- \n1.8.2.3\n"},{"id":"218510","messageId":"1369472904-12875-7-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 06/25] cmd_diff(): rename local variable \"list\" -> \"entry\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:05Z","receivedAt":"2013-05-25T09:08:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's not a list, it's an array entry.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/diff.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 661fdde..84243d9 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -339,9 +339,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \n \tfor (i = 0; i < rev.pending.nr; i++) {\n-\t\tstruct object_array_entry *list = rev.pending.objects+i;\n-\t\tstruct object *obj = list->item;\n-\t\tconst char *name = list->name;\n+\t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n+\t\tstruct object *obj = entry->item;\n+\t\tconst char *name = entry->name;\n \t\tint flags = (obj->flags & UNINTERESTING);\n \t\tif (!obj->parsed)\n \t\t\tobj = parse_object(obj->sha1);\n@@ -360,7 +360,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n \t\t\thashcpy(blob[blobs].sha1, obj->sha1);\n \t\t\tblob[blobs].name = name;\n-\t\t\tblob[blobs].mode = list->mode;\n+\t\t\tblob[blobs].mode = entry->mode;\n \t\t\tblobs++;\n \t\t\tcontinue;\n \n-- \n1.8.2.3\n"},{"id":"218532","messageId":"1369472904-12875-8-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 07/25] cmd_diff(): make it obvious which cases are exclusive of each other","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:06Z","receivedAt":"2013-05-25T09:08:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"At first glance the OBJ_COMMIT, OBJ_TREE, and OBJ_BLOB cases look like\nthey might be mutually exclusive.  But the OBJ_COMMIT case doesn't end\nthe loop iteration with \"continue\" like the other two cases, but\nrather falls through.  So use if...else if...else construct to make it\nmore obvious that only the last two cases are mutually exclusive.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/diff.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 84243d9..9fc273d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -350,22 +350,21 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"invalid object '%s' given.\"), name);\n \t\tif (obj->type == OBJ_COMMIT)\n \t\t\tobj = &((struct commit *)obj)->tree->object;\n+\n \t\tif (obj->type == OBJ_TREE) {\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (obj->type == OBJ_BLOB) {\n+\t\t} else if (obj->type == OBJ_BLOB) {\n \t\t\tif (2 <= blobs)\n \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n \t\t\thashcpy(blob[blobs].sha1, obj->sha1);\n \t\t\tblob[blobs].name = name;\n \t\t\tblob[blobs].mode = entry->mode;\n \t\t\tblobs++;\n-\t\t\tcontinue;\n \n+\t\t} else {\n+\t\t\tdie(_(\"unhandled object '%s' given.\"), name);\n \t\t}\n-\t\tdie(_(\"unhandled object '%s' given.\"), name);\n \t}\n \tif (rev.prune_data.nr) {\n \t\tif (!path)\n-- \n1.8.2.3\n"},{"id":"218531","messageId":"1369472904-12875-9-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 08/25] revision: split some overly-long lines","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:07Z","receivedAt":"2013-05-25T09:08:07Z","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 revision.c | 20 ++++++++++++++------\n revision.h | 32 +++++++++++++++++++++-----------\n 2 files changed, 35 insertions(+), 17 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 25e424c..8ac88d6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -70,7 +70,8 @@ static int show_path_truncated(FILE *out, const struct name_path *path)\n \treturn ours || emitted;\n }\n \n-void show_object_with_name(FILE *out, struct object *obj, const struct name_path *path, const char *component)\n+void show_object_with_name(FILE *out, struct object *obj,\n+\t\t\t   const struct name_path *path, const char *component)\n {\n \tstruct name_path leaf;\n \tleaf.up = (struct name_path *)path;\n@@ -186,7 +187,9 @@ void mark_parents_uninteresting(struct commit *commit)\n \t}\n }\n \n-static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)\n+static void add_pending_object_with_mode(struct rev_info *revs,\n+\t\t\t\t\t struct object *obj,\n+\t\t\t\t\t const char *name, unsigned mode)\n {\n \tif (!obj)\n \t\treturn;\n@@ -209,7 +212,8 @@ static void add_pending_object_with_mode(struct rev_info *revs, struct object *o\n \tadd_object_array_with_mode(obj, name, &revs->pending, mode);\n }\n \n-void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n+void add_pending_object(struct rev_info *revs,\n+\t\t\tstruct object *obj, const char *name)\n {\n \tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n }\n@@ -226,7 +230,9 @@ void add_head_to_pending(struct rev_info *revs)\n \tadd_pending_object(revs, obj, \"HEAD\");\n }\n \n-static struct object *get_reference(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags)\n+static struct object *get_reference(struct rev_info *revs, const char *name,\n+\t\t\t\t    const unsigned char *sha1,\n+\t\t\t\t    unsigned int flags)\n {\n \tstruct object *object;\n \n@@ -247,7 +253,8 @@ void add_pending_sha1(struct rev_info *revs, const char *name,\n \tadd_pending_object(revs, object, name);\n }\n \n-static struct commit *handle_commit(struct rev_info *revs, struct object *object, const char *name)\n+static struct commit *handle_commit(struct rev_info *revs,\n+\t\t\t\t    struct object *object, const char *name)\n {\n \tunsigned long flags = object->flags;\n \n@@ -368,7 +375,8 @@ static void file_change(struct diff_options *options,\n \tDIFF_OPT_SET(options, HAS_CHANGES);\n }\n \n-static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct commit *commit)\n+static int rev_compare_tree(struct rev_info *revs,\n+\t\t\t    struct commit *parent, struct commit *commit)\n {\n \tstruct tree *t1 = parent->tree;\n \tstruct tree *t2 = commit->tree;\ndiff --git a/revision.h b/revision.h\nindex 01bd2b7..9628465 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -195,19 +195,23 @@ struct setup_revision_opt {\n };\n \n extern void init_revisions(struct rev_info *revs, const char *prefix);\n-extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *);\n+extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,\n+\t\t\t   struct setup_revision_opt *);\n extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n-\t\t\t\t const struct option *options,\n-\t\t\t\t const char * const usagestr[]);\n+\t\t\t       const struct option *options,\n+\t\t\t       const char * const usagestr[]);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n-extern int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt);\n+extern int handle_revision_arg(const char *arg, struct rev_info *revs,\n+\t\t\t       int flags, unsigned revarg_opt);\n \n extern void reset_revision_walk(void);\n extern int prepare_revision_walk(struct rev_info *revs);\n extern struct commit *get_revision(struct rev_info *revs);\n-extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);\n-extern void put_revision_mark(const struct rev_info *revs, const struct commit *commit);\n+extern char *get_revision_mark(const struct rev_info *revs,\n+\t\t\t       const struct commit *commit);\n+extern void put_revision_mark(const struct rev_info *revs,\n+\t\t\t      const struct commit *commit);\n \n extern void mark_parents_uninteresting(struct commit *commit);\n extern void mark_tree_uninteresting(struct tree *tree);\n@@ -220,15 +224,19 @@ struct name_path {\n \n char *path_name(const struct name_path *path, const char *name);\n \n-extern void show_object_with_name(FILE *, struct object *, const struct name_path *, const char *);\n+extern void show_object_with_name(FILE *, struct object *,\n+\t\t\t\t  const struct name_path *, const char *);\n \n extern void add_object(struct object *obj,\n \t\t       struct object_array *p,\n \t\t       struct name_path *path,\n \t\t       const char *name);\n \n-extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);\n-extern void add_pending_sha1(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags);\n+extern void add_pending_object(struct rev_info *revs,\n+\t\t\t       struct object *obj, const char *name);\n+extern void add_pending_sha1(struct rev_info *revs,\n+\t\t\t     const char *name, const unsigned char *sha1,\n+\t\t\t     unsigned int flags);\n \n extern void add_head_to_pending(struct rev_info *);\n \n@@ -238,7 +246,9 @@ enum commit_action {\n \tcommit_error\n };\n \n-extern enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit);\n-extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);\n+extern enum commit_action get_commit_action(struct rev_info *revs,\n+\t\t\t\t\t    struct commit *commit);\n+extern enum commit_action simplify_commit(struct rev_info *revs,\n+\t\t\t\t\t  struct commit *commit);\n \n #endif\n-- \n1.8.2.3\n"},{"id":"218528","messageId":"1369472904-12875-10-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 09/25] object_array: add function object_array_filter()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:08Z","receivedAt":"2013-05-25T09:08:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a function that allows unwanted entries in an object_array to be\nremoved.  This encapsulation is a step towards giving object_array\nownership of its entries' name memory.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n object.c | 16 ++++++++++++++++\n object.h | 11 +++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/object.c b/object.c\nindex 20703f5..fcd4a82 100644\n--- a/object.c\n+++ b/object.c\n@@ -278,6 +278,22 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n \tarray->nr = ++nr;\n }\n \n+void object_array_filter(struct object_array *array,\n+\t\t\t object_array_each_func_t want, void *cb_data)\n+{\n+\tunsigned nr = array->nr, src, dst;\n+\tstruct object_array_entry *objects = array->objects;\n+\n+\tfor (src = dst = 0; src < nr; src++) {\n+\t\tif (want(&objects[src], cb_data)) {\n+\t\t\tif (src != dst)\n+\t\t\t\tobjects[dst] = objects[src];\n+\t\t\tdst++;\n+\t\t}\n+\t}\n+\tarray->nr = dst;\n+}\n+\n void object_array_remove_duplicates(struct object_array *array)\n {\n \tunsigned int ref, src, dst;\ndiff --git a/object.h b/object.h\nindex 97d384b..0d39ff4 100644\n--- a/object.h\n+++ b/object.h\n@@ -85,6 +85,17 @@ int object_list_contains(struct object_list *list, struct object *obj);\n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n+\n+typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n+\n+/*\n+ * Apply want to each entry in array, retaining only the entries for\n+ * which the function returns true.  Preserve the order of the entries\n+ * that are retained.\n+ */\n+void object_array_filter(struct object_array *array,\n+\t\t\t object_array_each_func_t want, void *cb_data);\n+\n void object_array_remove_duplicates(struct object_array *);\n \n void clear_object_flags(unsigned flags);\n-- \n1.8.2.3\n"},{"id":"218512","messageId":"1369472904-12875-11-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 10/25] revision: use object_array_filter() in implementation of gc_boundary()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:09Z","receivedAt":"2013-05-25T09:08:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use object_array_filter(), which will soon be made smarter about\ncleaning up discarded entries properly.  Also add a function comment.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nThis version changes the test to \"nr == alloc\" for clarity, but\ndoesn't move the test to the caller as did v1 of the patch series.\n\n revision.c | 32 +++++++++++++++-----------------\n 1 file changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8ac88d6..be73cb4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2435,25 +2435,23 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \treturn NULL;\n }\n \n-static void gc_boundary(struct object_array *array)\n+/*\n+ * Return true for entries that have not yet been shown.  (This is an\n+ * object_array_each_func_t.)\n+ */\n+static int entry_unshown(struct object_array_entry *entry, void *cb_data_unused)\n {\n-\tunsigned nr = array->nr;\n-\tunsigned alloc = array->alloc;\n-\tstruct object_array_entry *objects = array->objects;\n+\treturn !(entry->item->flags & SHOWN);\n+}\n \n-\tif (alloc <= nr) {\n-\t\tunsigned i, j;\n-\t\tfor (i = j = 0; i < nr; i++) {\n-\t\t\tif (objects[i].item->flags & SHOWN)\n-\t\t\t\tcontinue;\n-\t\t\tif (i != j)\n-\t\t\t\tobjects[j] = objects[i];\n-\t\t\tj++;\n-\t\t}\n-\t\tfor (i = j; i < nr; i++)\n-\t\t\tobjects[i].item = NULL;\n-\t\tarray->nr = j;\n-\t}\n+/*\n+ * If array is on the verge of a realloc, garbage-collect any entries\n+ * that have already been shown to try to free up some space.\n+ */\n+static void gc_boundary(struct object_array *array)\n+{\n+\tif (array->nr == array->alloc)\n+\t\tobject_array_filter(array, entry_unshown, NULL);\n }\n \n static void create_boundary_commit_list(struct rev_info *revs)\n-- \n1.8.2.3\n"},{"id":"218515","messageId":"1369472904-12875-12-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 11/25] object_array_remove_duplicates(): rewrite to reduce copying","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:10Z","receivedAt":"2013-05-25T09:08:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old version copied one entry to its destination position, then\ndeleted any matching entries from the tail of the array.  This\nrequired the tail of the array to be copied multiple times.  It didn't\naffect the complexity of the algorithm because the whole tail has to\nbe searched through anyway.  But all the copying was unnecessary.\n\nInstead, check for the existence of an entry with the same name in the\n*head* of the list before copying an entry to its final position.\nThis way each entry has to be copied at most one time.\n\nExtract a helper function contains_name() to do a bit of the work.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n object.c | 32 +++++++++++++++++++++-----------\n object.h |  6 +++++-\n 2 files changed, 26 insertions(+), 12 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex fcd4a82..10b5349 100644\n--- a/object.c\n+++ b/object.c\n@@ -294,22 +294,32 @@ void object_array_filter(struct object_array *array,\n \tarray->nr = dst;\n }\n \n+/*\n+ * Return true iff array already contains an entry with name.\n+ */\n+static int contains_name(struct object_array *array, const char *name)\n+{\n+\tunsigned nr = array->nr, i;\n+\tstruct object_array_entry *object = array->objects;\n+\n+\tfor (i = 0; i < nr; i++, object++)\n+\t\tif (!strcmp(object->name, name))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n void object_array_remove_duplicates(struct object_array *array)\n {\n-\tunsigned int ref, src, dst;\n+\tunsigned nr = array->nr, src;\n \tstruct object_array_entry *objects = array->objects;\n \n-\tfor (ref = 0; ref + 1 < array->nr; ref++) {\n-\t\tfor (src = ref + 1, dst = src;\n-\t\t     src < array->nr;\n-\t\t     src++) {\n-\t\t\tif (!strcmp(objects[ref].name, objects[src].name))\n-\t\t\t\tcontinue;\n-\t\t\tif (src != dst)\n-\t\t\t\tobjects[dst] = objects[src];\n-\t\t\tdst++;\n+\tarray->nr = 0;\n+\tfor (src = 0; src < nr; src++) {\n+\t\tif (!contains_name(array, objects[src].name)) {\n+\t\t\tif (src != array->nr)\n+\t\t\t\tobjects[array->nr] = objects[src];\n+\t\t\tarray->nr++;\n \t\t}\n-\t\tarray->nr = dst;\n \t}\n }\n \ndiff --git a/object.h b/object.h\nindex 0d39ff4..6c1c27f 100644\n--- a/object.h\n+++ b/object.h\n@@ -96,7 +96,11 @@ typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n void object_array_filter(struct object_array *array,\n \t\t\t object_array_each_func_t want, void *cb_data);\n \n-void object_array_remove_duplicates(struct object_array *);\n+/*\n+ * Remove from array all but the first entry with a given name.\n+ * Warning: this function uses an O(N^2) algorithm.\n+ */\n+void object_array_remove_duplicates(struct object_array *array);\n \n void clear_object_flags(unsigned flags);\n \n-- \n1.8.2.3\n"},{"id":"218514","messageId":"1369472904-12875-13-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 12/25] fsck: don't put a void*-shaped peg in a char*-shaped hole","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:11Z","receivedAt":"2013-05-25T09:08:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The source of this nonsense was\n\n    04d3975937 fsck: reduce stack footprint\n\n, which wedged a pointer to parent into the object_array_entry's name\nfield.  The parent pointer was passed to traverse_one_object(), even\nthough that function *didn't use it*.\n\nThe useless code has been deleted over time.  Commit\n\n    a1cdc25172 fsck: drop unused parameter from traverse_one_object()\n\nremoved the parent pointer from traverse_one_object()'s\nsignature. Commit\n\n    c0aa335c95 Remove unused variables\n\nremoved the code that read the parent pointer back out of the name\nfield.\n\nThis commit takes the last step: don't write the parent pointer into\nthe name field in the first place.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex bb9a2cd..9909b6d 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -112,7 +112,7 @@ static int mark_object(struct object *obj, int type, void *data)\n \t\treturn 1;\n \t}\n \n-\tadd_object_array(obj, (void *) parent, &pending);\n+\tadd_object_array(obj, NULL, &pending);\n \treturn 0;\n }\n \n-- \n1.8.2.3\n"},{"id":"218527","messageId":"1369472904-12875-14-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 13/25] find_first_merges(): initialize merges variable using initializer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:12Z","receivedAt":"2013-05-25T09:08:12Z","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 submodule.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e728025..b837c04 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -846,7 +846,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \t\tstruct commit *a, struct commit *b)\n {\n \tint i, j;\n-\tstruct object_array merges;\n+\tstruct object_array merges = OBJECT_ARRAY_INIT;\n \tstruct commit *commit;\n \tint contains_another;\n \n@@ -856,7 +856,6 @@ static int find_first_merges(struct object_array *result, const char *path,\n \tstruct rev_info revs;\n \tstruct setup_revision_opt rev_opts;\n \n-\tmemset(&merges, 0, sizeof(merges));\n \tmemset(result, 0, sizeof(struct object_array));\n \tmemset(&rev_opts, 0, sizeof(rev_opts));\n \n-- \n1.8.2.3\n"},{"id":"218529","messageId":"1369472904-12875-15-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 14/25] find_first_merges(): remove unnecessary code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:13Z","receivedAt":"2013-05-25T09:08:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"No names are ever set for the object_array_entries in merges, so there\nis no need to pretend to copy them to the result array.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n submodule.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b837c04..ad476ce 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -893,8 +893,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \t\t}\n \n \t\tif (!contains_another)\n-\t\t\tadd_object_array(merges.objects[i].item,\n-\t\t\t\t\t merges.objects[i].name, result);\n+\t\t\tadd_object_array(merges.objects[i].item, NULL, result);\n \t}\n \n \tfree(merges.objects);\n-- \n1.8.2.3\n"},{"id":"218524","messageId":"1369472904-12875-16-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 15/25] object_array_entry: fix memory handling of the name field","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:14Z","receivedAt":"2013-05-25T09:08:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, the memory management of the object_array_entry::name\nfield was inconsistent and undocumented.  object_array_entries are\nultimately created by a single function, add_object_array_with_mode(),\nwhich has an argument \"const char *name\".  This function used to\nsimply set the name field to reference the string pointed to by the\nname parameter, and nobody on the object_array side ever freed the\nmemory.  Thus, it assumed that the memory for the name field would be\nmanaged by the caller, and that the lifetime of that string would be\nat least as long as the lifetime of the object_array_entry.  But\ncallers were inconsistent:\n\n* Some passed pointers to constant strings or argv entries, which was\n  OK.\n\n* Some passed pointers to newly-allocated memory, but didn't arrange\n  for the memory ever to be freed.\n\n* Some passed the return value of sha1_to_hex(), which is a pointer to\n  a statically-allocated buffer that can be overwritten at any time.\n\n* Some passed pointers to refnames that they received from a\n  for_each_ref()-type iteration, but the lifetimes of such refnames is\n  not guaranteed by the refs API.\n\nBring consistency to this mess by changing object_array to make its\nown copy for the object_array_entry::name field and free this memory\nwhen an object_array_entry is deleted from the array.\n\nMany callers were passing the empty string as the name parameter, so\nas a performance optimization, treat the empty string specially.\nInstead of making a copy, store a pointer to a statically-allocated\nempty string to object_array_entry::name.  When deleting such an\nentry, skip the free().\n\nChange the callers that were already passing copies to\nadd_object_array_with_mode() to either skip the copy, or (if the\nmemory needed to be allocated anyway) freeing the memory itself.\n\nA part of this commit effectively reverts\n\n    70d26c6e76 read_revisions_from_stdin: make copies for handle_revision_arg\n\nbecause the copying introduced by that commit (which is still\nnecessary) is now done at a deeper level.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n bundle.c   |  2 +-\n object.c   | 26 +++++++++++++++++++++++---\n object.h   |  8 +++++++-\n revision.c |  6 ++++--\n 4 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 4b0e5cd..3d64311 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -281,7 +281,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\n-\t\t\t\tadd_pending_object(&revs, object, xstrdup(buf.buf));\n+\t\t\t\tadd_pending_object(&revs, object, buf.buf);\n \t\t\t}\n \t\t} else if (!get_sha1_hex(buf.buf, sha1)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\ndiff --git a/object.c b/object.c\nindex 10b5349..e4ff714 100644\n--- a/object.c\n+++ b/object.c\n@@ -260,11 +260,18 @@ void add_object_array(struct object *obj, const char *name, struct object_array\n \tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n }\n \n+/*\n+ * A zero-length string to which object_array_entry::name can be\n+ * initialized without requiring a malloc/free.\n+ */\n+char object_array_slopbuf[1];\n+\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n {\n \tunsigned nr = array->nr;\n \tunsigned alloc = array->alloc;\n \tstruct object_array_entry *objects = array->objects;\n+\tstruct object_array_entry *entry;\n \n \tif (nr >= alloc) {\n \t\talloc = (alloc + 32) * 2;\n@@ -272,9 +279,16 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n \t\tarray->alloc = alloc;\n \t\tarray->objects = objects;\n \t}\n-\tobjects[nr].item = obj;\n-\tobjects[nr].name = name;\n-\tobjects[nr].mode = mode;\n+\tentry = &objects[nr];\n+\tentry->item = obj;\n+\tif (!name)\n+\t\tentry->name = NULL;\n+\telse if (!*name)\n+\t\t/* Use our own empty string instead of allocating one: */\n+\t\tentry->name = object_array_slopbuf;\n+\telse\n+\t\tentry->name = xstrdup(name);\n+\tentry->mode = mode;\n \tarray->nr = ++nr;\n }\n \n@@ -289,6 +303,9 @@ void object_array_filter(struct object_array *array,\n \t\t\tif (src != dst)\n \t\t\t\tobjects[dst] = objects[src];\n \t\t\tdst++;\n+\t\t} else {\n+\t\t\tif (objects[src].name != object_array_slopbuf)\n+\t\t\t\tfree(objects[src].name);\n \t\t}\n \t}\n \tarray->nr = dst;\n@@ -319,6 +336,9 @@ void object_array_remove_duplicates(struct object_array *array)\n \t\t\tif (src != array->nr)\n \t\t\t\tobjects[array->nr] = objects[src];\n \t\t\tarray->nr++;\n+\t\t} else {\n+\t\t\tif (objects[src].name != object_array_slopbuf)\n+\t\t\t\tfree(objects[src].name);\n \t\t}\n \t}\n }\ndiff --git a/object.h b/object.h\nindex 6c1c27f..2ff68c5 100644\n--- a/object.h\n+++ b/object.h\n@@ -11,7 +11,13 @@ struct object_array {\n \tunsigned int alloc;\n \tstruct object_array_entry {\n \t\tstruct object *item;\n-\t\tconst char *name;\n+\t\t/*\n+\t\t * name or NULL.  If non-NULL, the memory pointed to\n+\t\t * is owned by this object *except* if it points at\n+\t\t * object_array_slopbuf, which is a static copy of the\n+\t\t * empty string.\n+\t\t */\n+\t\tchar *name;\n \t\tunsigned mode;\n \t} *objects;\n };\ndiff --git a/revision.c b/revision.c\nindex be73cb4..4aeda33 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -88,7 +88,9 @@ void add_object(struct object *obj,\n \t\tstruct name_path *path,\n \t\tconst char *name)\n {\n-\tadd_object_array(obj, path_name(path, name), p);\n+\tchar *pn = path_name(path, name);\n+\tadd_object_array(obj, pn, p);\n+\tfree(pn);\n }\n \n static void mark_blob_uninteresting(struct blob *blob)\n@@ -1288,7 +1290,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t}\n \t\t\tdie(\"options not supported in --stdin mode\");\n \t\t}\n-\t\tif (handle_revision_arg(xstrdup(sb.buf), revs, 0,\n+\t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\n-- \n1.8.2.3\n"},{"id":"218513","messageId":"1369472904-12875-17-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 16/25] do_fetch(): reduce scope of peer_item","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:15Z","receivedAt":"2013-05-25T09:08:15Z","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.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f949115..80c6e37 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -694,7 +694,6 @@ static int do_fetch(struct transport *transport,\n \t\t    struct refspec *refs, int ref_count)\n {\n \tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *peer_item = NULL;\n \tstruct ref *ref_map;\n \tstruct ref *rm;\n \tint autotags = (transport->remote->fetch_tags == 1);\n@@ -724,8 +723,9 @@ static int do_fetch(struct transport *transport,\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->peer_ref) {\n-\t\t\tpeer_item = string_list_lookup(&existing_refs,\n-\t\t\t\t\t\t       rm->peer_ref->name);\n+\t\t\tstruct string_list_item *peer_item =\n+\t\t\t\tstring_list_lookup(&existing_refs,\n+\t\t\t\t\t\t   rm->peer_ref->name);\n \t\t\tif (peer_item)\n \t\t\t\thashcpy(rm->peer_ref->old_sha1,\n \t\t\t\t\tpeer_item->util);\n-- \n1.8.2.3\n"},{"id":"218517","messageId":"1369472904-12875-18-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 17/25] do_fetch(): clean up existing_refs before exiting","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:16Z","receivedAt":"2013-05-25T09:08:16Z","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.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 80c6e37..48df5fa 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -697,6 +697,7 @@ static int do_fetch(struct transport *transport,\n \tstruct ref *ref_map;\n \tstruct ref *rm;\n \tint autotags = (transport->remote->fetch_tags == 1);\n+\tint retcode = 0;\n \n \tfor_each_ref(add_existing, &existing_refs);\n \n@@ -712,9 +713,9 @@ static int do_fetch(struct transport *transport,\n \n \t/* if not appending, truncate FETCH_HEAD */\n \tif (!append && !dry_run) {\n-\t\tint errcode = truncate_fetch_head();\n-\t\tif (errcode)\n-\t\t\treturn errcode;\n+\t\tretcode = truncate_fetch_head();\n+\t\tif (retcode)\n+\t\t\tgoto cleanup;\n \t}\n \n \tref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);\n@@ -736,7 +737,8 @@ static int do_fetch(struct transport *transport,\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n \tif (fetch_refs(transport, ref_map)) {\n \t\tfree_refs(ref_map);\n-\t\treturn 1;\n+\t\tretcode = 1;\n+\t\tgoto cleanup;\n \t}\n \tif (prune) {\n \t\t/* If --tags was specified, pretend the user gave us the canonical tags refspec */\n@@ -779,7 +781,9 @@ static int do_fetch(struct transport *transport,\n \t\tfree_refs(ref_map);\n \t}\n \n-\treturn 0;\n+ cleanup:\n+\tstring_list_clear(&existing_refs, 0);\n+\treturn retcode;\n }\n \n static void set_option(const char *name, const char *value)\n-- \n1.8.2.3\n"},{"id":"218516","messageId":"1369472904-12875-19-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 18/25] add_existing(): do not retain a reference to sha1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:17Z","receivedAt":"2013-05-25T09:08:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Its lifetime is not guaranteed, so make a copy.  Free the memory when\nthe string_list is cleared.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 48df5fa..fa6fe44 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -571,7 +571,8 @@ static int add_existing(const char *refname, const unsigned char *sha1,\n {\n \tstruct string_list *list = (struct string_list *)cbdata;\n \tstruct string_list_item *item = string_list_insert(list, refname);\n-\titem->util = (void *)sha1;\n+\titem->util = xmalloc(20);\n+\thashcpy(item->util, sha1);\n \treturn 0;\n }\n \n@@ -636,7 +637,7 @@ static void find_non_local_tags(struct transport *transport,\n \t\titem = string_list_insert(&remote_refs, ref->name);\n \t\titem->util = (void *)ref->old_sha1;\n \t}\n-\tstring_list_clear(&existing_refs, 0);\n+\tstring_list_clear(&existing_refs, 1);\n \n \t/*\n \t * We may have a final lightweight tag that needs to be\n@@ -782,7 +783,7 @@ static int do_fetch(struct transport *transport,\n \t}\n \n  cleanup:\n-\tstring_list_clear(&existing_refs, 0);\n+\tstring_list_clear(&existing_refs, 1);\n \treturn retcode;\n }\n \n-- \n1.8.2.3\n"},{"id":"218518","messageId":"1369472904-12875-20-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 19/25] show_head_ref(): do not shadow name of argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:18Z","receivedAt":"2013-05-25T09:08:18Z","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 http-backend.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 6b85ffa..3135835 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -416,8 +416,8 @@ static int show_head_ref(const char *name, const unsigned char *sha1,\n \tstruct strbuf *buf = cb_data;\n \n \tif (flag & REF_ISSYMREF) {\n-\t\tunsigned char sha1[20];\n-\t\tconst char *target = resolve_ref_unsafe(name, sha1, 1, NULL);\n+\t\tunsigned char unused[20];\n+\t\tconst char *target = resolve_ref_unsafe(name, unused, 1, NULL);\n \t\tconst char *target_nons = strip_namespace(target);\n \n \t\tstrbuf_addf(buf, \"ref: %s\\n\", target_nons);\n-- \n1.8.2.3\n"},{"id":"218522","messageId":"1369472904-12875-21-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 20/25] show_head_ref(): rename first parameter to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:19Z","receivedAt":"2013-05-25T09:08:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the usual convention.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n http-backend.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 3135835..0324417 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -410,14 +410,14 @@ static void get_info_refs(char *arg)\n \tstrbuf_release(&buf);\n }\n \n-static int show_head_ref(const char *name, const unsigned char *sha1,\n+static int show_head_ref(const char *refname, const unsigned char *sha1,\n \tint flag, void *cb_data)\n {\n \tstruct strbuf *buf = cb_data;\n \n \tif (flag & REF_ISSYMREF) {\n \t\tunsigned char unused[20];\n-\t\tconst char *target = resolve_ref_unsafe(name, unused, 1, NULL);\n+\t\tconst char *target = resolve_ref_unsafe(refname, unused, 1, NULL);\n \t\tconst char *target_nons = strip_namespace(target);\n \n \t\tstrbuf_addf(buf, \"ref: %s\\n\", target_nons);\n-- \n1.8.2.3\n"},{"id":"218525","messageId":"1369472904-12875-22-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 21/25] string_list_add_one_ref(): rename first parameter to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:20Z","receivedAt":"2013-05-25T09:08:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the usual convention.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex f63fd57..fa7cdf7 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -918,12 +918,12 @@ out:\n \treturn ret;\n }\n \n-static int string_list_add_one_ref(const char *path, const unsigned char *sha1,\n+static int string_list_add_one_ref(const char *refname, const unsigned char *sha1,\n \t\t\t\t   int flag, void *cb)\n {\n \tstruct string_list *refs = cb;\n-\tif (!unsorted_string_list_has_string(refs, path))\n-\t\tstring_list_append(refs, path);\n+\tif (!unsorted_string_list_has_string(refs, refname))\n+\t\tstring_list_append(refs, refname);\n \treturn 0;\n }\n \n-- \n1.8.2.3\n"},{"id":"218523","messageId":"1369472904-12875-23-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 22/25] string_list_add_refs_by_glob(): add a comment about memory management","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:21Z","receivedAt":"2013-05-25T09:08:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since string_list_add_one_ref() adds refname to the string list, but\nthe lifetime of refname is limited, it is important that the\nstring_list passed to string_list_add_one_ref() has strdup_strings\nset.  Document this fact.\n\nAll current callers do the right thing.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/notes.c b/notes.c\nindex fa7cdf7..602d956 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -927,6 +927,9 @@ static int string_list_add_one_ref(const char *refname, const unsigned char *sha\n \treturn 0;\n }\n \n+/*\n+ * The list argument must have strdup_strings set on it.\n+ */\n void string_list_add_refs_by_glob(struct string_list *list, const char *glob)\n {\n \tif (has_glob_specials(glob)) {\n-- \n1.8.2.3\n"},{"id":"218519","messageId":"1369472904-12875-24-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 23/25] exclude_existing(): set existing_refs.strdup_strings","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:22Z","receivedAt":"2013-05-25T09:08:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The each_ref_fn add_existing() adds refnames to the existing_refs\nlist.  But the lifetimes of these refnames is not guaranteed by the\nrefs API, so configure the string_list to make copies as it adds them.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/show-ref.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 8d9b76a..4a0310d 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -103,7 +103,7 @@ static int add_existing(const char *refname, const unsigned char *sha1, int flag\n  */\n static int exclude_existing(const char *match)\n {\n-\tstatic struct string_list existing_refs = STRING_LIST_INIT_NODUP;\n+\tstatic struct string_list existing_refs = STRING_LIST_INIT_DUP;\n \tchar buf[1024];\n \tint matchlen = match ? strlen(match) : 0;\n \n-- \n1.8.2.3\n"},{"id":"218521","messageId":"1369472904-12875-25-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 24/25] register_ref(): make a copy of the bad reference SHA-1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:23Z","receivedAt":"2013-05-25T09:08:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The lifetime of the sha1 parameter passed to an each_ref_fn callback\nis not guaranteed, so make a copy for later use.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n bisect.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 374d9e2..71c1958 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -15,7 +15,7 @@\n static struct sha1_array good_revs;\n static struct sha1_array skipped_revs;\n \n-static const unsigned char *current_bad_sha1;\n+static unsigned char *current_bad_sha1;\n \n static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n@@ -404,7 +404,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,\n \t\t\tint flags, void *cb_data)\n {\n \tif (!strcmp(refname, \"bad\")) {\n-\t\tcurrent_bad_sha1 = sha1;\n+\t\tcurrent_bad_sha1 = xmalloc(20);\n+\t\thashcpy(current_bad_sha1, sha1);\n \t} else if (!prefixcmp(refname, \"good-\")) {\n \t\tsha1_array_append(&good_revs, sha1);\n \t} else if (!prefixcmp(refname, \"skip-\")) {\n-- \n1.8.2.3\n"},{"id":"218520","messageId":"1369472904-12875-26-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 25/25] refs: document the lifetime of the args passed to each_ref_fn","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-25T09:08:24Z","receivedAt":"2013-05-25T09:08:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The lifetime of the memory pointed to by the refname and sha1\narguments to each_ref_fn was never documented, but some callers used\nto assume that it was essentially permanent.  In fact the API does\n*not* guarantee that these objects live beyond a single callback\ninvocation.\n\nIn the current code, the lifetimes are bound together with the\nlifetimes of the ref_caches.  Since these are usually long, the\ncallers usually got away with their sloppiness.  But even today, if a\nref_cache is invalidated the memory can be freed.  And planned changes\nto reference caching, needed to eliminate race conditions, will\nprobably need to shorten the lifetimes of these objects.\n\nThe commits leading up to this have (hopefully) fixed all of the\ncallers of the for_each_ref()-like functions.  This commit does the\nlast step: documents what each_ref_fn callbacks can assume about\nobject lifetimes.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.h | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex a35eafc..122ec03 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -15,13 +15,23 @@ struct ref_lock {\n #define REF_ISBROKEN 0x04\n \n /*\n- * Calls the specified function for each ref file until it returns\n- * nonzero, and returns the value.  Please note that it is not safe to\n- * modify references while an iteration is in progress, unless the\n- * same callback function invocation that modifies the reference also\n- * returns a nonzero value to immediately stop the iteration.\n+ * The signature for the callback function for the for_each_*()\n+ * functions below.  The memory pointed to by the refname and sha1\n+ * arguments is only guaranteed to be valid for the duration of a\n+ * single callback invocation.\n+ */\n+typedef int each_ref_fn(const char *refname,\n+\t\t\tconst unsigned char *sha1, int flags, void *cb_data);\n+\n+/*\n+ * The following functions invoke the specified callback function for\n+ * each reference indicated.  If the function ever returns a nonzero\n+ * value, stop the iteration and return that value.  Please note that\n+ * it is not safe to modify references while an iteration is in\n+ * progress, unless the same callback function invocation that\n+ * modifies the reference also returns a nonzero value to immediately\n+ * stop the iteration.\n  */\n-typedef int each_ref_fn(const char *refname, const unsigned char *sha1, int flags, void *cb_data);\n extern int head_ref(each_ref_fn, void *);\n extern int for_each_ref(each_ref_fn, void *);\n extern int for_each_ref_in(const char *, each_ref_fn, void *);\n-- \n1.8.2.3\n"},{"id":"218786","messageId":"87ppwaurfy.fsf@linux-k42r.v.cablecom.net","threadId":"33931","inReplyTo":"1369472904-12875-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 22/25] string_list_add_refs_by_glob(): add a comment about memory management","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-29T08:21:37Z","receivedAt":"2013-05-29T08:21:37Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Since string_list_add_one_ref() adds refname to the string list, but\n> the lifetime of refname is limited, it is important that the\n> string_list passed to string_list_add_one_ref() has strdup_strings\n> set.  Document this fact.\n>\n> All current callers do the right thing.\n[...]\n> +/*\n> + * The list argument must have strdup_strings set on it.\n> + */\n>  void string_list_add_refs_by_glob(struct string_list *list, const char *glob)\n>  {\n>  \tif (has_glob_specials(glob)) {\n\nMaybe add an assert() so that this is bulletproof?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"218787","messageId":"8761y2ura8.fsf@linux-k42r.v.cablecom.net","threadId":"33931","inReplyTo":"1369472904-12875-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/25] Remove assumptions about each_ref_fn arg lifetimes","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-29T08:25:03Z","receivedAt":"2013-05-29T08:25:03Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>\n\nI read the entire series on Monday, and give it an Ack at maybe 90%\nconfidence level -- sorry, I was short on caffeine and sleep ;-)\n\nI meant to verify this assertion:\n\n> I did a manual audit of the 50 (!) functions that are used as an\n> each_ref_fn callback to the for_each_ref()-style functions.  (I hope I\n> haven't missed any.)  I checked that they do not make the assumption\n> that the lifetimes of the refname and sha1 arguments extend past the\n> duration of the callback invocation.  There were a number of callers\n> that got this wrong; I believe I have fixed them all.\n\nBut time ran out, and I wouldn't want you to hold your breath.  The\nseries is a big improvement even if another caller slipped through the\ncracks.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"218848","messageId":"7vk3mhwyiq.fsf@alter.siamese.dyndns.org","threadId":"33931","inReplyTo":"1369472904-12875-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 11/25] object_array_remove_duplicates(): rewrite to reduce copying","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-29T16:18:05Z","receivedAt":"2013-05-29T16:18:05Z","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> The old version copied one entry to its destination position, then\n> deleted any matching entries from the tail of the array.  This\n> required the tail of the array to be copied multiple times.  It didn't\n> affect the complexity of the algorithm because the whole tail has to\n> be searched through anyway.  But all the copying was unnecessary.\n>\n> Instead, check for the existence of an entry with the same name in the\n> *head* of the list before copying an entry to its final position.\n> This way each entry has to be copied at most one time.\n>\n> Extract a helper function contains_name() to do a bit of the work.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  object.c | 32 +++++++++++++++++++++-----------\n>  object.h |  6 +++++-\n>  2 files changed, 26 insertions(+), 12 deletions(-)\n>\n> diff --git a/object.c b/object.c\n> index fcd4a82..10b5349 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -294,22 +294,32 @@ void object_array_filter(struct object_array *array,\n>  \tarray->nr = dst;\n>  }\n>  \n> +/*\n> + * Return true iff array already contains an entry with name.\n> + */\n> +static int contains_name(struct object_array *array, const char *name)\n> +{\n> +\tunsigned nr = array->nr, i;\n> +\tstruct object_array_entry *object = array->objects;\n> +\n> +\tfor (i = 0; i < nr; i++, object++)\n> +\t\tif (!strcmp(object->name, name))\n> +\t\t\treturn 1;\n> +\treturn 0;\n> +}\n\nBecause some codepaths (e.g. patch 14/25) stuff NULL in the name\nfield, we may want to be more careful with this.\n\nThis is not a new problem, and I think the longer term solution is\nto get rid of object_array_remove_duplicates(), so it is perfectly\nfine to leave this function broken with respect to NULL input as-is.\n\nThe only caller of remove-duplicates is bundle.c, which gets many\nstarting points and end points from the command line and tries to be\nnice by removing obvious duplicates, e.g.\n\n\tgit bundle create t.bundle master master\n\nbut I think its logic of deduping is wrong.  It runs dwim_ref() on\nthe incoming refs after the remove-duplicates call, so\n\n\tgit bundle create t.bundle master heads/mater\n\nwill end up with two copies of refs/heads/master.  To fix it, the\ncode must dedup the result of running dwim_ref(), and at that point,\nthere is no reason to call object_array_remove_duplicates().\n\n> +\n>  void object_array_remove_duplicates(struct object_array *array)\n>  {\n> -\tunsigned int ref, src, dst;\n> +\tunsigned nr = array->nr, src;\n>  \tstruct object_array_entry *objects = array->objects;\n>  \n> -\tfor (ref = 0; ref + 1 < array->nr; ref++) {\n> -\t\tfor (src = ref + 1, dst = src;\n> -\t\t     src < array->nr;\n> -\t\t     src++) {\n> -\t\t\tif (!strcmp(objects[ref].name, objects[src].name))\n> -\t\t\t\tcontinue;\n> -\t\t\tif (src != dst)\n> -\t\t\t\tobjects[dst] = objects[src];\n> -\t\t\tdst++;\n> +\tarray->nr = 0;\n> +\tfor (src = 0; src < nr; src++) {\n> +\t\tif (!contains_name(array, objects[src].name)) {\n> +\t\t\tif (src != array->nr)\n> +\t\t\t\tobjects[array->nr] = objects[src];\n> +\t\t\tarray->nr++;\n>  \t\t}\n> -\t\tarray->nr = dst;\n>  \t}\n>  }\n\n>  \n> diff --git a/object.h b/object.h\n> index 0d39ff4..6c1c27f 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -96,7 +96,11 @@ typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n>  void object_array_filter(struct object_array *array,\n>  \t\t\t object_array_each_func_t want, void *cb_data);\n>  \n> -void object_array_remove_duplicates(struct object_array *);\n> +/*\n> + * Remove from array all but the first entry with a given name.\n> + * Warning: this function uses an O(N^2) algorithm.\n> + */\n> +void object_array_remove_duplicates(struct object_array *array);\n>  \n>  void clear_object_flags(unsigned flags);\n"},{"id":"218850","messageId":"7vfvx5wy7x.fsf@alter.siamese.dyndns.org","threadId":"33931","inReplyTo":"1369472904-12875-16-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 15/25] object_array_entry: fix memory handling of the name field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-29T16:24:34Z","receivedAt":"2013-05-29T16:24: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> Change the callers that were already passing copies to\n> add_object_array_with_mode() to either skip the copy, or (if the\n> memory needed to be allocated anyway) freeing the memory itself.\n>\n> A part of this commit effectively reverts\n>\n>     70d26c6e76 read_revisions_from_stdin: make copies for handle_revision_arg\n>\n> because the copying introduced by that commit (which is still\n> necessary) is now done at a deeper level.\n\nGood.  Thanks for working on this.\n\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  bundle.c   |  2 +-\n>  object.c   | 26 +++++++++++++++++++++++---\n>  object.h   |  8 +++++++-\n>  revision.c |  6 ++++--\n>  4 files changed, 35 insertions(+), 7 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index 4b0e5cd..3d64311 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -281,7 +281,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n>  \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n>  \t\t\t\tobject->flags |= UNINTERESTING;\n> -\t\t\t\tadd_pending_object(&revs, object, xstrdup(buf.buf));\n> +\t\t\t\tadd_pending_object(&revs, object, buf.buf);\n>  \t\t\t}\n>  \t\t} else if (!get_sha1_hex(buf.buf, sha1)) {\n>  \t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n> diff --git a/object.c b/object.c\n> index 10b5349..e4ff714 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -260,11 +260,18 @@ void add_object_array(struct object *obj, const char *name, struct object_array\n>  \tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n>  }\n>  \n> +/*\n> + * A zero-length string to which object_array_entry::name can be\n> + * initialized without requiring a malloc/free.\n> + */\n> +char object_array_slopbuf[1];\n> +\n>  void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n>  {\n>  \tunsigned nr = array->nr;\n>  \tunsigned alloc = array->alloc;\n>  \tstruct object_array_entry *objects = array->objects;\n> +\tstruct object_array_entry *entry;\n>  \n>  \tif (nr >= alloc) {\n>  \t\talloc = (alloc + 32) * 2;\n> @@ -272,9 +279,16 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n>  \t\tarray->alloc = alloc;\n>  \t\tarray->objects = objects;\n>  \t}\n> -\tobjects[nr].item = obj;\n> -\tobjects[nr].name = name;\n> -\tobjects[nr].mode = mode;\n> +\tentry = &objects[nr];\n> +\tentry->item = obj;\n> +\tif (!name)\n> +\t\tentry->name = NULL;\n> +\telse if (!*name)\n> +\t\t/* Use our own empty string instead of allocating one: */\n> +\t\tentry->name = object_array_slopbuf;\n> +\telse\n> +\t\tentry->name = xstrdup(name);\n> +\tentry->mode = mode;\n>  \tarray->nr = ++nr;\n>  }\n>  \n> @@ -289,6 +303,9 @@ void object_array_filter(struct object_array *array,\n>  \t\t\tif (src != dst)\n>  \t\t\t\tobjects[dst] = objects[src];\n>  \t\t\tdst++;\n> +\t\t} else {\n> +\t\t\tif (objects[src].name != object_array_slopbuf)\n> +\t\t\t\tfree(objects[src].name);\n>  \t\t}\n>  \t}\n>  \tarray->nr = dst;\n> @@ -319,6 +336,9 @@ void object_array_remove_duplicates(struct object_array *array)\n>  \t\t\tif (src != array->nr)\n>  \t\t\t\tobjects[array->nr] = objects[src];\n>  \t\t\tarray->nr++;\n> +\t\t} else {\n> +\t\t\tif (objects[src].name != object_array_slopbuf)\n> +\t\t\t\tfree(objects[src].name);\n>  \t\t}\n>  \t}\n>  }\n> diff --git a/object.h b/object.h\n> index 6c1c27f..2ff68c5 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -11,7 +11,13 @@ struct object_array {\n>  \tunsigned int alloc;\n>  \tstruct object_array_entry {\n>  \t\tstruct object *item;\n> -\t\tconst char *name;\n> +\t\t/*\n> +\t\t * name or NULL.  If non-NULL, the memory pointed to\n> +\t\t * is owned by this object *except* if it points at\n> +\t\t * object_array_slopbuf, which is a static copy of the\n> +\t\t * empty string.\n> +\t\t */\n> +\t\tchar *name;\n>  \t\tunsigned mode;\n>  \t} *objects;\n>  };\n> diff --git a/revision.c b/revision.c\n> index be73cb4..4aeda33 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -88,7 +88,9 @@ void add_object(struct object *obj,\n>  \t\tstruct name_path *path,\n>  \t\tconst char *name)\n>  {\n> -\tadd_object_array(obj, path_name(path, name), p);\n> +\tchar *pn = path_name(path, name);\n> +\tadd_object_array(obj, pn, p);\n> +\tfree(pn);\n>  }\n>  \n>  static void mark_blob_uninteresting(struct blob *blob)\n> @@ -1288,7 +1290,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n>  \t\t\t}\n>  \t\t\tdie(\"options not supported in --stdin mode\");\n>  \t\t}\n> -\t\tif (handle_revision_arg(xstrdup(sb.buf), revs, 0,\n> +\t\tif (handle_revision_arg(sb.buf, revs, 0,\n>  \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n>  \t\t\tdie(\"bad revision '%s'\", sb.buf);\n>  \t}\n"},{"id":"218853","messageId":"7vk3mhviaw.fsf@alter.siamese.dyndns.org","threadId":"33931","inReplyTo":"1369472904-12875-25-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 24/25] register_ref(): make a copy of the bad reference SHA-1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-29T16:53:43Z","receivedAt":"2013-05-29T16:53:43Z","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> The lifetime of the sha1 parameter passed to an each_ref_fn callback\n> is not guaranteed, so make a copy for later use.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  bisect.c | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 374d9e2..71c1958 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -15,7 +15,7 @@\n>  static struct sha1_array good_revs;\n>  static struct sha1_array skipped_revs;\n>  \n> -static const unsigned char *current_bad_sha1;\n> +static unsigned char *current_bad_sha1;\n>  \n>  static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n>  static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n> @@ -404,7 +404,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,\n>  \t\t\tint flags, void *cb_data)\n>  {\n>  \tif (!strcmp(refname, \"bad\")) {\n> -\t\tcurrent_bad_sha1 = sha1;\n> +\t\tcurrent_bad_sha1 = xmalloc(20);\n> +\t\thashcpy(current_bad_sha1, sha1);\n\nThis, together with 18/25, may hint that we want hashdup()?\n\n>  \t} else if (!prefixcmp(refname, \"good-\")) {\n>  \t\tsha1_array_append(&good_revs, sha1);\n>  \t} else if (!prefixcmp(refname, \"skip-\")) {\n"},{"id":"218854","messageId":"7vd2s9via5.fsf@alter.siamese.dyndns.org","threadId":"33931","inReplyTo":"1369472904-12875-26-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 25/25] refs: document the lifetime of the args passed to each_ref_fn","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-29T16:54:10Z","receivedAt":"2013-05-29T16:54:10Z","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> The commits leading up to this have (hopefully) fixed all of the\n> callers of the for_each_ref()-like functions.  This commit does the\n> last step: documents what each_ref_fn callbacks can assume about\n> object lifetimes.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nI looked at all the patches in the series and they all looked\nsensible.\n\nI have a few comments (sent as individual review) but all of the\nsuggestions in them are \"by the way I noticed this issue that is not\nnew to this series while I was reading the code, and we may want to\nfix it elsewhere\", not \"this is broken in the patch, we need to fix\nit before queuing\".\n\nThanks for a pleasant read.\n\n> ---\n>  refs.h | 22 ++++++++++++++++------\n>  1 file changed, 16 insertions(+), 6 deletions(-)\n>\n> diff --git a/refs.h b/refs.h\n> index a35eafc..122ec03 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -15,13 +15,23 @@ struct ref_lock {\n>  #define REF_ISBROKEN 0x04\n>  \n>  /*\n> - * Calls the specified function for each ref file until it returns\n> - * nonzero, and returns the value.  Please note that it is not safe to\n> - * modify references while an iteration is in progress, unless the\n> - * same callback function invocation that modifies the reference also\n> - * returns a nonzero value to immediately stop the iteration.\n> + * The signature for the callback function for the for_each_*()\n> + * functions below.  The memory pointed to by the refname and sha1\n> + * arguments is only guaranteed to be valid for the duration of a\n> + * single callback invocation.\n> + */\n> +typedef int each_ref_fn(const char *refname,\n> +\t\t\tconst unsigned char *sha1, int flags, void *cb_data);\n> +\n> +/*\n> + * The following functions invoke the specified callback function for\n> + * each reference indicated.  If the function ever returns a nonzero\n> + * value, stop the iteration and return that value.  Please note that\n> + * it is not safe to modify references while an iteration is in\n> + * progress, unless the same callback function invocation that\n> + * modifies the reference also returns a nonzero value to immediately\n> + * stop the iteration.\n>   */\n> -typedef int each_ref_fn(const char *refname, const unsigned char *sha1, int flags, void *cb_data);\n>  extern int head_ref(each_ref_fn, void *);\n>  extern int for_each_ref(each_ref_fn, void *);\n>  extern int for_each_ref_in(const char *, each_ref_fn, void *);\n"},{"id":"219040","messageId":"51A7A88C.2020307@alum.mit.edu","threadId":"33931","inReplyTo":"87ppwaurfy.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v2 22/25] string_list_add_refs_by_glob(): add a comment about memory management","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-30T19:29:16Z","receivedAt":"2013-05-30T19:29:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/29/2013 10:21 AM, Thomas Rast wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Since string_list_add_one_ref() adds refname to the string list, but\n>> the lifetime of refname is limited, it is important that the\n>> string_list passed to string_list_add_one_ref() has strdup_strings\n>> set.  Document this fact.\n>>\n>> All current callers do the right thing.\n> [...]\n>> +/*\n>> + * The list argument must have strdup_strings set on it.\n>> + */\n>>  void string_list_add_refs_by_glob(struct string_list *list, const char *glob)\n>>  {\n>>  \tif (has_glob_specials(glob)) {\n> \n> Maybe add an assert() so that this is bulletproof?\n\nGood idea.  Will be added in v3.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"219042","messageId":"51A7AEB3.6090309@alum.mit.edu","threadId":"33931","inReplyTo":"8761y2ura8.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v2 00/25] Remove assumptions about each_ref_fn arg lifetimes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-30T19:55:31Z","receivedAt":"2013-05-30T19:55:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/29/2013 10:25 AM, Thomas Rast wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> I read the entire series on Monday, and give it an Ack at maybe 90%\n> confidence level -- sorry, I was short on caffeine and sleep ;-)\n\nThanks very much.  I'll buy you a coffee the next time I see you :-)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"219045","messageId":"51A7C146.6090605@alum.mit.edu","threadId":"33931","inReplyTo":"7vk3mhwyiq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 11/25] object_array_remove_duplicates(): rewrite to reduce copying","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-30T21:14:46Z","receivedAt":"2013-05-30T21:14:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/29/2013 06:18 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> The old version copied one entry to its destination position, then\n>> deleted any matching entries from the tail of the array.  This\n>> required the tail of the array to be copied multiple times.  It didn't\n>> affect the complexity of the algorithm because the whole tail has to\n>> be searched through anyway.  But all the copying was unnecessary.\n>>\n>> Instead, check for the existence of an entry with the same name in the\n>> *head* of the list before copying an entry to its final position.\n>> This way each entry has to be copied at most one time.\n>>\n>> Extract a helper function contains_name() to do a bit of the work.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>  object.c | 32 +++++++++++++++++++++-----------\n>>  object.h |  6 +++++-\n>>  2 files changed, 26 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/object.c b/object.c\n>> index fcd4a82..10b5349 100644\n>> --- a/object.c\n>> +++ b/object.c\n>> @@ -294,22 +294,32 @@ void object_array_filter(struct object_array *array,\n>>  \tarray->nr = dst;\n>>  }\n>>  \n>> +/*\n>> + * Return true iff array already contains an entry with name.\n>> + */\n>> +static int contains_name(struct object_array *array, const char *name)\n>> +{\n>> +\tunsigned nr = array->nr, i;\n>> +\tstruct object_array_entry *object = array->objects;\n>> +\n>> +\tfor (i = 0; i < nr; i++, object++)\n>> +\t\tif (!strcmp(object->name, name))\n>> +\t\t\treturn 1;\n>> +\treturn 0;\n>> +}\n> \n> Because some codepaths (e.g. patch 14/25) stuff NULL in the name\n> field, we may want to be more careful with this.\n> \n> This is not a new problem, and I think the longer term solution is\n> to get rid of object_array_remove_duplicates(), so it is perfectly\n> fine to leave this function broken with respect to NULL input as-is.\n\nYou make a good point about NULL names, but I agree to leave it for now\nsince it needs work anyway.\n\n> The only caller of remove-duplicates is bundle.c, which gets many\n> starting points and end points from the command line and tries to be\n> nice by removing obvious duplicates, e.g.\n> \n> \tgit bundle create t.bundle master master\n> \n> but I think its logic of deduping is wrong.  It runs dwim_ref() on\n> the incoming refs after the remove-duplicates call, so\n> \n> \tgit bundle create t.bundle master heads/mater\n> \n> will end up with two copies of refs/heads/master.  To fix it, the\n> code must dedup the result of running dwim_ref(), and at that point,\n> there is no reason to call object_array_remove_duplicates().\n\nThat sounds reasonable.\n\nI poked around this code a bit to understand what is going on, and it\noccurred to me that the object_array can include both positive and\nnegative references, right?  And yet object_array_remove_duplicates()\nonly considers names, not flags.  So it seems to me that if the deduping\ncode would see the same reference twice, once positive and once\nnegative, then it would throw an arbitrary one of them out, which would\nbe wrong.\n\nBut I couldn't provoke this situation, so perhaps setup_revisions()\nalready specially treats combinations like \"master ^master\"?  (If that's\ntrue then why? and wouldn't it get confused by \"master ^heads/master\"?)\n\nI suppose someday I will have to dig into these functions and maybe even\ndocument them.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"219047","messageId":"51A7C9E8.10108@alum.mit.edu","threadId":"33931","inReplyTo":"7vk3mhviaw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 24/25] register_ref(): make a copy of the bad reference SHA-1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-30T21:51:36Z","receivedAt":"2013-05-30T21:51:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/29/2013 06:53 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> [...]\n>> +\t\tcurrent_bad_sha1 = xmalloc(20);\n>> +\t\thashcpy(current_bad_sha1, sha1);\n> \n> This, together with 18/25, may hint that we want hashdup()?\n\nI thought about it, but was surprised that I could only find one or two\nother places in the existing code that would use such a function.  But\nsure, I would be happy to submit a patch.\n\nI think hashdup() needn't be inline, so the definition can't go with its\ncousins in cache.h.  There is no cache.c.  So where would be the best\nplace to define hashdup()?  object.c?  sha1_name.c?\n\nWhile I'm at it, I think it would be nice to have constants like\n\n#define SHA1_LEN 20\n#define SHA1_HEX_LEN 40\n\nand start using those instead of magic numbers.  Any objections (or\nsuggestions for better names)?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"219048","messageId":"1369951551-25662-1-git-send-email-mhagger@alum.mit.edu","threadId":"33931","inReplyTo":"87ppwaurfy.fsf@linux-k42r.v.cablecom.net","subject":"[PATCH v2 FIXUP 22/25] fixup! string_list_add_refs_by_glob(): add a comment about memory management","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-30T22:05:51Z","receivedAt":"2013-05-30T22:05:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nJunio, would you mind squashing this patch onto mh/reflife 22/25?\n\n notes.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/notes.c b/notes.c\nindex 602d956..b69c0b8 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -932,6 +932,7 @@ static int string_list_add_one_ref(const char *refname, const unsigned char *sha\n  */\n void string_list_add_refs_by_glob(struct string_list *list, const char *glob)\n {\n+\tassert(list->strdup_strings);\n \tif (has_glob_specials(glob)) {\n \t\tfor_each_glob_ref(string_list_add_one_ref, glob, list);\n \t} else {\n-- \n1.8.3\n"},{"id":"219049","messageId":"35D3DD46364C40A391B3DB72C7D18798@PhilipOakley","threadId":"33931","inReplyTo":"51A7C9E8.10108@alum.mit.edu","subject":"Re: [PATCH v2 24/25] register_ref(): make a copy of the bad reference SHA-1","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-05-30T22:09:09Z","receivedAt":"2013-05-30T22:09:09Z","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: Thursday, May 30, 2013 10:51 PM\n> On 05/29/2013 06:53 PM, Junio C Hamano wrote:\n>> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>>> [...]\n>>> + current_bad_sha1 = xmalloc(20);\n>>> + hashcpy(current_bad_sha1, sha1);\n>>\n>> This, together with 18/25, may hint that we want hashdup()?\n>\n> I thought about it, but was surprised that I could only find one or \n> two\n> other places in the existing code that would use such a function.  But\n> sure, I would be happy to submit a patch.\n>\n> I think hashdup() needn't be inline, so the definition can't go with \n> its\n> cousins in cache.h.  There is no cache.c.  So where would be the best\n> place to define hashdup()?  object.c?  sha1_name.c?\n>\n> While I'm at it, I think it would be nice to have constants like\n>\n> #define SHA1_LEN 20\n> #define SHA1_HEX_LEN 40\n>\n> and start using those instead of magic numbers.  Any objections (or\n> suggestions for better names)?\n>\n\nThe first named constant should be fully qualified to the same extent as \nthe second, perhaps:\n    #define SHA1_BYTE_LEN 20\n\nand perhaps with an alternate of (though HEX is just as good):\n    #define SHA1_CHAR_LEN 40\n\n\n> Michael\n>\n> -- \n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n> --\nPhilip \n"},{"id":"219192","messageId":"7vy5asmdji.fsf@alter.siamese.dyndns.org","threadId":"33931","inReplyTo":"51A7C146.6090605@alum.mit.edu","subject":"Re: [PATCH v2 11/25] object_array_remove_duplicates(): rewrite to reduce copying","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-02T21:02:41Z","receivedAt":"2013-06-02T21:02:41Z","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>> The only caller of remove-duplicates is bundle.c, which gets many\n>> starting points and end points from the command line and tries to be\n>> nice by removing obvious duplicates, e.g.\n>> \n>> \tgit bundle create t.bundle master master\n>> \n>> but I think its logic of deduping is wrong.  It runs dwim_ref() on\n>> the incoming refs after the remove-duplicates call, so\n>> \n>> \tgit bundle create t.bundle master heads/mater\n>> \n>> will end up with two copies of refs/heads/master.  To fix it, the\n>> code must dedup the result of running dwim_ref(), and at that point,\n>> there is no reason to call object_array_remove_duplicates().\n>\n> That sounds reasonable.\n>\n> I poked around this code a bit to understand what is going on, and it\n> occurred to me that the object_array can include both positive and\n> negative references, right?  And yet object_array_remove_duplicates()\n> only considers names, not flags.  So it seems to me that if the deduping\n> code would see the same reference twice, once positive and once\n> negative, then it would throw an arbitrary one of them out, which would\n> be wrong.\n>\n> But I couldn't provoke this situation, so perhaps setup_revisions()\n> already specially treats combinations like \"master ^master\"?  (If that's\n> true then why? and wouldn't it get confused by \"master ^heads/master\"?)\n\nWith \"git bundle create t.bundle ^master master\", you see two\nentries in revs.pending.objects[] but they are the same object and\nis already marked as uninteresting, so you will not see 'master' in\nthe result.\n\nThis parsing loop predates the more recent revs->cmdline mechanism,\nthat treats these two command line arguments as separate entities,\nso that we can more reliably tell what the real end-user input is.\n"},{"id":"219233","messageId":"7vfvwzjjmy.fsf@alter.siamese.dyndns.org","threadId":"33931","inReplyTo":"1369951551-25662-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 FIXUP 22/25] fixup! string_list_add_refs_by_glob(): add a comment about memory management","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T15:31:33Z","receivedAt":"2013-06-03T15:31:33Z","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> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n> Junio, would you mind squashing this patch onto mh/reflife 22/25?\n\nDone.  Thanks.\n\n>\n>  notes.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/notes.c b/notes.c\n> index 602d956..b69c0b8 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -932,6 +932,7 @@ static int string_list_add_one_ref(const char *refname, const unsigned char *sha\n>   */\n>  void string_list_add_refs_by_glob(struct string_list *list, const char *glob)\n>  {\n> +\tassert(list->strdup_strings);\n>  \tif (has_glob_specials(glob)) {\n>  \t\tfor_each_glob_ref(string_list_add_one_ref, glob, list);\n>  \t} else {\n"}]}