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`

Author: wxiaoguang

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))
 		}
 	}
 

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

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]] = ""
+	}
+}

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())
+}

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()

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/<target-branch>/<topic-branch>
 			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()})

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)
+	})
+}