From: Miklos Vajna Date: Mon, 11 Aug 2008 19:07:05 GMT Subject: Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive 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 wrote: > 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. ;-)