Refactor git command package to improve security and maintainability

[wxiaoguang]

This PR follows #21535 (and replace #22592)

Review without space diff

https://github.com/go-gitea/gitea/pull/22678/files?diff=split&w=1

Purpose of this PR

1. Make git module command completely safe (risky user inputs won't be passed as argument option anymore)
2. Avoid low-level mistakes like Use git command instead of exec.Cmd in blame #22098 (comment)
3. Remove deprecated and dirty CmdArgCheck function, hide the CmdArg type
4. Simplify code when using git command

The main idea of this PR

• Move the git.CmdArg to the internal package, then no other package except git could use it. Then developers could never do AddArguments(git.CmdArg(userInput)) any more.
• Introduce git.ToTrustedCmdArgs, it's for user-provided and already trusted arguments. It's only used in a few cases, for example: use git arguments from config file, help unit test with some arguments.
• Introduce AddOptionValues and AddOptionFormat, they make code more clear and simple:
  • Before: AddArguments("-m").AddDynamicArguments(message)
  • After: AddOptionValues("-m", message)
  • Before: AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)))
  • After: AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email)

FAQ

Why these changes were not done in #21535 ?

#21535 is mainly a search&replace, it did its best to not change too much logic.

Making the framework better needs a lot of changes, so this separate PR is needed as the second step.

The naming of AddOptionXxx

According to git's manual, the --xxx part is called option.

How can it guarantee that internal.CmdArg won't be not misused?

Go's specification guarantees that. Trying to access other package's internal package causes compilation error.

And, golangci-lint also denies the git/internal package. Only the git/command.go can use it carefully.

There is still a ToTrustedCmdArgs, will it still allow developers to make mistakes and pass untrusted arguments?

Generally speaking, no. Because when using ToTrustedCmdArgs, the code will be very complex (see the changes for examples). Then developers and reviewers can know that something might be unreasonable.

Why there was a CmdArgCheck and why it's removed?

At the moment of #21535, to reduce unnecessary changes, CmdArgCheck was introduced as a hacky patch. Now, almost all code could be written as cmd := NewCommand(); cmd.AddXxx(...), then there is no need for CmdArgCheck anymore.

Why many codes for signArg == "" is deleted?

Because in the old code, signArg could never be empty string, it's either -S[key-id] or --no-gpg-sign. So the signArg == "" is just dead code.

[delvh]

So, if I've seen that correctly, this PR can be classified as less performant, but more secure and readable?

[wxiaoguang]

So, if I've seen that correctly, this PR can be classified as less performant, but more secure and readable?

I don't see why it's less performant. Some more function calls won't affect performance -- at least, you won't feel it in daily usage, and won't see your CPU usage gets any higher.

[codecov-commenter]

Codecov Report

Merging #22678 (b9455b3) into main (3c5655c) will increase coverage by 47.21%.
The diff coverage is 61.61%.

❗ Current head b9455b3 differs from pull request most recent head 123144e. Consider uploading reports for the commit 123144e to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##           main   #22678       +/-   ##
=========================================
+ Coverage      0   47.21%   +47.21%     
=========================================
  Files         0     1106     +1106     
  Lines         0   148779   +148779     
=========================================
+ Hits          0    70249    +70249     
- Misses        0    70177    +70177     
- Partials      0     8353     +8353     

Impacted Files	Coverage Δ
modules/git/git.go	44.62% <0.00%> (ø)
modules/git/repo_tree.go	85.00% <0.00%> (ø)
routers/web/repo/compare.go	45.41% <ø> (ø)
routers/web/repo/lfs.go	0.00% <0.00%> (ø)
services/cron/tasks_basic.go	90.22% <0.00%> (ø)
services/cron/tasks_extended.go	70.71% <0.00%> (ø)
services/mirror/mirror_push.go	44.69% <0.00%> (ø)
services/pull/patch.go	33.64% <0.00%> (ø)
services/repository/check.go	0.00% <0.00%> (ø)
services/repository/files/patch.go	0.00% <0.00%> (ø)
... and 1130 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[delvh]

Well, what I meant was: We've replaced a ton of string concatenation with fmt.Sprintf which apparently can be much slower.
Additionally, the list of arguments needs to be copied more often as it is often as it isn't initialized with all arguments anymore, more arguments are added afterwards from what I've seen.

[wxiaoguang]

We've replaced a ton of string concatenation with fmt.Sprintf

No, only a few. Most refactored codes are: git.CmdArg(fmt.Sprintf(...)) => AddOptionFormat

(OK, maybe a ton and most are just personal opinions, indeed if you do benchmark for the whole git operation, the affect is very very trivial, I do not think there would be noticeable difference)

If it's proved that the performance is really affected, there could be a AddOptionConcat("--xxx=", val) to optimize it.

the list of arguments needs to be copied more often

It won't affect the performance too much. You can try to benchmark the whole git operation to see whether it really matters.

And I wouldn't agree with code like below:

Even if it would be proved that the slice affects performance (it's impossible indeed), you can still add something like NewCommand(ctx).GrowArgsCap(100) to optimize it -- there are always feasible methods in the new framework.