Re: [PATCH] merge-tree.c: add --merge-base=<commit> option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 27, 2022, 18:09 UTC
- Message-ID
- <xmqqczadm93w.fsf@gitster.g>
- In-Reply-To
- <pull.1397.git.1666872761355.gitgitgadget@gmail.com>
"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 11 quoted lines
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt > index d6c356740ef..e762209b76d 100644 > --- a/Documentation/git-merge-tree.txt > +++ b/Documentation/git-merge-tree.txt > @@ -64,6 +64,10 @@ OPTIONS > share no common history. This flag can be given to override that > check and make the merge proceed anyway. > > +--merge-base=<commit>:: > + Instead of finding the merge-bases for <branch1> and <branch2>, > + specify a merge-base for the merge.
I like adding and exposing this feature to allow the end-user specify which commit to use as the base (instead of allowing the tool compute it from the two branches), but I wonder if a new option is even needed.
In the original "trivial merge" mode, the command takes three trees without having to have this new option. In the new "write-tree" mode, currently it is incapable of taking the base, but it does not have to stay that way. Wouldn't it be sufficient to update the UI to
git merge-tree [--write-tree] [<options>] [<base-commit>] <branch1> <branch2>
git merge-tree [--trivial-merge] <base-commit> <branch1> <branch2>IOW, when you want to supply the base, you'd be explicit and ask for the new "write-tree" mode, i.e.
$ git merge-tree --write-tree $(git merge-base branch^ branch) HEAD branch
would be how you would use merge-tree to cherry-pick the commit at the tip of the branch on top of the current commit.
Show 5 quoted lines
> @@ -402,6 +403,7 @@ struct merge_tree_options {
> int allow_unrelated_histories;
> int show_messages;
> int name_only;
> + char* merge_base;Style. We write in C, not in C++, and our asterisks stick to variables and members of structs, not types.
Show 10 quoted lines
> - /*
> - * Get the merge bases, in reverse order; see comment above
> - * merge_incore_recursive in merge-ort.h
> - */
> - merge_bases = get_merge_bases(parent1, parent2);
> + if (o->merge_base) {
> + struct commit *c = lookup_commit_reference_by_name(o->merge_base);
> + if (!c)
> + die(_("could not lookup commit %s"), o->merge_base);
> + commit_list_insert(c, &merge_bases);Curious. The original code unconditionally assigned merge_bases, so there wasn't a good reason to initialize the variable before this point, but this new code assumes that merge_bases to be initialized to NULL.
Luckily, it is initialized in the original code, even though it wasn't necessary at all. So this new code can work correctly. Good.
Show 7 quoted lines
> + } else {
> + /*
> + * Get the merge bases, in reverse order; see comment above
> + * merge_incore_recursive in merge-ort.h
> + */
> + merge_bases = get_merge_bases(parent1, parent2);
> + }Yes, this feature was very much lacking and is a welcome addition.
I also have to wonder how this should interact with a topic that is in-flight to feed multiple merge-tree requests from the standard input to have a single process perform multiple (not necessarily related) merges. Elijah knows much better, but my gut feeling is that it shouldn't be hard to allow feeding an extra commit on the same line to be used as the base.
Thanks.