Re: [PATCH] merge-tree.c: add --merge-base=<commit> option

[Junio C Hamano <gitster@pobox.com>]

"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> @@ -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.

> -	/*
> -	 * 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.

> +	} 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.