linden123/githaven-fork
1
1
Fork 0
forked from Shiloh/githaven
Code Pull Requests Activity

Refactor merge/update git command calls (#23366)

Browse Source 
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.
This commit is contained in:
wxiaoguang 2023-03-09 23:48:52 +08:00 committed by GitHub
parent e52ac62d8e
commit 542cec98f8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 29 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
modules/git/command.go
View File

@ -179,7 +179,7 @@ func (c *Command) AddDashesAndList(list ...string) *Command {
} }
// ToTrustedCmdArgs converts a list of strings (trusted as argument) to TrustedCmdArgs // 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 { func ToTrustedCmdArgs(args []string) TrustedCmdArgs {
ret := make(TrustedCmdArgs, len(args)) ret := make(TrustedCmdArgs, len(args))
for i, arg := range args { for i, arg := range args {

9
services/pull/merge.go
View File

@ -332,8 +332,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
} }
func commitAndSignNoAuthor(ctx *mergeContext, message string) error { func commitAndSignNoAuthor(ctx *mergeContext, message string) error {
if err := git.NewCommand(ctx, "commit").AddArguments(ctx.signArg...).AddOptionFormat("--message=%s", message). cmdCommit := git.NewCommand(ctx, "commit").AddOptionFormat("--message=%s", message)
Run(ctx.RunOpts()); err != nil { 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()) 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()) return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
} }

19
services/pull/merge_prepare.go
View File

@ -28,7 +28,7 @@ type mergeContext struct {
doer *user_model.User doer *user_model.User
sig *git.Signature sig *git.Signature
committer *git.Signature committer *git.Signature
signArg git.TrustedCmdArgs signKeyID string // empty for no-sign, non-empty to sign
env []string env []string
} }
@ -85,12 +85,10 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
// Determine if we should sign // Determine if we should sign
sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch) sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch)
if sign { 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 { if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
mergeCtx.committer = signer mergeCtx.committer = signer
} }
} else {
mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"--no-gpg-sign"})
} }
commitTimeStr := time.Now().Format(time.RFC3339) 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) 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 { 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 { Run(ctx.RunOpts()); err != nil {
if value == "" { log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
value = "<>" return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
}
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())
} }
ctx.outbuf.Reset() ctx.outbuf.Reset()
ctx.errbuf.Reset() ctx.errbuf.Reset()

38
services/pull/merge_squash.go
View File

@ -14,8 +14,8 @@ import (
// doMergeStyleSquash squashes the tracking branch on the current HEAD (=base) // doMergeStyleSquash squashes the tracking branch on the current HEAD (=base)
func doMergeStyleSquash(ctx *mergeContext, message string) error { func doMergeStyleSquash(ctx *mergeContext, message string) error {
cmd := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch) cmdMerge := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmd); err != nil { if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmdMerge); err != nil {
log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err) log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err)
return err return err
} }
@ -25,27 +25,21 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
return fmt.Errorf("LoadPoster: %w", err) return fmt.Errorf("LoadPoster: %w", err)
} }
sig := ctx.pr.Issue.Poster.NewGitSig() sig := ctx.pr.Issue.Poster.NewGitSig()
if len(ctx.signArg) == 0 { if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
if err := git.NewCommand(ctx, "commit"). // add trailer
AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email). message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
AddOptionFormat("--message=%s", message). }
Run(ctx.RunOpts()); err != nil { cmdCommit := git.NewCommand(ctx, "commit").
log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
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()) AddOptionFormat("--message=%s", message)
} if ctx.signKeyID == "" {
cmdCommit.AddArguments("--no-gpg-sign")
} else { } else {
if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() { cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
// add trailer }
message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String()) 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())
if err := git.NewCommand(ctx, "commit"). 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())
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())
}
} }
ctx.outbuf.Reset() ctx.outbuf.Reset()
ctx.errbuf.Reset() ctx.errbuf.Reset()