On Wed, Oct 13, 2021 at 11:03 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > David Aguilar <davvid@xxxxxxxxx> 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
