Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mrnugget opened this issue Sep 21, 2020 · 1 comment · Fixed by #55
Closed

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

mrnugget opened this issue Sep 21, 2020 · 1 comment · Fixed by #55
Assignees

Comments

@mrnugget
Copy link
Contributor

PR #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:

go-diff/diff/parse.go

Lines 60 to 64 in 96789e3

case *ParseError:
if e.Err == ErrNoFileHeader || e.Err == ErrExtendedHeadersEOF {
return nil, io.EOF
}

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

@mrnugget mrnugget self-assigned this Sep 21, 2020
@keegancsmith
Copy link
Member

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 #53 and then re-introduce it once we can do it in a way that doesn't cause panics.

mrnugget added a commit that referenced this issue Sep 28, 2020
This fixes #54 by making the detection of non-extended file headers
(which start with `---` directly) that was introduced in #53 more
robust.

Instead of simply aborting when the current line starts with `---`
(which is a valid hunk line, if you, say, remove a line starting with
`--`), we confirm that the next line also starts with `+++` by peeking a
bit ahead.

That's also what `git` does: https://sourcegraph.com/github.com/git/git/-/blob/apply.c#L1574-1576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants