Re: [PATCH] builtin-revert.c: Make use of merge_recursive()

Hi,

Johannes Schindelin wrote:

Hi,

On Mon, 11 Aug 2008, Stephan Beyer wrote:

quoted

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 09aa830..d8bd21f 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch)
 	return name ? name : branch;
 }
 
-static struct commit *get_ref(const char *ref)
+struct commit *get_ref(const char *ref)

The name get_ref() is way too generic to be non-static.

That's right.

But I have a hunch that peel_to_type() does a lot of what we want here,
if not all of it.

get_ref() has a big advantage over peel_to_type(): it can handle trees,
via "virtual commits" (make_virtual_commit()).
If you wonder where we need to handle trees on cherry-pick/revert:
With the -n (no commit) option you are allowed to have a dirty index.
So the recursive merge is not done using the HEAD commit but using the
uncommitted tree of the index.

Well, a good alternative could be to just make the really cool
make_virtual_commit() function non-static.
The name could be generic enough. Is it? :-)
Or perhaps: make_virtual_commit_from_tree().

Btw I also need get_ref() (or make_virtual_commit()) for threeway
fallback of the sequencer "patch -3" instruction ("git am -3"). ;)

quoted

+	h1 = get_ref(head_sha1);
+	h2 = get_ref(next_sha1);
+
+	index_fd = hold_locked_index(lock, 1);
+	clean = merge_recursive(h1, h2, head_name, next_name, ca, &result);

h1 and h2 are not expressive.  head_commit and next_commit would be.

This is also quite true.
Those names, also "ca", were taken from cmd_merge_recursive().
(This is no excuse, just an explanation.)

Rest looks good to me -- even if I had to spend too much time (therefore 
being not really thorough in the end) verifying that merge_recursive() 
does not lock the index itself,

I can't help here.  Miklos has the same change in patch v2/2 and I
wonder if you really expect that I don't test my patches, because
a double lock wouldn't have worked.

and that GITHEAD_* definitions are not necessary anymore, since merge_recursive()
takes the arguments directly;

Ok, I hoped that would've been clear by using head_name/next_name
directly in the merge_recursive() arguments, but nevertheless
thanks for your comment, ...because: using get_ref() the GITHEAD_*
definitions *are* still needed, because it takes the GITHEAD_*
name for the virtual commits...

Under this additional circumstance, I really tend to make
make_virtual_commit() non-static.

Kind regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F