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

Fix IsObjectExist with gogit #31790

Merged
merged 8 commits into from
Aug 9, 2024
Merged

Fix IsObjectExist with gogit #31790

merged 8 commits into from
Aug 9, 2024

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented Aug 7, 2024

Fix #31271.

When gogit is enabled, IsObjectExist calls repo.gogitRepo.ResolveRevision, which is not correct. It's for checking references not objects, it could work with commit hash since it's both a valid reference and a commit object, but it doesn't work with blob objects.

So it causes #31271 because it reports that all blob objects do not exist.

@wolfogre wolfogre added type/bug issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP backport/v1.22 This PR should be backported to Gitea 1.22 labels Aug 7, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 7, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 7, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 7, 2024
@wolfogre
Copy link
Member Author

wolfogre commented Aug 7, 2024 •
edited
Loading

Commit the test change first since I want to check if the unit tests could find that.

image

Weird, it's not what happened on my local env with -tags 'gogit,sqlite,sqlite_unlock_notify'

image

Hmm...
image

Wait for #31791

Here it is ~

image

@wolfogre wolfogre mentioned this pull request Aug 7, 2024
Merged
techknowlogick pushed a commit that referenced this pull request Aug 7, 2024
@wolfogre
Add TAGS to TEST_TAGS and fix bugs found with gogit (#31791)
de2787a 
Found at
#31790 (comment)

`unit-tests-gogit` never work since the workflow set `TAGS` with
`gogit`, but the Makefile use `TEST_TAGS`.

This PR adds the values of `TAGS` to `TEST_TAGS`, ensuring that setting
`TAGS` is always acceptable and avoiding confusion about which one
should be set.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Aug 7, 2024
@wolfogre @GiteaBot
Add TAGS to TEST_TAGS and fix bugs found with gogit (go-gitea#31791)
cc9a69c 
Found at
go-gitea#31790 (comment)

`unit-tests-gogit` never work since the workflow set `TAGS` with
`gogit`, but the Makefile use `TEST_TAGS`.

This PR adds the values of `TAGS` to `TEST_TAGS`, ensuring that setting
`TAGS` is always acceptable and avoiding confusion about which one
should be set.
@GiteaBot GiteaBot mentioned this pull request Aug 7, 2024
Merged
wolfogre added a commit that referenced this pull request Aug 7, 2024
@GiteaBot @wolfogre
Add TAGS to TEST_TAGS and fix bugs found with gogit (#31791) (#31795
27e4b31 
)

Backport #31791 by @wolfogre

Found at
#31790 (comment)

`unit-tests-gogit` never work since the workflow set `TAGS` with
`gogit`, but the Makefile use `TEST_TAGS`.

<img width="690" alt="image"
src="https://github.com/user-attachments/assets/fb68df49-952b-42b9-8438-44200cefff43">


![image](https://github.com/user-attachments/assets/78ff88c7-3b5f-4d50-9c58-e607bf7b1a71)

This PR adds the values of `TAGS` to `TEST_TAGS`, ensuring that setting
`TAGS` is always acceptable and avoiding confusion about which one
should be set.

After this PR:

<img width="714" alt="image"
src="https://github.com/user-attachments/assets/54cc7f38-d95b-4dbc-a87c-daba63462b86">

Co-authored-by: Jason Song <i@wolfogre.com>
@wolfogre wolfogre marked this pull request as ready for review August 8, 2024 02:46
@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 Aug 8, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2024
@wolfogre
Copy link
Member Author

wolfogre commented Aug 8, 2024 •
edited
Loading

I noticed that there are more inconsistent behaviors between gogit and nogogit editions. I know there's some reason that Gitea should use gogit for Windows platform, but it could be a really heavy maintenance work.

Maybe Gitea should give up the support to Windows platform like Gitlab. Otherwise, here is the drama story:

  1. Yes, Gitea supports Windows since Golang supports it.
  2. But, there's some performance problems with git operations on Windows.
  3. Yes, Gitea use gogit for Windows to improve performance with git operations.
  4. But, there's a bug with gogit which could CAUSE ALL YOUR LFS OBJECTS TO BE LOST!
  5. Yes, this PR will fix it.
  6. But, the lost data cannot be restored forever, and there are still some inconsistent behaviors with gogit, and no one knows if it could explode again.
  7. Yes, Gitea supports Windows.

@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 Aug 8, 2024
@silverwind
Copy link
Member

silverwind commented Aug 8, 2024 •
edited
Loading

Agree with removing native Windows support. Windows users can and should run gitea in docker instead. Doing so would also remove a ton of cruft in the code base, like the hacky Windows service code.

BTW I would also remove gogit.

@lunny
Copy link
Member

lunny commented Aug 8, 2024

Agree with removing native Windows support. Windows users can and should run gitea in docker instead. Doing so would also remove a ton of cruft in the code base, like the hacky Windows service code.

BTW I would also remove gogit.

Maybe #31754 can resolve some problems when running native nongogit windows Gitea.

@wolfogre wolfogre added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 9, 2024
@wolfogre wolfogre enabled auto-merge (squash) August 9, 2024 02:11
@wolfogre wolfogre merged commit f4d3120 into go-gitea:main Aug 9, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Aug 9, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Aug 9, 2024
@wolfogre @GiteaBot
Fix IsObjectExist with gogit (go-gitea#31790)
5a4457d 
Fix go-gitea#31271.

When gogit is enabled, `IsObjectExist` calls
`repo.gogitRepo.ResolveRevision`, which is not correct. It's for
checking references not objects, it could work with commit hash since
it's both a valid reference and a commit object, but it doesn't work
with blob objects.

So it causes go-gitea#31271 because it reports that all blob objects do not
exist.
@GiteaBot GiteaBot mentioned this pull request Aug 9, 2024
Merged
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Aug 9, 2024
wolfogre added a commit that referenced this pull request Aug 9, 2024
@GiteaBot @wolfogre
Fix IsObjectExist with gogit (#31790) (#31806)
144648a 
Backport #31790 by @wolfogre

Fix #31271.

When gogit is enabled, `IsObjectExist` calls
`repo.gogitRepo.ResolveRevision`, which is not correct. It's for
checking references not objects, it could work with commit hash since
it's both a valid reference and a commit object, but it doesn't work
with blob objects.

So it causes #31271 because it reports that all blob objects do not
exist.

Co-authored-by: Jason Song <i@wolfogre.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 11, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
494abf6 
* giteaofficial/main:
  Show lock owner instead of repo owner on LFS setting page (go-gitea#31788)
  Move repository visibility to danger zone in the settings area (go-gitea#31126)
  [skip ci] Updated translations via Crowdin
  Add types to various low-level functions (go-gitea#31781)
  Add warning message in merge instructions when `AutodetectManualMerge` was not enabled (go-gitea#31805)
  Show latest run when visit /run/latest (go-gitea#31808)
  Fix typo for `LOG_COMPRESSION` in ini (go-gitea#31809)
  Add label `docs-update-needed` for PRs that modify `app.example.ini` (go-gitea#31810)
  Fix `IsObjectExist` with gogit (go-gitea#31790)
  Support compression for Actions logs (go-gitea#31761)
  Add issue comment when moving issues from one column to another of the project (go-gitea#29311)
  [skip ci] Updated translations via Crowdin
  Fix RPM resource leak (go-gitea#31794)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny approved these changes

@lafriks lafriks lafriks approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

Garbage Collection LFS Pointer deletes inuse LFS Objects
5 participants
@wolfogre @silverwind @lunny @lafriks @GiteaBot