Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 1, 2025

Fix #31113

After #22385 introduced LFS GC, it never worked due to a bug in the INI library: fields in structs embedded more than one level deep are not populated from the INI file.

This PR fixes the issue by replacing the multi-level embedded struct with a single-level struct for parsing the cron.gc_lfs configuration.

Added a new test for retrieving cron settings to demonstrate the bug in the INI package.

@lunny lunny requested a review from zeripath August 1, 2025 22:37
@lunny lunny added type/bug backport/v1.24 This PR should be backported to Gitea 1.24 labels Aug 1, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 1, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 1, 2025
@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 1, 2025
@wxiaoguang
Copy link
Contributor

But is it right? Is the Test_getCronSettings2 related to the real code?

In the future, if a change in registerGCLFS introduces new bug, the Test_getCronSettings2 still pass and lie to developers that "oh, everything is fine"?

@lunny
Copy link
Member Author

lunny commented Aug 2, 2025

Added a new test for retrieving cron settings to demonstrate the bug in the INI package.

The test just demonstrate there is a bug in the INI package.

@wxiaoguang
Copy link
Contributor

Added a new test for retrieving cron settings to demonstrate the bug in the INI package.

The test just demonstrate there is a bug in the INI package.

Why not provide a correct test for real code?

@lunny
Copy link
Member Author

lunny commented Aug 2, 2025

Added a new test for retrieving cron settings to demonstrate the bug in the INI package.

The test just demonstrate there is a bug in the INI package.

Why not provide a correct test for real code?

17e00d6 added a test for Get GC LFS configuration

@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 12, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 12, 2025
@lunny lunny enabled auto-merge (squash) August 12, 2025 05:31
@lunny lunny merged commit 90a48e9 into go-gitea:main Aug 12, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.25.0 milestone Aug 12, 2025
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 12, 2025
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Aug 12, 2025
Fix go-gitea#31113

After go-gitea#22385 introduced LFS GC, it never worked due to a bug in the INI
library: fields in structs embedded more than one level deep are not
populated from the INI file.

This PR fixes the issue by replacing the multi-level embedded struct
with a single-level struct for parsing the cron.gc_lfs configuration.

Added a new test for retrieving cron settings to demonstrate the bug in
the INI package.
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Aug 12, 2025
lafriks pushed a commit that referenced this pull request Aug 12, 2025
Backport #35198 by @lunny

Fix #31113

After #22385 introduced LFS GC, it never worked due to a bug in the INI
library: fields in structs embedded more than one level deep are not
populated from the INI file.

This PR fixes the issue by replacing the multi-level embedded struct
with a single-level struct for parsing the cron.gc_lfs configuration.

Added a new test for retrieving cron settings to demonstrate the bug in
the INI package.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@wxiaoguang wxiaoguang deleted the lunny/fix_cron_lfs-gc branch August 13, 2025 13:46
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 19, 2025
* giteaofficial/main:
  Refactor smal code snippeds in models/issues/pull.go  (go-gitea#35301)
  fix: remove duplicate IDs (go-gitea#35210)
  Add start time on perf trace because it seems some steps haven't been recorded. (go-gitea#35282)
  nix dev shell add zip (go-gitea#35300)
  [skip ci] Updated translations via Crowdin
  Fix LFS range size header response (go-gitea#35277)
  Skip "parentsigned" check when the repo is empty (go-gitea#35292)
  [skip ci] Updated translations via Crowdin
  Fix GitHub release assets URL validation (go-gitea#35287)
  nix flake use go1.25 (go-gitea#35288)
  go1.25.0 (go-gitea#35262)
  fix nix dev shell on darwin (go-gitea#35278)
  Fix token lifetime, closes go-gitea#35230 (go-gitea#35271)
  OneDev migration: fix broken migration caused by various REST API changes in OneDev 7.8.0 and later (go-gitea#35216)
  [skip ci] Updated translations via Crowdin
  Fix font-size in inline code comment preview (go-gitea#35209)
  Fix a bug where lfs gc never worked. (go-gitea#35198)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.24 This PR should be backported to Gitea 1.24 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 type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The cron job to garbage collect LFS pointers is not active
5 participants