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

Refactor legacy git init #20376

Merged
merged 10 commits into from
Aug 9, 2022
Merged

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jul 15, 2022

This PR follows:

Major changes:

  • merge CheckLFSVersion into InitFull (renamed from InitWithSyncOnce)
  • remove the Once during git init, no data-race now (although some global variables like globalCommandArgs should be eliminated, it could be done in the future)
  • for doctor sub-commands, InitFull should only be called in initialization stage

And some more hints: a behavior is changed after this PR for Git < 2.1.2 when LFS.StartServer=true:

  • Before: LFS.StartServer will be set to false when starting, and there will be an error log.
  • After: Gitea will report a fatal error to tell users that LFS requires Git >= 2.1.2 and exit.

Although it is a breaking change, it's very trivial, because Git 2.1.x is quite old (8 years ago. https://github.com/git/git/blob/master/Documentation/RelNotes/). I would assume that few people are still using it, and they should know how to handle the version problem according to the error prompt if they are upgrading Gitea to 1.18.

So, IMO it's not worth to mention such trivial breaking in changelog.

@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jul 15, 2022
@wxiaoguang wxiaoguang marked this pull request as draft July 15, 2022 02:14
@wxiaoguang wxiaoguang added the pr/wip This PR is not ready for review label Jul 15, 2022
@wxiaoguang wxiaoguang marked this pull request as ready for review July 15, 2022 03:07
@wxiaoguang wxiaoguang removed the pr/wip This PR is not ready for review label Jul 15, 2022
@wxiaoguang wxiaoguang added this to the 1.18.0 milestone Jul 15, 2022
@wxiaoguang wxiaoguang requested a review from zeripath July 15, 2022 03:07
@wxiaoguang wxiaoguang changed the title [WIP] Refactor legacy git init Refactor legacy git init Jul 15, 2022
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jul 23, 2022

@zeripath This PR follows your suggestion:

Hmm... I remember somewhere you commented about CheckLFSVersion causing problems.
I wonder if It could just be folded into the Init function?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 23, 2022
cmd/doctor.go Outdated Show resolved Hide resolved
models/migrations/migrations_test.go Outdated Show resolved Hide resolved
modules/git/git.go Outdated Show resolved Hide resolved
modules/git/git.go Outdated Show resolved Hide resolved
@wxiaoguang wxiaoguang requested a review from zeripath August 6, 2022 09:39
@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 7, 2022
cmd/doctor.go Show resolved Hide resolved
modules/git/git.go Show resolved Hide resolved
modules/git/git.go Show resolved Hide resolved
@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, 2022
@lunny
Copy link
Member

lunny commented Aug 9, 2022

make L-G-T-M work

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 9, 2022

@zeripath I would suppose that there is no more change from your side. I will merge this one.

@wxiaoguang wxiaoguang merged commit 75d96f4 into go-gitea:main Aug 9, 2022
@wxiaoguang wxiaoguang deleted the refactor-legacy-git-init branch August 9, 2022 03:22
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 9, 2022
* giteaofficial/main:
  Add support for `npm unpublish` (go-gitea#20688)
  Allow multiple files in generic packages (go-gitea#20661)
  Refactor legacy git init (go-gitea#20376)
  Fix typo in source (go-gitea#20723)
  [skip ci] Updated translations via Crowdin
  Add issue filter for Author (go-gitea#20578)
  Fix init mail render logic (go-gitea#20704)
  Frontport changelog v1.17.0 (go-gitea#20712)
  Fix disable download button (go-gitea#20701)
  docs: move search input to navbar (go-gitea#20551)
  Fix SecToTime edge-cases (go-gitea#20610)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
* merge `CheckLFSVersion` into `InitFull` (renamed from `InitWithSyncOnce`)
* remove the `Once` during git init, no data-race now
* for doctor sub-commands, `InitFull` should only be called in initialization stage

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants