On Mon, Oct 09, 2023 at 10:58:07AM +0100, Phillip Wood wrote:
> > @@ -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
