{"thread":{"id":"39327","subject":"\"HEAD -> branch\" decoration doesn't work with \"--decorate=full\"","startedAt":"2015-05-13T13:11:49Z","lastAt":"2015-05-22T21:38:53Z","messageCount":17,"participants":["Michael Haggerty","Junio C Hamano","Michael J Gruber","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"261175","messageId":"55534D95.60609@alum.mit.edu","threadId":"39327","inReplyTo":null,"subject":"\"HEAD -> branch\" decoration doesn't work with \"--decorate=full\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-13T13:11:49Z","receivedAt":"2015-05-13T13:11:49Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Hi,\n\nThe new-style \"HEAD -> branch\" style decoration doesn't work when\n\"--decorate=full\" is used:\n\n> $ bin-wrappers/git show --oneline --decorate\n> c518059 (HEAD -> master, gitster/master) Merge branch 'maint'\n> \n> $ bin-wrappers/git show --oneline --decorate=full\n> c518059 (HEAD, refs/remotes/gitster/master, refs/heads/master) Merge branch 'maint'\n\nI would have expected the second invocation to show \"HEAD ->\nrefs/heads/master\".\n\nWas that an oversight or a conscious decision?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"261181","messageId":"xmqq617wbkez.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"55534D95.60609@alum.mit.edu","subject":"Re: \"HEAD -> branch\" decoration doesn't work with \"--decorate=full\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-13T14:51:16Z","receivedAt":"2015-05-13T14:51:16Z","isPatch":false,"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 new-style \"HEAD -> branch\" style decoration doesn't work when\n> \"--decorate=full\" is used:\n> ...\n> Was that an oversight or a conscious decision?\n\nI do not recall making such a decision, and I doubt (the other)\nMichael wanted that way, either, so patches welcome, perhaps?\n"},{"id":"261183","messageId":"55536D28.6070001@drmicha.warpmail.net","threadId":"39327","inReplyTo":"xmqq617wbkez.fsf@gitster.dls.corp.google.com","subject":"Re: \"HEAD -> branch\" decoration doesn't work with \"--decorate=full\"","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-05-13T15:26:32Z","receivedAt":"2015-05-13T15:26:32Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 13.05.2015 16:51:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> The new-style \"HEAD -> branch\" style decoration doesn't work when\n>> \"--decorate=full\" is used:\n>> ...\n>> Was that an oversight or a conscious decision?\n> \n> I do not recall making such a decision, and I doubt (the other)\n> Michael wanted that way, either, so patches welcome, perhaps?\n\nI was not aware of an alternate codepath for full decorations (and am\nsurprised about it).\n\nI can't look into this before next week, but --decorate=full should\nproduce \"HEAD -> refs/heads/master\" and such.\n\nI even vaguely remember testing it, but that may have been for v1 which\nturned different knobs than the end version.\n\nMichael\n"},{"id":"261187","messageId":"xmqqwq0c9zc1.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"55534D95.60609@alum.mit.edu","subject":"Re: \"HEAD -> branch\" decoration doesn't work with \"--decorate=full\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-13T17:11:58Z","receivedAt":"2015-05-13T17:11:58Z","isPatch":false,"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 new-style \"HEAD -> branch\" style decoration doesn't work when\n> \"--decorate=full\" is used:\n>\n>> $ bin-wrappers/git show --oneline --decorate\n>> c518059 (HEAD -> master, gitster/master) Merge branch 'maint'\n>> \n>> $ bin-wrappers/git show --oneline --decorate=full\n>> c518059 (HEAD, refs/remotes/gitster/master, refs/heads/master) Merge branch 'maint'\n>\n> I would have expected the second invocation to show \"HEAD ->\n> refs/heads/master\".\n>\n> Was that an oversight or a conscious decision?\n\nI actually think this ultimately comes from a poor design of the\nname-decorations infrastructure.  The program is expected to call\nload_ref_decorations() only once and make the choice between the\nfull/short at that point, which is passed to add_ref_decoration() to\nrecord either 'refs/heads/master' or 'master' in the singleton\nname_decoration decoration.  But it does not store which one was\nchosen by the caller of load_ref_decorations() anywhere in the\nsubsystem.\n\nWhen current_pointed_by_HEAD() wants to see if decorations on an\nobject, e.g. 'master', matches what 'HEAD' resolves to, it cannot\ntell if the original set-up was done for the full decoration, and\nthe current code just assumes (without even realizing that it is\nmaking that assumption) the decoration must have been set up for the\nshort ones.\n\nPerhaps something like this, but I am not committing it without\ntests or a proper log messge.\n\nI moved \"static loaded\" outside as it is in the same category as the\nglobal name-decoration and decoration-flags, i.e. to be initialised\nonce at the beginning to a fixed setting and then be used with that\nsetting.\n\n\n log-tree.c | 23 ++++++++++++++++++-----\n 1 file changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 2c1ed0f..92259bc 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -13,6 +13,8 @@\n #include \"line-log.h\"\n \n static struct decoration name_decoration = { \"object names\" };\n+static int decoration_loaded;\n+static int decoration_flags;\n \n static char decoration_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n@@ -146,9 +148,9 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n \n void load_ref_decorations(int flags)\n {\n-\tstatic int loaded;\n-\tif (!loaded) {\n-\t\tloaded = 1;\n+\tif (!decoration_loaded) {\n+\t\tdecoration_loaded = 1;\n+\t\tdecoration_flags = flags;\n \t\tfor_each_ref(add_ref_decoration, &flags);\n \t\thead_ref(add_ref_decoration, &flags);\n \t\tfor_each_commit_graft(add_graft_decoration, NULL);\n@@ -196,8 +198,19 @@ static const struct name_decoration *current_pointed_by_HEAD(const struct name_d\n \tbranch_name = resolve_ref_unsafe(\"HEAD\", 0, unused, &rru_flags);\n \tif (!(rru_flags & REF_ISSYMREF))\n \t\treturn NULL;\n-\tif (!skip_prefix(branch_name, \"refs/heads/\", &branch_name))\n-\t\treturn NULL;\n+\n+\tif ((decoration_flags == DECORATE_SHORT_REFS)) {\n+\t\tif (!skip_prefix(branch_name, \"refs/heads/\", &branch_name))\n+\t\t\treturn NULL;\n+\t} else {\n+\t\t/*\n+\t\t * Each decoration has a refname in full; keep\n+\t\t * branch_name also in full, but still make sure\n+\t\t * HEAD is a reasonable ref.\n+\t\t */\n+\t\tif (!starts_with(branch_name, \"refs/\"))\n+\t\t\treturn NULL;\n+\t}\n \n \t/* OK, do we have that ref in the list? */\n \tfor (list = decoration; list; list = list->next)\n"},{"id":"261188","messageId":"xmqqsib09z8y.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"xmqqwq0c9zc1.fsf@gitster.dls.corp.google.com","subject":"Re: \"HEAD -> branch\" decoration doesn't work with \"--decorate=full\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-13T17:13:49Z","receivedAt":"2015-05-13T17:13:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>\n>> Was that an oversight or a conscious decision?\n>\n> I actually think this ultimately comes from a poor design of the\n> name-decorations infrastructure.\n\nI should have said \"poor design of the way how the name-decorations\ninfrastructure is used in log-tree (and commands in the log family)\".\n\nThe name-decorations infrastructure itself is just fine.\n"},{"id":"261198","messageId":"xmqqoalo9sgc.fsf_-_@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"xmqqsib09z8y.fsf@gitster.dls.corp.google.com","subject":"[PATCH 2/2] log: do not shorten decoration names too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-13T19:40:35Z","receivedAt":"2015-05-13T19:40:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The DECORATE_SHORT_REFS option given to load_ref_decorations()\naffects the way a copy of the refname is stored for each decorated\ncommit, and this forces later steps like current_pointed_by_HEAD()\nto adjust their behaviour based on this initial settings.\n\nInstead, we can always store the full refname and then shorten them\nwhen producing the output.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * [1/2] is just the earlier \"this should fix it\" patch, with\n   adjustments to the existing tests.\n\n   I suspect that it may be a good idea to lose the decoration_flags\n   from load_ref_decorations() and instead make that a new parameter\n   to format_decorations().  That way, the caller could decide which\n   ones to use.  It is not unconceivable to extend \"log --format=%d\"\n   that shows the decoration in the style given by --decorate arg\n   and let the callers specify two additional formats (i.e. decorate\n   always short, decorate always in full), and for that kind of\n   work, this patch will become a prerequisite.\n\n log-tree.c | 34 ++++++++++++++++------------------\n 1 file changed, 16 insertions(+), 18 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 92259bc..c931615 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -94,6 +94,8 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in\n \tstruct object *obj;\n \tenum decoration_type type = DECORATION_NONE;\n \n+\tassert(cb_data == NULL);\n+\n \tif (starts_with(refname, \"refs/replace/\")) {\n \t\tunsigned char original_sha1[20];\n \t\tif (!check_replace_refs)\n@@ -123,8 +125,6 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in\n \telse if (!strcmp(refname, \"HEAD\"))\n \t\ttype = DECORATION_REF_HEAD;\n \n-\tif (!cb_data || *(int *)cb_data == DECORATE_SHORT_REFS)\n-\t\trefname = prettify_refname(refname);\n \tadd_name_decoration(type, refname, obj);\n \twhile (obj->type == OBJ_TAG) {\n \t\tobj = ((struct tag *)obj)->tagged;\n@@ -151,8 +151,8 @@ void load_ref_decorations(int flags)\n \tif (!decoration_loaded) {\n \t\tdecoration_loaded = 1;\n \t\tdecoration_flags = flags;\n-\t\tfor_each_ref(add_ref_decoration, &flags);\n-\t\thead_ref(add_ref_decoration, &flags);\n+\t\tfor_each_ref(add_ref_decoration, NULL);\n+\t\thead_ref(add_ref_decoration, NULL);\n \t\tfor_each_commit_graft(add_graft_decoration, NULL);\n \t}\n }\n@@ -199,18 +199,8 @@ static const struct name_decoration *current_pointed_by_HEAD(const struct name_d\n \tif (!(rru_flags & REF_ISSYMREF))\n \t\treturn NULL;\n \n-\tif ((decoration_flags == DECORATE_SHORT_REFS)) {\n-\t\tif (!skip_prefix(branch_name, \"refs/heads/\", &branch_name))\n-\t\t\treturn NULL;\n-\t} else {\n-\t\t/*\n-\t\t * Each decoration has a refname in full; keep\n-\t\t * branch_name also in full, but still make sure\n-\t\t * HEAD is a reasonable ref.\n-\t\t */\n-\t\tif (!starts_with(branch_name, \"refs/\"))\n-\t\t\treturn NULL;\n-\t}\n+\tif (!starts_with(branch_name, \"refs/\"))\n+\t\treturn NULL;\n \n \t/* OK, do we have that ref in the list? */\n \tfor (list = decoration; list; list = list->next)\n@@ -222,6 +212,14 @@ static const struct name_decoration *current_pointed_by_HEAD(const struct name_d\n \treturn NULL;\n }\n \n+static void show_name(struct strbuf *sb, const struct name_decoration *decoration)\n+{\n+\tif (decoration_flags == DECORATE_SHORT_REFS)\n+\t\tstrbuf_addstr(sb, prettify_refname(decoration->name));\n+\telse\n+\t\tstrbuf_addstr(sb, decoration->name);\n+}\n+\n /*\n  * The caller makes sure there is no funny color before calling.\n  * format_decorations_extended makes sure the same after return.\n@@ -259,7 +257,7 @@ void format_decorations_extended(struct strbuf *sb,\n \t\t\tif (decoration->type == DECORATION_REF_TAG)\n \t\t\t\tstrbuf_addstr(sb, \"tag: \");\n \n-\t\t\tstrbuf_addstr(sb, decoration->name);\n+\t\t\tshow_name(sb, decoration);\n \n \t\t\tif (current_and_HEAD &&\n \t\t\t    decoration->type == DECORATION_REF_HEAD) {\n@@ -268,7 +266,7 @@ void format_decorations_extended(struct strbuf *sb,\n \t\t\t\tstrbuf_addstr(sb, \" -> \");\n \t\t\t\tstrbuf_addstr(sb, color_reset);\n \t\t\t\tstrbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));\n-\t\t\t\tstrbuf_addstr(sb, current_and_HEAD->name);\n+\t\t\t\tshow_name(sb, current_and_HEAD);\n \t\t\t}\n \t\t\tstrbuf_addstr(sb, color_reset);\n \n"},{"id":"261221","messageId":"20150514063317.GA22509@peff.net","threadId":"39327","inReplyTo":"xmqqoalo9sgc.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T06:33:18Z","receivedAt":"2015-05-14T06:33:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2015 at 12:40:35PM -0700, Junio C Hamano wrote:\n\n> The DECORATE_SHORT_REFS option given to load_ref_decorations()\n> affects the way a copy of the refname is stored for each decorated\n> commit, and this forces later steps like current_pointed_by_HEAD()\n> to adjust their behaviour based on this initial settings.\n> \n> Instead, we can always store the full refname and then shorten them\n> when producing the output.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> \n>  * [1/2] is just the earlier \"this should fix it\" patch, with\n>    adjustments to the existing tests.\n\nNice. After reading the first one, I was wondering why it did not look\nlike this one. :)\n\n>    I suspect that it may be a good idea to lose the decoration_flags\n>    from load_ref_decorations() and instead make that a new parameter\n>    to format_decorations().  That way, the caller could decide which\n>    ones to use.  It is not unconceivable to extend \"log --format=%d\"\n>    that shows the decoration in the style given by --decorate arg\n>    and let the callers specify two additional formats (i.e. decorate\n>    always short, decorate always in full), and for that kind of\n>    work, this patch will become a prerequisite.\n\nYeah, agreed.\n\nWhile we are on the subject of the name_decoration code, I had\nconsidered at one point replacing the use of the decorate.[ch] hash\ntable with a commit_slab (you can't do it in the general case, because\ndecorate.[ch] handles arbitrary objects, but the name_decoration code\nonly does commits). It would in theory be faster, though I don't know if\nthe time we spend on the hash table is actually measurable (we make a\nlot of queries on it, but it doesn't actually get that big in the first\nplace).\n\nIn case you are looking for something to do with your copious free time.\n:)\n\n-Peff\n"},{"id":"261249","messageId":"xmqq8ucr83h8.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"20150514063317.GA22509@peff.net","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T17:37:39Z","receivedAt":"2015-05-14T17:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> While we are on the subject of the name_decoration code, I had\n> considered at one point replacing the use of the decorate.[ch] hash\n> table with a commit_slab (you can't do it in the general case, because\n> decorate.[ch] handles arbitrary objects, but the name_decoration code\n> only does commits). It would in theory be faster, though I don't know if\n> the time we spend on the hash table is actually measurable (we make a\n> lot of queries on it, but it doesn't actually get that big in the first\n> place).\n\nHmmm, but I do not know if commit_slab is a good fit for the usage\npattern.  I expect commit objects to be fairly sparsely decorated\n(e.g. a tag or ref pointing at say 1-2% of commits or fewer).\nWouldn't the hashtable used by decorate.[ch] with the max load\nfactor capped to 66% a better economy?\n\nI notice that there is no API into commit_slab to ask \"Does this\ncommit have data in the slab?\"  *slabname##_at() may be the closest\nthing, but that would allocate the space and then says \"This is the\nslot for that commit; go check if there is data there already.\"\n\nIn the context of using commit_slab in log-tree.c for decoration, it\nwould mean that we assign low slab indices to commits at the tips by\nfirst calling \"for_each_ref(add_ref_decoration)\" and populate the\nslab fairly densely at the beginning.  But when we check if a commit\nthat we encountered during a traversal is decorated or not, we would\nask *slabname##_at() and that ends up enlarging the slab, even at\nthat point the only thing we are interested in is if the commit is\ndecorated and we are not adding a new decoration for it.\n\nFor example, we have this in commit.c:\n\n    const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n    {\n            struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n            if (sizep)\n                    *sizep = v->size;\n            return v->buffer;\n    }\n\nBut if we do not have the \"buffer\" data cached for that commit (via\nan earlier call to set_commit_buffer()), we don't have to enlarge\nthe slab, as we are not adding anything to the slab system with this\ncall.\n\nPerhaps we want a new function *slabname##_peek() with the same\nsignature as *slabname##_at() that returns NULL when commit->index\nis larger than the last existing element in the slab?  Then the\nabove would become more like:\n\n    const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n    {\n            struct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n            if (!v)\n                    return NULL;\n            if (sizep)\n                    *sizep = v->size;\n            return v->buffer;\n    }\n"},{"id":"261252","messageId":"20150514174945.GB7966@peff.net","threadId":"39327","inReplyTo":"xmqq8ucr83h8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T17:49:45Z","receivedAt":"2015-05-14T17:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2015 at 10:37:39AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > While we are on the subject of the name_decoration code, I had\n> > considered at one point replacing the use of the decorate.[ch] hash\n> > table with a commit_slab (you can't do it in the general case, because\n> > decorate.[ch] handles arbitrary objects, but the name_decoration code\n> > only does commits). It would in theory be faster, though I don't know if\n> > the time we spend on the hash table is actually measurable (we make a\n> > lot of queries on it, but it doesn't actually get that big in the first\n> > place).\n> \n> Hmmm, but I do not know if commit_slab is a good fit for the usage\n> pattern.  I expect commit objects to be fairly sparsely decorated\n> (e.g. a tag or ref pointing at say 1-2% of commits or fewer).\n> Wouldn't the hashtable used by decorate.[ch] with the max load\n> factor capped to 66% a better economy?\n\nGood point. A slab would be less memory efficient, but cheaper to look\nup (it is a direct memory reference, with no probing and no hashcmp()).\nBut cache effects matter, so it could even be slower.\n\nOn the other hand, the slab makes it easy to actually store the real\ntype (struct name_decoration), whereas the decorate hash stores only\npointers. So we'd save an extra malloc/pointer in each case.\n\nSo with your slab_peek() below, I'd guess that the slab would actually\nbe faster. But I'd also be unsurprised if it makes no appreciable\ndifference to the overall runtime of \"git log --decorate\". I think we'd\nhave to build it and profile (and please feel free to say \"eh, not worth\nthe time to think about further\").\n\n> I notice that there is no API into commit_slab to ask \"Does this\n> commit have data in the slab?\"  *slabname##_at() may be the closest\n> thing, but that would allocate the space and then says \"This is the\n> slot for that commit; go check if there is data there already.\"\n\nYes. I think it's not a big deal if your slab is reasonably full (you'll\nextend it to the full size of your commit space eventually either way).\nBut for a sparse slab, it does make that query much more expensive than\nit needs to be.\n\n> Perhaps we want a new function *slabname##_peek() with the same\n> signature as *slabname##_at() that returns NULL when commit->index\n> is larger than the last existing element in the slab?  Then the\n> above would become more like:\n> \n>     const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n>     {\n>             struct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n>             if (!v)\n>                     return NULL;\n>             if (sizep)\n>                     *sizep = v->size;\n>             return v->buffer;\n>     }\n\nYeah, I agree that is a good interface. It is basically a read-only\nversion of slab_at(). And it should be similarly agnostic as to the type\nwe are storing, because the pointer is into the slab array.\n\nI'm not sure that the memory overhead is that big a deal (even in the\nkernel, we are only talking about a few megabytes). But it is wasteful,\nand the interface above should be trivial to write, so it might be worth\ndoing anyway.\n\n-Peff\n"},{"id":"261253","messageId":"xmqqzj576nts.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"20150514174945.GB7966@peff.net","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T18:01:03Z","receivedAt":"2015-05-14T18:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, May 14, 2015 at 10:37:39AM -0700, Junio C Hamano wrote:\n> ...\n>> Wouldn't the hashtable used by decorate.[ch] with the max load\n>> factor capped to 66% a better economy?\n>\n> Good point. A slab would be less memory efficient, but cheaper to look\n> up (it is a direct memory reference, with no probing and no hashcmp()).\n> But cache effects matter, so it could even be slower.\n\nYes, that was what I meant by economy.  I do not think memory footprint\nis free in that sense.\n\n> On the other hand, the slab makes it easy to actually store the real\n> type (struct name_decoration), whereas the decorate hash stores only\n> pointers. So we'd save an extra malloc/pointer in each case.\n>\n> So with your slab_peek() below, I'd guess that the slab would actually\n> be faster. But I'd also be unsurprised if it makes no appreciable\n> difference to the overall runtime of \"git log --decorate\". I think we'd\n> have to build it and profile (and please feel free to say \"eh, not worth\n> the time to think about further\").\n\nWhile I think *slabname##_peek() would be worth doing regardless of\nthis caller, I suspect that the major overhead of decorate code\nwould come from the for_each_ref() that jumps deep into the history\nto parse old commits; it would trigger a lot of unpacking of objects\ndeep in the delta chain, which would be expensive than table look-up\nin either scheme.\n\n>> I notice that there is no API into commit_slab to ask \"Does this\n>> commit have data in the slab?\"  *slabname##_at() may be the closest\n>> thing, but that would allocate the space and then says \"This is the\n>> slot for that commit; go check if there is data there already.\"\n>\n> Yes. I think it's not a big deal if your slab is reasonably full (you'll\n> extend it to the full size of your commit space eventually either way).\n> But for a sparse slab, it does make that query much more expensive than\n> it needs to be.\n\nYes, and I think that commit decoration is such a use case.\n"},{"id":"261255","messageId":"20150514181008.GA8486@peff.net","threadId":"39327","inReplyTo":"xmqqzj576nts.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T18:10:08Z","receivedAt":"2015-05-14T18:10:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2015 at 11:01:03AM -0700, Junio C Hamano wrote:\n\n> > So with your slab_peek() below, I'd guess that the slab would actually\n> > be faster. But I'd also be unsurprised if it makes no appreciable\n> > difference to the overall runtime of \"git log --decorate\". I think we'd\n> > have to build it and profile (and please feel free to say \"eh, not worth\n> > the time to think about further\").\n> \n> While I think *slabname##_peek() would be worth doing regardless of\n> this caller, I suspect that the major overhead of decorate code\n> would come from the for_each_ref() that jumps deep into the history\n> to parse old commits; it would trigger a lot of unpacking of objects\n> deep in the delta chain, which would be expensive than table look-up\n> in either scheme.\n\nThat would depend on the particular repository and traversal. The\nexpensive \"load an old commit\" work is done once per ref in the repo.\nThe lookup work is done once per commit traversed. So even if the latter\nis much less work, we are typically doing it many more times, and there\nis probably a balance point.\n\nBut I suspect all of it compares to the actual work of a \"git log\" which\nhas to read all of the commits we are looking at anyway. IOW, we are\nprobably talking about optimizing 1%, while the other 99% is spent on\ninflate(), etc.\n\n-Peff\n"},{"id":"261300","messageId":"xmqqfv6yj0dl.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"xmqq8ucr83h8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T21:49:10Z","receivedAt":"2015-05-14T21:49:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> For example, we have this in commit.c:\n>\n>     const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n>     {\n>             struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n>             if (sizep)\n>                     *sizep = v->size;\n>             return v->buffer;\n>     }\n>\n> But if we do not have the \"buffer\" data cached for that commit (via\n> an earlier call to set_commit_buffer()), we don't have to enlarge\n> the slab, as we are not adding anything to the slab system with this\n> call.\n\nThe change may look something like this.  I do not think it would\nmake a difference to the get_cached_commit_buffer() codepath (when\nwe use the commit->buffer, we pretty much know we use that for all\ncommits), though.\n\n commit-slab.h | 27 ++++++++++++++++++++++++---\n commit.c      |  7 ++++++-\n 2 files changed, 30 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex 375c9c7..3334ab2 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -15,7 +15,13 @@\n  * - int *indegree_at(struct indegree *, struct commit *);\n  *\n  *   This function locates the data associated with the given commit in\n- *   the indegree slab, and returns the pointer to it.\n+ *   the indegree slab, and returns the pointer to it.  The location to\n+ *   store the data is allocated as necessary.\n+ *\n+ * - int *indegree_peek(struct indegree *, struct commit *);\n+ *\n+ *   This function is similar to indegree_at(), but it will return NULL\n+ *   until a call to indegree_at() was made for the commit.\n  *\n  * - void init_indegree(struct indegree *);\n  *   void init_indegree_with_stride(struct indegree *, int);\n@@ -80,8 +86,9 @@ static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n-\t\t\t\t       const struct commit *c)\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,\t\\\n+\t\t\t\t\t\t  const struct commit *c, \\\n+\t\t\t\t\t\t  int add_if_missing)   \\\n {\t\t\t\t\t\t\t\t\t\\\n \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n@@ -90,6 +97,8 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n \t\t\t\t\t\t\t\t\t\\\n \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n \t\tint i;\t\t\t\t\t\t\t\\\n+\t\tif (!add_if_missing)\t\t\t\t\t\\\n+\t\t\treturn NULL;\t\t\t\t\t\\\n \t\ts->slab = xrealloc(s->slab,\t\t\t\t\\\n \t\t\t\t   (nth_slab + 1) * sizeof(*s->slab));\t\\\n \t\tstat_ ##slabname## realloc++;\t\t\t\t\\\n@@ -103,6 +112,18 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n \treturn &s->slab[nth_slab][nth_slot * s->stride];\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n+\t\t\t\t\t     const struct commit *c)\t\\\n+{\t\t\t\t\t\t\t\t\t\\\n+\treturn slabname##_at_peek(s, c, 1);\t\t\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _peek(struct slabname *s,\t\\\n+\t\t\t\t\t     const struct commit *c)\t\\\n+{\t\t\t\t\t\t\t\t\t\\\n+\treturn slabname##_at_peek(s, c, 0);\t\t\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n static int stat_ ##slabname## realloc\n \n /*\ndiff --git a/commit.c b/commit.c\nindex 65179f9..2d1901f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -244,7 +244,12 @@ void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)\n \n const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (!v) {\n+\t\tif (sizep)\n+\t\t\t*sizep = 0;\n+\t\treturn NULL;\n+\t}\n \tif (sizep)\n \t\t*sizep = v->size;\n \treturn v->buffer;\n"},{"id":"261302","messageId":"20150514215453.GA16897@peff.net","threadId":"39327","inReplyTo":"xmqqfv6yj0dl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T21:54:53Z","receivedAt":"2015-05-14T21:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2015 at 02:49:10PM -0700, Junio C Hamano wrote:\n\n> > But if we do not have the \"buffer\" data cached for that commit (via\n> > an earlier call to set_commit_buffer()), we don't have to enlarge\n> > the slab, as we are not adding anything to the slab system with this\n> > call.\n> \n> The change may look something like this.  I do not think it would\n> make a difference to the get_cached_commit_buffer() codepath (when\n> we use the commit->buffer, we pretty much know we use that for all\n> commits), though.\n\nI'm not sure that's true. Most of the _users_ of the commit buffer will\ntry to look in the cache, and if it's not there, do a one-off read.  But\nthey don't attach the result to the commit; they throw it away. The\nreasoning is that we don't have a cached buffer because we are going to\nlook at a lot of commits (i.e., save_commit_buffer is off).\n\nSo basically anytime save_commit_buffer is off (e.g., in rev-list) we\nare expanding the slab unnecessarily, even though literally nobody will\nwrite to it.\n\n> diff --git a/commit.c b/commit.c\n> index 65179f9..2d1901f 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -244,7 +244,12 @@ void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)\n>  \n>  const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n>  {\n> -\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n> +\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n> +\tif (!v) {\n> +\t\tif (sizep)\n> +\t\t\t*sizep = 0;\n> +\t\treturn NULL;\n> +\t}\n>  \tif (sizep)\n>  \t\t*sizep = v->size;\n>  \treturn v->buffer;\n\nI think you need matching changes in unused_commit_buffer and\nfree_commit_buffer. And detach_commit_buffer, too, I guess. Basically\neverywhere except set_commit_buffer would want to use the peek version.\n\n-Peff\n"},{"id":"261309","messageId":"xmqq7fsaiyoy.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"20150514215453.GA16897@peff.net","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T22:25:33Z","receivedAt":"2015-05-14T22:25:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> The change may look something like this.  I do not think it would\n>> make a difference to the get_cached_commit_buffer() codepath (when\n>> we use the commit->buffer, we pretty much know we use that for all\n>> commits), though.\n>\n> I'm not sure that's true. Most of the _users_ of the commit buffer will\n> try to look in the cache, and if it's not there, do a one-off read.  But\n> they don't attach the result to the commit; they throw it away. The\n> reasoning is that we don't have a cached buffer because we are going to\n> look at a lot of commits (i.e., save_commit_buffer is off).\n>\n> So basically anytime save_commit_buffer is off (e.g., in rev-list) we\n> are expanding the slab unnecessarily, even though literally nobody will\n> write to it.\n> ...\n> I think you need matching changes in unused_commit_buffer and\n> free_commit_buffer. And detach_commit_buffer, too, I guess. Basically\n> everywhere except set_commit_buffer would want to use the peek version.\n\nYeah, I didn't consider the \"we are not using buffer but are still\ncalling into the slab code\" case at all.\n\nThe other two slabs (indegree and author) in commit.c are used only\nwhen needed, and when they are used they are fully populated anyway,\nso I think this patch covers that file fully.\n\nThere are saved_parents slab used in revision.c and ref_bitmap slab\nused in shallow.c; they may also need similar fixups to save memory.\n\n commit-slab.h | 27 ++++++++++++++++++++++++---\n commit.c      | 28 ++++++++++++++++++++--------\n 2 files changed, 44 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex 375c9c7..3334ab2 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -15,7 +15,13 @@\n  * - int *indegree_at(struct indegree *, struct commit *);\n  *\n  *   This function locates the data associated with the given commit in\n- *   the indegree slab, and returns the pointer to it.\n+ *   the indegree slab, and returns the pointer to it.  The location to\n+ *   store the data is allocated as necessary.\n+ *\n+ * - int *indegree_peek(struct indegree *, struct commit *);\n+ *\n+ *   This function is similar to indegree_at(), but it will return NULL\n+ *   until a call to indegree_at() was made for the commit.\n  *\n  * - void init_indegree(struct indegree *);\n  *   void init_indegree_with_stride(struct indegree *, int);\n@@ -80,8 +86,9 @@ static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n-\t\t\t\t       const struct commit *c)\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,\t\\\n+\t\t\t\t\t\t  const struct commit *c, \\\n+\t\t\t\t\t\t  int add_if_missing)   \\\n {\t\t\t\t\t\t\t\t\t\\\n \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n@@ -90,6 +97,8 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n \t\t\t\t\t\t\t\t\t\\\n \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n \t\tint i;\t\t\t\t\t\t\t\\\n+\t\tif (!add_if_missing)\t\t\t\t\t\\\n+\t\t\treturn NULL;\t\t\t\t\t\\\n \t\ts->slab = xrealloc(s->slab,\t\t\t\t\\\n \t\t\t\t   (nth_slab + 1) * sizeof(*s->slab));\t\\\n \t\tstat_ ##slabname## realloc++;\t\t\t\t\\\n@@ -103,6 +112,18 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n \treturn &s->slab[nth_slab][nth_slot * s->stride];\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n+\t\t\t\t\t     const struct commit *c)\t\\\n+{\t\t\t\t\t\t\t\t\t\\\n+\treturn slabname##_at_peek(s, c, 1);\t\t\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _peek(struct slabname *s,\t\\\n+\t\t\t\t\t     const struct commit *c)\t\\\n+{\t\t\t\t\t\t\t\t\t\\\n+\treturn slabname##_at_peek(s, c, 0);\t\t\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n static int stat_ ##slabname## realloc\n \n /*\ndiff --git a/commit.c b/commit.c\nindex 65179f9..4f2ce9f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -244,7 +244,12 @@ void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)\n \n const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (!v) {\n+\t\tif (sizep)\n+\t\t\t*sizep = 0;\n+\t\treturn NULL;\n+\t}\n \tif (sizep)\n \t\t*sizep = v->size;\n \treturn v->buffer;\n@@ -271,24 +276,31 @@ const void *get_commit_buffer(const struct commit *commit, unsigned long *sizep)\n \n void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n-\tif (v->buffer != buffer)\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (v && v->buffer != buffer)\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct commit *commit)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n-\tfree(v->buffer);\n-\tv->buffer = NULL;\n-\tv->size = 0;\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (v) {\n+\t\tfree(v->buffer);\n+\t\tv->buffer = NULL;\n+\t\tv->size = 0;\n+\t}\n }\n \n const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n \tvoid *ret;\n \n+\tif (!v) {\n+\t\tif (sizep)\n+\t\t\t*sizep = 0;\n+\t\treturn NULL;\n+\t}\n \tret = v->buffer;\n \tif (sizep)\n \t\t*sizep = v->size;\n"},{"id":"261311","messageId":"20150514223313.GA21149@peff.net","threadId":"39327","inReplyTo":"xmqq7fsaiyoy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T22:33:13Z","receivedAt":"2015-05-14T22:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2015 at 03:25:33PM -0700, Junio C Hamano wrote:\n\n> @@ -90,6 +97,8 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n>  \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n>  \t\tint i;\t\t\t\t\t\t\t\\\n> +\t\tif (!add_if_missing)\t\t\t\t\t\\\n> +\t\t\treturn NULL;\t\t\t\t\t\\\n>  \t\ts->slab = xrealloc(s->slab,\t\t\t\t\\\n>  \t\t\t\t   (nth_slab + 1) * sizeof(*s->slab));\t\\\n>  \t\tstat_ ##slabname## realloc++;\t\t\t\t\\\n\nThis skips extending the list of slabs if we would go past the nth slab.\nBut we don't fill in each slab in the first place. I.e., we may have 10\nslabs, but only s->slab[10] is non-NULL.\n\nA few lines below this, we xcalloc() it if necessary. I think that needs\nto respect add_if_missing, as well.\n\n>  void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n>  {\n> -\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n> -\tif (v->buffer != buffer)\n> +\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n> +\tif (v && v->buffer != buffer)\n>  \t\tfree((void *)buffer);\n>  }\n\nI think you want:\n\n  if (!v || v->buffer != buffer)\n\nhere. IOW, we free it only if it is not our cached buffer, and it cannot\nbe if we do not have a cached buffer. It may be easier to read by\nflipping the logic:\n\n  if (v && v->buffer == buffer)\n\treturn; /* it is saved in the cache */\n  free((void *)buffer);\n\nOr some variation on that.\n\n-Peff\n"},{"id":"261952","messageId":"xmqqmw0we2b7.fsf@gitster.dls.corp.google.com","threadId":"39327","inReplyTo":"20150514223313.GA21149@peff.net","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-22T21:21:16Z","receivedAt":"2015-05-22T21:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, May 14, 2015 at 03:25:33PM -0700, Junio C Hamano wrote:\n>\n>> @@ -90,6 +97,8 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n>>  \t\t\t\t\t\t\t\t\t\\\n>>  \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n>>  \t\tint i;\t\t\t\t\t\t\t\\\n>> +\t\tif (!add_if_missing)\t\t\t\t\t\\\n>> +\t\t\treturn NULL;\t\t\t\t\t\\\n>>  \t\ts->slab = xrealloc(s->slab,\t\t\t\t\\\n>>  \t\t\t\t   (nth_slab + 1) * sizeof(*s->slab));\t\\\n>>  \t\tstat_ ##slabname## realloc++;\t\t\t\t\\\n>\n> This skips extending the list of slabs if we would go past the nth slab.\n> But we don't fill in each slab in the first place. I.e., we may have 10\n> slabs, but only s->slab[10] is non-NULL.\n>\n> A few lines below this, we xcalloc() it if necessary. I think that needs\n> to respect add_if_missing, as well.\n\nYup, thanks.\n\n>\n>>  void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n>>  {\n>> -\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n>> -\tif (v->buffer != buffer)\n>> +\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n>> +\tif (v && v->buffer != buffer)\n>>  \t\tfree((void *)buffer);\n>>  }\n>\n> I think you want:\n>\n>   if (!v || v->buffer != buffer)\n>\n> here. IOW, we free it only if it is not our cached buffer, and it cannot\n> be if we do not have a cached buffer. It may be easier to read by\n> flipping the logic:\n>\n>   if (v && v->buffer == buffer)\n> \treturn; /* it is saved in the cache */\n>   free((void *)buffer);\n>\n> Or some variation on that.\n\nI ended up doing it as a variant of the latter, \"free unless we have\nv->buffer pointing at it\".\n\nSorry for a long delay.\n\n-- >8 --\nSubject: [PATCH] commit-slab: introduce slabname##_peek() function\n\nThere is no API to ask \"Does this commit have associated data in\nslab?\".  If an application wants to (1) parse just a few commits at\nthe beginning of a process, (2) store data for only these commits,\nand then (3) start processing many commits, taking into account the\ndata stored (for a few of them) in the slab, the application would\nuse slabname##_at() to allocate a space to store data in (2), but\nthere is no API other than slabname##_at() to use in step (3).  This\nallocates and wasts new space for these commits the caller is only\ninterested in checking if they have data stored in step (2).\n\nIntroduce slabname##_peek(), which is similar to slabname##_at() but\nreturns NULL when there is no data already associated to it in such\na use case.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n commit-slab.h | 34 +++++++++++++++++++++++++++++-----\n commit.c      | 28 ++++++++++++++++++++--------\n 2 files changed, 49 insertions(+), 13 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex 375c9c7..9d12ce2 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -15,7 +15,13 @@\n  * - int *indegree_at(struct indegree *, struct commit *);\n  *\n  *   This function locates the data associated with the given commit in\n- *   the indegree slab, and returns the pointer to it.\n+ *   the indegree slab, and returns the pointer to it.  The location to\n+ *   store the data is allocated as necessary.\n+ *\n+ * - int *indegree_peek(struct indegree *, struct commit *);\n+ *\n+ *   This function is similar to indegree_at(), but it will return NULL\n+ *   until a call to indegree_at() was made for the commit.\n  *\n  * - void init_indegree(struct indegree *);\n  *   void init_indegree_with_stride(struct indegree *, int);\n@@ -80,8 +86,9 @@ static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n-\t\t\t\t       const struct commit *c)\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,\t\\\n+\t\t\t\t\t\t  const struct commit *c, \\\n+\t\t\t\t\t\t  int add_if_missing)   \\\n {\t\t\t\t\t\t\t\t\t\\\n \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n@@ -90,6 +97,8 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n \t\t\t\t\t\t\t\t\t\\\n \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n \t\tint i;\t\t\t\t\t\t\t\\\n+\t\tif (!add_if_missing)\t\t\t\t\t\\\n+\t\t\treturn NULL;\t\t\t\t\t\\\n \t\ts->slab = xrealloc(s->slab,\t\t\t\t\\\n \t\t\t\t   (nth_slab + 1) * sizeof(*s->slab));\t\\\n \t\tstat_ ##slabname## realloc++;\t\t\t\t\\\n@@ -97,10 +106,25 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n \t\t\ts->slab[i] = NULL;\t\t\t\t\\\n \t\ts->slab_count = nth_slab + 1;\t\t\t\t\\\n \t}\t\t\t\t\t\t\t\t\\\n-\tif (!s->slab[nth_slab])\t\t\t\t\t\t\\\n+\tif (!s->slab[nth_slab]) {\t\t\t\t\t\\\n+\t\tif (!add_if_missing)\t\t\t\t\t\\\n+\t\t\treturn NULL;\t\t\t\t\t\\\n \t\ts->slab[nth_slab] = xcalloc(s->slab_size,\t\t\\\n \t\t\t\t\t    sizeof(**s->slab) * s->stride);\t\t\\\n-\treturn &s->slab[nth_slab][nth_slot * s->stride];\t\t\t\t\\\n+\t}\t\t\t\t\t\t\t\t\\\n+\treturn &s->slab[nth_slab][nth_slot * s->stride];\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,\t\\\n+\t\t\t\t\t     const struct commit *c)\t\\\n+{\t\t\t\t\t\t\t\t\t\\\n+\treturn slabname##_at_peek(s, c, 1);\t\t\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n+static MAYBE_UNUSED elemtype *slabname## _peek(struct slabname *s,\t\\\n+\t\t\t\t\t     const struct commit *c)\t\\\n+{\t\t\t\t\t\t\t\t\t\\\n+\treturn slabname##_at_peek(s, c, 0);\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n static int stat_ ##slabname## realloc\ndiff --git a/commit.c b/commit.c\nindex 65179f9..5fb9496 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -244,7 +244,12 @@ void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)\n \n const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (!v) {\n+\t\tif (sizep)\n+\t\t\t*sizep = 0;\n+\t\treturn NULL;\n+\t}\n \tif (sizep)\n \t\t*sizep = v->size;\n \treturn v->buffer;\n@@ -271,24 +276,31 @@ const void *get_commit_buffer(const struct commit *commit, unsigned long *sizep)\n \n void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n-\tif (v->buffer != buffer)\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (!(v && v->buffer == buffer))\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct commit *commit)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n-\tfree(v->buffer);\n-\tv->buffer = NULL;\n-\tv->size = 0;\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n+\tif (v) {\n+\t\tfree(v->buffer);\n+\t\tv->buffer = NULL;\n+\t\tv->size = 0;\n+\t}\n }\n \n const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep)\n {\n-\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);\n \tvoid *ret;\n \n+\tif (!v) {\n+\t\tif (sizep)\n+\t\t\t*sizep = 0;\n+\t\treturn NULL;\n+\t}\n \tret = v->buffer;\n \tif (sizep)\n \t\t*sizep = v->size;\n-- \n2.4.1-449-g1f6c7df\n"},{"id":"261954","messageId":"20150522213853.GA29804@peff.net","threadId":"39327","inReplyTo":"xmqqmw0we2b7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] log: do not shorten decoration names too early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T21:38:53Z","receivedAt":"2015-05-22T21:38:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2015 at 02:21:16PM -0700, Junio C Hamano wrote:\n\n> I ended up doing it as a variant of the latter, \"free unless we have\n> v->buffer pointing at it\".\n\nThanks, this version looks good to me minus one micro-nit below.\n\n> Sorry for a long delay.\n\nNo problem. I'm sometimes amazed you find time to write any patches at\nall. :)\n\n> -- >8 --\n> Subject: [PATCH] commit-slab: introduce slabname##_peek() function\n> \n> There is no API to ask \"Does this commit have associated data in\n> slab?\".  If an application wants to (1) parse just a few commits at\n> the beginning of a process, (2) store data for only these commits,\n> and then (3) start processing many commits, taking into account the\n> data stored (for a few of them) in the slab, the application would\n> use slabname##_at() to allocate a space to store data in (2), but\n> there is no API other than slabname##_at() to use in step (3).  This\n> allocates and wasts new space for these commits the caller is only\n> interested in checking if they have data stored in step (2).\n\ns/wasts/wastes/\n\n-Peff\n"}]}