git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v2 3/3] merge-ort: implement merge_incore_recursive()
Date: Wed, 16 Dec 2020 10:09:22 -0800	[thread overview]
Message-ID: <xmqqeejp4o8d.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <d8f79450a40e5a91b401ccbbedc7326cfe8a33d6.1608097965.git.gitgitgadget@gmail.com> (Elijah Newren via GitGitGadget's message of "Wed, 16 Dec 2020 05:52:44 +0000")

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	/*
> +	 * merge_incore_nonrecursive() exists for cases where we always
> +	 * know there is a well-defined single merge base.  However,
> +	 * despite a similar structure, merge-recursive.c noted that some
> +	 * callers preferred to call the recursive logic anyway and still
> +	 * set a special name for opt->ancestor that would appear in
> +	 * merge.conflictStyle=diff3 output.

Sorry, I do not understand the comment, especially the "anyway"
part.  There is no such thing as nonrecursive variant of
merge-recursive, is it?  If somebody wanted to perform a merge of
two trees with a designated single common ancestor ("am -3" would
want to do so using a fabricated tree, "cherry-pick" would want to
do so using the parent commit of what gets picked), it would be
natural to call "git merge-recursive O -- A B" if it is a scripted
Porcelain, or would call merge_recursive() with the single merge
base on the merge_bases commit_list parameter if it is written in C,
I would think.

> +	 * git-am was one such example (it wanted to set opt->ancestor to
> +	 * "constructed merge base", since it created a fake merge base);
> +	 * it called the recursive merge logic through a special
> +	 * merge_recursive_generic() wrapper.
> +	 *
> +	 * Allow the same kind of special case here.
> +	 */

In any case, the mention of "constructed merge base" may explain
very well why the assert in the previous iteration checked for the
string, but it does not seem relevant after the condition changed.

> +	int num_merge_bases_is_1 = (merge_bases && !merge_bases->next);
> +	assert(opt->ancestor == NULL || num_merge_bases_is_1);

The above comment may have explained why some callers that call the
machinery with a single merge base want to use its own diff3 label,
but the assert the comment applies to is stricter than that.

In other words, it is unclear why the caller is forbidden from
giving the diff3 label, when the recursive merge needs to synthesize
the virtual ancestor using all the given merge bases?

The answer could be a simple "opt->ancestor field is managed by the
recursive machinery itself when it needs to create virtual ancestor,
and must start as NULL, but when we do not create virtual ancestor,
it is allowed to start with any value, as the machinery itself does
not assign any new value to the field", but I cannot read if that is
the case from the comments in the patch.

> +
> +	merge_start(opt, result);
> +	merge_ort_internal(opt, merge_bases, side1, side2, result);
>  }
> diff --git a/merge-ort.h b/merge-ort.h
> index 55ae7ee865d..d53a0a339f3 100644
> --- a/merge-ort.h
> +++ b/merge-ort.h
> @@ -34,6 +34,16 @@ struct merge_result {
>  /*
>   * rename-detecting three-way merge with recursive ancestor consolidation.
>   * working tree and index are untouched.
> + *
> + * merge_bases will be consumed (emptied) so make a copy if you need it.
> + *
> + * NOTE: empirically, the recursive algorithm will perform better if you
> + *       pass the merge_bases in the order of oldest commit to the
> + *       newest[1][2].
> + *
> + *       [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1907252055500.21907@tvgsbejvaqbjf.bet/
> + *       [2] commit 8918b0c9c2 ("merge-recur: try to merge older merge bases
> + *           first", 2006-08-09)
>   */

I initially thought that this was a bit out-of-place for the comment
that explains why the merge bases list gets reversed in the code, but
it is actually the right place---it guides the callers that hand a
list of merge_bases to the function.

>  void merge_incore_recursive(struct merge_options *opt,
>  			    struct commit_list *merge_bases,

Thanks.

  reply	other threads:[~2020-12-16 18:10 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-15 17:53 [PATCH 0/3] merge-ort: implement recursive merges Elijah Newren via GitGitGadget
2020-12-15 17:53 ` [PATCH 1/3] merge-ort: copy a few small helper functions from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-16  1:16   ` Junio C Hamano
2020-12-16 16:12     ` Johannes Schindelin
2020-12-16 16:24       ` Elijah Newren
2020-12-16 13:30   ` Derrick Stolee
2020-12-16 17:43     ` Junio C Hamano
2020-12-16 18:54       ` Felipe Contreras
2020-12-16 19:20       ` Elijah Newren
2020-12-16 20:41         ` Junio C Hamano
2020-12-16 21:25           ` Felipe Contreras
2020-12-16 21:34           ` Elijah Newren
2020-12-15 17:53 ` [PATCH 2/3] merge-ort: make clear_internal_opts() aware of partial clearing Elijah Newren via GitGitGadget
2020-12-15 17:53 ` [PATCH 3/3] merge-ort: implement merge_incore_recursive() Elijah Newren via GitGitGadget
2020-12-16  2:07   ` Junio C Hamano
2020-12-16  4:09     ` Elijah Newren
2020-12-16  4:44       ` Elijah Newren
2020-12-16  5:52 ` [PATCH v2 0/3] merge-ort: implement recursive merges Elijah Newren via GitGitGadget
2020-12-16  5:52   ` [PATCH v2 1/3] merge-ort: copy a few small helper functions from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-16  5:52   ` [PATCH v2 2/3] merge-ort: make clear_internal_opts() aware of partial clearing Elijah Newren via GitGitGadget
2020-12-16  5:52   ` [PATCH v2 3/3] merge-ort: implement merge_incore_recursive() Elijah Newren via GitGitGadget
2020-12-16 18:09     ` Junio C Hamano [this message]
2020-12-16 18:37       ` Elijah Newren
2020-12-16 17:17   ` [PATCH v3 0/3] merge-ort: implement recursive merges Elijah Newren via GitGitGadget
2020-12-16 17:17     ` [PATCH v3 1/3] merge-ort: copy a few small helper functions from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-16 17:17     ` [PATCH v3 2/3] merge-ort: make clear_internal_opts() aware of partial clearing Elijah Newren via GitGitGadget
2020-12-16 17:17     ` [PATCH v3 3/3] merge-ort: implement merge_incore_recursive() Elijah Newren via GitGitGadget
2020-12-16 20:35     ` [PATCH v4 0/3] merge-ort: implement recursive merges Elijah Newren via GitGitGadget
2020-12-16 20:35       ` [PATCH v4 1/3] merge-ort: copy a few small helper functions from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-16 20:35       ` [PATCH v4 2/3] merge-ort: make clear_internal_opts() aware of partial clearing Elijah Newren via GitGitGadget
2020-12-16 20:35       ` [PATCH v4 3/3] merge-ort: implement merge_incore_recursive() Elijah Newren via GitGitGadget
2020-12-16 22:27       ` [PATCH v5 0/4] merge-ort: implement recursive merges Elijah Newren via GitGitGadget
2020-12-16 22:27         ` [PATCH v5 1/4] commit: move reverse_commit_list() from merge-recursive Elijah Newren via GitGitGadget
2020-12-17 14:03           ` Derrick Stolee
2020-12-16 22:28         ` [PATCH v5 2/4] merge-ort: copy a few small helper functions from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-16 22:28         ` [PATCH v5 3/4] merge-ort: make clear_internal_opts() aware of partial clearing Elijah Newren via GitGitGadget
2020-12-16 22:28         ` [PATCH v5 4/4] merge-ort: implement merge_incore_recursive() Elijah Newren via GitGitGadget

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

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

  git send-email \
    --in-reply-to=xmqqeejp4o8d.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.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.
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 NNTP newsgroup(s).