Re: [PATCH] mergetools/xxdiff: prevent segfaults from stopping difftool

[David Aguilar <davvid@gmail.com>]

On Wed, Oct 13, 2021 at 11:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

That also matches my train of thought.

I stopped short because I figured that the xxdiff scriptlet can special-case
this shortcoming initially and if the pattern recurs in other tools then we
can consider centralizing the workarounds in the helper then.

Thanks for reviewing, much appreciated.
-- 
David