STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat: support @{push} revision syntax

Browse source
This commit is contained in:
Kynan Ware 2025-02-25 14:42:06 -07:00
parent b61b2298d2
commit a8d01c70cd
6 changed files with 257 additions and 181 deletions
Download patch file Download diff file Expand all files Collapse all files

43
acceptance/testdata/pr/pr-create-respects-branch-pushremote.txtar vendored Normal file
View file

@ -0,0 +1,43 @@
# 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}
exec gh repo view ${ORG}/${REPO} --json id --jq '.id'
stdout2env REPO_ID
# Create a user fork of repository. This will be owned by USER.
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
# Defer repo cleanup of fork
defer gh repo delete --yes ${USER}/${FORK}
sleep 5
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 upstream/main
exec git config branch.feature-branch.pushRemote origin
exec git commit --allow-empty -m 'Empty Commit'
exec git push
# 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

48
acceptance/testdata/pr/pr-create-respects-push-destination.txtar vendored Normal file
View file

@ -0,0 +1,48 @@
# 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}
exec gh repo view ${ORG}/${REPO} --json id --jq '.id'
stdout2env REPO_ID
# Create a user fork of repository. This will be owned by USER.
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
# Defer repo cleanup of fork
defer gh repo delete --yes ${USER}/${FORK}
sleep 5
exec gh repo view ${USER}/${FORK} --json id --jq '.id'
stdout2env FORK_ID
# Clone the repo
exec gh repo clone ${USER}/${FORK}
cd ${FORK}
# Configure default push behavior so local and remote branches will be the same
exec git config push.default current
# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name
exec git checkout -b feature-branch
exec git branch --set-upstream-to origin/main
exec git rev-parse --abbrev-ref feature-branch@{upstream}
stdout origin/main
# Create the PR
exec git commit --allow-empty -m 'Empty Commit'
exec git push
exec gh pr create --title 'Feature Title' --body 'Feature Body'
stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1

43
acceptance/testdata/pr/pr-create-respects-remote-pushdefault.txtar vendored Normal file
View file

@ -0,0 +1,43 @@
# 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}
exec gh repo view ${ORG}/${REPO} --json id --jq '.id'
stdout2env REPO_ID
# Create a user fork of repository. This will be owned by USER.
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
# Defer repo cleanup of fork
defer gh repo delete --yes ${USER}/${FORK}
sleep 5
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 upstream/main
exec git config remote.pushDefault origin
exec git commit --allow-empty -m 'Empty Commit'
exec git push
# 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

45
acceptance/testdata/pr/pr-create-respects-simple-pushdefault.txtar vendored Normal file
View file

@ -0,0 +1,45 @@
# 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}
exec gh repo view ${ORG}/${REPO} --json id --jq '.id'
stdout2env REPO_ID
# Create a user fork of repository. This will be owned by USER.
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
# Defer repo cleanup of fork
defer gh repo delete --yes ${USER}/${FORK}
sleep 5
exec gh repo view ${USER}/${FORK} --json id --jq '.id'
stdout2env FORK_ID
# Clone the repo
exec gh repo clone ${USER}/${FORK}
cd ${FORK}
# Configure default push behavior so local and remote branches have to be the same
exec git config push.default simple
# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name
exec git checkout -b feature-branch origin/main
# Create the PR
exec git commit --allow-empty -m 'Empty Commit'
exec git push origin feature-branch
exec gh pr create --title 'Feature Title' --body 'Feature Body'
stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1

121
pkg/cmd/pr/create/create.go
View file

