From 542cec98f8c07e0f046a35f1d516807416536e74 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 9 Mar 2023 23:48:52 +0800 Subject: [PATCH] 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. --- modules/git/command.go | 2 +- services/pull/merge.go | 9 ++++++-- services/pull/merge_prepare.go | 19 +++++------------ services/pull/merge_squash.go | 38 ++++++++++++++-------------------- 4 files changed, 29 insertions(+), 39 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index 0bc8103116a0..9a65279a8cb5 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -179,7 +179,7 @@ func (c *Command) AddDashesAndList(list ...string) *Command { } // ToTrustedCmdArgs converts a list of strings (trusted as argument) to TrustedCmdArgs -// In most cases, it shouldn't be used. Use AddXxx function instead +// In most cases, it shouldn't be used. Use NewCommand().AddXxx() function instead func ToTrustedCmdArgs(args []string) TrustedCmdArgs { ret := make(TrustedCmdArgs, len(args)) for i, arg := range args { diff --git a/services/pull/merge.go b/services/pull/merge.go index afa924fc109a..12e01e8ce74a 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 2ba821961a2b..88f6c037ebc9 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 0a8cc0167e40..d6e7314988d3 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()