Re: [PATCH 04/12] merge-tree: implement real merges

From: Elijah Newren <newren@gmail.com>
Date: Jan 24, 2022, 17:12 UTC
Message-ID: <CABPp-BE+4rZNP-5mT2MNOWR6y6BgEG6mt1r_qcrZtarom6aGsw@mail.gmail.com>
In-Reply-To: <220124.86h79tihjm.gmgdl@evledraar.gmail.com>

On Mon, Jan 24, 2022 at 1:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines

>
> On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:
>
> > +     /*
> > +      * TODO: Support subtree and other -X options?
> > +     if (use_strategies_nr == 1 &&
> > +         !strcmp(use_strategies[0]->name, "subtree"))
> > +             opt.subtree_shift = "";
> > +     for (x = 0; x < xopts_nr; x++)
> > +             if (parse_merge_opt(&opt, xopts[x]))
>
> Better omitted WIP code in a non-RFC series?

It's RFC: https://lore.kernel.org/git/pull.1122.git.1642888562.gitgitgadget@gmail.com/

But yeah, I should drop it. Previous rounds of this RFC submission got me feedback that the other commented-out code bit I used to have was something folks wanted in the initial version of the series so I uncommented and cleaned it up. The fact that no one has commented on this part suggests these options don't need to be supported from the start.

> > +                     die(_("Unknown strategy option: -X%s"), xopts[x]);
>
> As a general issue with this series, die(), BUG() etc. messages should
> start with a non-capital letter.

Right, thanks for the reminder.  I'll go through and try to fix up.

Show 9 quoted lines

> > +     printf("%s\n", oid_to_hex(&result.tree->object.oid));
>
> And for both this...
>
> > +             printf(_("Conflicts!\n"));
>
> ... and this we can just use puts(). For the former it's just less code,
> but for the latter translators also don't need to see the always-there
> \n in the translated message.

Makes sense.

Show 9 quoted lines

> > +# This test is ort-specific
> > +test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort || {
>
> Is this ${} trickery really needed? We're not testing with "set -u". So just:
>
>         if test "$GIT_..." != "ort"
>         then
>                 ...
>         fi

Ah, that would be simpler; thanks.

Show 9 quoted lines

> > +test_expect_success 'Barf on too many arguments' '
> > +     test_expect_code 129 git merge-tree --write-tree side1 side2 side3 2>expect &&
> > +
> > +     grep "^usage: git merge-tree" expect
> > +'
>
> Nit: In most other tests these usage assertions are at the top of the
> test, and for those we also make do with just testing the 129 exit code,
> which is probably enough here too...

I see a fair number of counterexamples searching for 129 in the test suite, and I've been bitten enough times seeing tests expect an error but get a different kind of error than the commit message stated they were expecting that I prefer the extra check beyond the error code. Anyway, I'll leave this piece as-is.