aboutsummaryrefslogtreecommitdiff
path: root/services
diff options
context:
space:
mode:
authorwxiaoguang2023-03-09 23:48:52 +0800
committerGitHub2023-03-09 09:48:52 -0600
commit542cec98f8c07e0f046a35f1d516807416536e74 (patch)
tree31a6811704630fb6f057aac2193bf1f510daf19e /services
parente52ac62d8e645c721060e6c9a2b0ab77eedf8fd6 (diff)
Refactor merge/update git command calls (#23366)
Follow #22568 * Remove unnecessary ToTrustedCmdArgs calls * the FAQ in #22678 * Quote: 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. * The `signArg` couldn't be empty, it's either `-S{keyID}` or `--no-gpg-sign`. * Use `signKeyID` instead, add comment "empty for no-sign, non-empty to sign" * 5-line code could be extracted to a common `NewGitCommandCommit()` to handle the `signKeyID`, but I think it's not a must, current code is clear enough.
Diffstat (limited to 'services')
-rw-r--r--services/pull/merge.go9
-rw-r--r--services/pull/merge_prepare.go19
-rw-r--r--services/pull/merge_squash.go38
3 files changed, 28 insertions, 38 deletions
diff --git a/services/pull/merge.go b/services/pull/merge.go
index afa924fc1..12e01e8ce 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -332,8 +332,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
}
func commitAndSignNoAuthor(ctx *mergeContext, message string) error {
- if err := git.NewCommand(ctx, "commit").AddArguments(ctx.signArg...).AddOptionFormat("--message=%s", message).
- Run(ctx.RunOpts()); err != nil {
+ cmdCommit := git.NewCommand(ctx, "commit").AddOptionFormat("--message=%s", message)
+ if ctx.signKeyID == "" {
+ cmdCommit.AddArguments("--no-gpg-sign")
+ } else {
+ cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
+ }
+ if err := cmdCommit.Run(ctx.RunOpts()); err != nil {
log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
}
diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go
index 2ba821961..88f6c037e 100644
--- a/services/pull/merge_prepare.go
+++ b/services/pull/merge_prepare.go
@@ -28,7 +28,7 @@ type mergeContext struct {
doer *user_model.User
sig *git.Signature
committer *git.Signature
- signArg git.TrustedCmdArgs
+ signKeyID string // empty for no-sign, non-empty to sign
env []string
}
@@ -85,12 +85,10 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
// Determine if we should sign
sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch)
if sign {
- mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"-S" + keyID})
+ mergeCtx.signKeyID = keyID
if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
mergeCtx.committer = signer
}
- } else {
- mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"--no-gpg-sign"})
}
commitTimeStr := time.Now().Format(time.RFC3339)
@@ -136,18 +134,11 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
return fmt.Errorf("Unable to close .git/info/sparse-checkout file in tmpBasePath: %w", err)
}
- gitConfigCommand := func() *git.Command {
- return git.NewCommand(ctx, "config", "--local")
- }
-
setConfig := func(key, value string) error {
- if err := gitConfigCommand().AddArguments(git.ToTrustedCmdArgs([]string{key, value})...).
+ if err := git.NewCommand(ctx, "config", "--local").AddDynamicArguments(key, value).
Run(ctx.RunOpts()); err != nil {
- if value == "" {
- value = "<>"
- }
- log.Error("git config [%s -> %s ]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
- return fmt.Errorf("git config [%s -> %s ]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+ log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+ return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
}
ctx.outbuf.Reset()
ctx.errbuf.Reset()
diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go
index 0a8cc0167..d6e731498 100644
--- a/services/pull/merge_squash.go
+++ b/services/pull/merge_squash.go
@@ -14,8 +14,8 @@ import (
// doMergeStyleSquash squashes the tracking branch on the current HEAD (=base)
func doMergeStyleSquash(ctx *mergeContext, message string) error {
- cmd := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
- if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmd); err != nil {
+ cmdMerge := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
+ if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmdMerge); err != nil {
log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err)
return err
}
@@ -25,27 +25,21 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
return fmt.Errorf("LoadPoster: %w", err)
}
sig := ctx.pr.Issue.Poster.NewGitSig()
- if len(ctx.signArg) == 0 {
- if err := git.NewCommand(ctx, "commit").
- AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
- AddOptionFormat("--message=%s", message).
- Run(ctx.RunOpts()); err != nil {
- log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
- return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
- }
+ if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
+ // add trailer
+ message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
+ }
+ cmdCommit := git.NewCommand(ctx, "commit").
+ AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
+ AddOptionFormat("--message=%s", message)
+ if ctx.signKeyID == "" {
+ cmdCommit.AddArguments("--no-gpg-sign")
} else {
- if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
- // add trailer
- message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
- }
- if err := git.NewCommand(ctx, "commit").
- AddArguments(ctx.signArg...).
- AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
- AddOptionFormat("--message=%s", message).
- Run(ctx.RunOpts()); err != nil {
- log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
- return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
- }
+ cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
+ }
+ if err := cmdCommit.Run(ctx.RunOpts()); err != nil {
+ log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+ return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
}
ctx.outbuf.Reset()
ctx.errbuf.Reset()