@ -518,81 +518,34 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u
return nil
}
// TODO: Replace with the finder's PullRequestRefs struct
// trackingRef represents a ref for a remote tracking branch.
type trackingRef struct {
remoteName string
branchName string
}
func (r trackingRef) String() string {
return "refs/remotes/" + r.remoteName + "/" + r.branchName
}
func mustParseTrackingRef(text string) trackingRef {
parts := strings.SplitN(string(text), "/", 4)
// The only place this is called is tryDetermineTrackingRef, where we are reconstructing
// the same tracking ref we passed in. If it doesn't match the expected format, this is a
// programmer error we want to know about, so it's ok to panic.
if len(parts) != 4 {
panic(fmt.Errorf("tracking ref should have four parts: %s", text))
}
if parts[0] != "refs" || parts[1] != "remotes" {
panic(fmt.Errorf("tracking ref should start with refs/remotes/: %s", text))
// isRemoteHeadCurrent returns true if the remote head is on the same sha as the local head.
// This is used to determine if we might need to push the local head branch to the remote.
func isRemoteHeadCurrent(gitClient *git.Client, prRefs shared.PullRequestRefs, remotes ghContext.Remotes) bool {
headRemote, err := remotes.FindByRepo(prRefs.HeadRepo.RepoOwner(), prRefs.HeadRepo.RepoName())
if err != nil {
return false
}
return trackingRef{
remoteName: parts[2],
branchName: parts[3],
}
}
// tryDetermineTrackingRef is intended to try and find a remote branch on the same commit as the currently checked out
// HEAD, i.e. the local branch. If there are multiple branches that might match, the first remote is chosen, which in
// practice is determined by the sorting algorithm applied much earlier in the process, roughly "upstream", "github", "origin",
// and then everything else unstably sorted.
func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) (trackingRef, bool) {
// To try and determine the tracking ref for a local branch, we first construct a collection of refs
// that might be tracking, given the current branch's config, and the list of known remotes.
refsForLookup := []string{"HEAD"}
if headBranchConfig.RemoteName != "" && headBranchConfig.MergeRef != "" {
tr := trackingRef{
remoteName: headBranchConfig.RemoteName,
branchName: strings.TrimPrefix(headBranchConfig.MergeRef, "refs/heads/"),
}
refsForLookup = append(refsForLookup, tr.String())
refsForLookup := []string{"HEAD", fmt.Sprintf("refs/remotes/%s/%s", headRemote, prRefs.BranchName)}
resolvedRefs, err := gitClient.ShowRefs(context.Background(), refsForLookup)
if err != nil {
return false
}
for _, remote := range remotes {
tr := trackingRef{
remoteName: remote.Name,
branchName: localBranchName,
}
refsForLookup = append(refsForLookup, tr.String())
}
// Then we ask git for details about these refs, for example, refs/remotes/origin/trunk might return a hash
// for the remote tracking branch, trunk, for the remote, origin. If there is no ref, the git client returns
// no ref information.
//
// We also first check for the HEAD ref, so that we have the hash of the currently checked out commit.
resolvedRefs, _ := gitClient.ShowRefs(context.Background(), refsForLookup)
// If there is more than one resolved ref, that means that at least one ref was found in addition to the HEAD.
// If there is more than one resolved ref, then remote head ref was resolved.
if len(resolvedRefs) > 1 {
headRef := resolvedRefs[0]
for _, r := range resolvedRefs[1:] {
// If the hash of the remote ref doesn't match the hash of HEAD then the remote branch is not in the same
// state, so it can't be used.
// If the head ref is not the same as the remote head ref, then the remote head is not current.
if r.Hash != headRef.Hash {
continue
}
// Otherwise we can parse the returned ref into a tracking ref and return that
return mustParseTrackingRef(r.Name), true
return true
}
}
return trackingRef{}, false
return false
}
func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) {
@ -628,6 +581,7 @@ func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadata
}
func NewCreateContext(opts *CreateOptions) (*CreateContext, error) {
ctx := context.Background()
httpClient, err := opts.HttpClient()
if err != nil {
return nil, err
@ -662,6 +616,7 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) {
isPushEnabled := false
headBranch := opts.HeadBranch
headBranchLabel := opts.HeadBranch
if headBranch == "" {
headBranch, err = opts.Branch()
if err != nil {
@ -686,18 +641,38 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) {
return nil, err
}
if isPushEnabled {
// TODO: This doesn't respect the @{push} revision resolution or triagular workflows assembled with
// remote.pushDefault, or branch.<branchName>.pushremote config settings. The finder's ParsePRRefs
// may be able to replace this function entirely.
if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found {
// Suppressing these errors as we have other means of computing the PullRequestRefs when these fail.
parsedPushRevision, _ := opts.GitClient.ParsePushRevision(ctx, headBranch)
remotePushDefault, err := opts.GitClient.RemotePushDefault(ctx)
if err != nil {
return nil, err
}
pushDefault, err := opts.GitClient.PushDefault(ctx)
if err != nil {
return nil, err
}
prRefs, err := shared.ParsePRRefs(headBranch, headBranchConfig, parsedPushRevision, pushDefault, remotePushDefault, baseRepo, remotes)
if err != nil {
return nil, err
}
remoteHeadCurrent := isRemoteHeadCurrent(gitClient, prRefs, remotes)
// If the remote head is up-to-date, and we have the headRef, we do not need to push anything.
if remoteHeadCurrent && prRefs.HeadRepo != nil && prRefs.BranchName != "" {
isPushEnabled = false
if r, err := remotes.FindByName(trackingRef.remoteName); err == nil {
headRepo = r
headRemote = r
headBranchLabel = trackingRef.branchName
if !ghrepo.IsSame(baseRepo, headRepo) {
headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), trackingRef.branchName)
}
headRepo = prRefs.HeadRepo
headRemote, err = remotes.FindByRepo(headRepo.RepoOwner(), headRepo.RepoName())
// TODO: KW what does an err here mean?
if err != nil {
return nil, err
}
headBranchLabel = prRefs.BranchName
if !ghrepo.IsSame(baseRepo, headRepo) {
headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), prRefs.BranchName)
}
}
}

138
pkg/cmd/pr/create/create_test.go
View file

@ -637,6 +637,9 @@ func Test_createRun(t *testing.T) {
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "")
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
@ -700,6 +703,9 @@ func Test_createRun(t *testing.T) {
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "")
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
@ -746,6 +752,9 @@ func Test_createRun(t *testing.T) {
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "")
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
@ -795,6 +804,9 @@ func Test_createRun(t *testing.T) {
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "")
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
cs.Register("git remote rename origin upstream", 0, "")
cs.Register(`git remote add origin https://github.com/monalisa/REPO.git`, 0, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
@ -854,9 +866,12 @@ func Test_createRun(t *testing.T) {
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register("git show-ref --verify", 0, heredoc.Doc(`
deadbeef HEAD
deadb00f refs/remotes/upstream/feature
deadbeef refs/remotes/origin/feature`)) // determineTrackingBranch
deadbeef HEAD
deadb00f refs/remotes/upstream/feature
deadbeef refs/remotes/origin/feature`))
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
},
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
expectedErrOut: "\nCreating pull request for monalisa:feature into master in OWNER/REPO\n\n",
@ -894,6 +909,9 @@ func Test_createRun(t *testing.T) {
deadbeef HEAD
deadbeef refs/remotes/origin/my-feat2
`)) // determineTrackingBranch
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/my-feat2")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
},
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
expectedErrOut: "\nCreating pull request for my-feat2 into master in OWNER/REPO\n\n",
@ -1075,6 +1093,9 @@ func Test_createRun(t *testing.T) {
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "")
cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "")
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
@ -1107,6 +1128,9 @@ func Test_createRun(t *testing.T) {
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "")
cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "")
cs.Register("git rev-parse --abbrev-ref feature@{push}", 0, "origin/feature")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
@ -1524,6 +1548,9 @@ func Test_createRun(t *testing.T) {
deadbeef HEAD
deadb00f refs/remotes/upstream/feature/feat2
deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch
cs.Register("git rev-parse --abbrev-ref task1@{push}", 0, "origin/task1")
cs.Register("git config remote.pushDefault", 0, "")
cs.Register("git config push.default", 0, "")
},
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n",
@ -1622,111 +1649,6 @@ func Test_createRun(t *testing.T) {
}
}
func Test_tryDetermineTrackingRef(t *testing.T) {
tests := []struct {
name string
cmdStubs func(*run.CommandStubber)
headBranchConfig git.BranchConfig
remotes context.Remotes
expectedTrackingRef trackingRef
expectedFound bool
}{
{
name: "empty",
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD")
},
headBranchConfig: git.BranchConfig{},
expectedTrackingRef: trackingRef{},
expectedFound: false,
},
{
name: "no match",
cmdStubs: func(cs *run.CommandStubber) {
cs.Register("git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature", 0, "abc HEAD\nbca refs/remotes/upstream/feature")
},
headBranchConfig: git.BranchConfig{},
remotes: context.Remotes{
&context.Remote{
Remote: &git.Remote{Name: "upstream"},
Repo: ghrepo.New("octocat", "Spoon-Knife"),
},
&context.Remote{
Remote: &git.Remote{Name: "origin"},
Repo: ghrepo.New("hubot", "Spoon-Knife"),
},
},
expectedTrackingRef: trackingRef{},
expectedFound: false,
},
{
name: "match",
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature$`, 0, heredoc.Doc(`
deadbeef HEAD
deadb00f refs/remotes/upstream/feature
deadbeef refs/remotes/origin/feature
`))
},
headBranchConfig: git.BranchConfig{},
remotes: context.Remotes{
&context.Remote{
Remote: &git.Remote{Name: "upstream"},
Repo: ghrepo.New("octocat", "Spoon-Knife"),
},
&context.Remote{
Remote: &git.Remote{Name: "origin"},
Repo: ghrepo.New("hubot", "Spoon-Knife"),
},
},
expectedTrackingRef: trackingRef{
remoteName: "origin",
branchName: "feature",
},
expectedFound: true,
},
{
name: "respect tracking config",
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/great-feat refs/remotes/origin/feature$`, 0, heredoc.Doc(`
deadbeef HEAD
deadb00f refs/remotes/origin/feature
`))
},
headBranchConfig: git.BranchConfig{
RemoteName: "origin",
MergeRef: "refs/heads/great-feat",
},
remotes: context.Remotes{
&context.Remote{
Remote: &git.Remote{Name: "origin"},
Repo: ghrepo.New("hubot", "Spoon-Knife"),
},
},
expectedTrackingRef: trackingRef{},
expectedFound: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cs, cmdTeardown := run.Stub()
defer cmdTeardown(t)
tt.cmdStubs(cs)
gitClient := &git.Client{
GhPath: "some/path/gh",
GitPath: "some/path/git",
}
ref, found := tryDetermineTrackingRef(gitClient, tt.remotes, "feature", tt.headBranchConfig)
assert.Equal(t, tt.expectedTrackingRef, ref)
assert.Equal(t, tt.expectedFound, found)
})
}
}
func Test_generateCompareURL(t *testing.T) {
tests := []struct {
name string