Re: Inconsistent handling of corrupt patches based on line endings

[Elijah Newren <newren@gmail.com>]

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).
>
> Let's see if any recent apply.c folks have thoughts...:
>
> $ git shortlog -nse --no-merges --since=3.years.ago.. -- apply.c
>     21  Elijah Newren <newren@gmail.com>
>     12  Patrick Steinhardt <ps@pks.im>
>      7  Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      5  Junio C Hamano <gitster@pobox.com>
>      4  Jeff King <peff@peff.net>
>      4  René Scharfe <l.s.r@web.de>

...and thus we learn why no one wanted to clean up the header files in
git.git before me.  ;-)

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.