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@xxxxxxx>, PGP 0x6EDDD207FCC5040F
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
