On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Tim Henigan <tim.henigan@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html