Re: Inconsistent handling of corrupt patches based on line endings

From: René Scharfe <l.s.r@web.de>
Date: Oct 28, 2024, 22:07 UTC
Message-ID: <d74232a4-126d-4bd8-8f60-a3fac80492d5@web.de>
In-Reply-To: <CABPp-BFB+eG7WVJFm3EBfgKzuspAYn9KpDHodpgKuFmWefARBw@mail.gmail.com>

Am 28.10.24 um 19:39 schrieb Elijah Newren:

Show 42 quoted lines

> On Mon, Oct 28, 2024 at 11:11 AM Taylor Blau <me@ttaylorr.com> wrote:
>>
>> On Mon, Oct 28, 2024 at 05:57:54PM +0100, Peregi Tamás wrote:
>>> Hi all,
>>>
>>> I might've found an inconsistency in how git-apply treats corrupt patches
>>> (representing empty context lines with completely empty lines instead of
>>> lines containing a single space - usually a result of a "trim trailing
>>> whitespace" editor setting) based on whether the patch file uses
>>> Windows-style line endings (CRLF) or Unix-style line endings (LF only).
>
> I believe this behavior was caused by:
>
> $ git log -1 b507b465f7831612b9d9fc643e3e5218b64e5bfa
> commit b507b465f7831612b9d9fc643e3e5218b64e5bfa
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Thu Oct 19 19:26:08 2006 -0700
>
>     git-apply: prepare for upcoming GNU diff -u format change.
>
>     The latest GNU diff from CVS emits an empty line to express
>     an empty context line, instead of more traditional "single
>     white space followed by a newline".  Do not get broken by it.
>
>     Signed-off-by: Linus Torvalds <torvalds@osdl.org>
>     Signed-off-by: Junio C Hamano <junkio@cox.net>
>
> That code special-cased a line containing '\n' but not a line
> containing only '\r\n'.
>
> As to whether that's correct, personally I'd rather only special case
> workaround important existing clients.  Back in 2006, working with GNU
> diff was incredibly important, and I'd say is still important today.
> I can see Peregi's comment that this make line ending slightly
> inconsistent, but I feel like the blank-line handling is a workaround
> for an existing client we want to interoperate with and absent a
> similar important client with mis-behaving '\r\n'-only lines, I
> wouldn't be interested in adding support for it.  But that's just my
> off-the-cuff feeling and I don't feel strongly about it.  Further, all
> but one of my contributions above were mere header changes, so if
> others have other opinions, they should probably be weighted more
> heavily than mine on this topic.

What would the patch(1) do?

The first column is the exit code of the subsequent command (0: success, 1: one or more rejected lines, 2: failure):

0 patch -p1 --dry-run original-unix.txt <corrupt-unix.patch 0 patch -p1 --dry-run original-unix.txt <intact-unix.patch 0 patch -p1 --dry-run original-win.txt <intact-win.patch 0 patch --ignore-whitespace -p1 --dry-run original-unix.txt <corrupt-unix.patch 0 patch --ignore-whitespace -p1 --dry-run original-unix.txt <intact-unix.patch 0 patch --ignore-whitespace -p1 --dry-run original-unix.txt <intact-win.patch 0 patch --ignore-whitespace -p1 --dry-run original-win.txt <corrupt-unix.patch 0 patch --ignore-whitespace -p1 --dry-run original-win.txt <intact-unix.patch 0 patch --ignore-whitespace -p1 --dry-run original-win.txt <intact-win.patch 1 patch -p1 --dry-run original-unix.txt <intact-win.patch 1 patch -p1 --dry-run original-win.txt <corrupt-unix.patch 1 patch -p1 --dry-run original-win.txt <intact-unix.patch 2 patch -p1 --dry-run original-unix.txt <corrupt-win.patch 2 patch -p1 --dry-run original-win.txt <corrupt-win.patch 2 patch --ignore-whitespace -p1 --dry-run original-unix.txt <corrupt-win.patch 2 patch --ignore-whitespace -p1 --dry-run original-win.txt <corrupt-win.patch

So basically the same as git apply?

What does current GNU diff do?

diff -u <(printf 'a\n\n') <(printf 'b\n\n') | tail -1 | od -a 0000000 sp nl 0000002 diff -u --suppress-blank-empty <(printf 'a\n\n') <(printf 'b\n\n') | tail -1 | od -a 0000000 nl 0000001 diff -u <(printf 'a\r\n\r\n') <(printf 'b\r\n\r\n') | tail -1 | od -a 0000000 sp cr nl 0000003 diff -u --suppress-blank-empty <(printf 'a\r\n\r\n') <(printf 'b\r\n\r\n') | tail -1 | od -a 0000000 sp cr nl 0000003

So it omits the space only if --suppress-blank-empty is given and the blank line ends with \n, not with \r\n.

Anyway, I agree with Elijah: The targeted support for GNU diff's eccentricity is fine, and we're in good company with that. We could remove it, since it doesn't seem to be the default (anymore?), but I don't see much of a benefit. We could add support for blank context lines that end in CRLF if there's a notable source of that kind of that deviation from the original format.

René