Skip to content

Commit c378b18

Browse files
williammartinwilliammartin
authored
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
2 parents c0f993a + 4e68a61 commit c378b18

5 files changed

Lines changed: 169 additions & 5 deletions

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
skip 'it creates a fork owned by the user running the test'
2+
3+
env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
4+
env FORK=${REPO}-fork
5+
6+
# Use gh as a credential helper
7+
exec gh auth setup-git
8+
9+
# Get the current username for the fork owner
10+
exec gh api user --jq .login
11+
stdout2env USER
12+
13+
# Create a repository with a file so it has a default branch
14+
exec gh repo create ${ORG}/${REPO} --add-readme --private
15+
16+
# Defer repo cleanup
17+
defer gh repo delete --yes ${ORG}/${REPO}
18+
19+
# Create a user fork of repository. This will be owned by USER.
20+
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
21+
sleep 5
22+
23+
# Defer repo cleanup of fork
24+
defer gh repo delete --yes ${USER}/${FORK}
25+
26+
# Retrieve fork repository information
27+
exec gh repo view ${USER}/${FORK} --json id --jq '.id'
28+
stdout2env FORK_ID
29+
30+
exec gh repo clone ${USER}/${FORK}
31+
cd ${FORK}
32+
33+
# Prepare a branch to commit
34+
exec git checkout -b feature/branch
35+
exec git commit --allow-empty -m 'Upstream Commit'
36+
# Push without setting an upstream (-u or config)
37+
exec git push upstream feature/branch
38+
39+
# Prepare an additional commit
40+
exec git commit --allow-empty -m 'Fork Commit'
41+
# Push without setting an upstream (-u or config)
42+
exec git push origin feature/branch
43+
44+
# Create the PR
45+
exec gh pr create --title 'Feature Title' --body 'Feature Body'
46+
stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1
47+
48+
# Check the PR is indeed created
49+
exec gh pr view ${USER}:feature/branch --json headRefName,headRepository,baseRefName,isCrossRepository
50+
stdout {"baseRefName":"main","headRefName":"feature/branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
skip 'it creates a fork owned by the user running the test'
2+
13
env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
24
env FORK=${REPO}-fork
35

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
skip 'it creates a fork owned by the user running the test'
2+
3+
# Setup environment variables used for testscript
4+
env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
5+
env FORK=${REPO}-fork
6+
7+
# Use gh as a credential helper
8+
exec gh auth setup-git
9+
10+
# Get the current username for the fork owner
11+
exec gh api user --jq .login
12+
stdout2env USER
13+
14+
# Create a repository to act as upstream with a file so it has a default branch
15+
exec gh repo create ${ORG}/${REPO} --add-readme --private
16+
17+
# Defer repo cleanup of upstream
18+
defer gh repo delete --yes ${ORG}/${REPO}
19+
20+
# Create a user fork of repository. This will be owned by USER.
21+
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
22+
sleep 5
23+
24+
# Defer repo cleanup of fork
25+
defer gh repo delete --yes ${USER}/${FORK}
26+
27+
# Retrieve fork repository information
28+
exec gh repo view ${USER}/${FORK} --json id --jq '.id'
29+
stdout2env FORK_ID
30+
31+
# Clone the repo
32+
exec gh repo clone ${USER}/${FORK}
33+
cd ${FORK}
34+
35+
# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork
36+
exec git checkout -b feature/branch
37+
exec git commit --allow-empty -m 'Empty Commit'
38+
exec git push -u origin feature/branch
39+
40+
# Create the PR spanning upstream and fork repositories
41+
exec gh pr create --title 'Feature Title' --body 'Feature Body'
42+
stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1
43+
44+
# Assert that the PR was created with the correct head repository and refs
45+
exec gh pr view --json headRefName,headRepository,baseRefName,isCrossRepository
46+
stdout {"baseRefName":"main","headRefName":"feature/branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true}

‎git/client.go‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -518,15 +518,56 @@ func (r RemoteTrackingRef) String() string {
518518

519519
// ParseRemoteTrackingRef parses a string of the form "refs/remotes/<remote>/<branch>" into
520520
// a RemoteTrackingBranch struct. If the string does not match this format, an error is returned.
521+
//
522+
// For now, we assume that refnames are of the format "<remote>/<branch>", where
523+
// the remote is a single path component, and branch may have many path components e.g.
524+
// "origin/my/branch" is valid as: {Remote: "origin", Branch: "my/branch"}
525+
// but "my/origin/branch" would parse incorrectly as: {Remote: "my", Branch: "origin/branch"}
526+
// I don't believe there is a way to fix this without providing the list of remotes to this function.
527+
//
528+
// It becomes particularly confusing if you have something like:
529+
//
530+
// ```
531+
// [remote "foo"]
532+
// url = https://github.com/williammartin/test-repo.git
533+
// fetch = +refs/heads/*:refs/remotes/foo/*
534+
// [remote "foo/bar"]
535+
// url = https://github.com/williammartin/test-repo.git
536+
// fetch = +refs/heads/*:refs/remotes/foo/bar/*
537+
// [branch "bar/baz"]
538+
// remote = foo
539+
// merge = refs/heads/bar/baz
540+
// [branch "baz"]
541+
// remote = foo/bar
542+
// merge = refs/heads/baz
543+
// ```
544+
//
545+
// These @{push} refs would resolve identically:
546+
//
547+
// ```
548+
// ➜ git rev-parse --symbolic-full-name baz@{push}
549+
// refs/remotes/foo/bar/baz
550+
551+
// ➜ git rev-parse --symbolic-full-name bar/baz@{push}
552+
// refs/remotes/foo/bar/baz
553+
// ```
554+
//
555+
// When using this ref, git assumes it means `remote: foo` `branch: bar/baz`.
521556
func ParseRemoteTrackingRef(s string) (RemoteTrackingRef, error) {
522-
parts := strings.Split(s, "/")
523-
if len(parts) != 4 || parts[0] != "refs" || parts[1] != "remotes" {
557+
prefix := "refs/remotes/"
558+
if !strings.HasPrefix(s, prefix) {
559+
return RemoteTrackingRef{}, fmt.Errorf("remote tracking branch must have format refs/remotes/<remote>/<branch> but was: %s", s)
560+
}
561+
562+
refName := strings.TrimPrefix(s, prefix)
563+
refNameParts := strings.SplitN(refName, "/", 2)
564+
if len(refNameParts) != 2 {
524565
return RemoteTrackingRef{}, fmt.Errorf("remote tracking branch must have format refs/remotes/<remote>/<branch> but was: %s", s)
525566
}
526567

527568
return RemoteTrackingRef{
528-
Remote: parts[2],
529-
Branch: parts[3],
569+
Remote: refNameParts[0],
570+
Branch: refNameParts[1],
530571
}, nil
531572
}
532573

‎git/client_test.go‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1151,13 +1151,38 @@ func TestRemoteTrackingRef(t *testing.T) {
11511151
wantError error
11521152
}{
11531153
{
1154-
name: "valid remote tracking ref",
1154+
name: "valid remote tracking ref without slash in branch name",
11551155
remoteTrackingRef: "refs/remotes/origin/branchName",
11561156
wantRemoteTrackingRef: RemoteTrackingRef{
11571157
Remote: "origin",
11581158
Branch: "branchName",
11591159
},
11601160
},
1161+
{
1162+
name: "valid remote tracking ref with slash in branch name",
1163+
remoteTrackingRef: "refs/remotes/origin/branch/name",
1164+
wantRemoteTrackingRef: RemoteTrackingRef{
1165+
Remote: "origin",
1166+
Branch: "branch/name",
1167+
},
1168+
},
1169+
// TODO: Uncomment when we support slashes in remote names
1170+
// {
1171+
// name: "valid remote tracking ref with slash in remote name",
1172+
// remoteTrackingRef: "refs/remotes/my/origin/branchName",
1173+
// wantRemoteTrackingRef: RemoteTrackingRef{
1174+
// Remote: "my/origin",
1175+
// Branch: "branchName",
1176+
// },
1177+
// },
1178+
// {
1179+
// name: "valid remote tracking ref with slash in remote name and branch name",
1180+
// remoteTrackingRef: "refs/remotes/my/origin/branch/name",
1181+
// wantRemoteTrackingRef: RemoteTrackingRef{
1182+
// Remote: "my/origin",
1183+
// Branch: "branch/name",
1184+
// },
1185+
// },
11611186
{
11621187
name: "incorrect parts",
11631188
remoteTrackingRef: "refs/remotes/origin",

0 commit comments

Comments
 (0)