sourcegraph / go-diff

Unified diff parser and printer for Go
https://sourcegraph.com/github.com/sourcegraph/go-diff
Other
425 stars 47 forks source link

nil-panic when parsing diffs in which lines starting with `--` were removed #54

Closed mrnugget closed 3 years ago

mrnugget commented 3 years ago

PR https://github.com/sourcegraph/go-diff/pull/53 introduced behaviour that tries to detect new file headers while parsing hunks.

The problem is that it breaks the parsing of diffs like these:

diff --git a/foobar.sql b/foobar.sql
index 55140ba..236cd59 100644
--- a/foobar.sql
+++ b/foobar.sql
@@ -1,4 +1,3 @@
 select 1;
--- this is my query
 select 2;
 select 3;

If we try to parse this we get a nil-panic:

goroutine 20 [running]:
testing.tRunner.func1.1(0x11af8c0, 0x130cc70)
        /Users/thorstenball/.asdf/installs/golang/1.15/go/src/testing/testing.go:1057 +0x30d
testing.tRunner.func1(0xc00021e480)
        /Users/thorstenball/.asdf/installs/golang/1.15/go/src/testing/testing.go:1060 +0x41a
panic(0x11af8c0, 0x130cc70)
        /Users/thorstenball/.asdf/installs/golang/1.15/go/src/runtime/panic.go:969 +0x175
github.com/sourcegraph/go-diff/diff.(*MultiFileDiffReader).ReadFile(0xc000104ee8, 0x0, 0x0, 0x0)
        /Users/thorstenball/work/go-diff/diff/parse.go:77 +0x193
github.com/sourcegraph/go-diff/diff.TestParseWithoutNilPanic(0xc00021e480)
        /Users/thorstenball/work/go-diff/diff/diff_test.go:900 +0x2e7
testing.tRunner(0xc00021e480, 0x11e81e0)
        /Users/thorstenball/.asdf/installs/golang/1.15/go/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
        /Users/thorstenball/.asdf/installs/golang/1.15/go/src/testing/testing.go:1159 +0x386
FAIL    github.com/sourcegraph/go-diff/diff     0.161s
FAIL

The nil panic comes from a missing return here:

https://github.com/sourcegraph/go-diff/blob/96789e3401eadc2247f02d416dc90be9ccd98962/diff/parse.go#L60-L64

But the underlying cause is that we abort the parsing of the diff once we hit the --.

keegancsmith commented 3 years ago

cc @sofiia-tesliuk. I think @mrnugget will spend a little bit of time trying to fix this, but more than likely we will probably revert https://github.com/sourcegraph/go-diff/pull/53 and then re-introduce it once we can do it in a way that doesn't cause panics.