Re: [PATCH 1/2] Change default merge backend from recursive to ort
From: Johannes Schindelin <hidden>
Date: 2021-08-02 22:46:30
Hi Elijah, On Sun, 1 Aug 2021, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted> 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`:
quoted hunk ↗ jump to hunk
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")) {
quoted hunk ↗ jump to hunk
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
/* * TODO: Should use merge_incore_recursive() and * merge_switch_to_result(), skipping the call to -- gitgitgadget