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

Prevent hang in git cat-file if repository is not a valid repository and other fixes #17991

Merged
merged 20 commits into from
Dec 16, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Dec 15, 2021

This PR contains multiple fixes. The most important of which is:

Otherwise there are a few small other fixes included which this PR was initially intending to fix:

@zeripath zeripath added this to the 1.16.0 milestone Dec 15, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 15, 2021
@lunny lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Dec 15, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request Dec 15, 2021
…ory (Partial go-gitea#17991)

Unfortunately it appears that if git cat-file is run in an invalid
repository it will hang until stdin is closed. This will result in
deadlocked /pulls pages and dangling git cat-file calls if a broken
repository is tried to be reviewed or pulls exists for a broken
repository.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added backport/v1.15 and removed skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Dec 15, 2021
Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath changed the title Various small fixes Prevent hang in git cat-file if repository is not a valid repository and other fixes Dec 15, 2021
@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 15, 2021

Issue/Pull links are still not working in the news feed:
grafik

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2021

Codecov Report

Merging #17991 (f5c33da) into main (6e7d28c) will decrease coverage by 0.02%.
The diff coverage is 24.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17991      +/-   ##
==========================================
- Coverage   45.28%   45.26%   -0.03%     
==========================================
  Files         820      820              
  Lines       90973    91046      +73     
==========================================
+ Hits        41196    41209      +13     
- Misses      43219    43274      +55     
- Partials     6558     6563       +5     
Impacted Files Coverage Δ
models/action.go 49.62% <0.00%> (-1.96%) ⬇️
models/org_team.go 57.50% <0.00%> (-0.74%) ⬇️
models/repo/repo.go 66.02% <0.00%> (-1.12%) ⬇️
models/user/user.go 61.36% <0.00%> (-0.59%) ⬇️
modules/git/repo_base_nogogit.go 80.00% <0.00%> (-6.85%) ⬇️
modules/indexer/code/bleve.go 66.52% <0.00%> (-0.84%) ⬇️
modules/indexer/code/elastic_search.go 1.61% <0.00%> (-0.02%) ⬇️
services/pull/pull.go 42.10% <0.00%> (-0.09%) ⬇️
modules/git/repo_commit_nogogit.go 56.97% <33.33%> (-1.36%) ⬇️
models/unittest/testdb.go 18.18% <38.46%> (+7.22%) ⬆️
... and 11 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 6e7d28c...f5c33da. Read the comment docs.

Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Weird behavior for cat-file to not fail (almost sounds like a bug). Let's hope that other commands won't show this behavior as well.

@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 Dec 16, 2021
@zeripath zeripath merged commit 8354670 into go-gitea:main Dec 16, 2021
@zeripath zeripath deleted the various-small-fixes branch December 16, 2021 19:01
@ryanfitzsimon
Copy link

ryanfitzsimon commented Dec 17, 2021

Hi @zeripath, could you please explain how this resolves #14734?

I can't see how these changes will help in cases of slow performance, apart from in specific cases where hangs are occurring (which isn't what was reported).
The original report described PR opening being slow, but it eventually succeeded after nearly 2 minutes.

I have a repository that suffers from the performance problems, taking minutes to open PRs with large change counts.
I've just tried running git cat-file --batch in the bare repo on the Gitea host. I also ran a git fsck. Both ran without issue.

@ryanfitzsimon
Copy link

Also, what is meant by "not a valid repository"? Directories that aren't part of a repository (no .git directory)? Or repositories that have been somehow corrupted?

The former doesn't seem to cause a hang for me:

gitserver:~$ git version
git version 2.31.1
gitserver:~$ mkdir not-a-repo
gitserver:~$ cd not-a-repo/
gitserver::~/not-a-repo$ git cat-file --batch
fatal: not a git repository (or any of the parent directories): .git

If it's specific to a certain type of corrupt repository, I'm not sure how to test it.

@lunny
Copy link
Member

lunny commented Dec 17, 2021

@ryanfitzsimon This PR should resolve part of the hang, so I have reopened #14734

