checkstyle: Work around bug in difflib

19717 diff mbox series

If a file misses the newline at the end it gets detected by checkstyle,
but the resulting patch is incorrect and does not apply. It took me a
while to understand that it wasn't me using checkstyle incorrectly, but
that the patch was faulty. The bug itself is in difflib and dates back to
2008.

To reproduce:
- Remove trailing newline from a file
- git add the file
- run ./utils/checkstyle.py -s | patch -p0

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 utils/checkstyle.py | 6 ++++++
 1 file changed, 6 insertions(+)

Quoting Stefan Klug (2024-03-14 10:57:36)
> If a file misses the newline at the end it gets detected by checkstyle,
> but the resulting patch is incorrect and does not apply. It took me a
> while to understand that it wasn't me using checkstyle incorrectly, but
> that the patch was faulty. The bug itself is in difflib and dates back to
> 2008.

Open source speeds!

> To reproduce:
> - Remove trailing newline from a file
> - git add the file
> - run ./utils/checkstyle.py -s | patch -p0
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  utils/checkstyle.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index db5a550d..20a5a592 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -168,6 +168,12 @@ def parse_diff(diff):
>              hunk = DiffHunk(line)
>  
>          elif hunk is not None:
> +            # Work around https://github.com/python/cpython/issues/46395
> +            # See https://www.gnu.org/software/diffutils/manual/html_node/Incomplete-Lines.html
> +            if line[-1] != '\n':
> +                hunk.append(line + '\n')
> +                line = "\\ No newline at end of file\n"

This looks reasonable, but at a glance I can't tell if the \\ is an
escaped \ or a double \\ ?

Based on the text at 
https://www.gnu.org/software/diffutils/manual/html_node/Incomplete-Lines.html

```
 For example, suppose F and G are one-byte files that contain just ‘f’
 and ‘g’, respectively. Then ‘diff F G’ outputs
 
 1c1
 < f
 \ No newline at end of file
 ---
 > g
 \ No newline at end of file
```
I assume it's an escape, and only a single line is printed, and based on
that assumption:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
>              hunk.append(line)
>  
>      if hunk:
> -- 
> 2.40.1
>

Hi Kieran,

thanks for the review.

On Thu, Mar 14, 2024 at 12:19:18PM +0000, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-03-14 10:57:36)
> > If a file misses the newline at the end it gets detected by checkstyle,
> > but the resulting patch is incorrect and does not apply. It took me a
> > while to understand that it wasn't me using checkstyle incorrectly, but
> > that the patch was faulty. The bug itself is in difflib and dates back to
> > 2008.
> 
> Open source speeds!
> 
> > To reproduce:
> > - Remove trailing newline from a file
> > - git add the file
> > - run ./utils/checkstyle.py -s | patch -p0
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index db5a550d..20a5a592 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -168,6 +168,12 @@ def parse_diff(diff):
> >              hunk = DiffHunk(line)
> >  
> >          elif hunk is not None:
> > +            # Work around https://github.com/python/cpython/issues/46395
> > +            # See https://www.gnu.org/software/diffutils/manual/html_node/Incomplete-Lines.html
> > +            if line[-1] != '\n':
> > +                hunk.append(line + '\n')
> > +                line = "\\ No newline at end of file\n"
> 
> This looks reasonable, but at a glance I can't tell if the \\ is an
> escaped \ or a double \\ ?

Yes, it's an escaped backslash.

Cheers
Stefan

> 
> Based on the text at 
> https://www.gnu.org/software/diffutils/manual/html_node/Incomplete-Lines.html
> 
> ```
>  For example, suppose F and G are one-byte files that contain just ‘f’
>  and ‘g’, respectively. Then ‘diff F G’ outputs
>  
>  1c1
>  < f
>  \ No newline at end of file
>  ---
>  > g
>  \ No newline at end of file
> ```
> I assume it's an escape, and only a single line is printed, and based on
> that assumption:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +
> >              hunk.append(line)
> >  
> >      if hunk:
> > -- 
> > 2.40.1
> >

Hi Stefan,

Thank you for the patch.

On Thu, Mar 14, 2024 at 11:57:36AM +0100, Stefan Klug wrote:
> If a file misses the newline at the end it gets detected by checkstyle,
> but the resulting patch is incorrect and does not apply. It took me a
> while to understand that it wasn't me using checkstyle incorrectly, but
> that the patch was faulty. The bug itself is in difflib and dates back to
> 2008.
> 
> To reproduce:
> - Remove trailing newline from a file
> - git add the file
> - run ./utils/checkstyle.py -s | patch -p0
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  utils/checkstyle.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index db5a550d..20a5a592 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -168,6 +168,12 @@ def parse_diff(diff):
>              hunk = DiffHunk(line)
>  
>          elif hunk is not None:
> +            # Work around https://github.com/python/cpython/issues/46395
> +            # See https://www.gnu.org/software/diffutils/manual/html_node/Incomplete-Lines.html
> +            if line[-1] != '\n':
> +                hunk.append(line + '\n')
> +                line = "\\ No newline at end of file\n"

s/"/'/g

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>              hunk.append(line)
>  
>      if hunk: