Re: [PATCH] merge-tree: accept 3 trees as arguments

From: Elijah Newren <newren@gmail.com>
Date: Jan 31, 2024, 16:40 UTC
Message-ID: <CABPp-BF1p48hZ0Y7tRqKb8R53CjTB1oJGW_iV4dA29ALgbh8KQ@mail.gmail.com>
In-Reply-To: <557604aa-dea4-995a-4fb2-a71b515a8129@gmx.de>

Hi Johannes,

On Tue, Jan 30, 2024 at 12:04 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 8 quoted lines

>
> Hi Elijah,
>
> On Mon, 29 Jan 2024, Elijah Newren wrote:
>
> > On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >

[...]

> That's funny: I asked Victoria Dye to look over the patch, and she pointed
> out the exact opposite: I had written `<tree>` and she remarked that most
> of Git's manual pages would call this a `<tree-ish>` :-)

A code review isn't complete until you get two contradictory requests, I guess?

> On another funny note, I tried to establish the term "ent" for this category
> almost 222 months ago because I also disliked the "-ish" convention:
> https://lore.kernel.org/git/Pine.LNX.4.63.0508051655480.8418@wgmdd8.biozentrum.uni-wuerzburg.de/

:-)

Show 40 quoted lines

> > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > > index 3bdec53fbe5..cbd8e15af6d 100644
> > > --- a/builtin/merge-tree.c
> > > +++ b/builtin/merge-tree.c
> > > @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
> > >         struct merge_options opt;
> > >
> > >         copy_merge_options(&opt, &o->merge_options);
> > > -       parent1 = get_merge_parent(branch1);
> > > -       if (!parent1)
> > > -               help_unknown_ref(branch1, "merge-tree",
> > > -                                _("not something we can merge"));
> > > -
> > > -       parent2 = get_merge_parent(branch2);
> > > -       if (!parent2)
> > > -               help_unknown_ref(branch2, "merge-tree",
> > > -                                _("not something we can merge"));
> > > -
> > >         opt.show_rename_progress = 0;
> > >
> > >         opt.branch1 = branch1;
> > >         opt.branch2 = branch2;
> >
> > If branch1 and branch2 refer to trees, then when users hit conflicts
> > they'll see e.g.
> >
> > <<<<<<< aaaaaaa
> >   somecode();
> > =======
> >   othercode();
> > >>>>>>> bbbbbbb
> >
> > but aaaaaaa and bbbbbbb are not commits that they can find.
>
> That is true. And it is not totally obvious to many users that they could
> then call `git show aaaaaaa:file` to see the full pre-image on the
> first-parent side.
>
> On the other hand, the label that is shown is precisely what the user
> specified on the command-line.

So this is only for direct use? I was curious if the user was using some other tool of yours, perhaps even some web GUI, and thus that something else was controlling what was passed to git-merge-tree.

Show 23 quoted lines

> For example:
>
>         $ git merge-tree --merge-base=v2.42.0:t v2.43.0~11:t v2.43.0~10^2:t
>
> will result in the following conflict markers:
>
>         $ git show 021c3ce211:t0091-bugreport.sh
>         [...]
>         <<<<<<< v2.43.0~11:t
>                 grep usage output &&
>                 test_path_is_missing git-bugreport-*
>         '
>
>         test_expect_success 'incorrect positional arguments abort with usage and hint' '
>                 test_must_fail git bugreport false 2>output &&
>                 grep usage output &&
>                 grep false output &&
>         =======
>                 test_grep usage output &&
>         >>>>>>> v2.43.0~10^2:t
>         [...]
>
> which I personally find very pleasing output.

Oh, good point -- if users provide trees in the revision:path format then they still have access to more information about why the change was made via the revision.

Of course, if users are using the tool directly, presumably they have access to more information about where those trees came from anyway even without the conflict label.

Show 7 quoted lines

> Besides, the manual page of `git merge-tree` says in no sugar-coated
> words:
>
>         Do NOT look through the resulting toplevel tree to try to find which
>         files conflict; [...]
>
> :-)

Right, but this isn't using the tree to find which files conflict; they already are looking at the conflict. They are instead wanting to learn why certain textual changes were made on one side of history to better inform how to resolve an otherwise less than obvious conflict resolution. At least, that's the only thing I've seen those conflict labels be used for.