Repository: reviewdog/reviewdog
Pull request #1975 (2024-11-25 17:11:17)

Fix for suggestions not including an inserted EOF newline
When an end-of-file newline is inserted, Reviewdog currently doesn't include it in the resulting suggestion, often resulting in a diff (ex. as a GitHub PR review) that can't be applied because there are no actual changes suggested.

See #1927.

This adds some smarter handling of EOF newlines into the Diff Hunk parser, as well as special-case handling into Suggestion generation, and a handful of tests for both.

Happy to make revisions; I'm pretty green in Go.

- [x] Updated Unreleased section in [CHANGELOG](https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md) or it's not notable changes.




Patch:
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 171bc26f..22d3f9ec 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -2,7 +2,7 @@ name: release
 on:
   push:
     branches:
-      - master
+      - $default-branch
       - release-*
     tags:
       - "v*.*.*"
diff --git a/.github/workflows/reviewdog.yml b/.github/workflows/reviewdog.yml
index c2a07606..62873858 100644
--- a/.github/workflows/reviewdog.yml
+++ b/.github/workflows/reviewdog.yml
@@ -2,7 +2,7 @@ name: reviewdog
 on:
   push:
     branches:
-      - master
+      - $default-branch
   pull_request:
 
 jobs:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 483ffad9..415070cb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### :bug: Fixes
 - [#1983](https://github.com/reviewdog/reviewdog/pull/1983) Improve error message for diff command failure by including stderr output
+- [#1975](https://github.com/reviewdog/reviewdog/pull/1975) Fix for suggestions not including an inserted EOF newline
 
 ### :rotating_light: Breaking changes
 - ...
diff --git a/README.md b/README.md
index c246035c..a59ff8c1 100644
--- a/README.md
+++ b/README.md
@@ -629,7 +629,7 @@ name: reviewdog (github-check)
 on:
   push:
     branches:
-      - master
+      - $default-branch
   pull_request:
 
 jobs:
diff --git a/diff/diff.go b/diff/diff.go
index d52a9918..c980a7f8 100644
--- a/diff/diff.go
+++ b/diff/diff.go
@@ -59,6 +59,9 @@ type Hunk struct {
 
 	// the body lines of the hunk
 	Lines []*Line
+
+	// the status of the EOF newline, if any
+	EOFNewline LineType
 }
 
 // LineType represents the type of diff line.
diff --git a/diff/parse.go b/diff/parse.go
index 77f0b338..79b25c2e 100644
--- a/diff/parse.go
+++ b/diff/parse.go
@@ -251,9 +251,11 @@ func (p *hunkParser) Parse() (*Hunk, error) {
 		StartLineNew:  hr.lnew,
 		LineLengthNew: hr.snew,
 		Section:       hr.section,
+		EOFNewline:    LineUnchanged,
 	}
 	lold := hr.lold
 	lnew := hr.lnew
+	prevLineType := LineUnchanged
 endhunk:
 	for !p.done(lold, lnew, hr) {
 		b, err := p.r.Peek(1)
@@ -274,20 +276,37 @@ endhunk:
 				line.LnumNew = lnew
 				lold++
 				lnew++
+				prevLineType = LineUnchanged
 			case tokenAddedLine:
 				line.Type = LineAdded
 				line.LnumDiff = p.lnumdiff
 				line.LnumNew = lnew
 				lnew++
+				prevLineType = LineAdded
 			case tokenDeletedLine:
 				line.Type = LineDeleted
 				line.LnumDiff = p.lnumdiff
 				line.LnumOld = lold
 				lold++
+				prevLineType = LineDeleted
 			}
 			hunk.Lines = append(hunk.Lines, line)
 		case tokenNoNewlineAtEOF:
-			// skip \ No newline at end of file. just consume line
+			switch prevLineType {
+			case LineUnchanged:
+				hunk.EOFNewline = LineUnchanged
+			case LineAdded:
+				// special case - if there's no newline in both files,
+				// it will already have been reported as part of the old file
+				if hunk.EOFNewline == LineAdded {
+					hunk.EOFNewline = LineUnchanged
+				} else {
+					hunk.EOFNewline = LineDeleted
+				}
+			case LineDeleted:
+				hunk.EOFNewline = LineAdded
+			}
+			// skip the rest of the line
 			readline(p.r)
 		default:
 			break endhunk
diff --git a/parser/diff.go b/parser/diff.go
index 0229e2ff..fa1a8141 100644
--- a/parser/diff.go
+++ b/parser/diff.go
@@ -104,6 +104,16 @@ func (p *DiffParser) Parse(r io.Reader) ([]*rdf.Diagnostic, error) {
 				}
 				prevState = diffLine.Type
 			}
+			if hunk.EOFNewline == diff.LineAdded {
+				// Adding a blank line here should prompt an EOF newline
+				// to be inserted (rather than a complete blank line).
+				// This is known to work with GitHub review suggestions, at least.
+				// See https://github.com/reviewdog/reviewdog/pull/1975#issuecomment-2634826110
+				// It hasn't yet been tested with other reporters.
+				state.newLines = append(state.newLines, "")
+				// NOTE: this doesn't handle the case of a deleted eof newline
+				// because it's much rarer in practice.
+			}
 			if state.startLine > 0 {
 				emit() // Output a diagnostic at the end of hunk.
 			}


Test patch:
diff --git a/diff/testdata/addeofnewline.diff b/diff/testdata/addeofnewline.diff
new file mode 100644
index 00000000..0063a8a4
--- /dev/null
+++ b/diff/testdata/addeofnewline.diff
@@ -0,0 +1,7 @@
+--- addeofnewline.old.txt	2024-11-18 09:48:11
++++ addeofnewline.new.txt	2024-11-18 09:48:11
+@@ -1,2 +1,2 @@
+ " vim: nofixeol noendofline
+-this file will have a newline at the end
+\ No newline at end of file
++this file will have a newline at the end
diff --git a/diff/testdata/addeofnewline.diff.json b/diff/testdata/addeofnewline.diff.json
new file mode 100644
index 00000000..6dc012d2
--- /dev/null
+++ b/diff/testdata/addeofnewline.diff.json
@@ -0,0 +1,42 @@
+[
+  {
+    "PathOld": "addeofnewline.old.txt",
+    "PathNew": "addeofnewline.new.txt",
+    "TimeOld": "2024-11-18 09:48:11",
+    "TimeNew": "2024-11-18 09:48:11",
+    "Hunks": [
+      {
+        "StartLineOld": 1,
+        "LineLengthOld": 2,
+        "StartLineNew": 1,
+        "LineLengthNew": 2,
+        "Section": "",
+        "Lines": [
+          {
+            "Type": 0,
+            "Content": "\" vim: nofixeol noendofline",
+            "LnumDiff": 1,
+            "LnumOld": 1,
+            "LnumNew": 1
+          },
+          {
+            "Type": 2,
+            "Content": "this file will have a newline at the end",
+            "LnumDiff": 2,
+            "LnumOld": 2,
+            "LnumNew": 0
+          },
+          {
+            "Type": 1,
+            "Content": "this file will have a newline at the end",
+            "LnumDiff": 3,
+            "LnumOld": 0,
+            "LnumNew": 2
+          }
+        ],
+        "EofNewline": 1
+      }
+    ],
+    "Extended": null
+  }
+]
diff --git a/diff/testdata/addeofnewline.new.txt b/diff/testdata/addeofnewline.new.txt
new file mode 100644
index 00000000..796c98d1
--- /dev/null
+++ b/diff/testdata/addeofnewline.new.txt
@@ -0,0 +1,2 @@
+" vim: nofixeol noendofline
+this file will have a newline at the end
diff --git a/diff/testdata/addeofnewline.old.txt b/diff/testdata/addeofnewline.old.txt
new file mode 100644
index 00000000..8271a105
--- /dev/null
+++ b/diff/testdata/addeofnewline.old.txt
@@ -0,0 +1,2 @@
+" vim: nofixeol noendofline
+this file will have a newline at the end
\ No newline at end of file
diff --git a/diff/testdata/gen.sh b/diff/testdata/gen.sh
index 1d6a5bd8..43249f2c 100644
--- a/diff/testdata/gen.sh
+++ b/diff/testdata/gen.sh
@@ -12,4 +12,6 @@ git diff --no-index golint.{old,new}.go > golint.diff
 git diff --no-index empty.txt /dev/null > newline_and_empty_deleted.diff
 git diff --no-index golint.{old,new}.go >> newline_and_empty_deleted.diff
 git -c core.quotepath=true diff --no-index 日本語.{old,new}.txt > 日本語.diff
+diff -u addeofnewline.{old,new}.txt > addeofnewline.diff
+diff -u removeeofnewline.{old,new}.txt > removeeofnewline.diff
 gofmt -d gofmt.go > gofmt.diff
diff --git a/diff/testdata/gofmt.diff.json b/diff/testdata/gofmt.diff.json
index 4bf6ead5..062e1554 100644
--- a/diff/testdata/gofmt.diff.json
+++ b/diff/testdata/gofmt.diff.json
@@ -61,7 +61,8 @@
             "LnumOld": 6,
             "LnumNew": 6
           }
