Re: [PATCH] commit: introduce set_merge_remote_desc()

[Jeff King <peff@peff.net>]

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. :)

> 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.

-Peff