fix(#547): parse git-push dst ref, ignore redirections + tag pushes#566
Conversation
- _lib-extract-push-ref.sh: strip shell redirections (2>&1, 2>, >, |) before token parsing so redirect operators are never mistaken for branch names - _lib-extract-push-ref.sh: for src:dst refspecs, validate the DST (right of colon) — not the src — because the dst is what lands on the remote; src may be HEAD, a SHA, or a local tracking ref that doesn't follow the branch-name convention - _lib-extract-push-ref.sh: add is_tag_push() helper that returns true for --tags, `tag <name>`, and refs/tags/ forms - validate-branch-name.sh: call is_tag_push() before extract_push_ref() and exit 0 when true — a branch-name validator has nothing to check on a tag push - test_validate_branch_name_pushref.sh: 13 new cases covering all three fix axes plus regression guard for #194 behaviour; 28 total, all pass Refs #547
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #566
Commit: 404ebc505fe8b269907f0f269c5df6a2c11c3f53
Summary
Fixes the git push parsing in _lib-extract-push-ref.sh across three bug axes (#547): (1) a src:dst refspec now validates the dst (what lands on the remote) instead of the src, so git push upstream HEAD:feature/GH-1-foo is no longer wrongly blocked on HEAD; (2) shell redirections/pipes are stripped before token parsing so 2>/>/| can't be mistaken for a branch name; (3) tag pushes (--tags, tag <name>, refs/tags/) short-circuit to a no-op via a new is_tag_push() guard called in validate-branch-name.sh before extraction. Adds 13 targeted test cases (28/28 pass).
Checklist Results
- Architecture & Design: Pass
- Code Quality: Pass
- Testing: Pass
- Security: Pass (no secrets/leaks; diff is shell + tests only)
- Performance: Pass (N/A — string parsing in a hook)
- PR Description & Glossary: Pass (5-term glossary present)
- Summary Bullet Narrative: Pass (narrative, well above label-only threshold)
- Technical Decisions (AgDR): N/A (bug fix to existing parser; no new dep/framework/pattern)
- Adopter Handbooks: N/A (no handbooks loaded for this diff)
Verification performed
I checked out the PR HEAD (404ebc5) and ran the suite myself: 28 PASS / 0 FAIL, matching the reported result. Beyond the bundled tests, I probed the lib directly against edge cases not in the suite:
- dst extraction —
HEAD:dst→dst;+HEAD:dst(force marker) →dst;refs/heads/x:refs/heads/main→main(dst taken, thenrefs/heads/stripped). All correct. - redirection stripping —
2>&1 | tail,>,2>,1>,>>all stripped;&&/;/&/|separators also handled by the residual[^|;&]*grep guard. is_tag_pushfalse-positive guards — branch names containingtag/tags(feature/GH-1-add-tag-support,feature/GH-1-tags-page,tag-team) correctly classified as BRANCH, not TAG. The--tags(\s|$)andtag\s+\Sanchors are tight.--follow-tags <branch>correctly stays BRANCH (it pushes the branch + tags, so branch validation should still apply).
The ${ref#*:} dst split, the redirection-stripping sed, and the is_tag_push guard are all shell-sound and BSD/macOS-portable (POSIX-only constructs, no \b in parameter expansion paths). No regressions for plain pushes, -u, --force, --delete, or no-arg pushes.
Issues Found
None blocking.
Suggestions (non-blocking)
- nit (cosmetic): shellcheck flags
SC2001on the twoecho | sedlines (lines 51, 128) suggesting${var//search/replace}. The regex ([0-9]*[>|]with end-anchoring) isn't expressible in bash parameter expansion, sosedis the right tool — safe to leave as-is. - nit (theoretical):
is_tag_pushuses a stricter strip pattern ([>|][&>|0-9]) thanextract_push_ref([>|]). For the no-space formgit push upstream --tags>/tmp/othe tag guard misses (-->not followed by&>|0-9), so it's classified BRANCH. I verified the fallthrough is harmless:extract_push_refreturns empty for it (boolean--tagsconsumes no positional), so the caller falls back to local-HEAD resolution — which in a real worktree is the actual branch. No false block in practice. Aligning the two sed patterns would be tidier but isn't required.
Verdict
APPROVED
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 404ebc505fe8b269907f0f269c5df6a2c11c3f53
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #566
Commit: 404ebc505fe8b269907f0f269c5df6a2c11c3f53
Summary
Fixes the git push parsing in _lib-extract-push-ref.sh across three bug axes (#547): (1) a src:dst refspec now validates the dst (what lands on the remote) instead of the src, so git push upstream HEAD:feature/GH-1-foo is no longer wrongly blocked on HEAD; (2) shell redirections/pipes are stripped before token parsing so 2>/>/| can't be mistaken for a branch name; (3) tag pushes (--tags, tag <name>, refs/tags/) short-circuit to a no-op via a new is_tag_push() guard called in validate-branch-name.sh before extraction. Adds 13 targeted test cases (28/28 pass).
Checklist Results
- Architecture & Design: Pass
- Code Quality: Pass
- Testing: Pass
- Security: Pass (no secrets/leaks; diff is shell + tests only)
- Performance: Pass (N/A — string parsing in a hook)
- PR Description & Glossary: Pass (5-term glossary present)
- Summary Bullet Narrative: Pass (narrative, well above label-only threshold)
- Technical Decisions (AgDR): N/A (bug fix to existing parser; no new dep/framework/pattern)
- Adopter Handbooks: N/A (no handbooks loaded for this diff)
Verification performed
I checked out the PR HEAD (404ebc5) and ran the suite myself: 28 PASS / 0 FAIL, matching the reported result. Beyond the bundled tests, I probed the lib directly against edge cases not in the suite:
- dst extraction —
HEAD:dst→dst;+HEAD:dst(force marker) →dst;refs/heads/x:refs/heads/main→main(dst taken, thenrefs/heads/stripped). All correct. - redirection stripping —
2>&1 | tail,>,2>,1>,>>all stripped;&&/;/&/|separators also handled by the residual[^|;&]*grep guard. is_tag_pushfalse-positive guards — branch names containingtag/tags(feature/GH-1-add-tag-support,feature/GH-1-tags-page,tag-team) correctly classified as BRANCH, not TAG. The--tags(\s|$)andtag\s+\Sanchors are tight.--follow-tags <branch>correctly stays BRANCH (it pushes the branch + tags, so branch validation should still apply).
The ${ref#*:} dst split, the redirection-stripping sed, and the is_tag_push guard are all shell-sound and BSD/macOS-portable (POSIX-only constructs, no \b in parameter expansion paths). No regressions for plain pushes, -u, --force, --delete, or no-arg pushes.
Issues Found
None blocking.
Suggestions (non-blocking)
- nit (cosmetic): shellcheck flags
SC2001on the twoecho | sedlines (lines 51, 128) suggesting${var//search/replace}. The regex ([0-9]*[>|]with end-anchoring) isn't expressible in bash parameter expansion, sosedis the right tool — safe to leave as-is. - nit (theoretical):
is_tag_pushuses a stricter strip pattern ([>|][&>|0-9]) thanextract_push_ref([>|]). For the no-space formgit push upstream --tags>/tmp/othe tag guard misses (-->not followed by&>|0-9), so it's classified BRANCH. I verified the fallthrough is harmless:extract_push_refreturns empty for it (boolean--tagsconsumes no positional), so the caller falls back to local-HEAD resolution — which in a real worktree is the actual branch. No false block in practice. Aligning the two sed patterns would be tidier but isn't required.
Verdict
APPROVED
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 404ebc505fe8b269907f0f269c5df6a2c11c3f53
Summary
src:dstrefspecs instead of SRC —git push upstream HEAD:feature/GH-1-foowas blocked because the hook readHEAD(left side) as the branch name. The fix makesextract_push_ref()take the right side of the colon, which is what actually lands on the remote and what the branch-naming convention applies to.git push upstream --tags 2>&1 | tailhad2>mis-parsed as a positional branch-name token. The lib now strips everything from the first redirection operator (NNN>,|) onward before extracting tokens, so2>,>, and|can never be mistaken for branch names.is_tag_push()to_lib-extract-push-ref.sh(detects--tags,tag <name>,refs/tags/) and call it invalidate-branch-name.shbefore any ref extraction; exit 0 immediately. A branch-name validator has nothing to validate on a tag push.Testing
28 cases total — 15 pre-existing (#194 behaviour) + 13 new (#547 cases). All pass:
Refs #547
Glossary
git pushargument of the form<src>:<dst>(e.g.HEAD:feature/GH-1-foo) specifying the source local ref and the destination remote ref. The branch-naming convention applies only to the dst — what lands on the remote.2>&1,2>,>that redirect file descriptors. They appear as tokens in the raw command string the hook receives and were previously mis-parsed as branch-name candidates.--tags,tag <name>, or arefs/tags/refspec. A branch-name validator has no rule to apply and must exit 0.is_tag_push()_lib-extract-push-ref.shthat returns true when the command is a tag push. Called beforeextract_push_ref()so the tag-push guard is correct even when redirections are appended to the command.extract_push_ref()_lib-extract-push-ref.sh) that parses agit pushcommand and returns the branch ref to validate. Updated to return the DST of a refspec and to strip redirections before parsing.