git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / 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>,
	Taylor Blau <me@ttaylorr.com>,
	Peter Baumann <peter.baumann@gmail.com>
Subject: Re: [PATCH v2 1/4] merge-ort: barebones API of new merge strategy with empty implementation
Date: Mon, 26 Oct 2020 13:45:24 -0700	[thread overview]
Message-ID: <xmqqa6w8emxn.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <b9e73975eab1f349be678779ff57155feb4c3501.1603731448.git.gitgitgadget@gmail.com> (Elijah Newren via GitGitGadget's message of "Mon, 26 Oct 2020 16:57:25 +0000")

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

> + *   git merge [-s recursive]
> + *
> + * with
> + *
> + *   git merge -s ort
> + *
> + * Note: git's parser allows the space between '-s' and its argument to be
> + * missing.  (Should I have backronymed "ham", "alsa", "kip", "nap, "alvo",
> + * "cale", "peedy", or "ins" instead of "ort"?)

One thing that is quite unpleasant is "git grep ort" gives us too
many hits already, and it will be hard to locate ort related changes
with "git log --grep=ort", as the name is too short to serve as an
effective way to limit the search.

> diff --git a/merge-ort.h b/merge-ort.h
> new file mode 100644
> index 0000000000..47d30cf538
> --- /dev/null
> +++ b/merge-ort.h
> @@ -0,0 +1,49 @@
> +#ifndef MERGE_ORT_H
> +#define MERGE_ORT_H
> +
> +#include "merge-recursive.h"
> +
> +struct commit;
> +struct tree;
> +
> +struct merge_result {
> +	/* whether the merge is clean */
> +	int clean;
> +
> +	/* Result of merge.  If !clean, represents what would go in worktree */
> +	struct tree *tree;

Curious.  Because there is no way for "struct tree" to hold an
in-core pointer to a "struct blob" (iow, for a blob to be in a
"struct tree", it has to have been assigned an object name), unless
we are using the "pretend" mechanism, which has its own downsides,
we are committed to create a throw-away blob objects with conflict
markers in them, and write them to the object store.

If we were writing a new merge machinery from scratch, I would have
preferred a truly in-core implementation that does not have to write
out to the object store but if this makes the implementation simpler,
perhaps it is a small enough price to pay.

> +	/*
> +	 * Additional metadata used by merge_switch_to_result() or future calls
> +	 * to merge_inmemory_*().  Not for external use.
> +	 */
> +	void *priv;
> +	unsigned ate;

I'd prefer to see this named not so cute.  Will we hang random
variations of things, or would this be better to be made into a
pointer to union, with an enum that tells us which kind it is in
use?

> +};


> +/* rename-detecting three-way merge with recursive ancestor consolidation. */
> +void merge_inmemory_recursive(struct merge_options *opt,
> +			      struct commit_list *merge_bases,
> +			      struct commit *side1,
> +			      struct commit *side2,
> +			      struct merge_result *result);

I've seen "incore" spelled as a squashed-into-a-single-word, but not
"in_memory".

> +/* rename-detecting three-way merge, no recursion. */
> +void merge_inmemory_nonrecursive(struct merge_options *opt,
> +				 struct tree *merge_base,
> +				 struct tree *side1,
> +				 struct tree *side2,
> +				 struct merge_result *result);
> +
> +/* Update the working tree and index from head to result after inmemory merge */
> +void merge_switch_to_result(struct merge_options *opt,
> +			    struct tree *head,
> +			    struct merge_result *result,
> +			    int update_worktree_and_index,
> +			    int display_update_msgs);

To those who have known how our merge works, a natural expectation
for an "in-core" merge is that when the "in-core" merge finishes,
the index would hold the higher stages for the conflicted paths, and
cleanly merged paths would have the result at stage 0, and there is
an extra thing that we haven't had that represents what the working
tree files for conflicted paths should look like (historically we
wrote out the conflicted result to the working tree files---being
in-core operation we cannot afford to), so that (1) cleanly merged
paths can be externalized by writing from their stage 0 entries and
(2) contents with conflicts can be externalized by that "extra
thing".

But this helper says "working tree and index" are both updated, so
the "in-core" merge it expects must have not just the working tree
result (in result->tree, as the comment in the structure says) but
also how the higher stages of the index should look like somewhere
in the result structure.  How the latter is done is not at all clear
at this point in the mock-up.  Leaving it opaque is fine, but the
function, and the result structure, deserve clarification to avoid
confusing readers by highlighting how it is different from the
traditional ways (e.g. "we don't touch the index at all---instead we
store that in the priv/ate fields", if that is what is going on).

Thanks.

  reply	other threads:[~2020-10-26 20:45 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-21 13:22 [PATCH 0/4] Beginning of new merge strategy: New API, empty implementation Elijah Newren via GitGitGadget
2020-10-21 13:22 ` [PATCH 1/4] merge-ort: barebones API of new merge strategy with " Elijah Newren via GitGitGadget
2020-10-23 18:12   ` Taylor Blau
2020-10-23 19:02     ` Elijah Newren
2020-10-24 10:46   ` Peter Baumann
     [not found]   ` <CAJm9OHczJJyn=Oq2RBGvTit4hedqs6vaYH1gto-z6emo6=n2dw@mail.gmail.com>
     [not found]     ` <CAJm9OHdfxh8SGdteD48eDCA=ihGZmKJD-E67PFhCdFR63RSSTA@mail.gmail.com>
2020-10-24 14:54       ` Elijah Newren
2020-10-21 13:22 ` [PATCH 2/4] merge-ort-wrappers: new convience wrappers to mimic the old merge API Elijah Newren via GitGitGadget
2020-10-21 13:22 ` [PATCH 3/4] fast-rebase: demonstrate merge-ort's API via temporary/hidden command Elijah Newren via GitGitGadget
2020-10-21 13:22 ` [PATCH 4/4] merge,rebase,revert: select ort or recursive by config or environment Elijah Newren via GitGitGadget
2020-10-22  0:16 ` [PATCH 0/4] Beginning of new merge strategy: New API, empty implementation Elijah Newren
2020-10-26 21:56   ` Jonathan Tan
2020-10-27  0:52     ` Elijah Newren
2020-10-26 16:57 ` [PATCH v2 " Elijah Newren via GitGitGadget
2020-10-26 16:57   ` [PATCH v2 1/4] merge-ort: barebones API of new merge strategy with " Elijah Newren via GitGitGadget
2020-10-26 20:45     ` Junio C Hamano [this message]
2020-10-26 21:18       ` Elijah Newren
2020-10-26 22:10         ` Junio C Hamano
2020-10-26 22:28           ` Elijah Newren
2020-10-26 16:57   ` [PATCH v2 2/4] merge-ort-wrappers: new convience wrappers to mimic the old merge API Elijah Newren via GitGitGadget
2020-10-26 16:57   ` [PATCH v2 3/4] fast-rebase: demonstrate merge-ort's API via temporary/hidden command Elijah Newren via GitGitGadget
2020-10-26 16:57   ` [PATCH v2 4/4] merge,rebase,revert: select ort or recursive by config or environment Elijah Newren via GitGitGadget
2020-10-27  2:08   ` [PATCH v3 0/4] Beginning of new merge strategy: New API, empty implementation Elijah Newren via GitGitGadget
2020-10-27  2:08     ` [PATCH v3 1/4] merge-ort: barebones API of new merge strategy with " Elijah Newren via GitGitGadget
2020-10-27  2:33       ` Eric Sunshine
2020-10-27  4:57         ` Elijah Newren
2020-10-27  7:54           ` Eric Sunshine
2020-10-27  2:08     ` [PATCH v3 2/4] merge-ort-wrappers: new convience wrappers to mimic the old merge API Elijah Newren via GitGitGadget
2020-10-27  2:08     ` [PATCH v3 3/4] fast-rebase: demonstrate merge-ort's API via temporary/hidden command Elijah Newren via GitGitGadget
2020-10-27  9:47       ` SZEDER Gábor
2020-10-27  2:08     ` [PATCH v3 4/4] merge,rebase,revert: select ort or recursive by config or environment Elijah Newren via GitGitGadget
2020-10-29 20:32     ` [PATCH v4 0/4] Beginning of new merge strategy: New API, empty implementation Elijah Newren via GitGitGadget
2020-10-29 20:32       ` [PATCH v4 1/4] merge-ort: barebones API of new merge strategy with " Elijah Newren via GitGitGadget
2020-10-29 20:32       ` [PATCH v4 2/4] merge-ort-wrappers: new convience wrappers to mimic the old merge API Elijah Newren via GitGitGadget
2020-10-29 20:32       ` [PATCH v4 3/4] fast-rebase: demonstrate merge-ort's API via new test-tool command Elijah Newren via GitGitGadget
2020-10-29 20:32       ` [PATCH v4 4/4] merge,rebase,revert: select ort or recursive by config or environment Elijah Newren via GitGitGadget
2020-11-02  9:27       ` [PATCH v4 0/4] Beginning of new merge strategy: New API, empty implementation Jacob Keller
2020-11-02 18:52         ` Elijah Newren
2020-11-07  6:09           ` Elijah Newren
2020-11-02 23:45       ` [PATCH v5 " Elijah Newren via GitGitGadget
2020-11-02 23:45         ` [PATCH v5 1/4] merge-ort: barebones API of new merge strategy with " Elijah Newren via GitGitGadget
2020-11-02 23:45         ` [PATCH v5 2/4] merge-ort-wrappers: new convience wrappers to mimic the old merge API Elijah Newren via GitGitGadget
2020-11-02 23:45         ` [PATCH v5 3/4] fast-rebase: demonstrate merge-ort's API via new test-tool command Elijah Newren via GitGitGadget
2020-11-02 23:45         ` [PATCH v5 4/4] merge,rebase,revert: select ort or recursive by config or environment Elijah Newren via GitGitGadget
2020-11-03  1:03         ` [PATCH v5 0/4] Beginning of new merge strategy: New API, empty implementation 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=xmqqa6w8emxn.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    --cc=peter.baumann@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).