On Thu, Feb 03 2022, Elijah Newren wrote:

On Thu, Feb 3, 2022 at 1:04 AM Elijah Newren [off-list ref] wrote:

quoted

On Wed, Feb 2, 2022 at 6:09 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

Let merge-tree accept a `--write-tree` parameter for choosing real
merges instead of trivial merges, and accept an optional
`--trivial-merge` option to get the traditional behavior.  Note that
these accept different numbers of arguments, though, so these names
need not actually be used.

Maybe that ship has sailed, but just my 0.02: I thought this whole thing
was much less confusing with your initial merge-tree-ort proposal at
https://lore.kernel.org/git/CABPp-BEeBpJoU4yXdfA6vRAYVAUbd2gRhEV6j4VEqoqcu=FGSw@mail.gmail.com/ (local);
I.e. the end-state of merge-tree.c is that you end up reading largely
unrelated code (various static functions only used by one side or
another).

Christian's merge-tree-ort proposal?

quoted

But maybe that's all water under the bridge etc, however...

quoted

 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-     if (argc != 4)
-             usage(merge_tree_usage);
-     return trivial_merge(argc, argv);
+     struct merge_tree_options o = { 0 };
+     int expected_remaining_argc;
+
+     const char * const merge_tree_usage[] = {
+             N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+             N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
+             NULL
+     };
+     struct option mt_options[] = {
+             OPT_CMDMODE(0, "write-tree", &o.mode,
+                         N_("do a real merge instead of a trivial merge"),
+                         'w'),
+             OPT_CMDMODE(0, "trivial-merge", &o.mode,
+                         N_("do a trivial merge only"), 't'),
+             OPT_END()
+     };
+
+     /* Parse arguments */
+     argc = parse_options(argc, argv, prefix, mt_options,
+                          merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+     if (o.mode) {
+             expected_remaining_argc = (o.mode == 'w' ? 2 : 3);
+             if (argc != expected_remaining_argc)
+                     usage_with_options(merge_tree_usage, mt_options);
+     } else {
+             if (argc < 2 || argc > 3)
+                     usage_with_options(merge_tree_usage, mt_options);
+             o.mode = (argc == 2 ? 'w' : 't');
+     }

Do we really need to make this interface more special-casey by
auto-guessing based on argc what argument you want? I.e. instead of
usage like:

        N_("git merge-tree [--write-tree] <branch1> <branch2>"),
        N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),

Wouldn't it be simpler to just have the equivalent of:

        # old
        git merge-tree ...
        # new
        git merge-tree --new-thing ...

And not have to look at ... to figure out if we're dispatching to the
new or old thing.

You seem to be focusing on code simplicity?  Sure, that'd be simpler
code, it'd just be a less useful feature.

I think passing --write-tree all the time would be an annoyance.  I
don't see why anyone would ever use the other mode.  However, for as
long as both exist in the manual, it makes the manual easier to
explain to users, and example testcases more self-documenting by
having the flag there.  That's the sole purpose of the flag.

I'm never going to actually use it when I invoke it from the command
line.  And I suspect most would leave it off.

...also, even if we did require the `--write-tree` flag, we'd still
have to look at argc.  Since the option parsing handles both modes,
someone could leave off --write-tree, but include a bunch of other
options that only make sense with --write-tree.  Individually checking
the setting of every extra flag along with write_tree is a royal pain
and I don't want to repeat that for each new option added.  Simply
checking argc allows you to provide an error message if the user does
that.

(And I think it's sad that in Git we often forgot to warn and notify
users of options that are only functional with certain other
arguments; it makes it harder for users to figure out, and has in the
past even made it harder for other developers to figure out what was
meant and how things are to be used.  I think I've seen multiple Git
devs be confused over ls-files --directory and --no-empty-directory
options, assuming they'd do something sensible for tracked files, when
in fact those arguments are simply ignored because they are only
modifiers for how untracked files are treated.)

There's a much simpler way to do what you're trying to do here which is
to only parse --write-tree, and as soon as you have that pass off two
one function or the other, and have those functions call
parse_options().

This is how builtin/{bundle,stash,commit-graph}.c etc. solve the same
problem. It avoids having to the sort of check you did in 09/15:

	+	if (o.mode == 't' && original_argc < argc)
	+		die(_("--trivial-merge is incompatible with all other options"));

The builtin/submodule--helper.c then does it with a first argument that
--looks-like-an-option, but is really the same sort of sub-command
selector.

Which, at least for me brings this back to liking your merge-tree-ort
(or merge-tree-ng, merge-tree-new or whatever) better. I.e. both for the
code & manual we effectively have two completely different commands here
anyway. Splitting them up would make both the code simpler, and also
what we have to explain to users.