-        ]
+        ],
+        "EofNewline": 0
       },
       {
         "StartLineOld": 10,
@@ -175,7 +176,8 @@
             "LnumOld": 20,
             "LnumNew": 20
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": [
diff --git a/diff/testdata/golint.diff.json b/diff/testdata/golint.diff.json
index f7ff2a35..03d92f1e 100644
--- a/diff/testdata/golint.diff.json
+++ b/diff/testdata/golint.diff.json
@@ -96,7 +96,8 @@
             "LnumOld": 0,
             "LnumNew": 13
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": [
diff --git a/diff/testdata/newline_and_empty_deleted.diff.json b/diff/testdata/newline_and_empty_deleted.diff.json
index e1ba4df1..4e9bc973 100644
--- a/diff/testdata/newline_and_empty_deleted.diff.json
+++ b/diff/testdata/newline_and_empty_deleted.diff.json
@@ -108,7 +108,8 @@
             "LnumOld": 0,
             "LnumNew": 13
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": [
diff --git a/diff/testdata/nonewline.diff.json b/diff/testdata/nonewline.diff.json
index 3a4e6a88..39f7a34f 100644
--- a/diff/testdata/nonewline.diff.json
+++ b/diff/testdata/nonewline.diff.json
@@ -54,7 +54,8 @@
             "LnumOld": 0,
             "LnumNew": 4
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": null
diff --git a/diff/testdata/nonewline2.diff.json b/diff/testdata/nonewline2.diff.json
index dec71876..47b74c78 100644
--- a/diff/testdata/nonewline2.diff.json
+++ b/diff/testdata/nonewline2.diff.json
@@ -54,7 +54,8 @@
             "LnumOld": 4,
             "LnumNew": 4
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": null
diff --git a/diff/testdata/nonewline3.diff.json b/diff/testdata/nonewline3.diff.json
index 799cc1d3..763d4a1b 100644
--- a/diff/testdata/nonewline3.diff.json
+++ b/diff/testdata/nonewline3.diff.json
@@ -68,7 +68,8 @@
             "LnumOld": 6,
             "LnumNew": 7
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": null
diff --git a/diff/testdata/removeeofnewline.diff b/diff/testdata/removeeofnewline.diff
new file mode 100644
index 00000000..caaa33e1
--- /dev/null
+++ b/diff/testdata/removeeofnewline.diff
@@ -0,0 +1,7 @@
+--- removeeofnewline.old.txt	2024-11-19 15:49:41
++++ removeeofnewline.new.txt	2024-11-19 15:49:34
+@@ -1,2 +1,2 @@
+ " vim: nofixeol noendofline
+-this file will no longer have a newline at the end
++this file will no longer have a newline at the end
+\ No newline at end of file
diff --git a/diff/testdata/removeeofnewline.diff.json b/diff/testdata/removeeofnewline.diff.json
new file mode 100644
index 00000000..91de7b2c
--- /dev/null
+++ b/diff/testdata/removeeofnewline.diff.json
@@ -0,0 +1,42 @@
+[
+  {
+    "PathOld": "removeeofnewline.old.txt",
+    "PathNew": "removeeofnewline.new.txt",
+    "TimeOld": "2024-11-19 15:49:41",
+    "TimeNew": "2024-11-19 15:49:34",
+    "Hunks": [
+      {
+        "StartLineOld": 1,
+        "LineLengthOld": 2,
+        "StartLineNew": 1,
+        "LineLengthNew": 2,
+        "Section": "",
+        "Lines": [
+          {
+            "Type": 0,
+            "Content": "\" vim: nofixeol noendofline",
+            "LnumDiff": 1,
+            "LnumOld": 1,
+            "LnumNew": 1
+          },
+          {
+            "Type": 2,
+            "Content": "this file will no longer have a newline at the end",
+            "LnumDiff": 2,
+            "LnumOld": 2,
+            "LnumNew": 0
+          },
+          {
+            "Type": 1,
+            "Content": "this file will no longer have a newline at the end",
+            "LnumDiff": 3,
+            "LnumOld": 0,
+            "LnumNew": 2
+          }
+        ],
+        "EofNewline": 2
+      }
+    ],
+    "Extended": null
+  }
+]
diff --git a/diff/testdata/removeeofnewline.new.txt b/diff/testdata/removeeofnewline.new.txt
new file mode 100644
index 00000000..472993c9
--- /dev/null
+++ b/diff/testdata/removeeofnewline.new.txt
@@ -0,0 +1,2 @@
+" vim: nofixeol noendofline
+this file will no longer have a newline at the end
\ No newline at end of file
diff --git a/diff/testdata/removeeofnewline.old.txt b/diff/testdata/removeeofnewline.old.txt
new file mode 100644
index 00000000..ef47945f
--- /dev/null
+++ b/diff/testdata/removeeofnewline.old.txt
@@ -0,0 +1,2 @@
+" vim: nofixeol noendofline
+this file will no longer have a newline at the end
diff --git a/diff/testdata/sample.diff.json b/diff/testdata/sample.diff.json
index cc341527..01010287 100644
--- a/diff/testdata/sample.diff.json
+++ b/diff/testdata/sample.diff.json
@@ -47,7 +47,8 @@
             "LnumOld": 3,
             "LnumNew": 4
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": null
diff --git a/diff/testdata/sample.git.diff.json b/diff/testdata/sample.git.diff.json
index 34b35361..57099875 100644
--- a/diff/testdata/sample.git.diff.json
+++ b/diff/testdata/sample.git.diff.json
@@ -47,7 +47,8 @@
             "LnumOld": 3,
             "LnumNew": 4
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": [
diff --git "a/diff/testdata/\346\227\245\346\234\254\350\252\236.diff.json" "b/diff/testdata/\346\227\245\346\234\254\350\252\236.diff.json"
index 2657d6e7..9cd33181 100644
--- "a/diff/testdata/\346\227\245\346\234\254\350\252\236.diff.json"
+++ "b/diff/testdata/\346\227\245\346\234\254\350\252\236.diff.json"
@@ -47,7 +47,8 @@
             "LnumOld": 3,
             "LnumNew": 4
           }
