Skip to content
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

Fix raw diff parsing #51

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Fix raw diff parsing #51

merged 1 commit into from
Oct 18, 2019

Conversation

demett-brcm
Copy link

The old code assumed the action/path were always at fixed positions in the line. This is not the case for at least 2 reasons:

  • The length of abbreviated SHAs varies.
  • The action can be followed by a score (for renames/copies).

The new code splits by space/tab as per the format described in "git diff --help". This could still break if you have filenames containing tabs but it's certainly better.

I've also gotten rid of the made-up rename EditType and just used an add+delete instead. Without support for renaming at a higher level this seems better.

@@ -78,28 +69,10 @@ public String getPath() {
}

/**
* Returns the action performed on the file.
*/
public char getAction() {
Copy link
Member

Choose a reason for hiding this comment

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

This breaks binary compatibility by removing a public signature.
But the risk of breaking something is very low as it is not a very popular plugin and the class is package protected.
But bare that in mind for your next awesome contribution :)

@rsandell
Copy link
Member

I would have liked to see a test case for this.

@rsandell rsandell added the bug label Oct 18, 2019
@rsandell rsandell merged commit 63fd944 into jenkinsci:master Oct 18, 2019
@dsoprea
Copy link

dsoprea commented Jun 11, 2020

It looks like this PR served to fix https://issues.jenkins-ci.org/browse/JENKINS-49829 (errors like "java.lang.StringIndexOutOfBoundsException: String index out of range: -1"), correct?

@zdave
Copy link

zdave commented Jun 11, 2020

Think so yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants