Subject: Re: [PATCH v3 03/15] merge-tree: add option parsing and initial shell for real merge function

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 03 Feb 2022 10:45:56 +0100

On Thu, Feb 03 2022, Elijah Newren wrote:

> On Thu, Feb 3, 2022 at 1:04 AM Elijah Newren <[email-masked]> wrote:
>>
>> On Wed, Feb 2, 2022 at 6:09 PM Ævar Arnfjörð Bjarmason <[email-masked]> wrote:
>> >
>> > On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
>> >
>> > > From: Elijah Newren <[email-masked]>
>> > >
>> > > 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=[email-masked]/;
>> > 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?
>>
>> > But maybe that's all water under the bridge etc, however...
>> >
>> > >  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.