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

Only log non ErrNotExist errors in git.GetNote #19884

Merged
merged 7 commits into from
Jun 7, 2022
Merged

Only log non ErrNotExist errors in git.GetNote #19884

merged 7 commits into from
Jun 7, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 4, 2022 •
edited by zeripath
Loading

When attempting to get a note for a commit we should only log non-ErrNotExist errors.

The current code included a test to check if note was not found, but the err was updated in SubTree - which itself might return ErrNotExist. Therefore we need to retest within the second if block.

Fix #19819

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. backport/v1.16 pr/wip This PR is not ready for review labels Jun 4, 2022
@zeripath
Copy link
Contributor

zeripath commented Jun 4, 2022 •
edited
Loading

Are you sure about this...

The original code checked if the err was a ErrNotExist then:

  • attempted to read the sub tree commit[0:2]
  • re-setting the err to the err.
  • then updated the path to include the sharding and the commit id.

What are you trying to fix?

Ah I understand!

I've just pushed a fix - we simply need to recheck if the error is not errnotexist before logging here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 4, 2022
@zeripath zeripath changed the title Fix GetNote Only log non ErrNotExist errors in git.GetNote Jun 4, 2022
@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 Jun 4, 2022
@zeripath zeripath removed the pr/wip This PR is not ready for review label Jun 4, 2022
@zeripath zeripath mentioned this pull request Jun 4, 2022
Closed
@lunny
Copy link
Member Author

lunny commented Jun 4, 2022

Are you sure about this...

The original code checked if the err was a ErrNotExist then:

  • attempted to read the sub tree commit[0:2]
  • re-setting the err to the err.
  • then updated the path to include the sharding and the commit id.

What are you trying to fix?

Ah I understand!

I've just pushed a fix - we simply need to recheck if the error is not errnotexist before logging here.

That's why I added a WIP label, but now I can remove it.

@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 Jun 5, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #19884 (d7cb503) into main (73382d2) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main   #19884      +/-   ##
==========================================
- Coverage   47.32%   47.30%   -0.02%     
==========================================
  Files         959      959              
  Lines      133652   133654       +2     
==========================================
- Hits        63252    63231      -21     
- Misses      62699    62722      +23     
  Partials     7701     7701
Impacted Files Coverage Δ
modules/git/notes_nogogit.go 63.07% <25.00%> (-3.59%) ⬇️
modules/auth/openid/discovery_cache.go 8.00% <0.00%> (-92.00%) ⬇️
modules/queue/workerpool.go 51.55% <0.00%> (-2.08%) ⬇️
services/pull/pull.go 40.34% <0.00%> (-0.47%) ⬇️
routers/api/v1/repo/pull.go 47.17% <0.00%> (+0.17%) ⬆️
services/pull/check.go 29.13% <0.00%> (+1.18%) ⬆️
modules/log/event.go 62.25% <0.00%> (+2.33%) ⬆️
modules/queue/queue_channel.go 81.48% <0.00%> (+2.77%) ⬆️

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 d9b50e4...d7cb503. Read the comment docs.

@Gusted Gusted added this to the 1.17.0 milestone Jun 6, 2022
@lunny
Copy link
Member Author

lunny commented Jun 7, 2022

make L-G-T-M

@lunny lunny merged commit dbe415f into go-gitea:main Jun 7, 2022
@lunny lunny deleted the lunny/fix_gitnote branch June 7, 2022 08:39
lunny added a commit to lunny/gitea that referenced this pull request Jun 7, 2022
@lunny @zeripath
Only log non ErrNotExist errors in git.GetNote (go-gitea#19884)
961cfd7 
* Fix GetNote

* Only log errors if the error is not ErrNotExist

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

Co-authored-by: Andrew Thornton <[email protected]>
@lunny lunny mentioned this pull request Jun 7, 2022
Merged
@lunny lunny added the backport/done All backports for this PR have been created label Jun 7, 2022
lunny added a commit that referenced this pull request Jun 7, 2022
@lunny @zeripath
Only log non ErrNotExist errors in git.GetNote (#19884) (#19905)
ae91913 
* Fix GetNote

* Only log errors if the error is not ErrNotExist

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

Co-authored-by: Andrew Thornton <[email protected]>

Co-authored-by: Andrew Thornton <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 9, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
320f547 
* giteaofficial/main:
  Prevent NPE whilst migrating if there is a team request review (go-gitea#19855)
  [skip ci] Updated translations via Crowdin
  Add support for rendering terminal output with colors (go-gitea#19497)
  Fix viewed images not loading in a PR (go-gitea#19919)
  Remove out-dated comments (go-gitea#19921)
  Automatically render wiki TOC (go-gitea#19873)
  Improve wording on delete access token modal (go-gitea#19909)
  [skip ci] Updated translations via Crowdin
  Add breaking email restrictions checker in doctor (go-gitea#19903)
  Ensure minimum mirror interval is reported on settings page (go-gitea#19895)
  Improve UX on modal for deleting an access token (go-gitea#19894)
  update discord invite (go-gitea#19907)
  Only log non ErrNotExist errors in git.GetNote  (go-gitea#19884)
  [skip ci] Updated translations via Crowdin
  Update frontend guideline (go-gitea#19901)
  Make AppDataPath absolute against the AppWorkPath if it is not (go-gitea#19815)
zeripath added a commit to zeripath/gitea that referenced this pull request Jun 20, 2022
@zeripath
Changelog for 1.16.9
a9ceb9f 
## [1.16.9](https://github.com/go-gitea/gitea/releases/tag/1.16.9) - 2022-06-20

* BUGFIXES
  * Fix permission check for delete tag (go-gitea#19985) (go-gitea#20001)
  * Only log non ErrNotExist errors in git.GetNote  (go-gitea#19884) (go-gitea#19905)
  *  Use exact search instead of fuzzy search for branch filter dropdown (go-gitea#19885) (go-gitea#19893)
  * Set Setpgid on child git processes (go-gitea#19865) (go-gitea#19881)
  * Import git from alpine 3.16 repository as 2.30.4 is needed for `safe.directory = '*'` to work but alpine 3.13 has 2.30.3 (go-gitea#19876)
  * Ensure responses are context.ResponseWriters (go-gitea#19843) (go-gitea#19859)
  * Fix count bug (go-gitea#19850)
  * Fix raw endpoint PDF file headers (go-gitea#19825) (go-gitea#19826)
  * Make WIP prefixes case insensitive, e.g. allow `Draft` as a WIP prefix (go-gitea#19780) (go-gitea#19811)
  * Fix NotificationUnreadCount (go-gitea#19802)
  * Prevent NPE when cache service is disabled (go-gitea#19703) (go-gitea#19783)
  * Detect truncated utf-8 characters at the end of content as still representing utf-8 (go-gitea#19773) (go-gitea#19774)
  * Fix doctor pq: syntax error at or near "." quote user table name (go-gitea#19765) (go-gitea#19770)
  * Fix bug (go-gitea#19757)

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath mentioned this pull request Jun 20, 2022
Merged
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@lunny @zeripath @AbdulrhmnGhanem
Only log non ErrNotExist errors in git.GetNote (go-gitea#19884)
0a023d7 
* Fix GetNote

* Only log errors if the error is not ErrNotExist

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

Co-authored-by: Andrew Thornton <[email protected]>
@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

@zeripath zeripath zeripath approved these changes

@Gusted Gusted Gusted approved these changes

Assignees
No one assigned
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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Unable to find git note corresponding to the commit errors in log
5 participants
@lunny @zeripath @codecov-commenter @Gusted @GiteaBot