Refactor git module, make Gitea use internal git config

[wxiaoguang]

Related issues

This PR implements #19709

• Proposal: make Gitea use its own customized global config #19709

Close #19709

Before, Gitea uses current user's $HOME/.gitconfig. The config may conflict, some config options may affect security, and it's difficult for Gitea to maintain its own config options.

Details about this PR

1. Make Gitea use its internal customized git config, by setting a separate HOME for git command. The system git config (aka /etc/gitconfig) is still read and used at the moment.
2. Refactor the git config related functions, add test code for them.
3. Remove the unnecessary calls to git.LoadGitVersion, in fact the CheckGitVersionAtLeast already handles versions correctly.
4. The old code cmd/serv.go didn't call git.Init, did't respect the git env. This PR makes the git module always be initialized and used.
5. TODO: A lot of duplicate code for PrepareTestEnv-like functions, they should be refactored later .....

About gogit

gogit doesn't support customized gitconfig, luckily, at the moment, gogit doesn't rely the gitconfig too much

So it's fine to have this PR, no affecting on gogit side in my mind. And in the future, gogit should be maintained separately, instead of mixing with native git.

Backport or not

The git module code have changed a lot since 1.16, the backport would be a little quite dirty (update: I have a feeling it's too complex, it would change a lot of modules, not worth to be backported).

⚠️ BREAKING ⚠️

Before: Gitea's git uses system gitconfig (/etc/gitconfig) and current user's global gitconfig ($HOME/.gitconfig)

After: Gitea's git uses system gitconfig (/etc/gitconfig) and internal customized global gitconfig. And, when running git, the HOME is set to its internal HOME.

If you have your own customized config for git in Gitea, you should set these configs in /etc/gitconfig or the .gitconfig in its HOME. All related home files for git command (like .gnupg) should also be put in Gitea's git home directory.

NOTICE, after #20114 , the [git].HOME_PATH will be used for Git HOME directory.

[wxiaoguang]

OK, almost done (ready for review). CI passes. The PR description has been updated with more details.

[lunny]

There is a break if he has changed

[user]
        name = Gitea
        email = gitea@fake.local

[zeripath]

Yup.

It's not enough to lock around Init* you need lock every time you read or write to a share state. That means everything that Init* touches needs to have a lock on every read of it too.

[wxiaoguang]

I am working on it now. 2 choices:

1. use Mutex to protect everything
2. use atomic.Value to load/store a struct (I am trying this, since I think it's the direction for the refactoring of the whole setting module)

[zeripath]

Please be aware that massive refactors to the settings need to wait until after 1.17 is released.

[zeripath]

And if we complete context passing then we don't actually need concurrency - because the settings could be obtained from the context hierarchy.

For example, the use of setting.AppURL etc. should just be obtainable from the context hierarchy

[wxiaoguang]

Please be aware that massive refactors to the settings need to wait until after 1.17 is released.

Sure, it will be a small change for git module only, won't affect others.

And if we complete context passing then we don't actually need concurrency - because the settings could be obtained from the context hierarchy.
For example, the use of setting.AppURL etc. should just be obtainable from the context hierarchy

I agree to use/pass the context as much as possible. However I do not think setting.AppURL / HTMLURL / ROOT_URL is the perfect use-case for the context. Using a relative URL to replace the HTMLURL will resolve the ROOT_URL problem clearly, then no need to feed the ctx to every HTMLURL call. ps: HTMLURL is not a good name in my mind 😂

[zeripath]

And if we complete context passing then we don't actually need concurrency - because the settings could be obtained from the context hierarchy.
For example, the use of setting.AppURL etc. should just be obtainable from the context hierarchy

I agree to use/pass the context as much as possible. However I do not think setting.AppURL / HTMLURL / ROOT_URL is the perfect use-case for the context. Using a relative URL to replace the HTMLURL will resolve the ROOT_URL problem clearly, then no need to feed the ctx to every HTMLURL call. ps: HTMLURL is not a good name in my mind joy

This is going off-topic but I'll put a few points here:

• IMO models.* shouldn't be doing anything with URLs - that belongs at presentation layer.
  • Similarly URL building really shouldn't be happening in the templates.
• We cannot always emit relative URLs, most of the time we can but there are plenty of places where we need to actually emit absolute URLs.
  • At the very least we will always need some way providing a Full URL either to the API, to the Web or even to the SSH.
• Unless we abandon support for submounting gitea without URL rewriting we're always going to need to be aware of Subdirs
  • I absolutely do not think we should abandon support for submounting gitea.
• If we make the AppURL etc. gettable from the context or at least dependent on the request we can make hydra heading possible.

Why do we need to propagate the context?

• We have to feed the context into every call for rendering to get the Locale.
• We have to feed the context into every call for transparent db transactions
• We have to feed the context into every call for cancellation and process support
• We have to feed the context into every call for contextual logging

The first argument to almost every function really needs to become ctx context.Context (with the second line of those functions probably almost always logger := log.GetLogger(ctx))

I know that grates and feels like it's going back to Java like behaviour - and I expect that Go2 or Go3 is going to have to come up with some way to make this less omnipresent but the benefit of just accepting this will be immense.

[wxiaoguang]

Agree with your points of views. The question in my mind is: what will the {{.HTMLURL}} become then -- to resolve the absolute URL bug (it's only a question, maybe it's too early to be answered now, it might be answered when more code is ready).

And I proposed a patch #19934 to resolve the data-race problem, it seems that it works, the first round test passes, now it's on the second round.