scripts: harden tag-release.sh ref resolution - #11047
Conversation
🟢 PR Severity: LOW
🟢 Low (1 files)
AnalysisThis PR only modifies To override, add a |
ziggie1984
changed the title
6210575 to
f5f542a
Compare
Fully qualifies the fetch refspec and peels FETCH_HEAD to a commit before comparing it against HEAD. `git fetch <remote> <name>` resolves a tag named <name> ahead of a branch of the same name, so a tag shadowing a release branch would leave FETCH_HEAD pointing at unrelated history. Fetching `refs/heads/<name>` removes the ambiguity. The two changes belong together. Today the SHA comparison happens to fail closed under such a collision, because `git rev-parse FETCH_HEAD` yields the annotated tag object and that can never equal a commit id. Peeling on its own would turn that into a silent success on the tag's target commit, so it is only safe once the refspec pins refs/heads. Also passes --no-tags, which keeps a remote.<name>.tagOpt=--tags setting from pulling remote tags into the local repo as a side effect of what should be a read-only verification step, and rejects an empty --branch= value rather than silently falling back to the current branch.
f5f542a to
fc06ecc
Compare
Lrifton92
left a comment
Lrifton92
left a comment
There was a problem hiding this comment.
I reproduced the scenarios from the PR description on a throwaway upstream carrying a branch/tag collision (git 2.55): the unqualified git fetch <remote> <name> does resolve the same-named annotated tag ahead of the branch, FETCH_HEAD^{commit} peeled alone would silently pass at the tag's target, and with the qualified refspec it resolves the branch tip. Also confirmed the remote.<name>.tagOpt=--tags side effect and that --no-tags suppresses it. The reasoning for keeping the refspec and the peel in one commit is sound, and the new failure mode when the name exists only as a tag is a strict improvement (old script exited 0 on the fetch and later reported a confusing SHA mismatch; new script fails immediately with couldn't find remote ref).
One interaction is worth fixing before merge, though: when the shadow tag also exists locally, the default-branch derivation breaks. git symbolic-ref --quiet --short HEAD returns the shortest unambiguous name, so with a local tag v9.9.x-branch shadowing the checked-out branch it yields heads/v9.9.x-branch, and the new fetch becomes refs/heads/heads/v9.9.x-branch -> fatal: couldn't find remote ref. This is fail-closed, so nothing wrong can be tagged, but the error is misleading — and the precondition is exactly the collision this PR is about: since such a tag points into the branch's history, any plain git fetch <remote> auto-follows it into the local repo (verified), so in a live collision most clones would hit this. The pre-patch script happened to survive this sub-case, because its unqualified heads/<name> argument still resolved refs/heads/<name> on the remote.
| echo "Fetching ${UPSTREAM_REMOTE} ${UPSTREAM_BRANCH}..." | ||
| git fetch --quiet "${UPSTREAM_REMOTE}" "${UPSTREAM_BRANCH}" | ||
| git fetch --quiet --no-tags "${UPSTREAM_REMOTE}" \ | ||
| "refs/heads/${UPSTREAM_BRANCH}" |
There was a problem hiding this comment.
Suggest making the derived branch name immune to local ref ambiguity, e.g.:
UPSTREAM_BRANCH="$(git symbolic-ref --quiet HEAD || true)"
UPSTREAM_BRANCH="${UPSTREAM_BRANCH#refs/heads/}"in the default-branch block (currently git symbolic-ref --quiet --short HEAD). --short shortens to the shortest unambiguous name, which is heads/<name> when a local tag shadows the branch — precisely the collision this change hardens against — and that double-prefixes into refs/heads/heads/<name> here. Stripping the full refs/heads/ prefix from the unshortened symref sidesteps the ambiguity entirely. Relatedly (very minor), an explicitly passed --branch refs/heads/master now also double-prefixes and dies with couldn't find remote ref where it previously worked; fail-closed and arguably fine, but worth a mention in the usage text if you don't want to normalize it.
|
@ziggie1984, remember to re-request review from reviewers when ready |
1 similar comment
|
@ziggie1984, remember to re-request review from reviewers when ready |
3 participants
Summary
Hardens ref resolution in
scripts/tag-release.sh. The helper's behavior andCLI surface are unchanged: it still verifies that
HEADmatches the upstreambranch tip and tags
HEAD, and--branchstill selects the upstream branch toverify against.
refs/heads/<branch>refspecFETCH_HEADto a commit before comparing it againstHEAD--no-tags--branch=value instead of silently falling back to thecurrent branch
Motivation
git fetch <remote> <name>resolves a tag named<name>ahead of a branch ofthe same name. A tag shadowing a release branch would therefore leave
FETCH_HEADon unrelated history. Fetchingrefs/heads/<name>removes theambiguity.
The refspec and the peel belong in the same change. Today the SHA comparison
happens to fail closed under such a collision, because
git rev-parse FETCH_HEADyields the annotated tag object and that can never equal a commitid. Peeling on its own would convert that into a silent success on the tag's
target commit, so peeling is only safe once the refspec pins
refs/heads.--no-tagsis hygiene rather than a fix. With an explicit refspec, tagauto-following does not happen by default; it happens only when
remote.<name>.tagOpt=--tagsis configured, in which case a verification stepthat should be read-only silently mutates the local tag namespace. It cannot
affect any decision the script makes, since the duplicate-tag check is a
git ls-remotequery against the remote.Note that no such collision exists today — lnd release tags look like
v0.21.2-betaand would not shadowv0.21.x-branch. This closes the shaperather than a live bug.
Note on the previous revision of this PR
An earlier version of this branch changed
--branchto fetch and tag theupstream branch tip directly, so that a release could be cut without checking
the branch out. That has been dropped.
The motivating problem was that
scripts/tag-release.shdid not exist onv0.20.x-branch, which has since been addressed by backporting the script.Retargeting also gave up a property worth keeping: because the existing check
hard-fails on a
HEAD/upstream mismatch rather than merely reporting it, bothdesigns guarantee equally that the signed commit is the upstream branch tip, but
only the original guarantees that the maintainer had that tree checked out when
signing it.
Validation
bash -n scripts/tag-release.shpasses; no added line exceeds 80 characters--branch=and--branchwith no value both exit 1 with usagelive run against
origin masterwith a deliberately mismatched tag:FETCH_HEADresolved tobranch 'master'rather than a tag, peeled to acommit object, local tag count unchanged (412), and no tag was created
Exercised end to end on git 2.50.1 against a throwaway upstream carrying the
collision this change is about: a branch
v9.9.x-branchat commit Y(
build/version.go= 9.9.1) and an annotated tag of the same name at anunrelated commit X (9.9.0).
this branch, HEAD at Y: verification passes and targets Y, the branch tip,
rather than X (the run then stops at
gpgfor want of a key in the sandbox,which is past every check)
master's script, same situation: aborts, reporting anUpstream:SHA thatis neither X nor Y but the annotated tag object, since that can never equal a
commit id
peel applied without the qualified refspec, HEAD at X: passes verification and
would tag X rather than the branch tip, which is why the two changes are in
one commit
under
remote.origin.tagOpt=--tags, this branch leaves the local tag listempty where
master's script pulls in the remote tag