lunny pushed a commit that referenced this pull request Dec 17, 2021
…ory (Partial #17991) (#17992)

* Prevent hang in git cat-file if the repository is not a valid repository (Partial #17991)

Unfortunately it appears that if git cat-file is run in an invalid
repository it will hang until stdin is closed. This will result in
deadlocked /pulls pages and dangling git cat-file calls if a broken
repository is tried to be reviewed or pulls exists for a broken
repository.

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

* placate lint

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

* fix compilation bug

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

* Add the missing directories to the testrepos

* fixup! Add the missing directories to the testrepos

* and ensure that all of the other places have the objects directories too

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

Co-authored-by: techknowlogick <[email protected]>
@zeripath
Copy link
Contributor Author

Ok. I had thought that we had finally found the reason for this problem but clearly you disagree. Could you update the issue and report back if things have changed on main at all?

zeripath added a commit to zeripath/gitea that referenced this pull request Dec 19, 2021
 ## [1.15.8](https://github.com/go-gitea/gitea/releases/tag/v1.15.8) - 2021-12-19

* BUGFIXES
  * Reset locale on login (go-gitea#18023) (go-gitea#18025)
  * Fix reset password email template (go-gitea#17025) (go-gitea#18022)
  * Fix outType on gitea dump (go-gitea#18000) (go-gitea#18016)
  * Ensure complexity, minlength and isPwned are checked on password setting (go-gitea#18005) (go-gitea#18015)
  * Fix rename notification bug (go-gitea#18011)
  * Prevent double decoding of % in url params  (go-gitea#17997) (go-gitea#18001)
  * Prevent hang in git cat-file if the repository is not a valid repository (Partial go-gitea#17991) (go-gitea#17992)
  * Prevent deadlock in create issue (go-gitea#17970) (go-gitea#17982)
* TESTING
  * Use non-expiring key. (go-gitea#17984) (go-gitea#17985)

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath mentioned this pull request Dec 19, 2021
lafriks pushed a commit that referenced this pull request Dec 20, 2021
## [1.15.8](https://github.com/go-gitea/gitea/releases/tag/v1.15.8) - 2021-12-19

* BUGFIXES
  * Reset locale on login (#18023) (#18025)
  * Fix reset password email template (#17025) (#18022)
  * Fix outType on gitea dump (#18000) (#18016)
  * Ensure complexity, minlength and isPwned are checked on password setting (#18005) (#18015)
  * Fix rename notification bug (#18011)
  * Prevent double decoding of % in url params  (#17997) (#18001)
  * Prevent hang in git cat-file if the repository is not a valid repository (Partial #17991) (#17992)
  * Prevent deadlock in create issue (#17970) (#17982)
* TESTING
  * Use non-expiring key. (#17984) (#17985)

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

* Update CHANGELOG.md

Co-authored-by: 6543 <[email protected]>
@lunny lunny mentioned this pull request Feb 11, 2022
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…and other fixes (go-gitea#17991)

This PR contains multiple fixes. The most important of which is:

* Prevent hang in git cat-file if the repository is not a valid repository 
    
    Unfortunately it appears that if git cat-file is run in an invalid
    repository it will hang until stdin is closed. This will result in
    deadlocked /pulls pages and dangling git cat-file calls if a broken
    repository is tried to be reviewed or pulls exists for a broken
    repository.

    Fix go-gitea#14734
    Fix go-gitea#9271
    Fix go-gitea#16113

Otherwise there are a few small other fixes included which this PR was initially intending to fix:

* Fix panic on partial compares due to missing PullRequestWorkInProgressPrefixes
* Fix links on pulls pages  due to regression from go-gitea#17551 - by making most /issues routes match /pulls too - Fix go-gitea#17983
* Fix links on feeds pages due to another regression from go-gitea#17551 but also fix issue with syncing tags - Fix go-gitea#17943
* Add missing locale entries for oauth group claims
* Prevent NPEs if ColorFormat is called on nil users, repos or teams.
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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
8 participants