Re: [PATCH 1/2] Change default merge backend from recursive to ort

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 2, 2021, 22:46 UTC
Message-ID: <nycvar.QRO.7.76.6.2108030030000.55@tvgsbejvaqbjf.bet>
In-Reply-To: <8f6af8d494e0924aef4ae6963b8dca2228dad9b1.1627776462.git.gitgitgadget@gmail.com>

Hi Elijah,

On Sun, 1 Aug 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> There are a few reasons to switch the default:
> [...]

I think it would be really fantastic to change to the new default right after v2.33.0.

As to the patch, I only struggled slightly with the changes to `sequencer.c`:

Show 10 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..a98de9a8d15 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -636,7 +636,7 @@ static int do_recursive_merge(struct repository *r,
>  	for (i = 0; i < opts->xopts_nr; i++)
>  		parse_merge_opt(&o, opts->xopts[i]);
>
> -	if (opts->strategy && !strcmp(opts->strategy, "ort")) {
> +	if (!opts->strategy || strcmp(opts->strategy, "recursive")) {

At this stage, we're in `do_recursive_merge()`, and there is only one caller, `do_pick_commit()`, and the caller is guarded by the following condition:

        else if (!opts->strategy ||
                 !strcmp(opts->strategy, "recursive") ||
                 !strcmp(opts->strategy, "ort") ||
                 command == TODO_REVERT) {

The issue I see is with `git revert` allowing custom merge strategies. I _think_ we need a slightly different patch here, something like this:

- if (opts->strategy && !strcmp(opts->strategy, "ort")) { + if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

Show 9 quoted lines

>  		memset(&result, 0, sizeof(result));
>  		merge_incore_nonrecursive(&o, base_tree, head_tree, next_tree,
>  					    &result);
> @@ -3968,7 +3968,7 @@ static int do_merge(struct repository *r,
>  	o.branch2 = ref_name.buf;
>  	o.buffer_output = 2;
>
> -	if (opts->strategy && !strcmp(opts->strategy, "ort")) {
> +	if (!opts->strategy || strcmp(opts->strategy, "recursive")) {

It took me a while to convince myself that this is correct. At least now I _think_ it is correct: `do_merge()` defines:

        const char *strategy = !opts->xopts_nr &&
                (!opts->strategy ||
                 !strcmp(opts->strategy, "recursive") ||
                 !strcmp(opts->strategy, "ort")) ?
                NULL : opts->strategy;

and then hands off to `git merge -s <strategy>` if `strategy` is set, _before_ this hunk. Therefore we can be pretty certain that `opts->strategy` is either not set, or "ort", or "recursive" at that stage.

However, I think we could use the same idea I outlined in the previous hunk, to make things more obvious:

- if (opts->strategy && !strcmp(opts->strategy, "ort")) { + if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

Thank you, Dscho

Show 7 quoted lines

>  		/*
>  		 * TODO: Should use merge_incore_recursive() and
>  		 * merge_switch_to_result(), skipping the call to
> --
> gitgitgadget
>
>