From: René Scharfe Date: Sat, 13 Aug 2016 12:07:42 GMT Subject: Re: [PATCH] commit: introduce set_merge_remote_desc() Message-ID: <57AF0D8E.6040309@web.de> In-Reply-To: <20160813092330.vmy2hip4papyuula@sigill.intra.peff.net> Am 13.08.2016 um 11:23 schrieb Jeff King: > On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote: > >> Add a helper function for allocating, populating and attaching struct >> merge_remote_desc to a commit and use it consistently. It allocates the >> necessary memory in a single block. >> >> commit.c::get_merge_parent() forgot to check for memory allocation >> failures of strdup(3). >> >> merge-recursive.c::make_virtual_commit() didn't duplicate the string for >> the name member, even though one of it's callers (indirectly through >> get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer. > > It seems like you've buried the interesting part here. This isn't just > for cleanup, but a bugfix that the oids in our virtual commits might get > overwritten by subsequent actions. > > It seems like that should be the subject and beginning of the commit > message. And then the fix is to allocate, and by the way we can do so > easily with this nice new helper. :) Bugs are usually hidden, so why not hide fixes? ;-) >> diff --git a/commit.c b/commit.c >> index 71a360d..372b200 100644 >> --- a/commit.c >> +++ b/commit.c >> @@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len, >> return result; >> } >> >> +void set_merge_remote_desc(struct commit *commit, >> + const char *name, struct object *obj) >> +{ >> + struct merge_remote_desc *desc; >> + FLEXPTR_ALLOC_STR(desc, name, name); >> + desc->obj = obj; >> + commit->util = desc; >> +} > > I don't think there is any reason to prefer FLEXPTR_ALLOC over > FLEX_ALLOC, unless your struct interface is constrained by non-flex > users (that's why it is necessary for "struct exclude", for example, > which sometimes needs to carry its own string and sometimes not). > > Using FLEX_ALLOC saves a few bytes per struct, and avoids an extra > pointer indirection when accessing the data. > > Since it looks like you touch all of the allocations here, I think they > would both be happy as a regular flex array. Good idea. So let's turn this dish into a full menu: commit: use xstrdup() in get_merge_parent() commit: factor out set_merge_remote_desc() merge-recursive: fix verbose output for multiple base trees commit: use FLEX_ARRAY in struct merge_remote_desc commit.c | 18 +++++++++++------- commit.h | 4 +++- merge-recursive.c | 5 +---- t/t3030-merge-recursive.sh | 18 ++++++++++++++++++ 4 files changed, 33 insertions(+), 12 deletions(-)