git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "kylezhao(赵柯宇)" <kylezhao@tencent.com>
Cc: "Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	"Elijah Newren" <newren@gmail.com>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [Internet]Re: [PATCH v7 1/2] merge-tree.c: add --merge-base=<commit> option
Date: Tue, 22 Nov 2022 14:42:10 +0900	[thread overview]
Message-ID: <xmqqr0xvr1e5.fsf@gitster.g> (raw)
In-Reply-To: <xmqqcz9fsjdr.fsf@gitster.g> (Junio C. Hamano's message of "Tue, 22 Nov 2022 13:28:16 +0900")

Junio C Hamano <gitster@pobox.com> writes:

> "kylezhao(赵柯宇)" <kylezhao@tencent.com> writes:
>
>> I went to check the api-parse-option.txt, but I didn't found an
>> elegant solution to stop when the users uses the second
>> "--merge-base".
>
> That's not even a fix, as it does not allow specifying more than one
> merge bases, is it?
>
> Just like how builtin/merge-tree.c::real_merge() is prepared to
> handle parent1 and parent2 that have multiple merge bases and pass
> all of them to the underlying merge machinery, you can accumulate
> multiple strings using OPT_STRING_LIST() and use all of them as
> merge_bases, no?

I am a bit torn on this, though.  

Because it uses lookup_commit_reference_by_name() to obtain
base_commit in addition to parent1 and parent2, and then
get_commit_tree() on them to get their trees, the real_merge()
function in the posted patch is incapable of accepting a single
"pretend as if this tree object is the common ancestor tree" and
merging the two tree objects.  But once that flaw is fixed, using
merge_incore_nonrecursive() with an explicitly given "base", we can
merge totally trees regardless of how the commits they are found in
are related in the history, which is a very logical thing to do.
And while operating in that mode, there is no way to accept more
than one "base".

So, I would be PERFECTLY HAPPY if this new mode of operation can
take only one "base" tree object, if it allows BASE, PARENT1,
and PARENT2 to be all tree objects, not necessarily commit objects.

But if we insist that PARENT1 and PARENT2 must be commits, then I
would find it very unsatisfying if we only took a single BASE that
also must be a commit.  If the merge-base has to be a tree or trees,
then there is no way to perform recursive merge (because you cannot
compute common ancestors across tree objects) , so it is perfectly
justifiable to take only a single base (and error out upon seeing a
second --merge-base=X option).

But it has to be a commit, then there is no justification to forbid
recursive operation, and I do not see a good reason to take only one
COMMON thing.

So, it is easy to say "let's take the current patch as a good first
step", but it is unclear what the good second step would be.

We could correct the code to allow PARENT1, PARENT2 and BASE to be
tree objects, not necessarily commits, when only a single merge-base
is specified.  That corrects the current flaw that tree objects
cannot be used.  And then when more than one BASE is given (or no
BASE is given---which is the original code), we could correct the
code to call merge_incore_recursive() instead.

But the amount of the effort to get there from the current codebase
(without your patch) feels more or less the same ballpark as the
patch in question, so...

  reply	other threads:[~2022-11-22  5:42 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-27 12:12 [PATCH] merge-tree.c: add --merge-base=<commit> option Kyle Zhao via GitGitGadget
2022-10-27 18:09 ` Junio C Hamano
2022-10-28  8:20   ` Elijah Newren
2022-10-28 16:03     ` Junio C Hamano
2022-10-29  1:52       ` Elijah Newren
2022-10-28 11:43   ` [Internet]Re: " kylezhao(赵柯宇)
2022-10-28  8:13 ` Elijah Newren
2022-10-28 11:54   ` [Internet]Re: " kylezhao(赵柯宇)
2022-10-28 11:55 ` [PATCH v2] " Kyle Zhao via GitGitGadget
2022-10-29  1:32   ` Elijah Newren
2022-10-29  3:42   ` [PATCH v3] " Kyle Zhao via GitGitGadget
2022-11-01  1:08     ` Elijah Newren
2022-11-01  8:55     ` [PATCH v4 0/2] merge-tree: allow specifying a base commit when --write-tree is passed Kyle Zhao via GitGitGadget
2022-11-01  8:55       ` [PATCH v4 1/2] merge-tree.c: add --merge-base=<commit> option Kyle Zhao via GitGitGadget
2022-11-01  8:55       ` [PATCH v4 2/2] merge-tree.c: support --merge-base in conjunction with --stdin Kyle Zhao via GitGitGadget
2022-11-03  1:13         ` Elijah Newren
2022-11-03  3:28           ` [Internet]Re: " kylezhao(赵柯宇)
2022-11-01 21:19       ` [PATCH v4 0/2] merge-tree: allow specifying a base commit when --write-tree is passed Taylor Blau
2022-11-03  3:29       ` [PATCH v5 " Kyle Zhao via GitGitGadget
2022-11-03  3:29         ` [PATCH v5 1/2] merge-tree.c: add --merge-base=<commit> option Kyle Zhao via GitGitGadget
2022-11-03  8:36           ` Ævar Arnfjörð Bjarmason
2022-11-03  9:43             ` [Internet]Re: " kylezhao(赵柯宇)
2022-11-03  3:29         ` [PATCH v5 2/2] merge-tree.c: allow specifying the merge-base when --stdin is passed Kyle Zhao via GitGitGadget
2022-11-03 10:50         ` [PATCH v6 0/2] merge-tree: allow specifying a base commit when --write-tree " Kyle Zhao via GitGitGadget
2022-11-03 10:50           ` [PATCH v6 1/2] merge-tree.c: add --merge-base=<commit> option Kyle Zhao via GitGitGadget
2022-11-03 10:50           ` [PATCH v6 2/2] merge-tree.c: allow specifying the merge-base when --stdin is passed Kyle Zhao via GitGitGadget
2022-11-11 19:44             ` Elijah Newren
2022-11-11 23:45           ` [PATCH v7 0/2] merge-tree: allow specifying a base commit when --write-tree " Kyle Zhao via GitGitGadget
2022-11-11 23:45             ` [PATCH v7 1/2] merge-tree.c: add --merge-base=<commit> option Kyle Zhao via GitGitGadget
2022-11-22  3:04               ` Junio C Hamano
2022-11-22  3:52                 ` [Internet]Re: " kylezhao(赵柯宇)
2022-11-22  4:28                   ` Junio C Hamano
2022-11-22  5:42                     ` Junio C Hamano [this message]
2022-11-22  7:47                       ` [Internet]Re: " kylezhao(赵柯宇)
2022-11-11 23:45             ` [PATCH v7 2/2] merge-tree.c: allow specifying the merge-base when --stdin is passed Kyle Zhao via GitGitGadget
2022-11-12  0:32             ` [PATCH v7 0/2] merge-tree: allow specifying a base commit when --write-tree " Elijah Newren
2022-11-13  4:53             ` Taylor Blau
2022-11-13  4:54               ` Taylor Blau
2022-11-24  3:37             ` [PATCH v8 0/3] " Kyle Zhao via GitGitGadget
2022-11-24  3:37               ` [PATCH v8 1/3] merge-tree.c: add --merge-base=<commit> option Kyle Zhao via GitGitGadget
2022-11-24  3:37               ` [PATCH v8 2/3] merge-tree.c: allow specifying the merge-base when --stdin is passed Kyle Zhao via GitGitGadget
2022-11-24  3:37               ` [PATCH v8 3/3] docs: fix description of the `--merge-base` option Kyle Zhao via GitGitGadget
2022-11-25  5:28                 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr0xvr1e5.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=kylezhao@tencent.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).