Re: [PATCH v6] merge-tree: add -X strategy option
- From
Jeff King <peff@peff.net>
- Date
- Oct 9, 2023, 15:53 UTC
- Message-ID
- <20231009155315.GA3252778@coredump.intra.peff.net>
- In-Reply-To
- <a482d047-dd40-436d-8daa-0c74780af11f@gmail.com>
On Mon, Oct 09, 2023 at 10:58:07AM +0100, Phillip Wood wrote:
Show 16 quoted lines
> > @@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
> > {
> > struct commit *parent1, *parent2;
> > struct commit_list *merge_bases = NULL;
> > - struct merge_options opt;
> > + struct merge_options opt = o->merge_options;
>
> Copying struct merge_options by value here is unusual in our code base. I
> don't think it introduces a bug (there is no function to free the resources
> allocated in struct merge_options so we do not end up freeing them twice for
> example) but it would be clearer that it was safe if you did
>
> struct merge_options *opt = &o->merge_options;
>
> and updated the code to reflect that opt is now a pointer or just replaced
> all uses of "opt" with "o->merge_options"I agree that struct-copying is an unusual pattern, and we'd potentially run into problems with duplication. But I think it is even trickier than that here. We also go on to actually _modify_ opt in this function, assigning to various members (both directly, and I think the merge code itself will write to opt->priv).
So if we use a pointer (rather than struct assignment), those changes will persist in the merge_options struct that was passed in. Which is also weird.
Between the two, I think using a pointer is probably the least-weird. This real_merge() function is only called once, and is a static-local helper for cmd_merge_tree(). So the two functions work as a single unit, and munging "opt" is not a big deal.
-Peff