Merge pull request cli#10859 from cli/wm-babakks/fix-pr-create-with-remote-tracking-branch-contains-slashes

Fix pr create when branch name contains slashes

Author: williammartin

acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar

@@ -0,0 +1,50 @@
+skip 'it creates a fork owned by the user running the test'
+
+env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
+env FORK=${REPO}-fork
+
+# Use gh as a credential helper
+exec gh auth setup-git
+
+# Get the current username for the fork owner
+exec gh api user --jq .login
+stdout2env USER
+
+# Create a repository with a file so it has a default branch
+exec gh repo create ${ORG}/${REPO} --add-readme --private
+
+# Defer repo cleanup
+defer gh repo delete --yes ${ORG}/${REPO}
+
+# Create a user fork of repository. This will be owned by USER.
+exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
+sleep 5
+
+# Defer repo cleanup of fork
+defer gh repo delete --yes ${USER}/${FORK}
+
+# Retrieve fork repository information
+exec gh repo view ${USER}/${FORK} --json id --jq '.id'
+stdout2env FORK_ID
+
+exec gh repo clone ${USER}/${FORK}
+cd ${FORK}
+
+# Prepare a branch to commit
+exec git checkout -b feature/branch
+exec git commit --allow-empty -m 'Upstream Commit'
+# Push without setting an upstream (-u or config)
+exec git push upstream feature/branch
+
+# Prepare an additional commit
+exec git commit --allow-empty -m 'Fork Commit'
+# Push without setting an upstream (-u or config)
+exec git push origin feature/branch
+
+# Create the PR
+exec gh pr create --title 'Feature Title' --body 'Feature Body'
+stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1
+
+# Check the PR is indeed created
+exec gh pr view ${USER}:feature/branch --json headRefName,headRepository,baseRefName,isCrossRepository
+stdout {"baseRefName":"main","headRefName":"feature/branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true}

acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar

@@ -1,3 +1,5 @@
+skip 'it creates a fork owned by the user running the test'
+
 env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
 env FORK=${REPO}-fork
 

acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar

@@ -0,0 +1,46 @@
+skip 'it creates a fork owned by the user running the test'
+
+# Setup environment variables used for testscript
+env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
+env FORK=${REPO}-fork
+
+# Use gh as a credential helper
+exec gh auth setup-git
+
+# Get the current username for the fork owner
+exec gh api user --jq .login
+stdout2env USER
+
+# Create a repository to act as upstream with a file so it has a default branch
+exec gh repo create ${ORG}/${REPO} --add-readme --private
+
+# Defer repo cleanup of upstream
+defer gh repo delete --yes ${ORG}/${REPO}
+
+# Create a user fork of repository. This will be owned by USER.
+exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
+sleep 5
+
+# Defer repo cleanup of fork
+defer gh repo delete --yes ${USER}/${FORK}
+
+# Retrieve fork repository information
+exec gh repo view ${USER}/${FORK} --json id --jq '.id'
+stdout2env FORK_ID
+
+# Clone the repo
+exec gh repo clone ${USER}/${FORK}
+cd ${FORK}
+
+# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork
+exec git checkout -b feature/branch
+exec git commit --allow-empty -m 'Empty Commit'
+exec git push -u origin feature/branch
+
+# Create the PR spanning upstream and fork repositories
+exec gh pr create --title 'Feature Title' --body 'Feature Body'
+stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1
+
+# Assert that the PR was created with the correct head repository and refs
+exec gh pr view --json headRefName,headRepository,baseRefName,isCrossRepository
+stdout {"baseRefName":"main","headRefName":"feature/branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true}

git/client.go

@@ -518,15 +518,56 @@ func (r RemoteTrackingRef) String() string {
 
 // ParseRemoteTrackingRef parses a string of the form "refs/remotes/<remote>/<branch>" into
 // a RemoteTrackingBranch struct. If the string does not match this format, an error is returned.
+//
+// For now, we assume that refnames are of the format "<remote>/<branch>", where
+// the remote is a single path component, and branch may have many path components e.g.
+// "origin/my/branch" is valid as: {Remote: "origin", Branch: "my/branch"}
+// but "my/origin/branch" would parse incorrectly as: {Remote: "my", Branch: "origin/branch"}
+// I don't believe there is a way to fix this without providing the list of remotes to this function.
+//
+// It becomes particularly confusing if you have something like:
+//
+// ```
+// [remote "foo"]
+// 	url = https://github.com/williammartin/test-repo.git
+// 	fetch = +refs/heads/*:refs/remotes/foo/*
+// [remote "foo/bar"]
+// 	url = https://github.com/williammartin/test-repo.git
+// 	fetch = +refs/heads/*:refs/remotes/foo/bar/*
+// [branch "bar/baz"]
+// 	remote = foo
+// 	merge = refs/heads/bar/baz
+// [branch "baz"]
+// 	remote = foo/bar
+// 	merge = refs/heads/baz
+// ```
+//
+// These @{push} refs would resolve identically:
+//
+// ```
+// ➜ git rev-parse --symbolic-full-name baz@{push}
+// refs/remotes/foo/bar/baz
+
+// ➜ git rev-parse --symbolic-full-name bar/baz@{push}
+// refs/remotes/foo/bar/baz
+// ```
+//
+// When using this ref, git assumes it means `remote: foo` `branch: bar/baz`.
 func ParseRemoteTrackingRef(s string) (RemoteTrackingRef, error) {
-	parts := strings.Split(s, "/")
-	if len(parts) != 4 || parts[0] != "refs" || parts[1] != "remotes" {
+	prefix := "refs/remotes/"
+	if !strings.HasPrefix(s, prefix) {
+		return RemoteTrackingRef{}, fmt.Errorf("remote tracking branch must have format refs/remotes/<remote>/<branch> but was: %s", s)
+	}
+
+	refName := strings.TrimPrefix(s, prefix)
+	refNameParts := strings.SplitN(refName, "/", 2)
+	if len(refNameParts) != 2 {
 		return RemoteTrackingRef{}, fmt.Errorf("remote tracking branch must have format refs/remotes/<remote>/<branch> but was: %s", s)
 	}
 
 	return RemoteTrackingRef{
-		Remote: parts[2],
-		Branch: parts[3],
+		Remote: refNameParts[0],
+		Branch: refNameParts[1],
 	}, nil
 }
 

git/client_test.go

@@ -1151,13 +1151,38 @@ func TestRemoteTrackingRef(t *testing.T) {
 			wantError             error
 		}{
 			{
-				name:              "valid remote tracking ref",
+				name:              "valid remote tracking ref without slash in branch name",
 				remoteTrackingRef: "refs/remotes/origin/branchName",
 				wantRemoteTrackingRef: RemoteTrackingRef{
 					Remote: "origin",
 					Branch: "branchName",
 				},
 			},
+			{
+				name:              "valid remote tracking ref with slash in branch name",
+				remoteTrackingRef: "refs/remotes/origin/branch/name",
+				wantRemoteTrackingRef: RemoteTrackingRef{
+					Remote: "origin",
+					Branch: "branch/name",
+				},
+			},
+			// TODO: Uncomment when we support slashes in remote names
+			// {
+			// 	name: "valid remote tracking ref with slash in remote name",
+			// 	remoteTrackingRef: "refs/remotes/my/origin/branchName",
+			// 	wantRemoteTrackingRef: RemoteTrackingRef{
+			// 		Remote: "my/origin",
+			// 		Branch: "branchName",
+			// 	},
+			// },
+			// {
+			// 	name: 			"valid remote tracking ref with slash in remote name and branch name",
+			// 	remoteTrackingRef: "refs/remotes/my/origin/branch/name",
+			// 	wantRemoteTrackingRef: RemoteTrackingRef{
+			// 		Remote: "my/origin",
+			// 		Branch: "branch/name",
+			// 	},
+			// },
 			{
 				name:                  "incorrect parts",
 				remoteTrackingRef:     "refs/remotes/origin",