{"thread":{"id":"41394","subject":"[PATCH 0/5] drop \"struct name_path\" and path_name()","startedAt":"2016-02-11T22:23:15Z","lastAt":"2016-02-11T22:36:34Z","messageCount":7,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"277968","messageId":"20160211222314.GA31625@sigill.intra.peff.net","threadId":"41394","inReplyTo":null,"subject":"[PATCH 0/5] drop \"struct name_path\" and path_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:23:15Z","receivedAt":"2016-02-11T22:23:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The graph traversal code in list-objects.c uses \"struct name_path\" to\nbuild a linked list of path components, which it then feeds to the\ncallbacks. This is meant to be efficient, because we keep pointers into\nthe actual tree data for each name. However, there are two things that\nwork against this:\n\n  1. In some cases, we keep in parallel a strbuf with the running\n     pathname, so that we can feed it to tree_entry_interesting().\n\n  2. The ultimate fate of this linked list is often to get concatenated\n     into a single buffer anyway, via path_name().\n\nSo it's really not buying us much efficiency over just using\nstrbuf_addstr() and strbuf_setlen() in the first place. And it's extra\ncode that is slightly tricky to get right, especially with respect to\nsize_t and integer overflow (compare path_name() and\nshow_object_with_path() before and after).\n\nThis series drops the whole thing in favor of using a strbuf. Because I\nwanted to make sure we weren't regressing performance, I measured two\ncases before and after (and of course verified that they produce\nidentical output):\n\n  1. \"git rev-list --objects --all\", which prints the name of each tree\n     and blob we find directly from the linked list (without ever\n     constructing the whole string), and does not use\n     tree_entry_interesting (and so does not otherwise need to keep the\n     running strbuf). So we'd see any negative effects of the strategy\n     here.\n\n  2. \"git prune --dry-run\", which walks the complete graph but does\n     not do anything useful with the pathnames. So it would not\n     otherwise need to assemble or look at the path components at all.\n\nBoth of them showed no measurable difference in their best-of-five times\nwhen run on git.git. I didn't measure peak memory usage. For the reasons\nexplained in patch 3, it will actually be slightly _better_ for a normal\nrepo like git.git. But you could construct a pathological case where it\nis worse (e.g., if you had a tree with a 500MB path-name, the old code\nwould need 500MB to run rev-list, and the new code will need 2*500MB\nduring the callback). I think the cleanup is worth it.\n\n  [1/5]: http-push: stop using name_path\n  [2/5]: show_object_with_name: simplify by using path_name()\n  [3/5]: list-objects: convert name_path to a strbuf\n  [4/5]: list-objects: drop name_path entirely\n  [5/5]: list-objects: pass full pathname to callbacks\n\n-Peff\n"},{"id":"277969","messageId":"20160211222347.GA586@sigill.intra.peff.net","threadId":"41394","inReplyTo":"20160211222314.GA31625@sigill.intra.peff.net","subject":"[PATCH 1/5] http-push: stop using name_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:23:48Z","receivedAt":"2016-02-11T22:23:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The graph traversal code here passes along a name_path to\nbuild up the pathname at which we find each blob. But we\nnever actually do anything with the resulting names, making\nit a waste of code and memory.\n\nThis usage came in aa1dbc9 (Update http-push functionality,\n2006-03-07), and originally the result was passed to\n\"add_object\" (which stored it, but didn't really use it,\neither). But we stopped using that function in 1f1e895 (Add\n\"named object array\" concept, 2006-06-19) in favor of\nstoring just the objects themselves.\n\nMoreover, the generation of the name in process_tree() is\nbuggy. It sticks \"name\" onto the end of the name_path linked\nlist, and then passes it down again as it recurses (instead\nof \"entry.path\"). So it's a good thing this was unused, as\nthe resulting path for \"a/b/c/d\" would end up as \"a/a/a/a\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-push.c | 23 +++++++----------------\n 1 file changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex d857b13..bd60668 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1277,9 +1277,7 @@ static struct object_list **add_one_object(struct object *obj, struct object_lis\n }\n \n static struct object_list **process_blob(struct blob *blob,\n-\t\t\t\t\t struct object_list **p,\n-\t\t\t\t\t struct name_path *path,\n-\t\t\t\t\t const char *name)\n+\t\t\t\t\t struct object_list **p)\n {\n \tstruct object *obj = &blob->object;\n \n@@ -1293,14 +1291,11 @@ static struct object_list **process_blob(struct blob *blob,\n }\n \n static struct object_list **process_tree(struct tree *tree,\n-\t\t\t\t\t struct object_list **p,\n-\t\t\t\t\t struct name_path *path,\n-\t\t\t\t\t const char *name)\n+\t\t\t\t\t struct object_list **p)\n {\n \tstruct object *obj = &tree->object;\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n-\tstruct name_path me;\n \n \tobj->flags |= LOCAL;\n \n@@ -1310,21 +1305,17 @@ static struct object_list **process_tree(struct tree *tree,\n \t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \n \tobj->flags |= SEEN;\n-\tname = xstrdup(name);\n \tp = add_one_object(obj, p);\n-\tme.up = path;\n-\tme.elem = name;\n-\tme.elem_len = strlen(name);\n \n \tinit_tree_desc(&desc, tree->buffer, tree->size);\n \n \twhile (tree_entry(&desc, &entry))\n \t\tswitch (object_type(entry.mode)) {\n \t\tcase OBJ_TREE:\n-\t\t\tp = process_tree(lookup_tree(entry.sha1), p, &me, name);\n+\t\t\tp = process_tree(lookup_tree(entry.sha1), p);\n \t\t\tbreak;\n \t\tcase OBJ_BLOB:\n-\t\t\tp = process_blob(lookup_blob(entry.sha1), p, &me, name);\n+\t\t\tp = process_blob(lookup_blob(entry.sha1), p);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\t/* Subproject commit - not in this repository */\n@@ -1343,7 +1334,7 @@ static int get_delta(struct rev_info *revs, struct remote_lock *lock)\n \tint count = 0;\n \n \twhile ((commit = get_revision(revs)) != NULL) {\n-\t\tp = process_tree(commit->tree, p, NULL, \"\");\n+\t\tp = process_tree(commit->tree, p);\n \t\tcommit->object.flags |= LOCAL;\n \t\tif (!(commit->object.flags & UNINTERESTING))\n \t\t\tcount += add_send_request(&commit->object, lock);\n@@ -1362,11 +1353,11 @@ static int get_delta(struct rev_info *revs, struct remote_lock *lock)\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_TREE) {\n-\t\t\tp = process_tree((struct tree *)obj, p, NULL, name);\n+\t\t\tp = process_tree((struct tree *)obj, p);\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\n-\t\t\tp = process_blob((struct blob *)obj, p, NULL, name);\n+\t\t\tp = process_blob((struct blob *)obj, p);\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"unknown pending object %s (%s)\", oid_to_hex(&obj->oid), name);\n-- \n2.7.1.550.gf5fcbd3\n"},{"id":"277970","messageId":"20160211222418.GB586@sigill.intra.peff.net","threadId":"41394","inReplyTo":"20160211222314.GA31625@sigill.intra.peff.net","subject":"[PATCH 2/5] show_object_with_name: simplify by using path_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:24:18Z","receivedAt":"2016-02-11T22:24:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When \"git rev-list\" shows an object with its associated path\nname, it does so by walking the name_path linked list and\nprinting each component (stopping at any embedded NULs or\nnewlines).\n\nWe'd like to eventually get rid of name_path entirely in\nfavor of a single buffer, and dropping this custom printing\ncode is part of that. As a first step, let's use path_name()\nto format the list into a single buffer, and print that.\nThis is strictly less efficient than the original, but it's\na temporary step in the refactoring; our end game will be to\nget the fully formatted name in the first place.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c | 40 ++++++----------------------------------\n 1 file changed, 6 insertions(+), 34 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f24ead5..6387068 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -49,46 +49,18 @@ char *path_name(const struct name_path *path, const char *name)\n \treturn n;\n }\n \n-static int show_path_component_truncated(FILE *out, const char *name, int len)\n-{\n-\tint cnt;\n-\tfor (cnt = 0; cnt < len; cnt++) {\n-\t\tint ch = name[cnt];\n-\t\tif (!ch || ch == '\\n')\n-\t\t\treturn -1;\n-\t\tfputc(ch, out);\n-\t}\n-\treturn len;\n-}\n-\n-static int show_path_truncated(FILE *out, const struct name_path *path)\n-{\n-\tint emitted, ours;\n-\n-\tif (!path)\n-\t\treturn 0;\n-\temitted = show_path_truncated(out, path->up);\n-\tif (emitted < 0)\n-\t\treturn emitted;\n-\tif (emitted)\n-\t\tfputc('/', out);\n-\tours = show_path_component_truncated(out, path->elem, path->elem_len);\n-\tif (ours < 0)\n-\t\treturn ours;\n-\treturn ours || emitted;\n-}\n-\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-\tleaf.elem = component;\n-\tleaf.elem_len = strlen(component);\n+\tchar *name = path_name(path, component);\n+\tchar *p;\n \n \tfprintf(out, \"%s \", oid_to_hex(&obj->oid));\n-\tshow_path_truncated(out, &leaf);\n+\tfor (p = name; *p && *p != '\\n'; p++)\n+\t\tfputc(*p, out);\n \tfputc('\\n', out);\n+\n+\tfree(name);\n }\n \n static void mark_blob_uninteresting(struct blob *blob)\n-- \n2.7.1.550.gf5fcbd3\n"},{"id":"277971","messageId":"20160211222618.GC586@sigill.intra.peff.net","threadId":"41394","inReplyTo":"20160211222314.GA31625@sigill.intra.peff.net","subject":"[PATCH 3/5] list-objects: convert name_path to a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:26:18Z","receivedAt":"2016-02-11T22:26:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"struct name_path\" data is examined in only two places:\nwe generate it in process_tree(), and we convert it to a\nsingle string in path_name(). Everyone else just passes it\nthrough to those functions.\n\nWe can further note that process_tree() already keeps a\nsingle strbuf with the leading tree path, for use with\ntree_entry_interesting().\n\nInstead of building a separate name_path linked list, let's\njust use the one we already build in \"base\". This reduces\nthe amount of code (especially tricky code in path_name()\nwhich did not check for integer overflows caused by deep\nor large pathnames).\n\nIt is also more efficient in some instances.  Any time we\nwere using tree_entry_interesting, we were building up the\nstrbuf anyway, so this is an immediate and obvious win\nthere. In cases where we were not, we trade off storing\n\"pathname/\" in a strbuf on the heap for each level of the\npath, instead of two pointers and an int on the stack (with\none pointer into the tree object). On a 64-bit system, the\nlatter is 20 bytes; so if path components are less than that\non average, this has lower peak memory usage.  In practice\nit probably doesn't matter either way; we are already\nholding in memory all of the tree objects leading up to each\npathname, and for normal-depth pathnames, we are only\ntalking about hundreds of bytes.\n\nThis patch leaves \"struct name_path\" as a thin wrapper\naround the strbuf, to avoid disrupting callbacks. We should\nfix them, but leaving it out makes this diff easier to view.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects.c | 22 +++++++++-------------\n revision.c     | 25 +++++--------------------\n revision.h     |  4 +---\n 3 files changed, 15 insertions(+), 36 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex 11732d9..4f60a3e 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -62,7 +62,6 @@ static void process_gitlink(struct rev_info *revs,\n static void process_tree(struct rev_info *revs,\n \t\t\t struct tree *tree,\n \t\t\t show_object_fn show,\n-\t\t\t struct name_path *path,\n \t\t\t struct strbuf *base,\n \t\t\t const char *name,\n \t\t\t void *cb_data)\n@@ -86,17 +85,14 @@ static void process_tree(struct rev_info *revs,\n \t\t\treturn;\n \t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n+\n \tobj->flags |= SEEN;\n-\tshow(obj, path, name, cb_data);\n-\tme.up = path;\n-\tme.elem = name;\n-\tme.elem_len = strlen(name);\n-\n-\tif (!match) {\n-\t\tstrbuf_addstr(base, name);\n-\t\tif (base->len)\n-\t\t\tstrbuf_addch(base, '/');\n-\t}\n+\tme.base = base;\n+\tshow(obj, &me, name, cb_data);\n+\n+\tstrbuf_addstr(base, name);\n+\tif (base->len)\n+\t\tstrbuf_addch(base, '/');\n \n \tinit_tree_desc(&desc, tree->buffer, tree->size);\n \n@@ -113,7 +109,7 @@ static void process_tree(struct rev_info *revs,\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tprocess_tree(revs,\n \t\t\t\t     lookup_tree(entry.sha1),\n-\t\t\t\t     show, &me, base, entry.path,\n+\t\t\t\t     show, base, entry.path,\n \t\t\t\t     cb_data);\n \t\telse if (S_ISGITLINK(entry.mode))\n \t\t\tprocess_gitlink(revs, entry.sha1,\n@@ -220,7 +216,7 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t\tpath = \"\";\n \t\tif (obj->type == OBJ_TREE) {\n \t\t\tprocess_tree(revs, (struct tree *)obj, show_object,\n-\t\t\t\t     NULL, &base, path, data);\n+\t\t\t\t     &base, path, data);\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\ndiff --git a/revision.c b/revision.c\nindex 6387068..8dd0950 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -27,26 +27,11 @@ static const char *term_good;\n \n char *path_name(const struct name_path *path, const char *name)\n {\n-\tconst struct name_path *p;\n-\tchar *n, *m;\n-\tint nlen = strlen(name);\n-\tint len = nlen + 1;\n-\n-\tfor (p = path; p; p = p->up) {\n-\t\tif (p->elem_len)\n-\t\t\tlen += p->elem_len + 1;\n-\t}\n-\tn = xmalloc(len);\n-\tm = n + len - (nlen + 1);\n-\tmemcpy(m, name, nlen + 1);\n-\tfor (p = path; p; p = p->up) {\n-\t\tif (p->elem_len) {\n-\t\t\tm -= p->elem_len + 1;\n-\t\t\tmemcpy(m, p->elem, p->elem_len);\n-\t\t\tm[p->elem_len] = '/';\n-\t\t}\n-\t}\n-\treturn n;\n+\tstruct strbuf ret = STRBUF_INIT;\n+\tif (path)\n+\t\tstrbuf_addbuf(&ret, path->base);\n+\tstrbuf_addstr(&ret, name);\n+\treturn strbuf_detach(&ret, NULL);\n }\n \n void show_object_with_name(FILE *out, struct object *obj,\ndiff --git a/revision.h b/revision.h\nindex 23857c0..2a26310 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -258,9 +258,7 @@ extern void mark_parents_uninteresting(struct commit *commit);\n extern void mark_tree_uninteresting(struct tree *tree);\n \n struct name_path {\n-\tstruct name_path *up;\n-\tint elem_len;\n-\tconst char *elem;\n+\tstruct strbuf *base;\n };\n \n char *path_name(const struct name_path *path, const char *name);\n-- \n2.7.1.550.gf5fcbd3\n"},{"id":"277972","messageId":"20160211222644.GD586@sigill.intra.peff.net","threadId":"41394","inReplyTo":"20160211222314.GA31625@sigill.intra.peff.net","subject":"[PATCH 4/5] list-objects: drop name_path entirely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:26:44Z","receivedAt":"2016-02-11T22:26:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the previous commit, we left name_path as a thin wrapper\naround a strbuf. This patch drops it entirely. As a result,\nevery show_object_fn callback needs to be adjusted. However,\nnone of their code needs to be changed at all, because the\nonly use was to pass it to path_name(), which now handles\nthe bare strbuf.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c |  4 ++--\n builtin/rev-list.c     |  4 ++--\n list-objects.c         | 12 +++++-------\n list-objects.h         |  2 +-\n pack-bitmap-write.c    |  2 +-\n pack-bitmap.c          |  4 ++--\n reachable.c            |  2 +-\n revision.c             |  6 +++---\n revision.h             |  8 ++------\n 9 files changed, 19 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4dae5b1..8bbb9bd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2285,7 +2285,7 @@ static void show_commit(struct commit *commit, void *data)\n }\n \n static void show_object(struct object *obj,\n-\t\t\tconst struct name_path *path, const char *last,\n+\t\t\tstruct strbuf *path, const char *last,\n \t\t\tvoid *data)\n {\n \tchar *name = path_name(path, last);\n@@ -2480,7 +2480,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)\n }\n \n static void record_recent_object(struct object *obj,\n-\t\t\t\t const struct name_path *path,\n+\t\t\t\t struct strbuf *path,\n \t\t\t\t const char *last,\n \t\t\t\t void *data)\n {\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3aa89a1..a92c3ca 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -178,7 +178,7 @@ static void finish_commit(struct commit *commit, void *data)\n }\n \n static void finish_object(struct object *obj,\n-\t\t\t  const struct name_path *path, const char *name,\n+\t\t\t  struct strbuf *path, const char *name,\n \t\t\t  void *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\n@@ -189,7 +189,7 @@ static void finish_object(struct object *obj,\n }\n \n static void show_object(struct object *obj,\n-\t\t\tconst struct name_path *path, const char *component,\n+\t\t\tstruct strbuf *path, const char *component,\n \t\t\tvoid *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\ndiff --git a/list-objects.c b/list-objects.c\nindex 4f60a3e..4397766 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -11,7 +11,7 @@\n static void process_blob(struct rev_info *revs,\n \t\t\t struct blob *blob,\n \t\t\t show_object_fn show,\n-\t\t\t struct name_path *path,\n+\t\t\t struct strbuf *path,\n \t\t\t const char *name,\n \t\t\t void *cb_data)\n {\n@@ -52,7 +52,7 @@ static void process_blob(struct rev_info *revs,\n static void process_gitlink(struct rev_info *revs,\n \t\t\t    const unsigned char *sha1,\n \t\t\t    show_object_fn show,\n-\t\t\t    struct name_path *path,\n+\t\t\t    struct strbuf *path,\n \t\t\t    const char *name,\n \t\t\t    void *cb_data)\n {\n@@ -69,7 +69,6 @@ static void process_tree(struct rev_info *revs,\n \tstruct object *obj = &tree->object;\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n-\tstruct name_path me;\n \tenum interesting match = revs->diffopt.pathspec.nr == 0 ?\n \t\tall_entries_interesting: entry_not_interesting;\n \tint baselen = base->len;\n@@ -87,8 +86,7 @@ static void process_tree(struct rev_info *revs,\n \t}\n \n \tobj->flags |= SEEN;\n-\tme.base = base;\n-\tshow(obj, &me, name, cb_data);\n+\tshow(obj, base, name, cb_data);\n \n \tstrbuf_addstr(base, name);\n \tif (base->len)\n@@ -113,12 +111,12 @@ static void process_tree(struct rev_info *revs,\n \t\t\t\t     cb_data);\n \t\telse if (S_ISGITLINK(entry.mode))\n \t\t\tprocess_gitlink(revs, entry.sha1,\n-\t\t\t\t\tshow, &me, entry.path,\n+\t\t\t\t\tshow, base, entry.path,\n \t\t\t\t\tcb_data);\n \t\telse\n \t\t\tprocess_blob(revs,\n \t\t\t\t     lookup_blob(entry.sha1),\n-\t\t\t\t     show, &me, entry.path,\n+\t\t\t\t     show, base, entry.path,\n \t\t\t\t     cb_data);\n \t}\n \tstrbuf_setlen(base, baselen);\ndiff --git a/list-objects.h b/list-objects.h\nindex 136a1da..69c4c7d 100644\n--- a/list-objects.h\n+++ b/list-objects.h\n@@ -2,7 +2,7 @@\n #define LIST_OBJECTS_H\n \n typedef void (*show_commit_fn)(struct commit *, void *);\n-typedef void (*show_object_fn)(struct object *, const struct name_path *, const char *, void *);\n+typedef void (*show_object_fn)(struct object *, struct strbuf *, const char *, void *);\n void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);\n \n typedef void (*show_edge_fn)(struct commit *);\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 6bff970..65ed342 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -148,7 +148,7 @@ static uint32_t find_object_pos(const unsigned char *sha1)\n \treturn entry->in_pack_pos;\n }\n \n-static void show_object(struct object *object, const struct name_path *path,\n+static void show_object(struct object *object, struct strbuf *path,\n \t\t\tconst char *last, void *data)\n {\n \tstruct bitmap *base = data;\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex dd8dc16..51f790e 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -414,7 +414,7 @@ static int ext_index_add_object(struct object *object, const char *name)\n \treturn bitmap_pos + bitmap_git.pack->num_objects;\n }\n \n-static void show_object(struct object *object, const struct name_path *path,\n+static void show_object(struct object *object, struct strbuf *path,\n \t\t\tconst char *last, void *data)\n {\n \tstruct bitmap *base = data;\n@@ -895,7 +895,7 @@ struct bitmap_test_data {\n };\n \n static void test_show_object(struct object *object,\n-\t\t\t     const struct name_path *path,\n+\t\t\t     struct strbuf *path,\n \t\t\t     const char *last, void *data)\n {\n \tstruct bitmap_test_data *tdata = data;\ndiff --git a/reachable.c b/reachable.c\nindex 43616d4..e60f08d 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -43,7 +43,7 @@ static int add_one_ref(const char *path, const struct object_id *oid,\n  * The traversal will have already marked us as SEEN, so we\n  * only need to handle any progress reporting here.\n  */\n-static void mark_object(struct object *obj, const struct name_path *path,\n+static void mark_object(struct object *obj, struct strbuf *path,\n \t\t\tconst char *name, void *data)\n {\n \tupdate_progress(data);\ndiff --git a/revision.c b/revision.c\nindex 8dd0950..3c84781 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -25,17 +25,17 @@ volatile show_early_output_fn_t show_early_output;\n static const char *term_bad;\n static const char *term_good;\n \n-char *path_name(const struct name_path *path, const char *name)\n+char *path_name(struct strbuf *path, const char *name)\n {\n \tstruct strbuf ret = STRBUF_INIT;\n \tif (path)\n-\t\tstrbuf_addbuf(&ret, path->base);\n+\t\tstrbuf_addbuf(&ret, path);\n \tstrbuf_addstr(&ret, name);\n \treturn strbuf_detach(&ret, NULL);\n }\n \n void show_object_with_name(FILE *out, struct object *obj,\n-\t\t\t   const struct name_path *path, const char *component)\n+\t\t\t   struct strbuf *path, const char *component)\n {\n \tchar *name = path_name(path, component);\n \tchar *p;\ndiff --git a/revision.h b/revision.h\nindex 2a26310..7beab15 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -257,14 +257,10 @@ extern void put_revision_mark(const struct rev_info *revs,\n extern void mark_parents_uninteresting(struct commit *commit);\n extern void mark_tree_uninteresting(struct tree *tree);\n \n-struct name_path {\n-\tstruct strbuf *base;\n-};\n-\n-char *path_name(const struct name_path *path, const char *name);\n+char *path_name(struct strbuf *path, const char *name);\n \n extern void show_object_with_name(FILE *, struct object *,\n-\t\t\t\t  const struct name_path *, const char *);\n+\t\t\t\t  struct strbuf *, const char *);\n \n extern void add_pending_object(struct rev_info *revs,\n \t\t\t       struct object *obj, const char *name);\n-- \n2.7.1.550.gf5fcbd3\n"},{"id":"277973","messageId":"20160211222836.GE586@sigill.intra.peff.net","threadId":"41394","inReplyTo":"20160211222314.GA31625@sigill.intra.peff.net","subject":"[PATCH 5/5] list-objects: pass full pathname to callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:28:36Z","receivedAt":"2016-02-11T22:28:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we find a blob at \"a/b/c\", we currently pass this to\nour show_object_fn callbacks as two components: \"a/b/\" and\n\"c\". Callbacks which want the full value then call\npath_name(), which concatenates the two. But this is an\ninefficient interface; the path is a strbuf, and we could\nsimply append \"c\" to it temporarily, then roll back the\nlength, without creating a new copy.\n\nSo we could improve this by teaching the callsites of\npath_name() this trick (and there are only 3). But we can\nalso notice that no callback actually cares about the\nbroken-down representation, and simply pass each callback\nthe full path \"a/b/c\" as a string. The callback code becomes\neven simpler, then, as we do not have to worry about freeing\nan allocated buffer, nor rolling back our modification to\nthe strbuf.\n\nThis is theoretically less efficient, as some callbacks\nwould not bother to format the final path component. But in\npractice this is not measurable. Since we use the same\nstrbuf over and over, our work to grow it is amortized, and\nwe really only pay to memcpy a few bytes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 15 ++-------------\n builtin/rev-list.c     | 12 ++++--------\n list-objects.c         | 14 +++++++++-----\n list-objects.h         |  2 +-\n pack-bitmap-write.c    |  3 +--\n pack-bitmap.c          | 13 ++++---------\n reachable.c            |  5 ++---\n revision.c             | 17 ++---------------\n revision.h             |  3 +--\n 9 files changed, 26 insertions(+), 58 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8bbb9bd..a6609f1 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2284,21 +2284,11 @@ static void show_commit(struct commit *commit, void *data)\n \t\tindex_commit_for_bitmap(commit);\n }\n \n-static void show_object(struct object *obj,\n-\t\t\tstruct strbuf *path, const char *last,\n-\t\t\tvoid *data)\n+static void show_object(struct object *obj, const char *name, void *data)\n {\n-\tchar *name = path_name(path, last);\n-\n \tadd_preferred_base_object(name);\n \tadd_object_entry(obj->oid.hash, obj->type, name, 0);\n \tobj->flags |= OBJECT_ADDED;\n-\n-\t/*\n-\t * We will have generated the hash from the name,\n-\t * but not saved a pointer to it - we can free it\n-\t */\n-\tfree((char *)name);\n }\n \n static void show_edge(struct commit *commit)\n@@ -2480,8 +2470,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)\n }\n \n static void record_recent_object(struct object *obj,\n-\t\t\t\t struct strbuf *path,\n-\t\t\t\t const char *last,\n+\t\t\t\t const char *name,\n \t\t\t\t void *data)\n {\n \tsha1_array_append(&recent_objects, obj->oid.hash);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex a92c3ca..275da0d 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -177,9 +177,7 @@ static void finish_commit(struct commit *commit, void *data)\n \tfree_commit_buffer(commit);\n }\n \n-static void finish_object(struct object *obj,\n-\t\t\t  struct strbuf *path, const char *name,\n-\t\t\t  void *cb_data)\n+static void finish_object(struct object *obj, const char *name, void *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\n \tif (obj->type == OBJ_BLOB && !has_object_file(&obj->oid))\n@@ -188,15 +186,13 @@ static void finish_object(struct object *obj,\n \t\tparse_object(obj->oid.hash);\n }\n \n-static void show_object(struct object *obj,\n-\t\t\tstruct strbuf *path, const char *component,\n-\t\t\tvoid *cb_data)\n+static void show_object(struct object *obj, const char *name, void *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\n-\tfinish_object(obj, path, component, cb_data);\n+\tfinish_object(obj, name, cb_data);\n \tif (info->flags & REV_LIST_QUIET)\n \t\treturn;\n-\tshow_object_with_name(stdout, obj, path, component);\n+\tshow_object_with_name(stdout, obj, name);\n }\n \n static void show_edge(struct commit *commit)\ndiff --git a/list-objects.c b/list-objects.c\nindex 4397766..917cc5d 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -16,6 +16,7 @@ static void process_blob(struct rev_info *revs,\n \t\t\t void *cb_data)\n {\n \tstruct object *obj = &blob->object;\n+\tsize_t pathlen;\n \n \tif (!revs->blob_objects)\n \t\treturn;\n@@ -24,7 +25,11 @@ static void process_blob(struct rev_info *revs,\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n \tobj->flags |= SEEN;\n-\tshow(obj, path, name, cb_data);\n+\n+\tpathlen = path->len;\n+\tstrbuf_addstr(path, name);\n+\tshow(obj, path->buf, cb_data);\n+\tstrbuf_setlen(path, pathlen);\n }\n \n /*\n@@ -86,9 +91,8 @@ static void process_tree(struct rev_info *revs,\n \t}\n \n \tobj->flags |= SEEN;\n-\tshow(obj, base, name, cb_data);\n-\n \tstrbuf_addstr(base, name);\n+\tshow(obj, base->buf, cb_data);\n \tif (base->len)\n \t\tstrbuf_addch(base, '/');\n \n@@ -207,7 +211,7 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t\tcontinue;\n \t\tif (obj->type == OBJ_TAG) {\n \t\t\tobj->flags |= SEEN;\n-\t\t\tshow_object(obj, NULL, name, data);\n+\t\t\tshow_object(obj, name, data);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!path)\n@@ -219,7 +223,7 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\n \t\t\tprocess_blob(revs, (struct blob *)obj, show_object,\n-\t\t\t\t     NULL, path, data);\n+\t\t\t\t     &base, path, data);\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"unknown pending object %s (%s)\",\ndiff --git a/list-objects.h b/list-objects.h\nindex 69c4c7d..0cebf85 100644\n--- a/list-objects.h\n+++ b/list-objects.h\n@@ -2,7 +2,7 @@\n #define LIST_OBJECTS_H\n \n typedef void (*show_commit_fn)(struct commit *, void *);\n-typedef void (*show_object_fn)(struct object *, struct strbuf *, const char *, void *);\n+typedef void (*show_object_fn)(struct object *, const char *, void *);\n void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);\n \n typedef void (*show_edge_fn)(struct commit *);\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 65ed342..c30bcd0 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -148,8 +148,7 @@ static uint32_t find_object_pos(const unsigned char *sha1)\n \treturn entry->in_pack_pos;\n }\n \n-static void show_object(struct object *object, struct strbuf *path,\n-\t\t\tconst char *last, void *data)\n+static void show_object(struct object *object, const char *name, void *data)\n {\n \tstruct bitmap *base = data;\n \tbitmap_set(base, find_object_pos(object->oid.hash));\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 51f790e..b949e51 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -414,19 +414,15 @@ static int ext_index_add_object(struct object *object, const char *name)\n \treturn bitmap_pos + bitmap_git.pack->num_objects;\n }\n \n-static void show_object(struct object *object, struct strbuf *path,\n-\t\t\tconst char *last, void *data)\n+static void show_object(struct object *object, const char *name, void *data)\n {\n \tstruct bitmap *base = data;\n \tint bitmap_pos;\n \n \tbitmap_pos = bitmap_position(object->oid.hash);\n \n-\tif (bitmap_pos < 0) {\n-\t\tchar *name = path_name(path, last);\n+\tif (bitmap_pos < 0)\n \t\tbitmap_pos = ext_index_add_object(object, name);\n-\t\tfree(name);\n-\t}\n \n \tbitmap_set(base, bitmap_pos);\n }\n@@ -894,9 +890,8 @@ struct bitmap_test_data {\n \tsize_t seen;\n };\n \n-static void test_show_object(struct object *object,\n-\t\t\t     struct strbuf *path,\n-\t\t\t     const char *last, void *data)\n+static void test_show_object(struct object *object, const char *name,\n+\t\t\t     void *data)\n {\n \tstruct bitmap_test_data *tdata = data;\n \tint bitmap_pos;\ndiff --git a/reachable.c b/reachable.c\nindex e60f08d..ed35201 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -43,15 +43,14 @@ static int add_one_ref(const char *path, const struct object_id *oid,\n  * The traversal will have already marked us as SEEN, so we\n  * only need to handle any progress reporting here.\n  */\n-static void mark_object(struct object *obj, struct strbuf *path,\n-\t\t\tconst char *name, void *data)\n+static void mark_object(struct object *obj, const char *name, void *data)\n {\n \tupdate_progress(data);\n }\n \n static void mark_commit(struct commit *c, void *data)\n {\n-\tmark_object(&c->object, NULL, NULL, data);\n+\tmark_object(&c->object, NULL, data);\n }\n \n struct recent_data {\ndiff --git a/revision.c b/revision.c\nindex 3c84781..82f3ca4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -25,27 +25,14 @@ volatile show_early_output_fn_t show_early_output;\n static const char *term_bad;\n static const char *term_good;\n \n-char *path_name(struct strbuf *path, const char *name)\n+void show_object_with_name(FILE *out, struct object *obj, const char *name)\n {\n-\tstruct strbuf ret = STRBUF_INIT;\n-\tif (path)\n-\t\tstrbuf_addbuf(&ret, path);\n-\tstrbuf_addstr(&ret, name);\n-\treturn strbuf_detach(&ret, NULL);\n-}\n-\n-void show_object_with_name(FILE *out, struct object *obj,\n-\t\t\t   struct strbuf *path, const char *component)\n-{\n-\tchar *name = path_name(path, component);\n-\tchar *p;\n+\tconst char *p;\n \n \tfprintf(out, \"%s \", oid_to_hex(&obj->oid));\n \tfor (p = name; *p && *p != '\\n'; p++)\n \t\tfputc(*p, out);\n \tfputc('\\n', out);\n-\n-\tfree(name);\n }\n \n static void mark_blob_uninteresting(struct blob *blob)\ndiff --git a/revision.h b/revision.h\nindex 7beab15..dca0d38 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -259,8 +259,7 @@ extern void mark_tree_uninteresting(struct tree *tree);\n \n char *path_name(struct strbuf *path, const char *name);\n \n-extern void show_object_with_name(FILE *, struct object *,\n-\t\t\t\t  struct strbuf *, const char *);\n+extern void show_object_with_name(FILE *, struct object *, const char *);\n \n extern void add_pending_object(struct rev_info *revs,\n \t\t\t       struct object *obj, const char *name);\n-- \n2.7.1.550.gf5fcbd3\n"},{"id":"277975","messageId":"20160211223634.GC641@sigill.intra.peff.net","threadId":"41394","inReplyTo":"20160211222836.GE586@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] list-objects: pass full pathname to callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-11T22:36:34Z","receivedAt":"2016-02-11T22:36:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 11, 2016 at 05:28:36PM -0500, Jeff King wrote:\n\n> +void show_object_with_name(FILE *out, struct object *obj, const char *name)\n> [...]\n>  \tfprintf(out, \"%s \", oid_to_hex(&obj->oid));\n>  \tfor (p = name; *p && *p != '\\n'; p++)\n>  \t\tfputc(*p, out);\n>  \tfputc('\\n', out);\n\nBy the way, since I was timing things, I wondered if we would see any\nimprovement from using putc_unlocked, like:\n\ndiff --git a/revision.c b/revision.c\nindex 82f3ca4..ab72247 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -30,9 +30,11 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name)\n \tconst char *p;\n \n \tfprintf(out, \"%s \", oid_to_hex(&obj->oid));\n+\tflockfile(out);\n \tfor (p = name; *p && *p != '\\n'; p++)\n-\t\tfputc(*p, out);\n-\tfputc('\\n', out);\n+\t\tputc_unlocked(*p, out);\n+\tputc_unlocked('\\n', out);\n+\tfunlockfile(out);\n }\n \n static void mark_blob_uninteresting(struct blob *blob)\n\nBut I couldn't measure any speedup. I imagine if you had 500MB pathnames\nyou might see some improvement, but I don't think it is even worth the\nextra lines of code to worry about such a pathological case.\n\n-Peff\n"}]}