Re: What's cooking in git.git (Aug 2008, #05; Tue, 19)

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2008, 19:19 UTC
Message-ID: <7vabf83j9u.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080819125429.GD17582@genesis.frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 15 quoted lines

> On Tue, Aug 19, 2008 at 02:05:42AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> [On Hold]
>> (...)
>> * mv/merge-recursive (Tue Aug 12 22:14:00 2008 +0200) 3 commits
>>  - Make builtin-revert.c use merge_recursive_generic()
>>  - merge-recursive.c: Add more generic merge_recursive_generic()
>>  - Split out merge_recursive() to merge-recursive.c
>> 
>> I do not think builtlin-revert should use "recursive", but these patches
>> give a good starting point to separate the bulk of the "rename-aware
>> three-way merge" into library form.
>
> I wanted to send a patch that makes builtin-merge use the new
> merge_recursive_setup(), but then I was not able to decide to use
> merge_recursive_generic() or not.

I think git-merge and git-merge-recursive should be the only two that actually trigger the "recursive" behaviour. Everybody else should be using non-recursive one, and that non-recursive one can be shared by the one that is recursive.

Here is how the callchain looks like with your variant.

 cmd_merge_recursive()
 -> merge_recursive_setup()
 -> merge_recursive_generic()
    -> merge_recursive()
       -> merge_recursive()
       -> merge_trees()

The merge_recursive() is the "recursive" one. The workhorse that is not recursive is merge_trees().

Since the latter is what everybody else ("checkout -m", "revert", "cherry-pick", "am -3", "stash apply") should be using, I think it is pretty much up to "git-merge" and "git-merge-recursive" implementations how the caller of merge_recursive() function is structured. I suspect that you would not need two separate functions, _setup() and _generic(), for these two codepaths, but I didn't look closely.

And make_virtual_commit() should become static inside merge_recursive.c; use of these fake commits is strictly an internal implementation issue of how merge_recursive() function works and does not concern the caller, does it?

By the way, the calling convention of merge_recursive_generic() looks confusing (even though by the above reasoning it does not matter very much outside "git-merge" and "git-merge-recursive"). Why does it take textual object names for bases but binary object names for head and next?