Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive

From: Miklos Vajna <vmiklos@frugalware.org>
Date: Aug 11, 2008, 19:07 UTC
Message-ID: <20080811190705.GQ18960@genesis.frugalware.org>
In-Reply-To: <7vhc9rxuc4.fsf@gitster.siamese.dyndns.org>

On Mon, Aug 11, 2008 at 11:47:07AM -0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> Yes, it saves a fork, but is this really worth it in the bigger picture?
> 
> Doesn't the current code structure have benefit of allowing git-merge
> itself do necessary clean-up action when merge-recursive calls any of the
> die() it has in many places?

As far as I see in most cases merge-recursive does not call die().

Cases when it does are like:

- broken snprintf
- cache_tree_fully_valid() or cache_tree_update() fails
- diff_setup_done() fails
- flush_buffer() fails
- read_sha1_file() fails
- ll_merge() fails

etc.

In short, I think there are two cases when a die() would be problematic inside merge-recursive when calling it from builtin-merge:

- merge-recursive can't handle a merge, but an other strategy could do.
  This is the case when doing an octopus merge but in that case
  merge_recursive() is not called at all.

- merge-recursive results in conflicts, but an other strategy could
  handle the merge without conflicts. In this case die() isn't used
  either, so this will not be a problem.

So I don't think there is a case when a die() inside merge-recursive would occur, but an other strategy would handle the merge properly.

Unless I missed something. ;-)