{"thread":{"id":"56698","subject":"[PATCH] mergetools/xxdiff: prevent segfaults from stopping difftool","startedAt":"2021-10-13T02:45:44Z","lastAt":"2021-10-14T18:51:16Z","messageCount":3,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"438626","messageId":"20211013024539.49612-1-davvid@gmail.com","threadId":"56698","inReplyTo":null,"subject":"[PATCH] mergetools/xxdiff: prevent segfaults from stopping difftool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-10-13T02:45:39Z","receivedAt":"2021-10-13T02:45:44Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Users often use \"git difftool HEAD^\" to review their work, and have\n\"mergetool.prompt\" set to false so that difftool does not prompt them\nbefore diffing each file.\n\nThis is very convenient because users can see all their diffs by\nreviewing the xxdiff windows one at a time.\n\nA problem occurs when xxdiff encounters some binary files.\nIt can segfault and return exit code 128, which is special-cased\nby git-difftool-helper as being an extraordinary situation that\naborts the process.\n\nSuppress the exit code from xxdiff in its diff_cmd() implementation\nwhen we see exit code 128 so that the GIT_EXTERNAL_DIFF loop continues\non uninterrupted to the next file rather than aborting when it\nencounters the first binary file.\n\nSigned-off-by: David Aguilar \n---\n mergetools/xxdiff | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/mergetools/xxdiff b/mergetools/xxdiff\nindex ce5b8e9f29..d5ce467995 100644\n--- a/mergetools/xxdiff\n+++ b/mergetools/xxdiff\n@@ -3,6 +3,13 @@ diff_cmd () {\n \t\t-R 'Accel.Search: \"Ctrl+F\"' \\\n \t\t-R 'Accel.SearchForward: \"Ctrl+G\"' \\\n \t\t\"$LOCAL\" \"$REMOTE\"\n+\n+\t# xxdiff can segfault on binary files which are often uninteresting.\n+\t# Do not allow segfaults to stop us from continuing on to the next file.\n+\tif test $? = 128\n+\tthen\n+\t\treturn 1\n+\tfi\n }\n \n merge_cmd () {\n-- \n2.33.0.1144.g9656da23d7\n\n"},{"id":"438670","messageId":"xmqqlf2whke8.fsf@gitster.g","threadId":"56698","inReplyTo":"20211013024539.49612-1-davvid@gmail.com","subject":"Re: [PATCH] mergetools/xxdiff: prevent segfaults from stopping difftool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-13T18:03:43Z","receivedAt":"2021-10-13T18:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Users often use \"git difftool HEAD^\" to review their work, and have\n> \"mergetool.prompt\" set to false so that difftool does not prompt them\n> before diffing each file.\n>\n> This is very convenient because users can see all their diffs by\n> reviewing the xxdiff windows one at a time.\n>\n> A problem occurs when xxdiff encounters some binary files.\n> It can segfault and return exit code 128, which is special-cased\n> by git-difftool-helper as being an extraordinary situation that\n> aborts the process.\n>\n> Suppress the exit code from xxdiff in its diff_cmd() implementation\n> when we see exit code 128 so that the GIT_EXTERNAL_DIFF loop continues\n> on uninterrupted to the next file rather than aborting when it\n> encounters the first binary file.\n\nSounds like a reasonable workaround, but I wonder if this should be\nlimited to \"when xxdiff segfaults\" (in other words, if it is common\nfor other difftool backends to exit with status 128, perhaps a\nbetter workaround might be to teach difftool-helper that 128 is not\nall that special?), and if the answer is yes (in other words, no, it\nis not common among other backends and 128 from xxdiff is very\nspecial), if we can easily and cheaply avoid running xxdiff on\nbinaries---that way, other exists from xxdiff with status 128 can\nstill be treated as an extraordinary situation.\n\nIn any case, the above is a thinking-aloud by somebody who does not\nuse xxdiff himself, and should not be taken as \"I think this patch\nis not good enough\" at all.\n\nWill queue. Thanks.\n\n> Signed-off-by: David Aguilar \n> ---\n> mergetools/xxdiff | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/mergetools/xxdiff b/mergetools/xxdiff\n> index ce5b8e9f29..d5ce467995 100644\n> --- a/mergetools/xxdiff\n> +++ b/mergetools/xxdiff\n> @@ -3,6 +3,13 @@ diff_cmd () {\n> \t\t-R 'Accel.Search: \"Ctrl+F\"' \\\n> \t\t-R 'Accel.SearchForward: \"Ctrl+G\"' \\\n> \t\t\"$LOCAL\" \"$REMOTE\"\n> +\n> +\t# xxdiff can segfault on binary files which are often uninteresting.\n> +\t# Do not allow segfaults to stop us from continuing on to the next file.\n> +\tif test $? = 128\n> +\tthen\n> +\t\treturn 1\n> +\tfi\n> }\n> \n> merge_cmd () {\n"},{"id":"438778","messageId":"CAJDDKr5w3Le_QtsCKF6+i3ThFa-FF6EVVm80ukjPSMsJZkByOQ@mail.gmail.com","threadId":"56698","inReplyTo":"xmqqlf2whke8.fsf@gitster.g","subject":"Re: [PATCH] mergetools/xxdiff: prevent segfaults from stopping difftool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-10-14T18:50:34Z","receivedAt":"2021-10-14T18:51:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Oct 13, 2021 at 11:03 AM Junio C Hamano wrote:\n>\n> David Aguilar writes:\n>\n> > Users often use \"git difftool HEAD^\" to review their work, and have\n> > \"mergetool.prompt\" set to false so that difftool does not prompt them\n> > before diffing each file.\n> >\n> > This is very convenient because users can see all their diffs by\n> > reviewing the xxdiff windows one at a time.\n> >\n> > A problem occurs when xxdiff encounters some binary files.\n> > It can segfault and return exit code 128, which is special-cased\n> > by git-difftool-helper as being an extraordinary situation that\n> > aborts the process.\n> >\n> > Suppress the exit code from xxdiff in its diff_cmd() implementation\n> > when we see exit code 128 so that the GIT_EXTERNAL_DIFF loop continues\n> > on uninterrupted to the next file rather than aborting when it\n> > encounters the first binary file.\n>\n> Sounds like a reasonable workaround, but I wonder if this should be\n> limited to \"when xxdiff segfaults\" (in other words, if it is common\n> for other difftool backends to exit with status 128, perhaps a\n> better workaround might be to teach difftool-helper that 128 is not\n> all that special?), and if the answer is yes (in other words, no, it\n> is not common among other backends and 128 from xxdiff is very\n> special), if we can easily and cheaply avoid running xxdiff on\n> binaries---that way, other exists from xxdiff with status 128 can\n> still be treated as an extraordinary situation.\n>\n> In any case, the above is a thinking-aloud by somebody who does not\n> use xxdiff himself, and should not be taken as \"I think this patch\n> is not good enough\" at all.\n>\n> Will queue. Thanks.\n\nThat also matches my train of thought.\n\nI stopped short because I figured that the xxdiff scriptlet can special-case\nthis shortcoming initially and if the pattern recurs in other tools then we\ncan consider centralizing the workarounds in the helper then.\n\nThanks for reviewing, much appreciated.\n-- \nDavid\n"}]}