-        ]
+        ],
+        "EofNewline": 0
       }
     ],
     "Extended": [
diff --git a/parser/diff_test.go b/parser/diff_test.go
index fb938592..34491516 100644
--- a/parser/diff_test.go
+++ b/parser/diff_test.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"strings"
+	"testing"
 
 	"google.golang.org/protobuf/encoding/protojson"
 )
@@ -163,3 +164,34 @@ func ExampleDiffParser() {
 	//   "originalOutput": "gofmt.go:19:-type s struct { A int }"
 	// }
 }
+
+func TestSuggestionContainsEofNewline(t *testing.T) {
+	const sample = `diff --git a/addeofnewline.old.txt b/addeofnewline.new.txt
+--- a/addeofnewline.old.txt
++++ b/addeofnewline.new.txt
+@@ -1,2 +1,2 @@
+ " vim: nofixeol noendofline
+-this file will have a newline at the end
+\ No newline at end of file
++this file will have a newline at the end
+`
+
+	const strip = 1
+	p := NewDiffParser(strip)
+	diagnostics, err := p.Parse(strings.NewReader(sample))
+	if err != nil {
+		panic(err)
+	}
+
+	if len(diagnostics) != 1 {
+		t.Errorf("expected 1 diagnostic, got %d", len(diagnostics))
+	}
+	if len(diagnostics[0].Suggestions) != 1 {
+		t.Errorf("expected 1 suggestion, got %d", len(diagnostics[0].Suggestions))
+	}
+	suggestion := diagnostics[0].Suggestions[0]
+	want := "this file will have a newline at the end\n"
+	if suggestion.Text != want {
+		t.Errorf("suggestion.Text = %q, want %q", suggestion.Text, want)
+	}
+}