diff --git a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar new file mode 100644 index 000000000..542579b0a --- /dev/null +++ b/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} diff --git a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar index 52579b501..e263b0351 100644 --- a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar +++ b/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 diff --git a/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar b/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar new file mode 100644 index 000000000..395fce86a --- /dev/null +++ b/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} diff --git a/git/client.go b/git/client.go index fe2819cf0..5f547c99c 100644 --- a/git/client.go +++ b/git/client.go @@ -518,15 +518,56 @@ func (r RemoteTrackingRef) String() string { // ParseRemoteTrackingRef parses a string of the form "refs/remotes//" 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 "/", 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// 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// but was: %s", s) } return RemoteTrackingRef{ - Remote: parts[2], - Branch: parts[3], + Remote: refNameParts[0], + Branch: refNameParts[1], }, nil } diff --git a/git/client_test.go b/git/client_test.go index 3d7560228..f59b26077 100644 --- a/git/client_test.go +++ b/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",