Re: [PATCH] merge-tree: accept 3 trees as arguments
- From
Elijah Newren <newren@gmail.com>
- Date
- Jan 30, 2024, 07:04 UTC
- Message-ID
- <CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com>
- In-Reply-To
- <pull.1647.git.1706277694231.gitgitgadget@gmail.com>
On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 9 quoted lines
> > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > When specifying a merge base explicitly, there is actually no good > reason why the inputs need to be commits: that's only needed if the > merge base has to be deduced from the commit graph. > > This commit is best viewed with `--color-moved > --color-moved-ws=allow-indentation-change`.
Makes sense.
Show 27 quoted lines
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > merge-tree: accept 3 trees as arguments > > I was asked to implement this at $dayjob and it seems like a feature > that might be useful to other users, too. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1647%2Fdscho%2Fallow-merge-tree-to-accept-3-trees-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1647/dscho/allow-merge-tree-to-accept-3-trees-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1647 > > 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
Show 31 quoted lines
> Instead of finding the merge-bases for <branch1> and <branch2>,
> specify a merge-base for the merge, and specifying multiple bases is
> currently not supported. This option is incompatible with `--stdin`.
> ++
> +As the merge-base is provided directly, <branch1> and <branch2> do not need
> +o specify commits; it is sufficient if they specify trees.
>
> [[OUTPUT]]
> OUTPUT
> 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. They may discover how to show the contents of the tree objects (most users I run into seem to be unaware that hashes in git can refer to anything other than commits), but they certainly won't get any context from git-log as they might hope.
The other places in the code that deal directly with merging trees, git-am and git-merge-recursive, both provide specially overridden values for both branch1 and branch2. (They probably ought to do something with opt->ancestor as well, but that's been the single ugly edge case problem that is solely responsible for merge-recursive not having been fully replaced by merge-ort yet and I haven't wanted to go back and mess with it.)
That raises the question: if the user passes trees in, should we require helpful names be provided as additional parameters? 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?