Re: [PATCH] commit: introduce set_merge_remote_desc()

From: René Scharfe <l.s.r@web.de>
Date: Aug 13, 2016, 12:07 UTC
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:

Show 20 quoted lines

> 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? ;-)

Show 27 quoted lines

>> 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(-)