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

[Stephan Beyer <s-beyer@gmx.net>]

Hi,

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 11 Aug 2008, Stephan Beyer wrote:
> 
> > 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"). ;)

> > +	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 <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F