On Thu, Feb 03 2022, Elijah Newren wrote: > On Thu, Feb 3, 2022 at 1:04 AM Elijah Newren <newren@xxxxxxxxx> wrote: >> >> On Wed, Feb 2, 2022 at 6:09 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: >> > >> > On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote: >> > >> > > From: Elijah Newren <newren@xxxxxxxxx> >> > > >> > > 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@xxxxxxxxxxxxxx/; >> > 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.
