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

Miklos Vajna [off-list ref] writes:

On Tue, Aug 19, 2008 at 02:05:42AM -0700, Junio C Hamano [off-list ref] wrote:

quoted

[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?

On Tue, Aug 19, 2008 at 12:19:09PM -0700, Junio C Hamano [off-list ref] wrote:

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.

Sure, I can avoid _generic() and use merge_recursive() directly, that's
why I asked.

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?

Not exactly. builtin-merge-recursive uses get_ref() - which should not
be in merge-recursive.c IMHO - and get_ref() uses make_virtual_commit().
merge_recursive() itself takes commits, so it can be only static if we
copy it builtin-merge-recursive as well, causing a code duplication. Or
have I missed something here?

Thanks.