Re: [PATCH v6] merge-tree: add -X strategy option

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 9, 2023, 09:58 UTC
Message-ID: <a482d047-dd40-436d-8daa-0c74780af11f@gmail.com>
In-Reply-To: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>

On 24/09/2023 03:23, Izzy via GitGitGadget wrote:

Thanks for updating the patch, sorry it has taken me a while to look at this.

Show 7 quoted lines

> From: Tang Yuyi <winglovet@gmail.com>
> 
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
> 
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---

Show 7 quoted lines

>   static int real_merge(struct merge_tree_options *o,
> @@ -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"

> +	if (xopts.nr && o.mode == MODE_TRIVIAL)
> +		die(_("--trivial-merge is incompatible with all other options"));

This is good, thanks for adding it.

> +	for (int x = 0; x < xopts.nr; x++)

This is not worth a re-roll on its own but as xopts.nr is a size_t we should declare x to be the same type rather than an int.

The tests are pretty minimal, ideally we would check that "-X" works with "--stdin", that it is rejected by a trivial merge and that one of the other strategy options works.

Best Wishes

Phillip