fix: handle git branch name with special chars correctly (#39483)

Fix the bugs:
* Commit graph page doesn't show
* PR command line instructions are wrong

---------

Co-authored-by: silverwind <me@silverwind.io>

models/issues/pull.go

@@ -407,6 +407,21 @@ func (pr *PullRequest) GetGitHeadRefName() string { // TODO: make it return RefN
 	return git.RefNameFromPullIndex(pr.Index).String()
 }
 
+func (pr *PullRequest) GetInstructionsCliArgs() (ret struct {
+	BaseBranchArg  string
+	HeadBranchArg  string
+	LocalBranchArg string
+},
+) {
+	ret.BaseBranchArg = util.ShellEscape(pr.BaseBranch)
+	ret.HeadBranchArg = util.ShellEscape(pr.HeadBranch)
+	ret.LocalBranchArg = ret.HeadBranchArg
+	if pr.HeadRepo != nil && pr.HeadRepoID != pr.BaseRepoID {
+		ret.LocalBranchArg = util.ShellEscape(pr.HeadRepo.OwnerName) + "-" + ret.HeadBranchArg
+	}
+	return ret
+}
+
 // GetReviewCommentsCount returns the number of review comments made on the diff of a PR review (not including comments on commits or issues in a PR)
 func (pr *PullRequest) GetReviewCommentsCount(ctx context.Context) int {
 	opts := FindCommentsOptions{

modules/util/shellquote.go

@@ -36,6 +36,7 @@ import "strings"
 
 const (
 	tildePrefix      = '~'
+	commentPrefix    = '#'
 	needsEscape      = " \t\n|&;()<>${}[]*?!\"'`\\"
 	needsSingleQuote = "!\n"
 )
@@ -74,7 +75,7 @@ func ShellEscape(toEscape string) string {
 	}
 
 	// Now for simplicity we'll look at the rest of the string
-	if !strings.ContainsAny(toEscape[start:], needsEscape) {
+	if !strings.ContainsAny(toEscape[start:], needsEscape) && toEscape[0] != commentPrefix {
 		return toEscape
 	}
 

modules/util/shellquote_test.go

@@ -75,6 +75,10 @@ func TestShellEscape(t *testing.T) {
 			"Double quote and escape `...",
 			"~/gitea`",
 			"~/\"gitea\\`\"",
+		}, {
+			"Double quote leading #",
+			"#123",
+			`"#123"`,
 		}, {
 			"Double quotes can handle a number of things without having to escape them but not everything ...",
 			"~/<gitea> ${gitea} `gitea` [gitea] (gitea) \"gitea\" \\gitea\\ 'gitea'",

services/repository/gitgraph/graph.go

@@ -7,20 +7,26 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"strings"
 
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/setting"
 )
 
+const gitLogGraphFormatSep = "^" // disallowed char in git ref names
+
 // GetCommitGraph return a list of commit (GraphItems) from all branches
 func GetCommitGraph(ctx context.Context, gitRepo *git.Repository, page, maxAllowedColors int, hidePRRefs bool, refs, files []string) (*Graph, error) {
-	format := "DATA:%D|%H|%ad|%h|%s"
-
-	if page == 0 {
-		page = 1
-	}
+	format := "DATA:" + strings.Join([]string{
+		"%D",  // ref names without the " (", ")" wrapping.
+		"%H",  // commit hash
+		"%ad", // author date (format respects --date= option)
+		"%h",  // abbreviated commit hash
+		"%s",  // subject
+	}, gitLogGraphFormatSep)
 
+	page = max(page, 1)
 	graphCmd := gitcmd.NewCommand("log", "--graph", "--date-order", "--decorate=full")
 
 	if hidePRRefs {
@@ -31,7 +37,7 @@ func GetCommitGraph(ctx context.Context, gitRepo *git.Repository, page, maxAllow
 		graphCmd.AddArguments("--tags", "--branches")
 	}
 
-	graphCmd.AddArguments("-C", "-M", "--date=iso-strict").
+	graphCmd.AddArguments("--find-copies", "--find-renames", "--date=iso-strict").
 		AddOptionFormat("-n %d", setting.UI.GraphMaxCommitNum*page).
 		AddOptionFormat("--pretty=format:%s", format)
 

services/repository/gitgraph/graph_models.go

@@ -216,7 +216,7 @@ func parseGitTime(timeStr string) time.Time {
 
 // NewCommit creates a new commit from a provided line
 func NewCommit(row, column int, line []byte) (*Commit, error) {
-	data := bytes.SplitN(line, []byte("|"), 5)
+	data := bytes.SplitN(line, []byte(gitLogGraphFormatSep), 5)
 	if len(data) < 5 {
 		return nil, fmt.Errorf("malformed data section on line %d with commit: %s", row, string(line))
 	}

services/repository/gitgraph/graph_test.go

@@ -35,7 +35,7 @@ func BenchmarkGetCommitGraph(b *testing.B) {
 }
 
 func BenchmarkParseCommitString(b *testing.B) {
-	testString := "* DATA:|4e61bacab44e9b4730e44a6615d04098dd3a8eaf|2016-12-20 21:10:41 +0100|4e61bac|Add route for graph"
+	testString := "* DATA:^4e61bacab44e9b4730e44a6615d04098dd3a8eaf^2016-12-20 21:10:41 +0100^4e61bac^Add route for graph"
 
 	parser := &Parser{}
 	parser.Reset()
@@ -224,14 +224,14 @@ func TestParseGlyphs(t *testing.T) {
 }
 
 func TestCommitStringParsing(t *testing.T) {
-	dataFirstPart := "* DATA:|4e61bacab44e9b4730e44a6615d04098dd3a8eaf|2016-12-20 21:10:41 +0100|4e61bac|"
+	dataFirstPart := "* DATA:^4e61bacab44e9b4730e44a6615d04098dd3a8eaf^2016-12-20 21:10:41 +0100^4e61bac^"
 	tests := []struct {
 		shouldPass    bool
 		testName      string
 		commitMessage string
 	}{
 		{true, "normal", "not a fancy message"},
-		{true, "extra pipe", "An extra pipe: |"},
+		{true, "extra sep", "An extra sep"},
 		{true, "extra 'Data:'", "DATA: might be trouble"},
 	}
 

templates/repo/issue/view_content/pull_merge_instruction.tmpl

@@ -9,18 +9,15 @@
 			<h3 class="tw-m-0">{{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_title"}}</h3>
 			{{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_desc"}}
 		</div>
-		{{$localBranch := $pull.HeadBranch}}
-		{{if ne $pull.HeadRepo.ID $pull.BaseRepo.ID}}
-			{{$localBranch = print $pull.HeadRepo.OwnerName "-" $pull.HeadBranch}}
-		{{end}}
+		{{$args := $pull.GetInstructionsCliArgs}}
 		<div class="ui secondary segment tw-font-mono">
 			{{$gitRemoteName := ctx.RootData.SystemConfig.Repository.GitGuideRemoteName.Value ctx}}
 			{{if eq $pull.Flow 0}}
-			<div>git fetch -u {{if ne $pull.HeadRepo.ID $pull.BaseRepo.ID}}{{$pull.HeadRepo.HTMLURL ctx}}{{else}}{{$gitRemoteName}}{{end}} {{$pull.HeadBranch}}:{{$localBranch}}</div>
+			<div>git fetch -u {{if ne $pull.HeadRepo.ID $pull.BaseRepo.ID}}{{$pull.HeadRepo.HTMLURL ctx}}{{else}}{{$gitRemoteName}}{{end}} {{$args.HeadBranchArg}}:{{$args.LocalBranchArg}}</div>
 			{{else}}
-			<div>git fetch -u {{$gitRemoteName}} {{$pull.GetGitHeadRefName}}:{{$localBranch}}</div>
+			<div>git fetch -u {{$gitRemoteName}} {{$pull.GetGitHeadRefName}}:{{$args.LocalBranchArg}}</div>
 			{{end}}
-			<div>git checkout {{$localBranch}}</div>
+			<div>git checkout {{$args.LocalBranchArg}}</div>
 		</div>
 		{{if $data.ShowMergeInstructions}}
 		<div>
@@ -32,32 +29,32 @@
 		</div>
 		<div class="ui secondary segment tw-font-mono">
 			<div data-pull-merge-style="merge">
-				<div>git checkout {{$pull.BaseBranch}}</div>
-				<div>git merge --no-ff {{$localBranch}}</div>
+				<div>git checkout {{$args.BaseBranchArg}}</div>
+				<div>git merge --no-ff {{$args.LocalBranchArg}}</div>
 			</div>
 			<div class="tw-hidden" data-pull-merge-style="rebase">
-				<div>git checkout {{$pull.BaseBranch}}</div>
-				<div>git merge --ff-only {{$localBranch}}</div>
+				<div>git checkout {{$args.BaseBranchArg}}</div>
+				<div>git merge --ff-only {{$args.LocalBranchArg}}</div>
 			</div>
 			<div class="tw-hidden" data-pull-merge-style="rebase-merge">
-				<div>git checkout {{$localBranch}}</div>
-				<div>git rebase {{$pull.BaseBranch}}</div>
-				<div>git checkout {{$pull.BaseBranch}}</div>
-				<div>git merge --no-ff {{$localBranch}}</div>
+				<div>git checkout {{$args.LocalBranchArg}}</div>
+				<div>git rebase {{$args.BaseBranchArg}}</div>
+				<div>git checkout {{$args.BaseBranchArg}}</div>
+				<div>git merge --no-ff {{$args.LocalBranchArg}}</div>
 			</div>
 			<div class="tw-hidden" data-pull-merge-style="squash">
-				<div>git checkout {{$pull.BaseBranch}}</div>
-				<div>git merge --squash {{$localBranch}}</div>
+				<div>git checkout {{$args.BaseBranchArg}}</div>
+				<div>git merge --squash {{$args.LocalBranchArg}}</div>
 			</div>
 			<div class="tw-hidden" data-pull-merge-style="fast-forward-only">
-				<div>git checkout {{$pull.BaseBranch}}</div>
-				<div>git merge --ff-only {{$localBranch}}</div>
+				<div>git checkout {{$args.BaseBranchArg}}</div>
+				<div>git merge --ff-only {{$args.LocalBranchArg}}</div>
 			</div>
 			<div class="tw-hidden" data-pull-merge-style="manually-merged">
-				<div>git checkout {{$pull.BaseBranch}}</div>
-				<div>git merge {{$localBranch}}</div>
+				<div>git checkout {{$args.BaseBranchArg}}</div>
+				<div>git merge {{$args.LocalBranchArg}}</div>
 			</div>
-			<div>git push {{$gitRemoteName}} {{$pull.BaseBranch}}</div>
+			<div>git push {{$gitRemoteName}} {{$args.BaseBranchArg}}</div>
 		</div>
 		{{end}}
 	</div>