Skip to content

Fix how line numbers are counted in Patch#removed_lines #183

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix how line numbers are counted in Patch#removed_lines #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c8e0ab
Line numbers within patch for removed lines should increase with each…
joroshiba Feb 12, 2018
bbb441f
Use previous file line numbers for removed lines
joroshiba Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/git_diff_parser/patch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module GitDiffParser
# Parsed patch
class Patch
RANGE_INFORMATION_LINE = /^@@ .+\+(?<line_number>\d+),/
RANGE_INFORMATION_LINE = /^@@ -(?<old_line_number>\d+),.+\+(?<line_number>\d+),/
Copy link

Choose a reason for hiding this comment

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

From what I can, tell ,\d+ should be optional.

For example:

@@ -1 +1 @@
-type Query { field: String! }
+type Query { field2: String! }

I believe we can change this line to:

RANGE_INFORMATION_LINE = /^@@ -(?<old_line_number>\d+)(,\d+)? \+(?<line_number>\d+)(,\d+)?/

I wrote a failing test and fix here: cjoudrey@858a945

MODIFIED_LINE = /^\+(?!\+|\+)/
REMOVED_LINE = /^[-]/
NOT_REMOVED_LINE = /^[^-]/
Expand Down Expand Up @@ -84,7 +84,7 @@ def removed_lines
lines.each_with_index.inject([]) do |lines, (content, patch_position)|
case content
when RANGE_INFORMATION_LINE
line_number = Regexp.last_match[:line_number].to_i
line_number = Regexp.last_match[:old_line_number].to_i
when REMOVED_LINE
line = Line.new(
content: content,
Expand All @@ -93,6 +93,8 @@ def removed_lines
)
lines << line
line_number += 1
when NOT_REMOVED_LINE
Copy link

Choose a reason for hiding this comment

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

I could be wrong, but I think we should only be incrementing line_number on unchanged lines, i.e. when /^\s/.

Here is a fix and a failing test: cjoudrey@a37ad98#diff-5167fa12a2c6d40aaa8884e40efa7ac4

Explanation:

Original file:

type Query {
  a: String
  b: String
  c: String
}

Patch:

@@ -1,5 +1,5 @@
 type Query {
+  d: String
   a: String
   b: String
-  c: String
 }

The number of the line being removed here is number 4 and patch position 5.

With this current pull request, the number is 5 and patch position is 5.

line_number += 1
end

lines
Expand Down
2 changes: 1 addition & 1 deletion spec/git_diff_parser/patch_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ module GitDiffParser
patch = Patch.new(patch_body)

expect(patch.removed_lines.size).to eq(7)
expect(patch.removed_lines.map(&:number)).to eq [11, 36, 37, 38, 39, 40, 48]
expect(patch.removed_lines.map(&:number)).to eq [14, 38, 39, 40, 41, 42, 58]
expect(patch.removed_lines.map(&:patch_position)).to eq [4, 21, 22, 23, 24, 25, 36]
end

Expand Down