{"thread":{"id":"37410","subject":"[PATCH] bisect: save heap memory. allocate only the required amount","startedAt":"2014-08-24T14:17:24Z","lastAt":"2014-08-27T00:30:08Z","messageCount":32,"participants":["Arjun Sreedharan","Stefan Beller","Ramsay Jones","Junio C Hamano","Jeff King","Christian Couder","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"248202","messageId":"1408889844-5407-1-git-send-email-arjun024@gmail.com","threadId":"37410","inReplyTo":null,"subject":"[PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Arjun Sreedharan","fromEmail":"arjun024@gmail.com","sentAt":"2014-08-24T14:17:24Z","receivedAt":"2014-08-24T14:17:24Z","isPatch":true,"sender":{"key":"arjun024@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2861611?v=4"},"body":"Find and allocate the required amount instead of\nallocating extra 100 bytes\n\nSigned-off-by: Arjun Sreedharan \n---\n bisect.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex d6e851d..c96aab0 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t}\n \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n \tfor (p = list, i = 0; i < cnt; i++) {\n-\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n+\t\tchar name[100];\n+\t\tsprintf(name, \"dist=%d\", array[i].distance);\n+\t\tint name_len = strlen(name);\n+\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n \t\tstruct object *obj = &(array[i].commit->object);\n \n-\t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n+\t\tmemcpy(r->name, name, name_len + 1);\n \t\tr->next = add_decoration(&name_decoration, obj, r);\n \t\tp->item = array[i].commit;\n \t\tp = p->next;\n-- \n1.7.11.7\n"},{"id":"248203","messageId":"53FA0054.5060808@gmail.com","threadId":"37410","inReplyTo":"1408889844-5407-1-git-send-email-arjun024@gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-24T15:10:12Z","receivedAt":"2014-08-24T15:10:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 24.08.2014 16:17, Arjun Sreedharan wrote:\n> Find and allocate the required amount instead of\n> allocating extra 100 bytes\n> \n> Signed-off-by: Arjun Sreedharan \n> ---\n> bisect.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/bisect.c b/bisect.c\n> index d6e851d..c96aab0 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n> \t}\n> \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n> \tfor (p = list, i = 0; i < cnt; i++) {\n> -\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n> +\t\tchar name[100];\n\nWould it make sense to convert the 'name' into a git strbuf?\nPlease have a look at Documentation/technical/api-strbuf.txt\n\n> +\t\tsprintf(name, \"dist=%d\", array[i].distance);\n> +\t\tint name_len = strlen(name);\n> +\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n> \t\tstruct object *obj = &(array[i].commit->object);\n> \n> -\t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n> +\t\tmemcpy(r->name, name, name_len + 1);\n> \t\tr->next = add_decoration(&name_decoration, obj, r);\n> \t\tp->item = array[i].commit;\n> \t\tp = p->next;\n> \n"},{"id":"248204","messageId":"53FA057C.3070206@ramsay1.demon.co.uk","threadId":"37410","inReplyTo":"1408889844-5407-1-git-send-email-arjun024@gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-24T15:32:12Z","receivedAt":"2014-08-24T15:32:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 24/08/14 15:17, Arjun Sreedharan wrote:\n> Find and allocate the required amount instead of\n> allocating extra 100 bytes\n> \n> Signed-off-by: Arjun Sreedharan \n> ---\n> bisect.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/bisect.c b/bisect.c\n> index d6e851d..c96aab0 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n> \t}\n> \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n> \tfor (p = list, i = 0; i < cnt; i++) {\n> -\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n> +\t\tchar name[100];\n> +\t\tsprintf(name, \"dist=%d\", array[i].distance);\n> +\t\tint name_len = strlen(name);\n> +\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n> \t\tstruct object *obj = &(array[i].commit->object);\n\ndeclaration(s) after statement.\n\n> \n> -\t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n> +\t\tmemcpy(r->name, name, name_len + 1);\n> \t\tr->next = add_decoration(&name_decoration, obj, r);\n> \t\tp->item = array[i].commit;\n> \t\tp = p->next;\n> \n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"248217","messageId":"1408917358-2372-1-git-send-email-arjun024@gmail.com","threadId":"37410","inReplyTo":"53FA057C.3070206@ramsay1.demon.co.uk","subject":"[PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Arjun Sreedharan","fromEmail":"arjun024@gmail.com","sentAt":"2014-08-24T21:55:58Z","receivedAt":"2014-08-24T21:55:58Z","isPatch":true,"sender":{"key":"arjun024@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2861611?v=4"},"body":"find and allocate the required amount instead of\nallocating extra 100 bytes\n\nSigned-off-by: Arjun Sreedharan \n---\n bisect.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex d6e851d..a52631e 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -215,12 +215,16 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t}\n \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n \tfor (p = list, i = 0; i < cnt; i++) {\n-\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n+\t\tstruct strbuf name = STRBUF_INIT;\t\n+\t\tstruct name_decoration *r;\n \t\tstruct object *obj = &(array[i].commit->object);\n \n-\t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n+\t\tstrbuf_addf(&name, \"dist=%d\", array[i].distance);\n+\t\tr = xmalloc(sizeof(*r) + name.len);\n+\t\tmemcpy(r->name, name.buf, name.len + 1);\n \t\tr->next = add_decoration(&name_decoration, obj, r);\n \t\tp->item = array[i].commit;\n+\t\tstrbuf_release(&name);\n \t\tp = p->next;\n \t}\n \tif (p)\n-- \n1.7.11.7\n"},{"id":"248218","messageId":"CAPc5daWheSH8E-PycSUq2Coqp19t_+_6TuBEOKhK4QwsEtzkkA@mail.gmail.com","threadId":"37410","inReplyTo":"53FA0054.5060808@gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-24T23:39:37Z","receivedAt":"2014-08-24T23:39:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sun, Aug 24, 2014 at 8:10 AM, Stefan Beller wrote:\n>> for (p = list, i = 0; i < cnt; i++) {\n>> - struct name_decoration *r = xmalloc(sizeof(*r) + 100);\n>> + char name[100];\n>\n> Would it make sense to convert the 'name' into a git strbuf?\n> Please have a look at Documentation/technical/api-strbuf.txt\n\nWhy not go one step further and format it twice, once only\nto measure the necessary size to allocate, allocate and\nthen format into it for real? Then you do not need to print\ninto a temporary strbuf, copy the result and free the strbuf,\nno?\n\nBUT.\n\nThe string will always be \"dist=\" followed by decimal representation of\na count that fits in \"int\" anyway, so I actually think use of strbuf is way\noverkill (and formatting it twice also is); the patch as posted should be\njust fine.\n\nOr am I missing some uses that require larger/unbounded buffer outside\nthe context of the patch?\n"},{"id":"248225","messageId":"20140825130732.GD17288@peff.net","threadId":"37410","inReplyTo":"CAPc5daWheSH8E-PycSUq2Coqp19t_+_6TuBEOKhK4QwsEtzkkA@mail.gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T13:07:32Z","receivedAt":"2014-08-25T13:07:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 24, 2014 at 04:39:37PM -0700, Junio C Hamano wrote:\n\n> On Sun, Aug 24, 2014 at 8:10 AM, Stefan Beller wrote:\n> >> for (p = list, i = 0; i < cnt; i++) {\n> >> - struct name_decoration *r = xmalloc(sizeof(*r) + 100);\n> >> + char name[100];\n> >\n> > Would it make sense to convert the 'name' into a git strbuf?\n> > Please have a look at Documentation/technical/api-strbuf.txt\n> \n> Why not go one step further and format it twice, once only\n> to measure the necessary size to allocate, allocate and\n> then format into it for real? Then you do not need to print\n> into a temporary strbuf, copy the result and free the strbuf,\n> no?\n> \n> BUT.\n> \n> The string will always be \"dist=\" followed by decimal representation of\n> a count that fits in \"int\" anyway, so I actually think use of strbuf is way\n> overkill (and formatting it twice also is); the patch as posted should be\n> just fine.\n\nI think you are right, and the patch is the right direction (assuming we\nwant to do this; I question whether there are enough elements in the\nlist for us to care about the size, and if there are, we are probably\nbetter off storing the int and formatting the strings on the fly).\n\nI wonder if there is a way we could get rid of the magic \"100\" here,\nthough. Its meaning is \"enough to hold 'dist=' and any integer\". But you\nhave to read carefully to see that this call to sprintf is not a buffer\noverflow. A strbuf is one way to get rid of it, though it is awkward\nbecause we then have to copy the result into a flex-array structure.\n\nIt would be nice if there was some way to abstract the idea of\nformatting a buffer directly into a flex-array. That would involve the\ndouble-format you mention, but we could use it in lots of places to make\nthe code nicer. Maybe like:\n\n void *fmt_flex_array(size_t base, const char *fmt, ...)\n {\n va_list ap;\n\t size_t flex;\n\t unsigned char *ret;\n\n\t va_start(ap, fmt);\n\t flex = vsnprintf(NULL, 0, fmt, ap);\n\t va_end(ap);\n\n\t ret = xmalloc(base + flex + 1);\n\t va_start(ap, fmt);\n\t /* Eek, see below */\n\t vsnprintf(ret + flex, flex + 1, fmt, ap);\n\n\t return ret;\n }\n\nand you'd call it like:\n\n struct name_decoration *r = fmt_flex_array(sizeof(*r), \"dist=%d\", x);\n\nExcept that I don't think we are guaranteed that offsetof(mystruct,\nflex_member) is equal to sizeof(mystruct). If FLEX_ARRAY is 0, it should\nbe, but some platforms use FLEX_ARRAY=1. So you'd have to pass in the\noffset like:\n\n struct name_decoration *r = fmt_flex_array(sizeof(*r),\n offsetof(*r, name),\n\t\t\t\t\t \"dist=%d\", x);\n\nwhich is a little less nice. You could make it nicer with a macro, but\nwe don't assume variadic macros. \n\n-Peff\n"},{"id":"248226","messageId":"20140825133550.GE17288@peff.net","threadId":"37410","inReplyTo":"1408889844-5407-1-git-send-email-arjun024@gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T13:35:50Z","receivedAt":"2014-08-25T13:35:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 24, 2014 at 07:47:24PM +0530, Arjun Sreedharan wrote:\n\n> diff --git a/bisect.c b/bisect.c\n> index d6e851d..c96aab0 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n> \t}\n> \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n> \tfor (p = list, i = 0; i < cnt; i++) {\n> -\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n> +\t\tchar name[100];\n> +\t\tsprintf(name, \"dist=%d\", array[i].distance);\n> +\t\tint name_len = strlen(name);\n> +\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n\nThis allocation should be name_len + 1 for the NUL-terminator, no?\n\nIt looks like add_name_decoration in log-tree already handles half of\nwhat you are adding here. Can we just make that available globally (it\nis manipulating the already-global \"struct decoration name_decoration\")?\n\nI also notice that we do not set r->type at all, meaning the decoration\nlookup code in log-tree will access uninitialized memory (worse, it will\nuse it as a pointer offset into the color list; I got a segfault when I\ntried to run \"git rev-list --bisect-all v1.8.0..v1.9.0\").\n\nI think we need this:\n\ndiff --git a/bisect.c b/bisect.c\nindex d6e851d..e2a7682 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -219,6 +219,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t\tstruct object *obj = &(array[i].commit->object);\n \n \t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n+\t\tr->type = 0;\n \t\tr->next = add_decoration(&name_decoration, obj, r);\n \t\tp->item = array[i].commit;\n \t\tp = p->next;\n\nat a minimum.\n\nIt looks like this was a regression caused by eb3005e (commit.h: add\n'type' to struct name_decoration, 2010-06-19). Which makes me wonder if\nanybody actually _uses_ --bisect-all (which AFAICT is the only way to\ntrigger the problem), but since it's public, I guess we should keep it.\n\nI think the sane thing here is to stop advertising name_decoration as a\nglobal, and make all callers use add_name_decoration. That makes it\neasier for callers like this one, and would have caught the regression\ncaused be eb3005e (the compiler would have noticed that we were not\npassing a type parameter to the function).\n\n-Peff\n"},{"id":"248228","messageId":"CAP8UFD2FAfg5GenJXOkOsjU9vmCO3R3Difp6-mrP_cp4zXQENg@mail.gmail.com","threadId":"37410","inReplyTo":"20140825133550.GE17288@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-08-25T14:06:52Z","receivedAt":"2014-08-25T14:06:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Aug 25, 2014 at 3:35 PM, Jeff King wrote:\n> On Sun, Aug 24, 2014 at 07:47:24PM +0530, Arjun Sreedharan wrote:\n>\n>> diff --git a/bisect.c b/bisect.c\n>> index d6e851d..c96aab0 100644\n>> --- a/bisect.c\n>> +++ b/bisect.c\n>> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n>> }\n>> qsort(array, cnt, sizeof(*array), compare_commit_dist);\n>> for (p = list, i = 0; i < cnt; i++) {\n>> - struct name_decoration *r = xmalloc(sizeof(*r) + 100);\n>> + char name[100];\n>> + sprintf(name, \"dist=%d\", array[i].distance);\n>> + int name_len = strlen(name);\n>> + struct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n>\n> This allocation should be name_len + 1 for the NUL-terminator, no?\n\nI wondered about that too, but as struct name_decoration is defined like this:\n\nstruct name_decoration {\n struct name_decoration *next;\n int type;\n char name[1];\n};\n\nthe .name field of this struct already has one char, so the allocation\nabove should be ok.\n\n> It looks like add_name_decoration in log-tree already handles half of\n> what you are adding here. Can we just make that available globally (it\n> is manipulating the already-global \"struct decoration name_decoration\")?\n\nYeah, it looks like it should be better.\n\nNote that add_name_decoration() does:\n\nint nlen = strlen(name);\nstruct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen);\n\nso it also relies on the fact that .name contains one char.\n\n> I also notice that we do not set r->type at all, meaning the decoration\n> lookup code in log-tree will access uninitialized memory (worse, it will\n> use it as a pointer offset into the color list; I got a segfault when I\n> tried to run \"git rev-list --bisect-all v1.8.0..v1.9.0\").\n>\n> I think we need this:\n>\n> diff --git a/bisect.c b/bisect.c\n> index d6e851d..e2a7682 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -219,6 +219,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n> struct object *obj = &(array[i].commit->object);\n>\n> sprintf(r->name, \"dist=%d\", array[i].distance);\n> + r->type = 0;\n> r->next = add_decoration(&name_decoration, obj, r);\n> p->item = array[i].commit;\n> p = p->next;\n>\n> at a minimum.\n\nYeah if we don't use add_name_decoration() we would need that.\nThanks for noticing.\n\n> It looks like this was a regression caused by eb3005e (commit.h: add\n> 'type' to struct name_decoration, 2010-06-19). Which makes me wonder if\n> anybody actually _uses_ --bisect-all (which AFAICT is the only way to\n> trigger the problem), but since it's public, I guess we should keep it.\n\nYeah, we should probably keep it.\n\n> I think the sane thing here is to stop advertising name_decoration as a\n> global, and make all callers use add_name_decoration. That makes it\n> easier for callers like this one, and would have caught the regression\n> caused be eb3005e (the compiler would have noticed that we were not\n> passing a type parameter to the function).\n\nI agree.\n\nThanks,\nChristian.\n"},{"id":"248231","messageId":"20140825150028.GA28176@peff.net","threadId":"37410","inReplyTo":"CAP8UFD2FAfg5GenJXOkOsjU9vmCO3R3Difp6-mrP_cp4zXQENg@mail.gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T15:00:28Z","receivedAt":"2014-08-25T15:00:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 04:06:52PM +0200, Christian Couder wrote:\n\n> > This allocation should be name_len + 1 for the NUL-terminator, no?\n> \n> I wondered about that too, but as struct name_decoration is defined like this:\n> \n> struct name_decoration {\n> struct name_decoration *next;\n> int type;\n> char name[1];\n> };\n> \n> the .name field of this struct already has one char, so the allocation\n> above should be ok.\n\nYeah, you're right. I would argue it should just be FLEX_ARRAY for\nconsistency with other spots, though (in which case add_name_decoration\nneeds to be updated with a +1).\n\nRunning \"git grep '^\tchar [^ ]*\\[[01]]' -- '*.[ch]'\" shows that this\nis one of only two spots that don't use FLEX_ARRAY (and the other has a\ncomment explaining why not).\n\n-Peff\n"},{"id":"248250","messageId":"xmqq8umcmnmo.fsf@gitster.dls.corp.google.com","threadId":"37410","inReplyTo":"20140825150028.GA28176@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T18:26:39Z","receivedAt":"2014-08-25T18:26:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 25, 2014 at 04:06:52PM +0200, Christian Couder wrote:\n>\n>> > This allocation should be name_len + 1 for the NUL-terminator, no?\n>> \n>> I wondered about that too, but as struct name_decoration is defined like this:\n>> \n>> struct name_decoration {\n>> struct name_decoration *next;\n>> int type;\n>> char name[1];\n>> };\n>> \n>> the .name field of this struct already has one char, so the allocation\n>> above should be ok.\n>\n> Yeah, you're right. I would argue it should just be FLEX_ARRAY for\n> consistency with other spots, though (in which case add_name_decoration\n> needs to be updated with a +1).\n>\n> Running \"git grep '^\tchar [^ ]*\\[[01]]' -- '*.[ch]'\" shows that this\n> is one of only two spots that don't use FLEX_ARRAY (and the other has a\n> comment explaining why not).\n\nGood digging, and I agree that it should use the FLEX_ARRAY for\nconsistency.\n"},{"id":"248259","messageId":"20140825193519.GH30953@peff.net","threadId":"37410","inReplyTo":"xmqq8umcmnmo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T19:35:20Z","receivedAt":"2014-08-25T19:35:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 11:26:39AM -0700, Junio C Hamano wrote:\n\n> Good digging, and I agree that it should use the FLEX_ARRAY for\n> consistency.\n\nI can produce a patch, but I did not want to steal Arjun's thunder.\n\nArjun, did my proposal make sense? Do you want to try implementing that?\n\n-Peff\n"},{"id":"248266","messageId":"CAJFMrCGK8uBYYGVHx2GgDEY+6OCS3pT8O8BE7TKpV0v1A0-kGQ@mail.gmail.com","threadId":"37410","inReplyTo":"20140825193519.GH30953@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Arjun Sreedharan","fromEmail":"arjun024@gmail.com","sentAt":"2014-08-25T20:27:36Z","receivedAt":"2014-08-25T20:27:36Z","isPatch":true,"sender":{"key":"arjun024@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2861611?v=4"},"body":"On 26 August 2014 01:05, Jeff King wrote:\n> On Mon, Aug 25, 2014 at 11:26:39AM -0700, Junio C Hamano wrote:\n>\n>> Good digging, and I agree that it should use the FLEX_ARRAY for\n>> consistency.\n>\n> I can produce a patch, but I did not want to steal Arjun's thunder.\n\nPlease feel free to do so.\n\n>\n> Arjun, did my proposal make sense? Do you want to try implementing that?\n>\n> -Peff\n"},{"id":"248271","messageId":"xmqqha10l1g2.fsf@gitster.dls.corp.google.com","threadId":"37410","inReplyTo":"20140825193519.GH30953@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T21:11:09Z","receivedAt":"2014-08-25T21:11:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 25, 2014 at 11:26:39AM -0700, Junio C Hamano wrote:\n>\n>> Good digging, and I agree that it should use the FLEX_ARRAY for\n>> consistency.\n>\n> I can produce a patch, but I did not want to steal Arjun's thunder.\n\nHmph, would it have to overlap? I think we can queue Arjun's patch\nwith +1 fix and FLEX_ARRAY thing separately, and they can go in in\nany order, no?\n\n> Arjun, did my proposal make sense? Do you want to try implementing that?\n"},{"id":"248272","messageId":"xmqqd2bol1ad.fsf@gitster.dls.corp.google.com","threadId":"37410","inReplyTo":"1408889844-5407-1-git-send-email-arjun024@gmail.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T21:14:34Z","receivedAt":"2014-08-25T21:14:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arjun Sreedharan writes:\n\n> Find and allocate the required amount instead of allocating extra\n> 100 bytes\n>\n> Signed-off-by: Arjun Sreedharan \n> ---\n\nInteresting. How much memory do we typically waste (in other words,\nhow big did \"cnt\" grew in your repository where you noticed this)?\n\n> bisect.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index d6e851d..c96aab0 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n> \t}\n> \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n> \tfor (p = list, i = 0; i < cnt; i++) {\n> -\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n> +\t\tchar name[100];\n> +\t\tsprintf(name, \"dist=%d\", array[i].distance);\n> +\t\tint name_len = strlen(name);\n\nDecl-after-stmt.\n\nYou do not have to run a separate strlen() for this. The return\nvalue from sprintf() should tell you how much you need to allocate,\nI think.\n\n> +\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n> \t\tstruct object *obj = &(array[i].commit->object);\n> \n> -\t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n> +\t\tmemcpy(r->name, name, name_len + 1);\n> \t\tr->next = add_decoration(&name_decoration, obj, r);\n> \t\tp->item = array[i].commit;\n> \t\tp = p->next;\n"},{"id":"248274","messageId":"xmqq8umcl0al.fsf@gitster.dls.corp.google.com","threadId":"37410","inReplyTo":"20140825130732.GD17288@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T21:36:02Z","receivedAt":"2014-08-25T21:36:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> The string will always be \"dist=\" followed by decimal representation of\n>> a count that fits in \"int\" anyway, so I actually think use of strbuf is way\n>> overkill (and formatting it twice also is); the patch as posted should be\n>> just fine.\n>\n> I think you are right, and the patch is the right direction (assuming we\n> want to do this; I question whether there are enough elements in the\n> list for us to care about the size, and if there are, we are probably\n> better off storing the int and formatting the strings on the fly).\n\n;-)\n\n> It would be nice if there was some way to abstract the idea of\n> formatting a buffer directly into a flex-array. That would involve the\n> double-format you mention, but we could use it in lots of places to make\n> the code nicer....\n> ...\n> struct name_decoration *r = fmt_flex_array(sizeof(*r),\n> offsetof(*r, name),\n> \t\t\t\t\t \"dist=%d\", x);\n>\n> which is a little less nice. You could make it nicer with a macro, but\n> we don't assume variadic macros. \n\nAt first I thought \"Yuck. A helper only to format into the flex\nmember that holds a string?\", and I tried to change my mind, but I\ncouldn't quite convince myself. At least not yet.\n\nAmong the flex arrays we use, some are arrays of bools, some others\nare arrays of object names, and there are many arrays of even more\nesoteric types. Only a small number of them are \"We want a struct\nwith a constant string, and we do not want to do two allocations and\nto pay an extra dereference cost by using 'const char *'\".\n\nFor them, by the time we allocate a struct, by definition we should\nhave sufficient information to compute how large to make that\nstructure and a printf-format plus its args would be the preferred\nform of that \"sufficient information\", I would think.\n\nThe name \"fmt_flex_array()\", which stresses too much on the\n\"formatting\" side without implying that it is the way to allocate\nthe thing, may be horrible, and I agree with you that without\nvariadic macros the end result may not read very well. Unless we\nhave great many number of places we can use the helper to make the\ncode to create these objects look nicer, I am afraid that the\npros-and-cons may not be very favourable.\n\nThanks for an interesting tangent.\n"},{"id":"248282","messageId":"CAJFMrCFaBGHQqGwpxSChaigOiaNL1OcBKGHjgYoJoDN-d0iE0w@mail.gmail.com","threadId":"37410","inReplyTo":"xmqqd2bol1ad.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Arjun Sreedharan","fromEmail":"arjun024@gmail.com","sentAt":"2014-08-26T07:30:45Z","receivedAt":"2014-08-26T07:30:45Z","isPatch":true,"sender":{"key":"arjun024@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2861611?v=4"},"body":"On 26 August 2014 02:44, Junio C Hamano wrote:\n> Arjun Sreedharan writes:\n>\n>> Find and allocate the required amount instead of allocating extra\n>> 100 bytes\n>>\n>> Signed-off-by: Arjun Sreedharan \n>> ---\n>\n> Interesting. How much memory do we typically waste (in other words,\n> how big did \"cnt\" grew in your repository where you noticed this)?\n>\n\nI did not try to make an observation of that. I was thinking we're\nanyway better off\nnot using a magic number to allot memory on the heap for each item in\nthe commit list.\n\n>> bisect.c | 7 +++++--\n>> 1 file changed, 5 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/bisect.c b/bisect.c\n>> index d6e851d..c96aab0 100644\n>> --- a/bisect.c\n>> +++ b/bisect.c\n>> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n>> }\n>> qsort(array, cnt, sizeof(*array), compare_commit_dist);\n>> for (p = list, i = 0; i < cnt; i++) {\n>> - struct name_decoration *r = xmalloc(sizeof(*r) + 100);\n>> + char name[100];\n>> + sprintf(name, \"dist=%d\", array[i].distance);\n>> + int name_len = strlen(name);\n>\n> Decl-after-stmt.\n>\n> You do not have to run a separate strlen() for this. The return\n> value from sprintf() should tell you how much you need to allocate,\n> I think.\n>\n\nYes. yes.\n\n>> + struct name_decoration *r = xmalloc(sizeof(*r) + name_len);\n>> struct object *obj = &(array[i].commit->object);\n>>\n>> - sprintf(r->name, \"dist=%d\", array[i].distance);\n>> + memcpy(r->name, name, name_len + 1);\n>> r->next = add_decoration(&name_decoration, obj, r);\n>> p->item = array[i].commit;\n>> p = p->next;\n"},{"id":"248285","messageId":"20140826102051.GA4885@peff.net","threadId":"37410","inReplyTo":"xmqqha10l1g2.fsf@gitster.dls.corp.google.com","subject":"[PATCH 0/3] name_decoration cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T10:20:51Z","receivedAt":"2014-08-26T10:20:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 02:11:09PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Mon, Aug 25, 2014 at 11:26:39AM -0700, Junio C Hamano wrote:\n> >\n> >> Good digging, and I agree that it should use the FLEX_ARRAY for\n> >> consistency.\n> >\n> > I can produce a patch, but I did not want to steal Arjun's thunder.\n> \n> Hmph, would it have to overlap? I think we can queue Arjun's patch\n> with +1 fix and FLEX_ARRAY thing separately, and they can go in in\n> any order, no?\n\nI more meant my suggestion to use add_name_decoration consistently. That\nfixes the r->type thing _and_ replaces Arjun's patch. Fixing FLEX_ARRAY\non top is just gravy. :)\n\nHere's the patch series I was thinking of:\n\n [1/3]: log-tree: make add_name_decoration a public function\n [2/3]: log-tree: make name_decoration hash static\n [3/3]: log-tree: use FLEX_ARRAY in name_decoration\n\nI almost added a 4/3 to convert \"name_decoration\" to\n\"commit_decoration\", since that is how it is used (and name_decoration\nis somewhat vague). But we actually do annotate other non-commit objects\nthat refs point to, as well. I'm not sure there is a way to _show_ them\ncurrently, but I'd just as soon leave it as-is.\n\n-Peff\n"},{"id":"248286","messageId":"20140826102335.GA25687@peff.net","threadId":"37410","inReplyTo":"20140826102051.GA4885@peff.net","subject":"[PATCH 1/3] log-tree: make add_name_decoration a public function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T10:23:36Z","receivedAt":"2014-08-26T10:23:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The log-tree code keeps a \"struct decoration\" hash to show\ntext decorations for each commit during log traversals. It\nmakes this available to other files by providing global\naccess to the hash. This can result in other code adding\nentries that do not conform to what log-tree expects.\n\nFor example, the bisect code adds its own \"dist\"\ndecorations to be shown. Originally the bisect code was\ncorrect, but when the name_decoration code grew a new field\nin eb3005e (commit.h: add 'type' to struct name_decoration,\n2010-06-19), the bisect code was not updated. As a result,\nthe log-tree code can access uninitialized memory and even\nsegfault.\n\nWe can fix this by making name_decoration's adding function\npublic. If all callers use it, then any changes to structi\ninitialization only need to happen in one place (and because\nthe members come in as parameters, the compiler can notice a\ncaller who does not supply enough information).\n\nAs a bonus, this also means that the decoration hashes\ncreated by the bisect code will use less memory (previously\nwe over-allocated space for the distance integer, but not we\nformat it into a temporary buffer and copy it to the final\nflex-array).\n\nSigned-off-by: Jeff King \n---\n bisect.c | 7 ++++---\n commit.h | 12 ++++++++++++\n log-tree.c | 12 +-----------\n 3 files changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex d6e851d..df09cbc 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -215,11 +215,12 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t}\n \tqsort(array, cnt, sizeof(*array), compare_commit_dist);\n \tfor (p = list, i = 0; i < cnt; i++) {\n-\t\tstruct name_decoration *r = xmalloc(sizeof(*r) + 100);\n+\t\tchar buf[100]; /* enough for dist=%d */\n \t\tstruct object *obj = &(array[i].commit->object);\n \n-\t\tsprintf(r->name, \"dist=%d\", array[i].distance);\n-\t\tr->next = add_decoration(&name_decoration, obj, r);\n+\t\tsnprintf(buf, sizeof(buf), \"dist=%d\", array[i].distance);\n+\t\tadd_name_decoration(DECORATION_NONE, buf, obj);\n+\n \t\tp->item = array[i].commit;\n \t\tp = p->next;\n \t}\ndiff --git a/commit.h b/commit.h\nindex a8cbf52..4902f97 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -33,6 +33,18 @@ struct name_decoration {\n \tchar name[1];\n };\n \n+enum decoration_type {\n+\tDECORATION_NONE = 0,\n+\tDECORATION_REF_LOCAL,\n+\tDECORATION_REF_REMOTE,\n+\tDECORATION_REF_TAG,\n+\tDECORATION_REF_STASH,\n+\tDECORATION_REF_HEAD,\n+\tDECORATION_GRAFTED,\n+};\n+\n+void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);\n+\n struct commit *lookup_commit(const unsigned char *sha1);\n struct commit *lookup_commit_reference(const unsigned char *sha1);\n struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\ndiff --git a/log-tree.c b/log-tree.c\nindex 0c53dc1..a821258 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -14,16 +14,6 @@\n \n struct decoration name_decoration = { \"object names\" };\n \n-enum decoration_type {\n-\tDECORATION_NONE = 0,\n-\tDECORATION_REF_LOCAL,\n-\tDECORATION_REF_REMOTE,\n-\tDECORATION_REF_TAG,\n-\tDECORATION_REF_STASH,\n-\tDECORATION_REF_HEAD,\n-\tDECORATION_GRAFTED,\n-};\n-\n static char decoration_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n \tGIT_COLOR_BOLD_GREEN,\t/* REF_LOCAL */\n@@ -84,7 +74,7 @@ int parse_decorate_color_config(const char *var, const int ofs, const char *valu\n #define decorate_get_color_opt(o, ix) \\\n \tdecorate_get_color((o)->use_color, ix)\n \n-static void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)\n+void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)\n {\n \tint nlen = strlen(name);\n \tstruct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen);\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248287","messageId":"20140826102353.GB25687@peff.net","threadId":"37410","inReplyTo":"20140826102051.GA4885@peff.net","subject":"[PATCH 2/3] log-tree: make name_decoration hash static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T10:23:54Z","receivedAt":"2014-08-26T10:23:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the previous commit, we made add_name_decoration global\nso that adders would not have to access the hash directly.\nWe now make the hash itself static so that callers _have_ to\nadd through our function, making sure that all additions go\nthrough a single point. To do this, we have to add one more\naccessor function: a way to lookup entries in the hash.\n\nSince the only caller doesn't actually look at the returned\nvalue, but rather only asks whether there is a decoration or\nnot, we could provide only a boolean \"has_name_decoration\".\nThat would allow us to make \"struct name_decoration\" local\nto log-tree, as well.\n\nHowever, it's unlikely to cause any maintainability harm\nmaking the actual data public, and this interface is more\nflexible if we need to look at decorations from other parts\nof the code in the future.\n\nSigned-off-by: Jeff King \n---\n commit.h | 2 +-\n log-tree.c | 11 ++++++++---\n revision.c | 2 +-\n 3 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 4902f97..263b49e 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -26,7 +26,6 @@ extern int save_commit_buffer;\n extern const char *commit_type;\n \n /* While we can decorate any object with a name, it's only used for commits.. */\n-extern struct decoration name_decoration;\n struct name_decoration {\n \tstruct name_decoration *next;\n \tint type;\n@@ -44,6 +43,7 @@ enum decoration_type {\n };\n \n void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);\n+const struct name_decoration *get_name_decoration(const struct object *obj);\n \n struct commit *lookup_commit(const unsigned char *sha1);\n struct commit *lookup_commit_reference(const unsigned char *sha1);\ndiff --git a/log-tree.c b/log-tree.c\nindex a821258..7cbc4ee 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -12,7 +12,7 @@\n #include \"sequencer.h\"\n #include \"line-log.h\"\n \n-struct decoration name_decoration = { \"object names\" };\n+static struct decoration name_decoration = { \"object names\" };\n \n static char decoration_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n@@ -83,6 +83,11 @@ void add_name_decoration(enum decoration_type type, const char *name, struct obj\n \tres->next = add_decoration(&name_decoration, obj, res);\n }\n \n+const struct name_decoration *get_name_decoration(const struct object *obj)\n+{\n+\treturn lookup_decoration(&name_decoration, obj);\n+}\n+\n static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n {\n \tstruct object *obj;\n@@ -177,13 +182,13 @@ void format_decorations(struct strbuf *sb,\n \t\t\tint use_color)\n {\n \tconst char *prefix;\n-\tstruct name_decoration *decoration;\n+\tconst struct name_decoration *decoration;\n \tconst char *color_commit =\n \t\tdiff_get_color(use_color, DIFF_COMMIT);\n \tconst char *color_reset =\n \t\tdecorate_get_color(use_color, DECORATION_NONE);\n \n-\tdecoration = lookup_decoration(&name_decoration, &commit->object);\n+\tdecoration = get_name_decoration(&commit->object);\n \tif (!decoration)\n \t\treturn;\n \tprefix = \" (\";\ndiff --git a/revision.c b/revision.c\nindex 2571ada..5aff2b4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -473,7 +473,7 @@ static int rev_compare_tree(struct rev_info *revs,\n \t\t * If we are simplifying by decoration, then the commit\n \t\t * is worth showing if it has a tag pointing at it.\n \t\t */\n-\t\tif (lookup_decoration(&name_decoration, &commit->object))\n+\t\tif (get_name_decoration(&commit->object))\n \t\t\treturn REV_TREE_DIFFERENT;\n \t\t/*\n \t\t * A commit that is not pointed by a tag is uninteresting\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248288","messageId":"20140826102420.GC25687@peff.net","threadId":"37410","inReplyTo":"20140826102051.GA4885@peff.net","subject":"[PATCH 3/3] log-tree: use FLEX_ARRAY in name_decoration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T10:24:20Z","receivedAt":"2014-08-26T10:24:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We are already using the flex-array technique; let's\nannotate it with our usual FLEX_ARRAY macro. Besides being\nmore readable, this is slightly more efficient on compilers\nthat understand flex-arrays.\n\nNote that we need to bump the allocation in add_name_decoration,\nwhich did not explicitly add one byte for the NUL terminator\nof the string we putting into the flex-array (it did not\nneed to before, because the struct itself was over-allocated\nby one byte).\n\nSigned-off-by: Jeff King \n---\nThis could come first in the series, but doing it last means we only\nhave to update one spot. :)\n\n commit.h | 2 +-\n log-tree.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 263b49e..1516fc9 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -29,7 +29,7 @@ extern const char *commit_type;\n struct name_decoration {\n \tstruct name_decoration *next;\n \tint type;\n-\tchar name[1];\n+\tchar name[FLEX_ARRAY];\n };\n \n enum decoration_type {\ndiff --git a/log-tree.c b/log-tree.c\nindex 7cbc4ee..fb60018 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -77,7 +77,7 @@ int parse_decorate_color_config(const char *var, const int ofs, const char *valu\n void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)\n {\n \tint nlen = strlen(name);\n-\tstruct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen);\n+\tstruct name_decoration *res = xmalloc(sizeof(*res) + nlen + 1);\n \tmemcpy(res->name, name, nlen + 1);\n \tres->type = type;\n \tres->next = add_decoration(&name_decoration, obj, res);\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248289","messageId":"20140826110303.GA25736@peff.net","threadId":"37410","inReplyTo":"xmqq8umcl0al.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T11:03:03Z","receivedAt":"2014-08-26T11:03:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 02:36:02PM -0700, Junio C Hamano wrote:\n\n> > I think you are right, and the patch is the right direction (assuming we\n> > want to do this; I question whether there are enough elements in the\n> > list for us to care about the size, and if there are, we are probably\n> > better off storing the int and formatting the strings on the fly).\n> \n> ;-)\n\nHaving now dug into this much further, the answers to those questions\nare:\n\n 1. Yes, we might actually have quite a lot of these, if you are\n bisecting a large range of commits. However, the code only runs\n when you are doing --bisect-all or skipping a commit, so probably\n nobody actually cares that much.\n\n 2. It would be nicer to hold just an \"int\", but we are hooking into\n the log-tree decorate machinery here, which expects a string. We'd\n need some kind of decorate-like callback machinery from log-tree to\n do this right. It's probably not worth the effort.\n\n> Among the flex arrays we use, some are arrays of bools, some others\n> are arrays of object names, and there are many arrays of even more\n> esoteric types. Only a small number of them are \"We want a struct\n> with a constant string, and we do not want to do two allocations and\n> to pay an extra dereference cost by using 'const char *'\".\n\nYeah, I was working under the assumption that most of them were holding\na string. I just did a quick skim of the grep results for FLEX_ARRAY. Of\nthe 36 instances, 26 hold strings. 9 hold something else entirely, and 1\nholds a char buffer that does not need to be NUL-terminated (so it\n_could_ be handled by a similar helper, but using \"%s\" would be wrong).\n\nSo it's definitely the majority, but certainly not all. I decided to\nlook into this a little further, and my results are below. The tl;dr is\nthat no, we probably shouldn't do it. So you can stop reading if you\ndon't find this interesting. :)\n\n> For them, by the time we allocate a struct, by definition we should\n> have sufficient information to compute how large to make that\n> structure and a printf-format plus its args would be the preferred\n> form of that \"sufficient information\", I would think.\n\nI was tempted to also suggest a pure-string form (i.e., just take a\nstring, run strlen on it, and use that as the final value). That would\nmake the variadic macro problem go away. But besides name_decoration,\nthere are other cases that really do want formatting. For instance,\nalloc_ref basically wants to do (\"%s%s\", prefix, name).\n\n> The name \"fmt_flex_array()\", which stresses too much on the\n> \"formatting\" side without implying that it is the way to allocate\n> the thing, may be horrible, and I agree with you that without\n> variadic macros the end result may not read very well. Unless we\n> have great many number of places we can use the helper to make the\n> code to create these objects look nicer, I am afraid that the\n> pros-and-cons may not be very favourable.\n\nYeah, reading my suggestion again, it should clearly be\nalloc_flex_struct or something.\n\nHere's a fully-converted sample spot, where I think there's a slight\nbenefit:\n\ndiff --git a/remote.c b/remote.c\nindex 3d6c86a..ba32d40 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -928,14 +928,30 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)\n \treturn query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);\n }\n \n+static void *alloc_flex_struct(size_t base, size_t offset, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tsize_t extra;\n+\tchar *ret;\n+\n+\tva_start(ap, fmt);\n+\textra = vsnprintf(NULL, 0, fmt, ap);\n+\textra++; /* for NUL terminator */\n+\tva_end(ap);\n+\n+\tret = xcalloc(1, base + extra);\n+\tva_start(ap, fmt);\n+\tvsnprintf(ret + offset, extra, fmt, ap);\n+\tva_end(ap);\n+\n+\treturn ret;\n+}\n+\n static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,\n \t\tconst char *name)\n {\n-\tsize_t len = strlen(name);\n-\tstruct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);\n-\tmemcpy(ref->name, prefix, prefixlen);\n-\tmemcpy(ref->name + prefixlen, name, len);\n-\treturn ref;\n+\treturn alloc_flex_struct(sizeof(struct ref), offsetof(struct ref, name),\n+\t\t\t\t \"%.*s%s\", prefixlen, prefix, name);\n }\n \n struct ref *alloc_ref(const char *name)\n\nObviously the helper is much longer than the code it is replacing, but\nit would be used in multiple spots. The main thing I like here is that\nwe are dropping the manual length computations, which are easy to get\nwrong (it's easy to forget a +1 for a NUL terminator, etc).\n\nThe offsetof is a little ugly. And the fact that we have a pre-computed\nlength for prefixlen makes the format string a little ugly.\n\nHere's a another example:\n\ndiff --git a/attr.c b/attr.c\nindex 734222d..100c423 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -89,8 +89,8 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \tif (invalid_attr_name(name, len))\n \t\treturn NULL;\n \n-\ta = xmalloc(sizeof(*a) + len + 1);\n-\tmemcpy(a->name, name, len);\n+\ta = alloc_flex_array(sizeof(*a), offsetof(struct git_attr, name),\n+\t\t\t \"%.*s\", len, name);\n \ta->name[len] = 0;\n \ta->h = hval;\n \ta->next = git_attr_hash[pos];\n\nI think this is strictly worse for reading. The original computation was\npretty easy in the first place, so we are not getting much benefit\nthere. And again we have the precomputed \"len\" passed into the function,\nso we have to use the less readable \"%.*s\". And note that offsetof()\nrequires us to pass a real typename instead of just \"*a\", as sizeof()\nallows (I suspect passing \"a->name - a\" would work on many systems, but\nI also suspect that is a gross violation of the C standard when \"a\" has\nnot yet been initialized).\n\nSo given that the benefit ranges from \"a little\" to \"negative\" in these\ntwo examples, I'm inclined to give up this line of inquiry.\n\n-Peff\n"},{"id":"248291","messageId":"53FC7408.1030708@ramsay1.demon.co.uk","threadId":"37410","inReplyTo":"20140826102335.GA25687@peff.net","subject":"Re: [PATCH 1/3] log-tree: make add_name_decoration a public function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-26T11:48:24Z","receivedAt":"2014-08-26T11:48:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 26/08/14 11:23, Jeff King wrote:\n> The log-tree code keeps a \"struct decoration\" hash to show\n> text decorations for each commit during log traversals. It\n> makes this available to other files by providing global\n> access to the hash. This can result in other code adding\n> entries that do not conform to what log-tree expects.\n> \n> For example, the bisect code adds its own \"dist\"\n> decorations to be shown. Originally the bisect code was\n> correct, but when the name_decoration code grew a new field\n> in eb3005e (commit.h: add 'type' to struct name_decoration,\n> 2010-06-19), the bisect code was not updated. As a result,\n> the log-tree code can access uninitialized memory and even\n> segfault.\n> \n> We can fix this by making name_decoration's adding function\n> public. If all callers use it, then any changes to structi\n\ns/structi/struct/\n\n> initialization only need to happen in one place (and because\n> the members come in as parameters, the compiler can notice a\n> caller who does not supply enough information).\n> \n> As a bonus, this also means that the decoration hashes\n> created by the bisect code will use less memory (previously\n> we over-allocated space for the distance integer, but not we\n\ns/not/now/\n\n> format it into a temporary buffer and copy it to the final\n> flex-array).\n> \n> Signed-off-by: Jeff King \n\nATB,\nRamsay Jones\n"},{"id":"248292","messageId":"53FC7621.7090102@ramsay1.demon.co.uk","threadId":"37410","inReplyTo":"20140826110303.GA25736@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-26T11:57:21Z","receivedAt":"2014-08-26T11:57:21Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 26/08/14 12:03, Jeff King wrote:\n[snip]\n> \n> Yeah, reading my suggestion again, it should clearly be\n> alloc_flex_struct or something.\n> \n> Here's a fully-converted sample spot, where I think there's a slight\n> benefit:\n> \n> diff --git a/remote.c b/remote.c\n> index 3d6c86a..ba32d40 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -928,14 +928,30 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)\n> \treturn query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);\n> }\n> \n> +static void *alloc_flex_struct(size_t base, size_t offset, const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tsize_t extra;\n> +\tchar *ret;\n> +\n> +\tva_start(ap, fmt);\n> +\textra = vsnprintf(NULL, 0, fmt, ap);\n> +\textra++; /* for NUL terminator */\n> +\tva_end(ap);\n> +\n> +\tret = xcalloc(1, base + extra);\n> +\tva_start(ap, fmt);\n> +\tvsnprintf(ret + offset, extra, fmt, ap);\n\nWhat is the relationship between 'base' and 'offset'?\n\nLet me assume that base is always, depending on your compiler, either\nequal to offset or offset+1. Yes? (I'm assuming base is always the\nsizeof(struct whatever)). Do you need both base and offset?\n\n> +\tva_end(ap);\n> +\n> +\treturn ret;\n> +}\n> +\n> static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,\n> \t\tconst char *name)\n> {\n> -\tsize_t len = strlen(name);\n> -\tstruct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);\n> -\tmemcpy(ref->name, prefix, prefixlen);\n> -\tmemcpy(ref->name + prefixlen, name, len);\n> -\treturn ref;\n> +\treturn alloc_flex_struct(sizeof(struct ref), offsetof(struct ref, name),\n> +\t\t\t\t \"%.*s%s\", prefixlen, prefix, name);\n> }\n> \n> struct ref *alloc_ref(const char *name)\n> \n> Obviously the helper is much longer than the code it is replacing, but\n> it would be used in multiple spots. The main thing I like here is that\n> we are dropping the manual length computations, which are easy to get\n> wrong (it's easy to forget a +1 for a NUL terminator, etc).\n> \n> The offsetof is a little ugly. And the fact that we have a pre-computed\n> length for prefixlen makes the format string a little ugly.\n> \n> Here's a another example:\n> \n> diff --git a/attr.c b/attr.c\n> index 734222d..100c423 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -89,8 +89,8 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n> \tif (invalid_attr_name(name, len))\n> \t\treturn NULL;\n> \n> -\ta = xmalloc(sizeof(*a) + len + 1);\n> -\tmemcpy(a->name, name, len);\n> +\ta = alloc_flex_array(sizeof(*a), offsetof(struct git_attr, name),\n> +\t\t\t \"%.*s\", len, name);\n> \ta->name[len] = 0;\n> \ta->h = hval;\n> \ta->next = git_attr_hash[pos];\n> \n> I think this is strictly worse for reading. The original computation was\n> pretty easy in the first place, so we are not getting much benefit\n> there. And again we have the precomputed \"len\" passed into the function,\n> so we have to use the less readable \"%.*s\". And note that offsetof()\n> requires us to pass a real typename instead of just \"*a\", as sizeof()\n> allows (I suspect passing \"a->name - a\" would work on many systems, but\n> I also suspect that is a gross violation of the C standard when \"a\" has\n> not yet been initialized).\n> \n> So given that the benefit ranges from \"a little\" to \"negative\" in these\n> two examples, I'm inclined to give up this line of inquiry.\n\nIndeed. :-D\n\nATB,\nRamsay Jones\n"},{"id":"248294","messageId":"20140826121359.GA29180@peff.net","threadId":"37410","inReplyTo":"53FC7621.7090102@ramsay1.demon.co.uk","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T12:14:00Z","receivedAt":"2014-08-26T12:14:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 12:57:21PM +0100, Ramsay Jones wrote:\n\n> > +\tret = xcalloc(1, base + extra);\n> > +\tva_start(ap, fmt);\n> > +\tvsnprintf(ret + offset, extra, fmt, ap);\n> \n> What is the relationship between 'base' and 'offset'?\n> \n> Let me assume that base is always, depending on your compiler, either\n> equal to offset or offset+1. Yes? (I'm assuming base is always the\n> sizeof(struct whatever)). Do you need both base and offset?\n\nIt's much more complicated than that. Take \"struct name_decoration\", for\ninstance, which looks like this:\n\n struct name_decoration {\n\tstruct name_decoration *next;\n\tint type;\n\tchar name[FLEX_ARRAY];\n };\n\nOn my 64-bit system using gcc, sizeof() returns 16; it has to pad the\nwhole thing to 64-bit alignment in case I put two of them in an array.\nBut offsetof(name) is 12, since the array of char does not need the same\nalignment; it can go right after the type and make use of the padding\nbits.\n\nAs a side note, that means that the original \"char name[1]\" (before it\nbecame FLEX_ARRAY) was not any less efficient on 64-bit machines (the\n1-byte went into the padding, and sizeof() was the same). It did matter\non 32-bit systems, though where it bumped the empty struct size from 12\nto 16.\n\n-Peff\n"},{"id":"248295","messageId":"20140826121710.GB29180@peff.net","threadId":"37410","inReplyTo":"53FC7408.1030708@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/3] log-tree: make add_name_decoration a public function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T12:17:11Z","receivedAt":"2014-08-26T12:17:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 12:48:24PM +0100, Ramsay Jones wrote:\n\n> > We can fix this by making name_decoration's adding function\n> > public. If all callers use it, then any changes to structi\n> \n> s/structi/struct/\n\nI blame vi finger-cruft.\n\n> > initialization only need to happen in one place (and because\n> > the members come in as parameters, the compiler can notice a\n> > caller who does not supply enough information).\n> > \n> > As a bonus, this also means that the decoration hashes\n> > created by the bisect code will use less memory (previously\n> > we over-allocated space for the distance integer, but not we\n> \n> s/not/now/\n\nEr, I blame vi again. Yeah, that's my story and I'm sticking to it.\n\nThanks for proofreading. :)\n\n-Peff\n"},{"id":"248298","messageId":"53FC7F98.5070204@ramsay1.demon.co.uk","threadId":"37410","inReplyTo":"20140826121359.GA29180@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-26T12:37:44Z","receivedAt":"2014-08-26T12:37:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 26/08/14 13:14, Jeff King wrote:\n> On Tue, Aug 26, 2014 at 12:57:21PM +0100, Ramsay Jones wrote:\n> \n>>> +\tret = xcalloc(1, base + extra);\n>>> +\tva_start(ap, fmt);\n>>> +\tvsnprintf(ret + offset, extra, fmt, ap);\n>>\n>> What is the relationship between 'base' and 'offset'?\n>>\n>> Let me assume that base is always, depending on your compiler, either\n>> equal to offset or offset+1. Yes? (I'm assuming base is always the\n>> sizeof(struct whatever)). Do you need both base and offset?\n> \n> It's much more complicated than that. Take \"struct name_decoration\", for\n> instance, which looks like this:\n> \n> struct name_decoration {\n> \tstruct name_decoration *next;\n> \tint type;\n> \tchar name[FLEX_ARRAY];\n> };\n> \n> On my 64-bit system using gcc, sizeof() returns 16; it has to pad the\n> whole thing to 64-bit alignment in case I put two of them in an array.\n> But offsetof(name) is 12, since the array of char does not need the same\n> alignment; it can go right after the type and make use of the padding\n> bits.\n\nHmm, interesting. I must re-read the standard. I was convinced that the\nstandard *requires* any alignment padding to come *before* the name field.\n(how would you put a, non-trivial, variable data structure into an array?)\n\nATB,\nRamsay Jones\n"},{"id":"248300","messageId":"20140826124344.GF29180@peff.net","threadId":"37410","inReplyTo":"53FC7F98.5070204@ramsay1.demon.co.uk","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T12:43:44Z","receivedAt":"2014-08-26T12:43:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 01:37:44PM +0100, Ramsay Jones wrote:\n\n> > On my 64-bit system using gcc, sizeof() returns 16; it has to pad the\n> > whole thing to 64-bit alignment in case I put two of them in an array.\n> > But offsetof(name) is 12, since the array of char does not need the same\n> > alignment; it can go right after the type and make use of the padding\n> > bits.\n> \n> Hmm, interesting. I must re-read the standard. I was convinced that the\n> standard *requires* any alignment padding to come *before* the name field.\n> (how would you put a, non-trivial, variable data structure into an array?)\n\nI think you don't. How would you compute a[1] if a[0] has a variable\nsize?\n\nYou can put a flex-array structure into an array, but then each element\nhas the flex member as zero-size (and you should not access it, of\ncourse).\n\n-Peff\n"},{"id":"248301","messageId":"53FC84C8.1020300@ramsay1.demon.co.uk","threadId":"37410","inReplyTo":"20140826124344.GF29180@peff.net","subject":"Re: [PATCH] bisect: save heap memory. allocate only the required amount","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-26T12:59:52Z","receivedAt":"2014-08-26T12:59:52Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 26/08/14 13:43, Jeff King wrote:\n> On Tue, Aug 26, 2014 at 01:37:44PM +0100, Ramsay Jones wrote:\n> \n>>> On my 64-bit system using gcc, sizeof() returns 16; it has to pad the\n>>> whole thing to 64-bit alignment in case I put two of them in an array.\n>>> But offsetof(name) is 12, since the array of char does not need the same\n>>> alignment; it can go right after the type and make use of the padding\n>>> bits.\n>>\n>> Hmm, interesting. I must re-read the standard. I was convinced that the\n>> standard *requires* any alignment padding to come *before* the name field.\n>> (how would you put a, non-trivial, variable data structure into an array?)\n> \n> I think you don't. How would you compute a[1] if a[0] has a variable\n> size?\n> \n> You can put a flex-array structure into an array, but then each element\n> has the flex member as zero-size (and you should not access it, of\n> course).\n\nExactly. ;-)\n\nATB,\nRamsay Jones\n"},{"id":"248329","messageId":"xmqq38cjjgjp.fsf@gitster.dls.corp.google.com","threadId":"37410","inReplyTo":"20140826102353.GB25687@peff.net","subject":"Re: [PATCH 2/3] log-tree: make name_decoration hash static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T17:40:10Z","receivedAt":"2014-08-26T17:40:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In the previous commit, we made add_name_decoration global\n> so that adders would not have to access the hash directly.\n> We now make the hash itself static so that callers _have_ to\n> add through our function, making sure that all additions go\n> through a single point. To do this, we have to add one more\n> accessor function: a way to lookup entries in the hash.\n>\n> Since the only caller doesn't actually look at the returned\n> value, but rather only asks whether there is a decoration or\n> not, we could provide only a boolean \"has_name_decoration\".\n> That would allow us to make \"struct name_decoration\" local\n> to log-tree, as well.\n>\n> However, it's unlikely to cause any maintainability harm\n> making the actual data public, and this interface is more\n> flexible if we need to look at decorations from other parts\n> of the code in the future.\n\nI didn't think we want only-bool version, but it is nice to see it\nexplained well.\n\nI may have called it lookup_name_decoration() to match, though, if I\nwere doing this patch ;-)\n\nThanks.\n"},{"id":"248331","messageId":"20140826174336.GA17101@peff.net","threadId":"37410","inReplyTo":"xmqq38cjjgjp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] log-tree: make name_decoration hash static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T17:43:36Z","receivedAt":"2014-08-26T17:43:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 10:40:10AM -0700, Junio C Hamano wrote:\n\n> I may have called it lookup_name_decoration() to match, though, if I\n> were doing this patch ;-)\n\nHmph. I called it \"get\" because that was the opposite of \"add\" to me,\nand I was matching \"add_name_decoration\". Of course, in the regular\ndecoration code, the add function is also \"add\" and its opposite is\n\"lookup\". So mine is gratuitously different. I do not mind if you adjust\nwhile applying.\n\n-Peff\n"},{"id":"248337","messageId":"xmqqr403hx36.fsf@gitster.dls.corp.google.com","threadId":"37410","inReplyTo":"20140826174336.GA17101@peff.net","subject":"Re: [PATCH 2/3] log-tree: make name_decoration hash static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T19:25:49Z","receivedAt":"2014-08-26T19:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 26, 2014 at 10:40:10AM -0700, Junio C Hamano wrote:\n>\n>> I may have called it lookup_name_decoration() to match, though, if I\n>> were doing this patch ;-)\n>\n> Hmph. I called it \"get\" because that was the opposite of \"add\" to me,\n> and I was matching \"add_name_decoration\". Of course, in the regular\n> decoration code, the add function is also \"add\" and its opposite is\n> \"lookup\". So mine is gratuitously different. I do not mind if you adjust\n> while applying.\n\nI do not care too deeply either way, either ;-) I just thought that\nsharing the verb with the underlying function being wrapped would be\nmore consistent.\n\nI wish we used lookup vs get more consistently, though. One should\nmean \"give us if we already have one otherwise fail\" while the other\nshould mean \"give us one, or create one if there isn't yet\".\n\nUnfortunately lookup_commit() and remote_get() both do auto-vivify X-<.\n"},{"id":"248360","messageId":"CAPig+cTHLOAe+mHr9jiR-k-3yrXfrAEjUSrHG=92kRSEGyto8Q@mail.gmail.com","threadId":"37410","inReplyTo":"20140826102420.GC25687@peff.net","subject":"Re: [PATCH 3/3] log-tree: use FLEX_ARRAY in name_decoration","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-08-27T00:30:08Z","receivedAt":"2014-08-27T00:30:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 26, 2014 at 6:24 AM, Jeff King wrote:\n> We are already using the flex-array technique; let's\n> annotate it with our usual FLEX_ARRAY macro. Besides being\n> more readable, this is slightly more efficient on compilers\n> that understand flex-arrays.\n>\n> Note that we need to bump the allocation in add_name_decoration,\n> which did not explicitly add one byte for the NUL terminator\n> of the string we putting into the flex-array (it did not\n\ns/we/we are/\n\n> need to before, because the struct itself was over-allocated\n> by one byte).\n>\n> Signed-off-by: Jeff King \n> ---\n> This could come first in the series, but doing it last means we only\n> have to update one spot. :)\n>\n> commit.h | 2 +-\n> log-tree.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit.h b/commit.h\n> index 263b49e..1516fc9 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -29,7 +29,7 @@ extern const char *commit_type;\n> struct name_decoration {\n> struct name_decoration *next;\n> int type;\n> - char name[1];\n> + char name[FLEX_ARRAY];\n> };\n>\n> enum decoration_type {\n> diff --git a/log-tree.c b/log-tree.c\n> index 7cbc4ee..fb60018 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -77,7 +77,7 @@ int parse_decorate_color_config(const char *var, const int ofs, const char *valu\n> void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)\n> {\n> int nlen = strlen(name);\n> - struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen);\n> + struct name_decoration *res = xmalloc(sizeof(*res) + nlen + 1);\n> memcpy(res->name, name, nlen + 1);\n> res->type = type;\n> res->next = add_decoration(&name_decoration, obj, res);\n> --\n> 2.1.0.346.ga0367b9\n"}]}