Skip to content

Conversation

@zeripath
Copy link
Contributor

#12771 attempted to fix diff by avoiding the git diff line as
it is possible to have an ambiguous line here.

#12254 attempted to fix diff by assuming that names would quoted
if they needed to be and if one was quoted then both would be.

Both of these were wrong.

I have now discovered --src-prefix and --dst-prefix which
means that we can set this in such a way to force the git diff
to always be unambiguous.

Therefore this PR rollsback most of the changes in #12771 and
uses these options to fix this.

Fix #13060

Signed-off-by: Andrew Thornton [email protected]

go-gitea#12771 attempted to fix diff by avoiding the git diff line as
it is possible to have an ambiguous line here.

go-gitea#12254 attempted to fix diff by assuming that names would quoted
if they needed to be and if one was quoted then both would be.

Both of these were wrong.

I have now discovered `--src-prefix` and `--dst-prefix` which
means that we can set this in such a way to force the git diff
to always be unambiguous.

Therefore this PR rollsback most of the changes in go-gitea#12771 and
uses these options to fix this.

Signed-off-by: Andrew Thornton <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 13, 2020
@lafriks
Copy link
Member

lafriks commented Oct 13, 2020

Why are rename from and rename to cases removed?

@zeripath
Copy link
Contributor Author

Because they're no longer needed. That code was added when we couldn't get the names from the git diff line - so I've stopped back to the code that was there before. I can add them back in if preferred.

Parsepatch needs rewriting in any case as can't parse anything with -- or ++ at the start of a line

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

I've confirmed that this works on git 1.8

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 14, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 14, 2020
@lafriks
Copy link
Member

lafriks commented Oct 14, 2020

🚀

@codecov-io
Copy link

Codecov Report

Merging #13136 into master will decrease coverage by 0.28%.
The diff coverage is 9.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13136      +/-   ##
==========================================
- Coverage   42.36%   42.07%   -0.29%     
==========================================
  Files         677      678       +1     
  Lines       74627    75029     +402     
==========================================
- Hits        31616    31571      -45     
- Misses      37865    38320     +455     
+ Partials     5146     5138       -8     
Impacted Files Coverage Δ
modules/migrations/base/downloader.go 17.51% <0.00%> (-0.13%) ⬇️
modules/migrations/base/pullrequest.go 0.00% <ø> (ø)
modules/migrations/git.go 43.33% <0.00%> (ø)
modules/migrations/gitea_uploader.go 6.73% <0.00%> (ø)
modules/migrations/gitlab.go 1.04% <0.00%> (ø)
modules/migrations/migrate.go 21.55% <0.00%> (ø)
modules/structs/repo.go 60.00% <ø> (+10.00%) ⬆️
modules/migrations/gitea_downloader.go 0.93% <0.93%> (ø)
modules/migrations/github.go 80.80% <40.00%> (ø)
services/gitdiff/gitdiff.go 74.88% <84.00%> (+2.03%) ⬆️
... and 14 more

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 aa73b7b...9f83834. Read the comment docs.

@lafriks lafriks merged commit edfebe6 into go-gitea:master Oct 14, 2020
@lafriks
Copy link
Member

lafriks commented Oct 14, 2020

Please send backport

@zeripath zeripath deleted the finally-fix-diff-names branch October 14, 2020 08:12
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 14, 2020
Backport go-gitea#13136

it is possible to have an ambiguous line here.

if they needed to be and if one was quoted then both would be.

Both of these were wrong.

I have now discovered `--src-prefix` and `--dst-prefix` which
means that we can set this in such a way to force the git diff
to always be unambiguous.

Therefore this PR rollsback most of the changes in go-gitea#12771 and
uses these options to fix this.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 14, 2020
lafriks pushed a commit that referenced this pull request Oct 14, 2020
Backport #13136

it is possible to have an ambiguous line here.

if they needed to be and if one was quoted then both would be.

Both of these were wrong.

I have now discovered `--src-prefix` and `--dst-prefix` which
means that we can set this in such a way to force the git diff
to always be unambiguous.

Therefore this PR rollsback most of the changes in #12771 and
uses these options to fix this.

Signed-off-by: Andrew Thornton <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Images and other binary files not shown in diff

5 participants