Skip to content

Fix parsing of non-extended file headers and lines starting with -- #55

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

Merged
merged 3 commits into from
Sep 28, 2020

Conversation

mrnugget
Copy link
Contributor

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

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
@codecov
Copy link

codecov bot commented Sep 28, 2020

Codecov Report

Merging #55 into master will decrease coverage by 1.88%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
- Coverage   75.99%   74.10%   -1.89%     
==========================================
  Files           4        4              
  Lines         454      421      -33     
==========================================
- Hits          345      312      -33     
+ Misses         63       62       -1     
- Partials       46       47       +1     
Impacted Files Coverage Δ
diff/parse.go 81.16% <71.42%> (-1.04%) ⬇️
diff/print.go 35.29% <0.00%> (-6.82%) ⬇️
diff/diff.go 84.37% <0.00%> (-2.12%) ⬇️
diff/reader_util.go 84.61% <0.00%> (-2.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96789e3...837bd65. Read the comment docs.

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, 2 inline questions

Comment on lines 1 to 5
@@ -1,4 +1,3 @@
select 1;
--- this is my query
select 2;
select 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@@ -1,4 +1,3 @@
select 1;
--- this is my query
select 2;
select 3;
@@ -1,4 +1,3 @@
select 1;
--- this is my query
+++ this could break?
select 2;
select 3;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a case that works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, with the fix here or in general?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the logic above that checks the next hunk line for +++ and fails if it finds it.
Like when I change a line prefix from -- to ++, the diff would look like

line
---line
+++line
line

I guess. Would this be parseable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use this diff, it crashes:

diff --git a/foobar.sql b/foobar.sql
index 9537d7b..f73c856 100644
--- a/foobar.sql
+++ b/foobar.sql
@@ -1,5 +1,5 @@
 select 1;
--- this is my query
+++ this is my query
 select 2;
 select 3;
--- this is the last line
+++ this is the last line

I'll try to see if I can make that work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keegancsmith @eseliger what do you think of this? #56

It adds a custom type, lineReader, so we can do two-line lookahead and check whether the --- and +++ are followed by a @@ , which is what git does. It's built on top of this PR and all tests pass, including the case raised here by @eseliger.

Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some inline comments, not approving just yet :)

@mrnugget
Copy link
Contributor Author

I'm merging this and will take another stab at fixing the --- foo\n+++ bar problem in a separate PR, since that behaviour was never supported and this PR here is to intended to fix and restore old behaviour.

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.

nil-panic when parsing diffs in which lines starting with -- were removed
3 participants