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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2024, 20:04 UTC
Message-ID: <557604aa-dea4-995a-4fb2-a71b515a8129@gmx.de>
In-Reply-To: <CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com>

Hi Elijah,

On Mon, 29 Jan 2024, Elijah Newren wrote:

Show 23 quoted lines

> On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> >  Documentation/git-merge-tree.txt |  5 +++-
> >  builtin/merge-tree.c             | 42 +++++++++++++++++++-------------
> >  t/t4301-merge-tree-write-tree.sh |  8 ++++++
> >  3 files changed, 37 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > index b50acace3bc..214e30c70ba 100644
> > --- a/Documentation/git-merge-tree.txt
> > +++ b/Documentation/git-merge-tree.txt
> > @@ -64,10 +64,13 @@ OPTIONS
> >         share no common history.  This flag can be given to override that
> >         check and make the merge proceed anyway.
> >
> > ---merge-base=<commit>::
> > +--merge-base=<tree-ish>::
>
> A very minor point, but any chance we can just use `<tree>`, like
> git-restore does?  I've never liked the '-ish' that we use as it seems
> to throw users, and I think they understand that they can use a commit
> or a tag where a tree is expected

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>` :-)

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 33 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. 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.

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; [...]

:-)

Ciao, Johannes