From afa8dd45af29f529f3695b4d2bab7ed98ac830db Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 12 Oct 2024 13:42:10 +0800 Subject: [PATCH] Make git push options accept short name (#32245) Just like what most CLI parsers do: `--opt` means `opt=true` Then users could use `-o force-push` as `-o force-push=true` --- cmd/hook.go | 13 ++----- modules/private/hook.go | 21 ---------- modules/private/pushoptions.go | 45 ++++++++++++++++++++++ modules/private/pushoptions_test.go | 30 +++++++++++++++ routers/private/hook_post_receive.go | 2 +- services/agit/agit.go | 25 ++++++------ tests/integration/git_test.go | 57 ++++++++++++++++++++++++++++ 7 files changed, 149 insertions(+), 44 deletions(-) create mode 100644 modules/private/pushoptions.go create mode 100644 modules/private/pushoptions_test.go diff --git a/cmd/hook.go b/cmd/hook.go index 11d0d072c9..578380ab40 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -591,8 +591,9 @@ Gitea or set your environment appropriately.`, "") // S: ... ... // S: flush-pkt hookOptions := private.HookOptions{ - UserName: pusherName, - UserID: pusherID, + UserName: pusherName, + UserID: pusherID, + GitPushOptions: make(map[string]string), } hookOptions.OldCommitIDs = make([]string, 0, hookBatchSize) hookOptions.NewCommitIDs = make([]string, 0, hookBatchSize) @@ -617,8 +618,6 @@ Gitea or set your environment appropriately.`, "") hookOptions.RefFullNames = append(hookOptions.RefFullNames, git.RefName(t[2])) } - hookOptions.GitPushOptions = make(map[string]string) - if hasPushOptions { for { rs, err = readPktLine(ctx, reader, pktLineTypeUnknow) @@ -629,11 +628,7 @@ Gitea or set your environment appropriately.`, "") if rs.Type == pktLineTypeFlush { break } - - kv := strings.SplitN(string(rs.Data), "=", 2) - if len(kv) == 2 { - hookOptions.GitPushOptions[kv[0]] = kv[1] - } + hookOptions.GitPushOptions.AddFromKeyValue(string(rs.Data)) } } diff --git a/modules/private/hook.go b/modules/private/hook.go index 49d9298744..745c200619 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -7,11 +7,9 @@ import ( "context" "fmt" "net/url" - "strconv" "time" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" ) @@ -24,25 +22,6 @@ const ( GitPushOptionCount = "GIT_PUSH_OPTION_COUNT" ) -// GitPushOptions is a wrapper around a map[string]string -type GitPushOptions map[string]string - -// GitPushOptions keys -const ( - GitPushOptionRepoPrivate = "repo.private" - GitPushOptionRepoTemplate = "repo.template" -) - -// Bool checks for a key in the map and parses as a boolean -func (g GitPushOptions) Bool(key string) optional.Option[bool] { - if val, ok := g[key]; ok { - if b, err := strconv.ParseBool(val); err == nil { - return optional.Some(b) - } - } - return optional.None[bool]() -} - // HookOptions represents the options for the Hook calls type HookOptions struct { OldCommitIDs []string diff --git a/modules/private/pushoptions.go b/modules/private/pushoptions.go new file mode 100644 index 0000000000..7616e6b941 --- /dev/null +++ b/modules/private/pushoptions.go @@ -0,0 +1,45 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package private + +import ( + "strconv" + "strings" + + "code.gitea.io/gitea/modules/optional" +) + +// GitPushOptions is a wrapper around a map[string]string +type GitPushOptions map[string]string + +// GitPushOptions keys +const ( + GitPushOptionRepoPrivate = "repo.private" + GitPushOptionRepoTemplate = "repo.template" + GitPushOptionForcePush = "force-push" +) + +// Bool checks for a key in the map and parses as a boolean +// An option without value is considered true, eg: "-o force-push" or "-o repo.private" +func (g GitPushOptions) Bool(key string) optional.Option[bool] { + if val, ok := g[key]; ok { + if val == "" { + return optional.Some(true) + } + if b, err := strconv.ParseBool(val); err == nil { + return optional.Some(b) + } + } + return optional.None[bool]() +} + +// AddFromKeyValue adds a key value pair to the map by "key=value" format or "key" for empty value +func (g GitPushOptions) AddFromKeyValue(line string) { + kv := strings.SplitN(line, "=", 2) + if len(kv) == 2 { + g[kv[0]] = kv[1] + } else { + g[kv[0]] = "" + } +} diff --git a/modules/private/pushoptions_test.go b/modules/private/pushoptions_test.go new file mode 100644 index 0000000000..98b1552212 --- /dev/null +++ b/modules/private/pushoptions_test.go @@ -0,0 +1,30 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package private + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGitPushOptions(t *testing.T) { + o := GitPushOptions{} + + v := o.Bool("no-such") + assert.False(t, v.Has()) + assert.False(t, v.Value()) + + o.AddFromKeyValue("opt1=a=b") + o.AddFromKeyValue("opt2=false") + o.AddFromKeyValue("opt3=true") + o.AddFromKeyValue("opt4") + + assert.Equal(t, "a=b", o["opt1"]) + assert.False(t, o.Bool("opt1").Value()) + assert.True(t, o.Bool("opt2").Has()) + assert.False(t, o.Bool("opt2").Value()) + assert.True(t, o.Bool("opt3").Value()) + assert.True(t, o.Bool("opt4").Value()) +} diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 5c01216356..8d12b7a953 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -208,7 +208,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { return } - cols := make([]string, 0, len(opts.GitPushOptions)) + cols := make([]string, 0, 2) if isPrivate.Has() { repo.IsPrivate = isPrivate.Value() diff --git a/services/agit/agit.go b/services/agit/agit.go index 52a70469e0..82aa2791aa 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "os" - "strconv" "strings" issues_model "code.gitea.io/gitea/models/issues" @@ -24,10 +23,10 @@ import ( // ProcReceive handle proc receive work func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, opts *private.HookOptions) ([]private.HookProcReceiveRefResult, error) { results := make([]private.HookProcReceiveRefResult, 0, len(opts.OldCommitIDs)) + forcePush := opts.GitPushOptions.Bool(private.GitPushOptionForcePush) topicBranch := opts.GitPushOptions["topic"] - forcePush, _ := strconv.ParseBool(opts.GitPushOptions["force-push"]) title := strings.TrimSpace(opts.GitPushOptions["title"]) - description := strings.TrimSpace(opts.GitPushOptions["description"]) // TODO: Add more options? + description := strings.TrimSpace(opts.GitPushOptions["description"]) objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName) userName := strings.ToLower(opts.UserName) @@ -56,19 +55,19 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } baseBranchName := opts.RefFullNames[i].ForBranchName() - curentTopicBranch := "" + currentTopicBranch := "" if !gitRepo.IsBranchExist(baseBranchName) { // try match refs/for// for p, v := range baseBranchName { if v == '/' && gitRepo.IsBranchExist(baseBranchName[:p]) && p != len(baseBranchName)-1 { - curentTopicBranch = baseBranchName[p+1:] + currentTopicBranch = baseBranchName[p+1:] baseBranchName = baseBranchName[:p] break } } } - if len(topicBranch) == 0 && len(curentTopicBranch) == 0 { + if len(topicBranch) == 0 && len(currentTopicBranch) == 0 { results = append(results, private.HookProcReceiveRefResult{ OriginalRef: opts.RefFullNames[i], OldOID: opts.OldCommitIDs[i], @@ -78,18 +77,18 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. continue } - if len(curentTopicBranch) == 0 { - curentTopicBranch = topicBranch + if len(currentTopicBranch) == 0 { + currentTopicBranch = topicBranch } // because different user maybe want to use same topic, // So it's better to make sure the topic branch name - // has user name prefix + // has username prefix var headBranch string - if !strings.HasPrefix(curentTopicBranch, userName+"/") { - headBranch = userName + "/" + curentTopicBranch + if !strings.HasPrefix(currentTopicBranch, userName+"/") { + headBranch = userName + "/" + currentTopicBranch } else { - headBranch = curentTopicBranch + headBranch = currentTopicBranch } pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, repo.ID, headBranch, baseBranchName, issues_model.PullRequestFlowAGit) @@ -178,7 +177,7 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. continue } - if !forcePush { + if !forcePush.Value() { output, _, err := git.NewCommand(ctx, "rev-list", "--max-count=1"). AddDynamicArguments(oldCommitID, "^"+opts.NewCommitIDs[i]). RunStdString(&git.RunOpts{Dir: repo.RepoPath(), Env: os.Environ()}) diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index f024d22c4a..76db3c6932 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -5,6 +5,7 @@ package integration import ( "bytes" + "context" "crypto/rand" "encoding/hex" "fmt" @@ -943,3 +944,59 @@ func TestDataAsync_Issue29101(t *testing.T) { defer r2.Close() }) } + +func TestAgitPullPush(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + baseAPITestContext := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + + u.Path = baseAPITestContext.GitPath() + u.User = url.UserPassword("user2", userPassword) + + dstPath := t.TempDir() + doGitClone(dstPath, u)(t) + + gitRepo, err := git.OpenRepository(context.Background(), dstPath) + assert.NoError(t, err) + defer gitRepo.Close() + + doGitCreateBranch(dstPath, "test-agit-push") + + // commit 1 + _, err = generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") + assert.NoError(t, err) + + // push to create an agit pull request + err = git.NewCommand(git.DefaultContext, "push", "origin", + "-o", "title=test-title", "-o", "description=test-description", + "HEAD:refs/for/master/test-agit-push", + ).Run(&git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + // check pull request exist + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: 1, Flow: issues_model.PullRequestFlowAGit, HeadBranch: "user2/test-agit-push"}) + assert.NoError(t, pr.LoadIssue(db.DefaultContext)) + assert.Equal(t, "test-title", pr.Issue.Title) + assert.Equal(t, "test-description", pr.Issue.Content) + + // commit 2 + _, err = generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-2-") + assert.NoError(t, err) + + // push 2 + err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-agit-push").Run(&git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + // reset to first commit + err = git.NewCommand(git.DefaultContext, "reset", "--hard", "HEAD~1").Run(&git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + + // test force push without confirm + _, stderr, err := git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-agit-push").RunStdString(&git.RunOpts{Dir: dstPath}) + assert.Error(t, err) + assert.Contains(t, stderr, "[remote rejected] HEAD -> refs/for/master/test-agit-push (request `force-push` push option)") + + // test force push with confirm + err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-agit-push", "-o", "force-push").Run(&git.RunOpts{Dir: dstPath}) + assert.NoError(t, err) + }) +}