Fix ParsePatch function to work with quoted diff --git strings #6323
Conversation
Currently ParsePatch fails when a diff contains a quoted diff line like: diff --git "a/file" "b/file" This patch makes it properly parse the line when that happens. Fixes go-gitea#6309
Codecov Report
@@ Coverage Diff @@
## master #6323 +/- ##
==========================================
+ Coverage 38.8% 38.83% +0.03%
==========================================
Files 359 359
Lines 51175 51182 +7
==========================================
+ Hits 19857 19877 +20
+ Misses 28447 28435 -12
+ Partials 2871 2870 -1
Continue to review full report at Codecov.
|
GiteaBot
added
the
lgtm/need 2
There was a problem hiding this comment.
I wonder if there's a way of testing the whole pipeline through an integration test. But this is not necessarily required for this case. I just worry that dumping the diffs in rather than generating the diffs means that we can't add more pathological cases.
(If you follow the example in https://github.com/go-gitea/gitea/blob/master/integrations/git_test.go or https://github.com/go-gitea/gitea/blob/master/integrations/ssh_key_test.go you should be able to create story like tests.)
models/git_diff.go
Outdated
| if hasQuote { | ||
| // When /a and /b are surrounded by double quotes, we want to first | ||
| // make sure we keep everything the quotes and then strip out the leading /a /b | ||
| a = strings.Replace(line[beg:middle], "\"a/", "\"", -1) |
There was a problem hiding this comment.
Why don't we just unquote then remove the first two runes. Using replace here is just complicating things.
Consider the, admittedly unusual case, of a file called: /a/a/a/a which is git mv and editted to /b/b/b/b. Your replace will just remove the entire names. (Yes you could change the replace to only replace one time but removing the first two characters is your aim.)
There was a problem hiding this comment.
Ah yea that makes more sense/less complicated thx. Slipped my mind to change the string after unquoting. I've changed it to just remove the first 2 characters afterwards as you've suggested thx!
I agree entire integration tests are a good goal but I don't want to complicate or hold up what is otherwise just a couple line bug fix + adding a unit test here.
I'll look into understanding the integration testing more and perhaps help with something in a separate request if I can.
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
|
Can you please backport this to 1.7 release branch |
|
Sure, how do you backport it? Just create a pull request against release/1.7? |
|
@mrsdizzie yup, that’s exactly it |
|
Create new branch from release/v1.7 branch and cherry-pick commit 7780ea8 and submit PR to release/v1.7 branch |
…tea#6323) * Fix ParsePatch to work properly with quoted diff --git string Currently ParsePatch fails when a diff contains a quoted diff line like: diff --git "a/file" "b/file" This patch makes it properly parse the line when that happens. Fixes go-gitea#6309 * Add test for regular case while here * Simplify string modification
techknowlogick
added
the
backport/done
This fixes the ParsePatch function to work when a diff puts the a/ b/ filenames inside double quotes. See #6309 for more details.
Added some new tests for this function as well.