From: Tim Henigan Date: Fri, 02 Mar 2012 13:22:09 GMT Subject: Re: [PATCH] mergetools: add support for DeltaWalker Message-ID: In-Reply-To: <7vpqcvk496.fsf@alter.siamese.dyndns.org> On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano wrote: > Tim Henigan writes: > > I see that the earlier refactoring to make mergetool backend pluggable is > starting to pay off rather nicely.  It is not "since ..., requires ...", > but "thanks to ..., adding a random new tool is just a matter of dropping > a trivial shell snippet in the directory". I will reword the commit message in v2. > Perhaps doing the above like this might make it a bit less of an eye-sore. > >        if $base_present >        then >                "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" -merged="$PWD/$MERGED" >        else >                "$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED" >        fi >/dev/null 2>&1 Will update in v2. >> +       status=$? > > This is highly dubious.  Looking at existing mergetools/*, I think the > caller expects merge_cmd to signal success or failure with $?, so you > probably just want to drop this line; the caller will then get the $? > that was set by the "$merge_tool_path" command. > > That is how your diff_cmd is communicating with its caller after all, no? It seems that only two of the existing scripts use this 'status=$?' line (emerge and kdiff3). I used the 'kdiff3' file as my starting point without actually trying to understand why that line was present. Local testing shows that I don't need this line for DeltaWalker, so it will be removed in v2.