[Bug]: Files without newline seem to fail at applying patch

[fzakaria]

What happened?

Lint results for //tools/lint:SortImportOrder:

Some problems have automated fixes available:

  tools/lint/SortImportOrder.java | 4 ++--
  1 file, 2 insertions(+), 2 deletions(-)

Error: failed to apply patch file for //tools/lint:SortImportOrder: conflict: fragment line does not match src line

If we build it manually

> bazel build //tools/lint:SortImportOrder  --aspects //tools/lint:linters.bzl%sort_imports --output_groups=rules_lint_patch --@aspect_rules_lint//lint:fix  --remote_download_all=true

> cat  bazel-bin/tools/lint/SortImportOrder.AspectRulesLintKeepJavaImportsSorted.patch
--- a/tools/lint/SortImportOrder.java
+++ b/tools/lint/SortImportOrder.java
@@ -1,14 +1,14 @@
 package tools.lint;

 import java.io.OutputStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.Scanner;
 import java.util.Set;
\ No newline at end of file

That patch seems to be messed up if we apply it with git apply
(notice lack of newline after java.util.Set)

> git patch tools
diff --git a/tools/lint/SortImportOrder.java b/tools/lint/SortImportOrder.java
index 35a3e87bdc2..0bb6c25fc47 100644
--- a/tools/lint/SortImportOrder.java
+++ b/tools/lint/SortImportOrder.java
@@ -1,17 +1,17 @@
 package tools.lint;

 import java.io.OutputStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.Scanner;
-import java.util.Set;
-import java.util.TreeSet;
+import java.util.Set;import java.util.TreeSet;
 import java.util.stream.Collectors;

If we apply a newline at the end of our file; our patch file is slightly different

> cat bazel-bin/tools/lint/SortImportOrder.AspectRulesLintKeepJavaImportsSorted.patch
--- a/tools/lint/SortImportOrder.java
+++ b/tools/lint/SortImportOrder.java
@@ -1,14 +1,14 @@
 package tools.lint;

 import java.io.OutputStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.Scanner;
 import java.util.Set;

It is missing \ No newline at end of file in the patch.
This patch when applied produces the correct results

Version

> cat .bazeliskrc
BAZELISK_BASE_URL=https://static.aspect.build/aspect
USE_BAZEL_VERSION=aspect/2024.51.11

http_archive(
    name = "aspect_rules_lint",
    sha256 = "f60e4a737a5e09402f5fa3bd182efa80dac5523ca4b9bc5c6fa8c06fbfb46630",
    strip_prefix = "rules_lint-1.1.0",
    url = "https://github.com/aspect-build/rules_lint/releases/download/v1.1.0/rules_lint-v1.1.0.tar.gz",
)

How to reproduce

I was able to reproduce it locally with the linters already setup.
I changed Bar.java

> git patch
diff --git a/example/src/Bar.java b/example/src/Bar.java
index b1ea6f4..e9a5825 100644
--- a/example/src/Bar.java
+++ b/example/src/Bar.java
@@ -10,11 +10,11 @@ public class Bar {
     // keep-sorted start
     B(),
     A(),
-    D(),
     C(),
+    D(),
     // keep-sorted end
   }

   // Max line length set to 20, so this should raise issue.
   protected void finalize(int a) {}
-}
+}
\ No newline at end of file

• Make the keep-sorted changed to fail
• remove new line

INFO: From Linting //src:bar with Checkstyle:
Checkstyle ends with 5 errors.
INFO: From Linting //src:foo with Checkstyle:
Checkstyle ends with 21 errors.
INFO: Found 26 targets...
INFO: Elapsed time: 364.087s, Critical Path: 8.10s
INFO: 1089 processes: 424 disk cache hit, 616 internal, 48 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 1089 total actions
Lint results for //src:bar:

Some problems have automated fixes available:

  src/Bar.java | 2 +-
  1 file, 1 insertion(+), 1 deletion(-)

Error: failed to apply patch file for //src:bar: conflict: fragment line does not match src line

Any other information?

No response

[alexeagle]

Could you please fill out the "How to reproduce" prompt? I'm not able to reproduce this, I think whatever tool is backing the SortImportOrder linter you mentioned is required to produce the malformed patch file.

[fzakaria]

@alexeagle did you try with a file without a newline?

I was able to reproduce it locally with the linters already setup.
I changed Bar.java

> git patch
diff --git a/example/src/Bar.java b/example/src/Bar.java
index b1ea6f4..e9a5825 100644
--- a/example/src/Bar.java
+++ b/example/src/Bar.java
@@ -10,11 +10,11 @@ public class Bar {
     // keep-sorted start
     B(),
     A(),
-    D(),
     C(),
+    D(),
     // keep-sorted end
   }

   // Max line length set to 20, so this should raise issue.
   protected void finalize(int a) {}
-}
+}
\ No newline at end of file

• Make the keep-sorted changed to fail
• remove new line

INFO: From Linting //src:bar with Checkstyle:
Checkstyle ends with 5 errors.
INFO: From Linting //src:foo with Checkstyle:
Checkstyle ends with 21 errors.
INFO: Found 26 targets...
INFO: Elapsed time: 364.087s, Critical Path: 8.10s
INFO: 1089 processes: 424 disk cache hit, 616 internal, 48 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 1089 total actions
Lint results for //src:bar:

Some problems have automated fixes available:

  src/Bar.java | 2 +-
  1 file, 1 insertion(+), 1 deletion(-)

Error: failed to apply patch file for //src:bar: conflict: fragment line does not match src line

[fzakaria]

I tried bumping the version but latest (0.8.1) still has the bug bluekeyes/go-gitdiff#57

[fzakaria]

Okay I did some debugging with @vinnybod -- looks to be an issue where BSD diff will produce diffs with multiple \ No newline at end of file

This causes bugs in go-gitdiff and git itself (we sent an email to their mailing list).

Switching to GNU diff fixes the problem but how can we get around this in rules_lint?

Ideas 💡

• go-gitdiff itself can generate patches; should we leverage that instead ?
• check if git exists before using diff ?
• check if diff --version says "BSD" and print a friendly warning?

[alexeagle]

I think we just need to resolve this TODO
https://github.com/aspect-build/rules_lint/blob/main/lint/private/patcher.mjs#L91
so we can rely on having GNU diff (or any specific non-buggy diff implementation)

[alexeagle]

https://gitlab.arm.com/bazel/ape can give us a GNU diff binary, which I think is a reasonable way to wire this up.