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 initial commit page & binary munching problem #13249

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Oct 21, 2020

This PR fixes two issues:

  • Missing diff following binary files

There was a missing check to break out of the current file loop in ParsePatch

Fix #13248

  • 404 Errors on viewing diff for parentless commits

Unfortunately as a result of properly fixing ParsePatch the hack that
used git show <initial_commit_id> to get the diff for this failed.

This PR fixes this using the "super-secret" empty tree ref to make the
diff against.

Signed-off-by: Andrew Thornton art27@cantab.net

Unfortunately as a result of properly fixing ParsePatch the hack that
used git show <initial_commit_id> to get the diff for this failed.

This PR fixes this using the "super-secret" empty tree ref to make the
diff against.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath changed the title Fix initial commit page Fix initial commit page & binary munching problem Oct 21, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

this works for me.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 21, 2020
if len(whitespaceBehavior) != 0 {
diffArgs = append(diffArgs, whitespaceBehavior)
}
diffArgs = append(diffArgs, "4b825dc642cb6eb9a060e54bf8d69288fbee4904")
Copy link
Member

Choose a reason for hiding this comment

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

This probably should be beforeCommitID?

Copy link
Contributor Author

@zeripath zeripath Oct 21, 2020

Choose a reason for hiding this comment

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

Nope. That is the beforecommitid in this case

The commit has no parent so we use the magic empty tree ref.

Believe it or not this is what git uses internally to generate the "diff" on the git show in cases where the commit has no parents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't set this as the beforecommitid as it breaks other things lower down in the function.

@codecov-io
Copy link

Codecov Report

Merging #13249 into master will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13249      +/-   ##
==========================================
+ Coverage   42.21%   42.22%   +0.01%     
==========================================
  Files         683      683              
  Lines       75458    75463       +5     
==========================================
+ Hits        31851    31866      +15     
+ Misses      38382    38375       -7     
+ Partials     5225     5222       -3     
Impacted Files Coverage Δ
services/gitdiff/gitdiff.go 67.77% <0.00%> (-0.48%) ⬇️
modules/indexer/stats/db.go 52.17% <0.00%> (-17.40%) ⬇️
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
modules/git/repo.go 46.19% <0.00%> (-0.51%) ⬇️
services/pull/pull.go 41.27% <0.00%> (+0.49%) ⬆️
modules/queue/unique_queue_disk_channel.go 55.38% <0.00%> (+1.53%) ⬆️
services/pull/patch.go 55.68% <0.00%> (+1.70%) ⬆️
services/pull/check.go 52.55% <0.00%> (+2.91%) ⬆️
services/pull/temp_repo.go 29.78% <0.00%> (+3.19%) ⬆️
modules/charset/charset.go 73.03% <0.00%> (+4.49%) ⬆️
... and 1 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 f0fe568...c514eb9. Read the comment docs.

@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 21, 2020
Co-authored-by: 6543 <6543@obermui.de>
@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 21, 2020
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

sorry about that ^, will not reference PRs in this manner anymore, unless necessary ofc 🚀

@techknowlogick techknowlogick merged commit 327f18c into go-gitea:master Oct 21, 2020
@techknowlogick
Copy link
Member

please send backports :)

@zeripath zeripath deleted the fix-commit-page-of-initial-commit branch October 22, 2020 08:17
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 22, 2020
Backport go-gitea#13249

* Fix initial commit page

Unfortunately as a result of properly fixing ParsePatch the hack that
used git show <initial_commit_id> to get the diff for this failed.

This PR fixes this using the "super-secret" empty tree ref to make the
diff against.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Also fix go-gitea#13248

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update services/gitdiff/gitdiff.go

Co-authored-by: 6543 <6543@obermui.de>
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 22, 2020
Backport go-gitea#13249

* Fix initial commit page

Unfortunately as a result of properly fixing ParsePatch the hack that
used git show <initial_commit_id> to get the diff for this failed.

This PR fixes this using the "super-secret" empty tree ref to make the
diff against.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Also fix go-gitea#13248

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update services/gitdiff/gitdiff.go

Co-authored-by: 6543 <6543@obermui.de>
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 22, 2020
zeripath added a commit that referenced this pull request Oct 22, 2020
Backport #13249

* Fix initial commit page

Unfortunately as a result of properly fixing ParsePatch the hack that
used git show <initial_commit_id> to get the diff for this failed.

This PR fixes this using the "super-secret" empty tree ref to make the
diff against.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Also fix #13248

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update services/gitdiff/gitdiff.go

Co-authored-by: 6543 <6543@obermui.de>

Co-authored-by: 6543 <6543@obermui.de>
zeripath added a commit that referenced this pull request Oct 22, 2020
Backport #13249

* Fix initial commit page

Unfortunately as a result of properly fixing ParsePatch the hack that
used git show <initial_commit_id> to get the diff for this failed.

This PR fixes this using the "super-secret" empty tree ref to make the
diff against.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Also fix #13248

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update services/gitdiff/gitdiff.go

Co-authored-by: 6543 <6543@obermui.de>

Co-authored-by: 6543 <6543@obermui.de>
@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.

Diffs are rendered incompletely
8 participants