patchmergetools/xxdiff: prevent segfaults from stopping difftool

> Users often use "git difftool HEAD^" to review their work, and have
> "mergetool.prompt" set to false so that difftool does not prompt them
> before diffing each file.
>
> This is very convenient because users can see all their diffs by
> reviewing the xxdiff windows one at a time.
>
> A problem occurs when xxdiff encounters some binary files.
> It can segfault and return exit code 128, which is special-cased
> by git-difftool-helper as being an extraordinary situation that
> aborts the process.
>
> Suppress the exit code from xxdiff in its diff_cmd() implementation
> when we see exit code 128 so that the GIT_EXTERNAL_DIFF loop continues
> on uninterrupted to the next file rather than aborting when it
> encounters the first binary file.

> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  mergetools/xxdiff | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/mergetools/xxdiff b/mergetools/xxdiff
> index ce5b8e9f29..d5ce467995 100644
> --- a/mergetools/xxdiff
> +++ b/mergetools/xxdiff
> @@ -3,6 +3,13 @@ diff_cmd () {
>  		-R 'Accel.Search: "Ctrl+F"' \
>  		-R 'Accel.SearchForward: "Ctrl+G"' \
>  		"$LOCAL" "$REMOTE"
> +
> +	# xxdiff can segfault on binary files which are often uninteresting.
> +	# Do not allow segfaults to stop us from continuing on to the next file.
> +	if test $? = 128
> +	then
> +		return 1
> +	fi
>  }
>  
>  merge_cmd () {

>
> David Aguilar <davvid@gmail.com> writes:
>
> > Users often use "git difftool HEAD^" to review their work, and have
> > "mergetool.prompt" set to false so that difftool does not prompt them
> > before diffing each file.
> >
> > This is very convenient because users can see all their diffs by
> > reviewing the xxdiff windows one at a time.
> >
> > A problem occurs when xxdiff encounters some binary files.
> > It can segfault and return exit code 128, which is special-cased
> > by git-difftool-helper as being an extraordinary situation that
> > aborts the process.
> >
> > Suppress the exit code from xxdiff in its diff_cmd() implementation
> > when we see exit code 128 so that the GIT_EXTERNAL_DIFF loop continues
> > on uninterrupted to the next file rather than aborting when it
> > encounters the first binary file.
>
> Sounds like a reasonable workaround, but I wonder if this should be
> limited to "when xxdiff segfaults" (in other words, if it is common
> for other difftool backends to exit with status 128, perhaps a
> better workaround might be to teach difftool-helper that 128 is not
> all that special?), and if the answer is yes (in other words, no, it
> is not common among other backends and 128 from xxdiff is very
> special), if we can easily and cheaply avoid running xxdiff on
> binaries---that way, other exists from xxdiff with status 128 can
> still be treated as an extraordinary situation.
>
> In any case, the above is a thinking-aloud by somebody who does not
> use xxdiff himself, and should not be taken as "I think this patch
> is not good enough" at all.
>
> Will queue.  Thanks.