Skip to content

Detect file header, when parsing Hunk #53

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

Conversation

sofiia-tesliuk
Copy link
Contributor

@sofiia-tesliuk sofiia-tesliuk commented Aug 26, 2020

The version before was failing to identify next file header (without extended lines before it), while was parsing Hunk.
Hunk reader identify line as a part of the hunk, if it starts with , + or -. Because of this, file header starting with --- , was also identified as part of the hunk.

@keegancsmith keegancsmith merged commit 7024450 into sourcegraph:master Aug 26, 2020
@keegancsmith
Copy link
Member

Thanks!

@sofiia-tesliuk sofiia-tesliuk deleted the parse_multi_file_without_extended branch August 26, 2020 09:48
@mrnugget
Copy link
Contributor

Heads up: I think this commit broke some behaviour when parsing hunk lines that do contain -- at the beginning.

Example diff:

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;

In SQL -- starts a comment and this patch deletes a comment.

If I try to parse this with a new MultiFileDiffReader I run into a nil-panic, because we don't handle an error correctly. But the underlying cause for the error is the addition here.

I'm currently investigating to see whether we can make that detect-file-header check more robust (maybe we need to read ahead and check whether the next line starts with +++).

mrnugget added a commit that referenced this pull request 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 this pull request may close these issues.

3 participants