Fully-qualify branch refspecs when pushing#165
Open
skarim wants to merge 2 commits into
Open
Conversation
gh-stack builds `git push` arguments from stack branch names in `internal/git`. The force path passed `<branch>:refs/heads/<branch>`, and the non-force path passed bare branch names. A git refspec treats a leading `+` as "force update", and Git allows branch names that begin with `+`, so a branch named `+feature` was parsed as refspec syntax for `feature`: the force path pushed local `feature` into remote `+feature`, and the non-force path force-updated remote `feature`. Build fully-qualified refspecs for both the source and destination of every push: `refs/heads/<branch>:refs/heads/<branch>`. A branch name can no longer be reinterpreted as a refspec modifier. Force updates are still requested via the existing `--force-with-lease` flags, whose ref names were already fully-qualified. `DeleteRemoteBranch` is fully-qualified the same way. The `Push` signature and every call site are unchanged. Add real-git integration tests covering the force and non-force paths with a `+`-prefixed branch.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens branch pushing/deletion in the git layer so that branch names beginning with + (or other refspec-significant characters) can never be reinterpreted as git refspec modifiers. Previously the force path built <branch>:refs/heads/<branch> and the non-force path passed bare branch names, so a branch named +feature was parsed as force syntax targeting feature. The change makes every push/delete use fully-qualified refs/heads/<branch> refspecs on both sides, mirroring the form gh pr create uses.
Changes:
Pushnow buildsrefs/heads/<b>:refs/heads/<b>refspecs in both the force and non-force paths.DeleteRemoteBranchdeletesrefs/heads/<branch>instead of a bare branch name.- Adds real-git integration tests covering the force and non-force paths with a
+-prefixed branch, asserting the correct remote ref is updated and the siblingfeatureref is untouched.
Show a summary per file
| File | Description |
|---|---|
internal/git/gitops.go |
Fully-qualifies refspecs in Push (force + non-force) and DeleteRemoteBranch so branch names can't be parsed as refspec syntax. |
internal/git/gitops_test.go |
Adds TestIntegration_Push_PlusPrefixedBranch_Force/_NonForce verifying a +-prefixed branch pushes its own ref without disturbing a similarly named sibling. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gh stack push,submit,sync, andlinkpush stack branches by passing their names togit push. Branch names were used as raw refspec operands: the force path built<branch>:refs/heads/<branch>and the non-force path passed bare branch names. A git refspec treats a leading+as "force update", and Git allows branch names that begin with+, so a branch named+featurewas parsed as refspec syntax forfeature— the force path pushed localfeatureinto remote+feature, and the non-force path force-updated remotefeature.This builds fully-qualified refspecs for both the source and destination of every push, so a branch name can never be reinterpreted as a refspec modifier. It mirrors the fully-qualified form
gh pr createuses.Behavior
refs/heads/<branch>:refs/heads/<branch>refspec; a leading+(or any other refspec-significant character) is treated as part of the branch name.--force-with-leaseflags — their ref names were already fully-qualified — so the force / non-force behavior ofpush,submit,sync, andlinkis otherwise unchanged.Changes
internal/git/gitops.go:Pushbuildsrefs/heads/<branch>:refs/heads/<branch>refspecs in both the force and non-force paths, replacing the bare source (<branch>:refs/heads/<branch>) and the bare branch operands.DeleteRemoteBranchdeletesrefs/heads/<branch>rather than a bare branch name.FetchBranchesalready used fully-qualified refspecs and is unchanged.The
Pushsignature and every call site (push/submit/sync/link) are unchanged.Testing
internal/git/gitops_test.go:+-prefixed branch and assert the remote+featureref receives the+featurecommit while the siblingfeatureref is left untouched.Stack created with GitHub Stacks CLI • Give Feedback 💬