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

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

Hi,

Junio C Hamano wrote:
> I am not absolutely sure if the phoney commit-looking object that has
> nonsense SHA-1 created by make_virtual_commit() would have any unintended
> side effects to the rest of the system, but it does not look like it is
> even inserted into the global object hash table, so this should be Ok.

Hmm, the git-merge-recursive process that was run before, calls get_ref()
and that also used make_virtual_commit() and thus this virtual_id.
So if there is some danger regarding this, then it has also been there
before my patch :-)

> That was the last piece of worry coming from me regarding this "call
> recursive internally" theme.

Well, for simple cherry-pick/revert your other worries can be ignored,
because it only runs merge_recursive() _once_ and the die()s do not
hurt.  Wrong?

> Would we need to consolidate this and Miklos's "call recursive internally
> from git-merge wrapper" by making them into three patches?
> I.e.
> 
>  (1) move bulk of code from builtin-merge-recursive.c to a new file
>      merge-recursive.c and introduce merge_recursive_helper() in there so
>      that both of you and cmd_merge_recursive() itself can call it;

I'd like to see a "libified" merge-recursive.c, but I wouldn't call the
interesting function merge_recursive_helper(), I'd just take
merge_recursive().
Of course those index locking could be done in it.

Looking at my sequencer code, I'd also be satisfied, if it takes SHAs
as argument and no "struct commit *".
But then this should be more generic, i.e. OBJ_TAG has to be handled
correctly (builtin-revert does that at the beginning at parse-args()).

Hmm, then step (1) is ok. :-)

>  (2) make revert.c use merge_recursive_helper();
> 
>  (3) make builtin-merge.c use merge_recursive_helper().

This is ok then.

Thanks and regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F