On Mon, Aug 2, 2021 at 4:46 PM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
>
> Hi Elijah,
>
> On Sun, 1 Aug 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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`:
>
> > 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")) {
>
> > 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
I'll include both suggestions in my next re-roll. Thanks for the feedback!
