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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 30, 2024, 17:15 UTC
Message-ID: <xmqqbk92bv43.fsf@gitster.g>
In-Reply-To: <CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 7 quoted lines

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

You are right that the "X-ish" was invented exactly so that folks (including the nitpicky types among us) can tell if only "X" is accepted, or other types of objects that uniquely can be peeled to a "X" also can be used. Many commands at the Porcelain level may take a commit or a tag that eventually peel to a tree where at the lowest level we only need it to be a tree, but it still matters in some lower level corners of the system where the plumbing commands that only accept a tree but not a tree-ish exist. We've discussed this in the past, e.g.:

    https://lore.kernel.org/git/7voc8ihq4a.fsf@alter.siamese.dyndns.org/

In general, I am OK to update the documentation and usage strings to drop the "-ish" suffix when it is clear from the context. But what does it exactly mean that "--merge-base=<tree>" is meant to take any tree-ish from the context of this documentation we are discussing?

 - Is it because merge-tree is a Porcelain command and we would
   adopt "Porcelains are to peel liberally what they accept, and
   when they are documented to take X they always accept any X-ish"
   rule?

 - Is it because the description that follows "--merge-base=<tree>"
   header does not mention "contrary to our usual convention, this
   option only accepts a tree and not a commit or a tag that points
   at a tree" and we would adopt "all commands and options that are
   documented to take X take X-ish, unless explicitly documented
   they only take X"?

As long as we clearly spell out such a ground rule and make sure readers of the documentation understand it, I will not object to such a tree-wide clean-up. The current ground rule is "We write X when we mean to take only X and we write X-ish otherwise", if I am not mistaken.

Show 13 quoted lines

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

Correct. They are what they fed as two trees to be merged, aren't they? They (or the script that drives merge-tree and fed these two trees) should be recognise these two trees, as long as they are told that is what we show, no?

> That raises the question: if the user passes trees in, should we
> require helpful names be provided as additional parameters?

If the user wants to give human-readable name to override whatever the code would internally produce, such new options may help them, and should probably apply equally to input that happens to be a commit (or a tag that is a tree-ish) as well, I would think.

> Or are
> the usecases such that we don't expect callers to have any useful
> information about where these trees come from and these suboptimal
> conflicts are the best we can do?

I do not necessarily think the output is "suboptimal" from the point of view of our users, exactly because that is what they fed into the command themselves.