On Mon, Jan 24, 2022 at 1:55 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > 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@xxxxxxxxx/ 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. > > + 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. > > +# 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. > > +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.
