Re: [PATCH] commit: introduce set_merge_remote_desc()
- From
Jeff King <peff@peff.net>
- Date
- Aug 13, 2016, 09:23 UTC
- Message-ID
- <20160813092330.vmy2hip4papyuula@sigill.intra.peff.net>
- In-Reply-To
- <57AEE4F7.7090804@web.de>
On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:
Show 10 quoted lines
> 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. :)
Show 16 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.
-Peff