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

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 [off-list ref], PGP 0x6EDDD207FCC5040F