Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

Hi Eric,

On Wed, 11 Jul 2018, Eric Sunshine wrote:

> On Wed, Jul 11, 2018 at 8:38 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:

> > diff --git a/sequencer.c b/sequencer.c
> > @@ -2932,7 +2966,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
> > -                       strbuf_addf(&buf, "Merge branch '%.*s'",
> > +                       strbuf_addf(&buf, "Merge %s '%.*s'",
> > +                                   to_merge->next ? "branches" : "branch",
> 
> Error messages in this file are already localizable. If this text ever
> becomes localizable, then this "sentence lego" could be problematic
> for translators.

As pointed out by Junio, this is not really a message eligible for
localization, at least not right now. If it becomes localized later, it
will be very easy to change the sentence lego then.

I am just wary of making life harder for the compiler that wants to verify
that the printf() style parameters match the format.

> > @@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
> > +               cmd.git_cmd = 1;
> > +               argv_array_push(&cmd.args, "merge");
> > +               argv_array_push(&cmd.args, "-s");
> > +               argv_array_push(&cmd.args, "octopus");
> 
> argv_array_pushl(&cmd.args, "-s", "octopus", NULL);
> 
> which would make it clear that these two arguments must remain
> together, and prevent someone from coming along and inserting a new
> argument between them.

I use a single `argv_array_pushl()` call now, with intuitive line
wrapping.

> > +               argv_array_push(&cmd.args, "--no-edit");
> > +               argv_array_push(&cmd.args, "--no-ff");
> > +               argv_array_push(&cmd.args, "--no-log");
> > +               argv_array_push(&cmd.args, "--no-stat");
> > +               argv_array_push(&cmd.args, "-F");
> > +               argv_array_push(&cmd.args, git_path_merge_msg());
> 
> Ditto:
> argv_array_pushl(&cmd.args, "-L", git_path_merge_msg(), NULL);
> 
> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> > @@ -329,4 +329,38 @@ test_expect_success 'labels that are object IDs are rewritten' '
> > +test_expect_success 'octopus merges' '
> > +       git checkout -b three &&
> > +       test_commit before-octopus &&
> > +       test_commit three &&
> > +       git checkout -b two HEAD^ &&
> > +       test_commit two &&
> > +       git checkout -b one HEAD^ &&
> > +       test_commit one &&
> > +       test_tick &&
> > +       (GIT_AUTHOR_NAME="Hank" GIT_AUTHOR_EMAIL="hank@sea.world" \
> > +        git merge -m "Tüntenfüsch" two three) &&
> 
> Unnecessary subshell?

Yes, I agree. I don't recall why I did it that way, probably I tried
something else originally that required a subshell, or something.

Thank you for helping me make the patches better.

Ciao,
Dscho