{"thread":{"id":"43803","subject":"[PATCH] commit: introduce set_merge_remote_desc()","startedAt":"2016-08-13T09:14:45Z","lastAt":"2016-08-13T12:24:58Z","messageCount":8,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"298958","messageId":"57AEE4F7.7090804@web.de","threadId":"43803","inReplyTo":null,"subject":"[PATCH] commit: introduce set_merge_remote_desc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T09:14:31Z","receivedAt":"2016-08-13T09:14:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a helper function for allocating, populating and attaching struct\nmerge_remote_desc to a commit and use it consistently.  It allocates the\nnecessary memory in a single block.\n\ncommit.c::get_merge_parent() forgot to check for memory allocation\nfailures of strdup(3).\n\nmerge-recursive.c::make_virtual_commit() didn't duplicate the string for\nthe name member, even though one of it's callers (indirectly through\nget_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n commit.c          | 18 +++++++++++-------\n commit.h          |  2 ++\n merge-recursive.c |  5 +----\n 3 files changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 71a360d..372b200 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n \treturn result;\n }\n \n+void set_merge_remote_desc(struct commit *commit,\n+\t\t\t   const char *name, struct object *obj)\n+{\n+\tstruct merge_remote_desc *desc;\n+\tFLEXPTR_ALLOC_STR(desc, name, name);\n+\tdesc->obj = obj;\n+\tcommit->util = desc;\n+}\n+\n struct commit *get_merge_parent(const char *name)\n {\n \tstruct object *obj;\n@@ -1585,13 +1594,8 @@ struct commit *get_merge_parent(const char *name)\n \t\treturn NULL;\n \tobj = parse_object(oid.hash);\n \tcommit = (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n-\tif (commit && !commit->util) {\n-\t\tstruct merge_remote_desc *desc;\n-\t\tdesc = xmalloc(sizeof(*desc));\n-\t\tdesc->obj = obj;\n-\t\tdesc->name = strdup(name);\n-\t\tcommit->util = desc;\n-\t}\n+\tif (commit && !commit->util)\n+\t\tset_merge_remote_desc(commit, name, obj);\n \treturn commit;\n }\n \ndiff --git a/commit.h b/commit.h\nindex 23ae0c1..84bb507 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -365,6 +365,8 @@ struct merge_remote_desc {\n \tconst char *name;\n };\n #define merge_remote_util(commit) ((struct merge_remote_desc *)((commit)->util))\n+extern void set_merge_remote_desc(struct commit *commit,\n+\t\t\t\t  const char *name, struct object *obj);\n \n /*\n  * Given \"name\" from the command line to merge, find the commit object\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e5243c2..e349126 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -73,12 +73,9 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,\n static struct commit *make_virtual_commit(struct tree *tree, const char *comment)\n {\n \tstruct commit *commit = alloc_commit_node();\n-\tstruct merge_remote_desc *desc = xmalloc(sizeof(*desc));\n \n-\tdesc->name = comment;\n-\tdesc->obj = (struct object *)commit;\n+\tset_merge_remote_desc(commit, comment, (struct object *)commit);\n \tcommit->tree = tree;\n-\tcommit->util = desc;\n \tcommit->object.parsed = 1;\n \treturn commit;\n }\n-- \n2.9.3\n\n"},{"id":"298960","messageId":"20160813092330.vmy2hip4papyuula@sigill.intra.peff.net","threadId":"43803","inReplyTo":"57AEE4F7.7090804@web.de","subject":"Re: [PATCH] commit: introduce set_merge_remote_desc()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-13T09:23:31Z","receivedAt":"2016-08-13T09:25:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:\n\n> Add a helper function for allocating, populating and attaching struct\n> merge_remote_desc to a commit and use it consistently.  It allocates the\n> necessary memory in a single block.\n> \n> commit.c::get_merge_parent() forgot to check for memory allocation\n> failures of strdup(3).\n> \n> merge-recursive.c::make_virtual_commit() didn't duplicate the string for\n> the name member, even though one of it's callers (indirectly through\n> get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.\n\nIt seems like you've buried the interesting part here. This isn't just\nfor cleanup, but a bugfix that the oids in our virtual commits might get\noverwritten by subsequent actions.\n\nIt seems like that should be the subject and beginning of the commit\nmessage.  And then the fix is to allocate, and by the way we can do so\neasily with this nice new helper. :)\n\n> diff --git a/commit.c b/commit.c\n> index 71a360d..372b200 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n>  \treturn result;\n>  }\n>  \n> +void set_merge_remote_desc(struct commit *commit,\n> +\t\t\t   const char *name, struct object *obj)\n> +{\n> +\tstruct merge_remote_desc *desc;\n> +\tFLEXPTR_ALLOC_STR(desc, name, name);\n> +\tdesc->obj = obj;\n> +\tcommit->util = desc;\n> +}\n\nI don't think there is any reason to prefer FLEXPTR_ALLOC over\nFLEX_ALLOC, unless your struct interface is constrained by non-flex\nusers (that's why it is necessary for \"struct exclude\", for example,\nwhich sometimes needs to carry its own string and sometimes not).\n\nUsing FLEX_ALLOC saves a few bytes per struct, and avoids an extra\npointer indirection when accessing the data.\n\nSince it looks like you touch all of the allocations here, I think they\nwould both be happy as a regular flex array.\n\n-Peff\n"},{"id":"298965","messageId":"57AF0D8E.6040309@web.de","threadId":"43803","inReplyTo":"20160813092330.vmy2hip4papyuula@sigill.intra.peff.net","subject":"Re: [PATCH] commit: introduce set_merge_remote_desc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T12:07:42Z","receivedAt":"2016-08-13T12:08:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.08.2016 um 11:23 schrieb Jeff King:\n> On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:\n>\n>> Add a helper function for allocating, populating and attaching struct\n>> merge_remote_desc to a commit and use it consistently.  It allocates the\n>> necessary memory in a single block.\n>>\n>> commit.c::get_merge_parent() forgot to check for memory allocation\n>> failures of strdup(3).\n>>\n>> merge-recursive.c::make_virtual_commit() didn't duplicate the string for\n>> the name member, even though one of it's callers (indirectly through\n>> get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.\n>\n> It seems like you've buried the interesting part here. This isn't just\n> for cleanup, but a bugfix that the oids in our virtual commits might get\n> overwritten by subsequent actions.\n>\n> It seems like that should be the subject and beginning of the commit\n> message.  And then the fix is to allocate, and by the way we can do so\n> easily with this nice new helper. :)\n\nBugs are usually hidden, so why not hide fixes? ;-)\n\n>> diff --git a/commit.c b/commit.c\n>> index 71a360d..372b200 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n>>   \treturn result;\n>>   }\n>>\n>> +void set_merge_remote_desc(struct commit *commit,\n>> +\t\t\t   const char *name, struct object *obj)\n>> +{\n>> +\tstruct merge_remote_desc *desc;\n>> +\tFLEXPTR_ALLOC_STR(desc, name, name);\n>> +\tdesc->obj = obj;\n>> +\tcommit->util = desc;\n>> +}\n>\n> I don't think there is any reason to prefer FLEXPTR_ALLOC over\n> FLEX_ALLOC, unless your struct interface is constrained by non-flex\n> users (that's why it is necessary for \"struct exclude\", for example,\n> which sometimes needs to carry its own string and sometimes not).\n>\n> Using FLEX_ALLOC saves a few bytes per struct, and avoids an extra\n> pointer indirection when accessing the data.\n>\n> Since it looks like you touch all of the allocations here, I think they\n> would both be happy as a regular flex array.\n\nGood idea.\n\nSo let's turn this dish into a full menu:\n\n   commit: use xstrdup() in get_merge_parent()\n   commit: factor out set_merge_remote_desc()\n   merge-recursive: fix verbose output for multiple base trees\n   commit: use FLEX_ARRAY in struct merge_remote_desc\n\n  commit.c                   | 18 +++++++++++-------\n  commit.h                   |  4 +++-\n  merge-recursive.c          |  5 +----\n  t/t3030-merge-recursive.sh | 18 ++++++++++++++++++\n  4 files changed, 33 insertions(+), 12 deletions(-)\n\n\n"},{"id":"298966","messageId":"57AF0E0D.9030205@web.de","threadId":"43803","inReplyTo":"57AF0D8E.6040309@web.de","subject":"[PATCH v2 1/4] commit: use xstrdup() in get_merge_parent()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T12:09:49Z","receivedAt":"2016-08-13T12:10:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Handle allocation errors for the name member just like we already do\nfor the struct merge_remote_desc itself.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 71a360d..ccd232a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1589,7 +1589,7 @@ struct commit *get_merge_parent(const char *name)\n \t\tstruct merge_remote_desc *desc;\n \t\tdesc = xmalloc(sizeof(*desc));\n \t\tdesc->obj = obj;\n-\t\tdesc->name = strdup(name);\n+\t\tdesc->name = xstrdup(name);\n \t\tcommit->util = desc;\n \t}\n \treturn commit;\n-- \n2.9.3\n\n"},{"id":"298967","messageId":"57AF0E6F.4010902@web.de","threadId":"43803","inReplyTo":"57AF0D8E.6040309@web.de","subject":"[PATCH v2 2/4] commit: factor out set_merge_remote_desc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T12:11:27Z","receivedAt":"2016-08-13T12:11:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Export a helper function for allocating, populating and attaching a\nmerge_remote_desc to a commit.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n commit.c | 19 ++++++++++++-------\n commit.h |  2 ++\n 2 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex ccd232a..8bad713 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1576,6 +1576,16 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n \treturn result;\n }\n \n+void set_merge_remote_desc(struct commit *commit,\n+\t\t\t   const char *name, struct object *obj)\n+{\n+\tstruct merge_remote_desc *desc;\n+\tdesc = xmalloc(sizeof(*desc));\n+\tdesc->obj = obj;\n+\tdesc->name = xstrdup(name);\n+\tcommit->util = desc;\n+}\n+\n struct commit *get_merge_parent(const char *name)\n {\n \tstruct object *obj;\n@@ -1585,13 +1595,8 @@ struct commit *get_merge_parent(const char *name)\n \t\treturn NULL;\n \tobj = parse_object(oid.hash);\n \tcommit = (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n-\tif (commit && !commit->util) {\n-\t\tstruct merge_remote_desc *desc;\n-\t\tdesc = xmalloc(sizeof(*desc));\n-\t\tdesc->obj = obj;\n-\t\tdesc->name = xstrdup(name);\n-\t\tcommit->util = desc;\n-\t}\n+\tif (commit && !commit->util)\n+\t\tset_merge_remote_desc(commit, name, obj);\n \treturn commit;\n }\n \ndiff --git a/commit.h b/commit.h\nindex 23ae0c1..84bb507 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -365,6 +365,8 @@ struct merge_remote_desc {\n \tconst char *name;\n };\n #define merge_remote_util(commit) ((struct merge_remote_desc *)((commit)->util))\n+extern void set_merge_remote_desc(struct commit *commit,\n+\t\t\t\t  const char *name, struct object *obj);\n \n /*\n  * Given \"name\" from the command line to merge, find the commit object\n-- \n2.9.3\n\n"},{"id":"298968","messageId":"57AF0F84.9040206@web.de","threadId":"43803","inReplyTo":"57AF0D8E.6040309@web.de","subject":"[PATCH v2 3/4] merge-recursive: fix verbose output for multiple base trees","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T12:16:04Z","receivedAt":"2016-08-13T12:16:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"One of the indirect callers of make_virtual_commit() passes the result of\noid_to_hex() as the name, i.e. a pointer to a static buffer.  Since the\nfunction uses that string pointer directly in building a struct\nmerge_remote_desc, multiple entries can end up sharing the same name\ninadvertently.\n\nFix that by calling set_merge_remote_desc(), which creates a copy of the\nstring, instead of building the struct by hand.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n merge-recursive.c          |  5 +----\n t/t3030-merge-recursive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e5243c2..e349126 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -73,12 +73,9 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,\n static struct commit *make_virtual_commit(struct tree *tree, const char *comment)\n {\n \tstruct commit *commit = alloc_commit_node();\n-\tstruct merge_remote_desc *desc = xmalloc(sizeof(*desc));\n \n-\tdesc->name = comment;\n-\tdesc->obj = (struct object *)commit;\n+\tset_merge_remote_desc(commit, comment, (struct object *)commit);\n \tcommit->tree = tree;\n-\tcommit->util = desc;\n \tcommit->object.parsed = 1;\n \treturn commit;\n }\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex f7b0e59..470f334 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -660,4 +660,22 @@ test_expect_success 'merging with triple rename across D/F conflict' '\n \tgit merge other\n '\n \n+test_expect_success 'merge-recursive remembers the names of all base trees' '\n+\tgit reset --hard HEAD &&\n+\n+\t# more trees than static slots used by oid_to_hex()\n+\tfor commit in $c0 $c2 $c4 $c5 $c6 $c7\n+\tdo\n+\t\tgit rev-parse \"$commit^{tree}\"\n+\tdone >trees &&\n+\n+\t# ignore the return code -- it only fails because the input is weird\n+\ttest_must_fail git -c merge.verbosity=5 merge-recursive $(cat trees) -- $c1 $c3 >out &&\n+\n+\t# merge-recursive prints in reverse order, but we do not care\n+\tsort <trees >expect &&\n+\tsed -n \"s/^virtual //p\" out | sort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.9.3\n\n"},{"id":"298969","messageId":"57AF10C7.8070001@web.de","threadId":"43803","inReplyTo":"57AF0D8E.6040309@web.de","subject":"[PATCH v2 4/4] commit: use FLEX_ARRAY in struct merge_remote_desc","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T12:21:27Z","receivedAt":"2016-08-13T12:21:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Convert the name member of struct merge_remote_desc to a FLEX_ARRAY and\nuse FLEX_ALLOC_STR to build the struct.  This halves the number of\nmemory allocations, saves the storage for a pointer and avoids an\nindirection when reading the name.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n commit.c | 3 +--\n commit.h | 2 +-\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 8bad713..ba6dee3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1580,9 +1580,8 @@ void set_merge_remote_desc(struct commit *commit,\n \t\t\t   const char *name, struct object *obj)\n {\n \tstruct merge_remote_desc *desc;\n-\tdesc = xmalloc(sizeof(*desc));\n+\tFLEX_ALLOC_STR(desc, name, name);\n \tdesc->obj = obj;\n-\tdesc->name = xstrdup(name);\n \tcommit->util = desc;\n }\n \ndiff --git a/commit.h b/commit.h\nindex 84bb507..32e1a11 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -362,7 +362,7 @@ extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *\n \n struct merge_remote_desc {\n \tstruct object *obj; /* the named object, could be a tag */\n-\tconst char *name;\n+\tchar name[FLEX_ARRAY];\n };\n #define merge_remote_util(commit) ((struct merge_remote_desc *)((commit)->util))\n extern void set_merge_remote_desc(struct commit *commit,\n-- \n2.9.3\n\n"},{"id":"298970","messageId":"20160813122451.vrqqjjpqicqtz2uj@sigill.intra.peff.net","threadId":"43803","inReplyTo":"57AF0D8E.6040309@web.de","subject":"Re: [PATCH] commit: introduce set_merge_remote_desc()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-13T12:24:51Z","receivedAt":"2016-08-13T12:24:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 13, 2016 at 02:07:42PM +0200, René Scharfe wrote:\n\n> So let's turn this dish into a full menu:\n> \n>   commit: use xstrdup() in get_merge_parent()\n>   commit: factor out set_merge_remote_desc()\n>   merge-recursive: fix verbose output for multiple base trees\n>   commit: use FLEX_ARRAY in struct merge_remote_desc\n\nVery nice. Four patches seems like a lot for such a small fix, but each\none is so trivial and easy to understand, I think it's worth it.\n\nThey all look good to me.\n\n-Peff\n"}]}