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.

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

Jump to bottom

Handle the error of a missing blob object fix #19530 #19552

Merged
merged 16 commits into from
May 2, 2022
Merged

Handle the error of a missing blob object fix #19530 #19552

merged 16 commits into from
May 2, 2022

Conversation

99rgosse
Copy link
Contributor

Following bug #19530
this is a small hack to avoid a 500 Error when PR on blob objects missing : the error was not handled if it was other than a CSV Too large.

For context there is a missing blob on my repository that render a 500 Error when trying to Pull Request between two branches. Restoring the CSV File avoid the Error.

I have the feeling that has somehow something to do with https://github.com/go-gitea/gitea/blob/main/modules/git/tree_blob_nogogit.go
but I am not enough proficient with git cat-file to ensure the handle of objects.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 29, 2022
wxiaoguang
wxiaoguang approved these changes Apr 29, 2022
View reviewed changes
routers/web/repo/compare.go Outdated Show resolved Hide resolved
@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 Apr 29, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Apr 29, 2022
@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 Apr 29, 2022
zeripath
zeripath previously requested changes Apr 29, 2022
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

the error message is lacking in sufficient detail to investigate the problem further. Please add more context.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 29, 2022 •
edited
Loading

Oh, one more thing, the returned err in baseReader, baseBlobCloser, err should also be checked. This PR only checks the err in headReader, headBlobCloser, err.

6543
6543 reviewed May 1, 2022
View reviewed changes
routers/web/repo/compare.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

@zeripath I edited on this PR directly, now it should be nearly perfect.

@lunny
Copy link
Member

lunny commented May 2, 2022

@zeripath Need your approval.

@zeripath zeripath dismissed their stale review May 2, 2022 08:06

Resolved

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@c18d8d6). Click here to learn what that means.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main   #19552   +/-   ##
=======================================
  Coverage        ?   47.39%           
=======================================
  Files           ?      952           
  Lines           ?   132660           
  Branches        ?        0           
=======================================
  Hits            ?    62878           
  Misses          ?    62202           
  Partials        ?     7580
Impacted Files Coverage Δ
routers/web/repo/compare.go 46.95% <0.00%> (ø)

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 c18d8d6...666be24. Read the comment docs.

@6543 6543 merged commit 438646e into go-gitea:main May 2, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 3, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
94f1981 
* giteaofficial/main:
  Fix some slice problems (incorrect slice length) (go-gitea#19592)
  Fix sending empty notifications (go-gitea#19589)
  Handle the error of a missing blob object fix go-gitea#19530 (go-gitea#19552)
  Remove legacy `+build:` constraint (go-gitea#19582)
  Federation: return useful statistic information for nodeinfo (go-gitea#19561)
  Upgrade required git version to 2.0 (go-gitea#19577)
  add smtp password to install page (go-gitea#17564)
  ignore DNS error when doing migration allow/block check (go-gitea#19566)
  [skip ci] Updated translations via Crowdin
  Dont overwrite err with nil & rename PullCheckingFuncs to reflect there usage (go-gitea#19572)
  Improve UI on mobile (go-gitea#19546)
  Add API to check if team has repo access (go-gitea#19540)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@99rgosse @wxiaoguang
@6543 @techknowlogick @AbdulrhmnGhanem
Handle the error of a missing blob object fix go-gitea#19530 (go-gite…
a16e8b3 
…a#19552)

* Handle the error of a missing blob object

* Show error in logs

* as per @zeripath

* Add missing error check

* Add missing error check

* Update compare.go

* Use formal code

* Update compare.go

Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: 6543 <[email protected]>
Co-authored-by: techknowlogick <[email protected]>
@KN4CK3R KN4CK3R mentioned this pull request Sep 16, 2022
Closed
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 16, 2022 •
edited
Loading

@99rgosse it seems that what we did in this PR is incorrect, the CreateCsvDiff is intended to handle nil head or nil base commit.

I have tried on my side without this PR's change (remove these err checks), then for added or deleted CSV files, the code works well. But this PR's change makes the diff for added/deleted CSV files report errors (see #21184).

So the question is, what's the root case of the 500 error on your side?

@KN4CK3R KN4CK3R mentioned this pull request Sep 16, 2022
Merged
@99rgosse
Copy link
Contributor Author

99rgosse commented Sep 16, 2022 •
edited
Loading

@wxiaoguang

So the question is, what's the root case of the 500 error on your side?
The root case of the error 500 is from a missing CSV file

Now that I see in the new PR that the "nil" blob could be handled, it should work as intended.

In any case, it's now fixed from our side ! thank you

lunny pushed a commit that referenced this pull request Sep 17, 2022
@KN4CK3R
Fix CSV diff for added/deleted files (#21189)
43c10de 
Fixes #21184
Regression of #19552

Instead of using `GetBlobByPath` I use the already existing instances.

We need more information from #19530 if that error is still present.
wxiaoguang pushed a commit to wxiaoguang/gitea that referenced this pull request Sep 17, 2022
@KN4CK3R @wxiaoguang
Fix CSV diff for added/deleted files (go-gitea#21189)
7a220b6 
Fixes go-gitea#21184
Regression of go-gitea#19552

Instead of using `GetBlobByPath` I use the already existing instances.

We need more information from go-gitea#19530 if that error is still present.
@wxiaoguang wxiaoguang mentioned this pull request Sep 17, 2022
Merged
wxiaoguang added a commit that referenced this pull request Sep 17, 2022
@wxiaoguang
Fix CSV diff for added/deleted files (#21189) (#21193)
5cb1037 
Backport #21189
Fixes #21184
Regression of #19552

Instead of using `GetBlobByPath`, use the already existing instances.
@wxiaoguang wxiaoguang mentioned this pull request Feb 17, 2023
Closed
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

@zeripath zeripath zeripath left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
8 participants
@99rgosse @wxiaoguang @lunny @codecov-commenter @zeripath @6543 @GiteaBot @techknowlogick