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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2008, 23:27 UTC
Message-ID: <7v7ianuo8q.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1218491096-28756-1-git-send-email-s-beyer@gmx.net>

Stephan Beyer <s-beyer@gmx.net> writes:

Show 15 quoted lines

> Cherry-pick and revert always ran the merging in a separate process.
> This patch makes cherry-pick/revert call merge_recursive() instead
> of running git-merge-recursive.
>
> To be able to cherry-pick/revert -n (without committing) on a dirty
> index, make_virtual_commit() is needed and thus declared non-static.
>
> Also the GITHEAD_* environment definitions are not needed anymore,
> since the names are direct arguments to make_virtual_commit() and
> merge_recursive().
>
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
> 	Hi,
> 	so I give it a new try.

Looks good from a cursory reading.

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.

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

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;

 (2) make revert.c use merge_recursive_helper();

 (3) make builtin-merge.c use merge_recursive_helper().