The defect
scripts/local_review.py's target_ref() returns a target name as written,
unqualified, whenever it resolves as a remote-tracking ref (or, in the pre-existing
fallback, whenever it resolves at all). Whatever it returns is passed straight to
git merge-base <ref> HEAD by merge_base().
Git's own short-name disambiguation for a bare ref tries refs/heads/<name> before
refs/remotes/<name> (see gitrevisions(7)). So a target such as upstream/main
that is meant to name a remote-tracking ref silently resolves against a local
branch of the identical name instead, whenever such a local branch exists, with only
git's own ambiguity warning (not surfaced by this tool) as a clue.
Confirmed empirically against a throwaway fixture with three distinct commits (the
target's own base, a commit only on refs/remotes/upstream/main, and a commit only
on a local branch literally named upstream/main): target_ref("upstream/main", ...)
returns the bare string "upstream/main", and the subsequent
git merge-base "upstream/main" HEAD call resolves against the local branch's
tip, never touching the remote-tracking ref that was the one actually intended.
This is pre-existing rather than introduced by #1112's fix: the same scenario
reproduces identically against scripts/local_review.py as it stood on develop
before that PR, via the function's original if ref_exists(target, root): return target
fallback. #1112 only added a second, earlier code path that also returns the target
bare, so it does not create this exposure, it inherits it.
Why this is a real, if narrow, exposure
The whole point of naming a remote-tracking ref like upstream/main explicitly is a
fork-based workflow (see #1112 and the target_ref() docstring). A contributor's
checkout in exactly that kind of workflow is precisely where a local branch checked
out from upstream/main and left with the same name is a plausible, not exotic, state.
Suggested direction (not prescribed)
Once target_ref() has decided that a value should be honored as a remote-tracking
ref, hand merge_base() (and any other consumer) an unambiguous form, for example the
fully qualified refs/remotes/<target> rather than the bare short name, so a
same-named local branch cannot shadow it. The origin/<target> branch has the same
theoretical shadowing exposure against a local branch literally named
origin/<target>, which is narrower still but not zero, and a fix should consider
whether to qualify that return value too.
Scope note
Filed rather than fixed inline in #1112, per that PR's explicit bounds against fixing
adjacent defects in scripts/local_review.py, and because this predates that PR's
change rather than being introduced by it.
The defect
scripts/local_review.py'starget_ref()returns a target name as written,unqualified, whenever it resolves as a remote-tracking ref (or, in the pre-existing
fallback, whenever it resolves at all). Whatever it returns is passed straight to
git merge-base <ref> HEADbymerge_base().Git's own short-name disambiguation for a bare ref tries
refs/heads/<name>beforerefs/remotes/<name>(seegitrevisions(7)). So a target such asupstream/mainthat is meant to name a remote-tracking ref silently resolves against a local
branch of the identical name instead, whenever such a local branch exists, with only
git's own ambiguity warning (not surfaced by this tool) as a clue.
Confirmed empirically against a throwaway fixture with three distinct commits (the
target's own base, a commit only on
refs/remotes/upstream/main, and a commit onlyon a local branch literally named
upstream/main):target_ref("upstream/main", ...)returns the bare string
"upstream/main", and the subsequentgit merge-base "upstream/main" HEADcall resolves against the local branch'stip, never touching the remote-tracking ref that was the one actually intended.
This is pre-existing rather than introduced by #1112's fix: the same scenario
reproduces identically against
scripts/local_review.pyas it stood ondevelopbefore that PR, via the function's original
if ref_exists(target, root): return targetfallback. #1112 only added a second, earlier code path that also returns the target
bare, so it does not create this exposure, it inherits it.
Why this is a real, if narrow, exposure
The whole point of naming a remote-tracking ref like
upstream/mainexplicitly is afork-based workflow (see #1112 and the
target_ref()docstring). A contributor'scheckout in exactly that kind of workflow is precisely where a local branch checked
out from
upstream/mainand left with the same name is a plausible, not exotic, state.Suggested direction (not prescribed)
Once
target_ref()has decided that a value should be honored as a remote-trackingref, hand
merge_base()(and any other consumer) an unambiguous form, for example thefully qualified
refs/remotes/<target>rather than the bare short name, so asame-named local branch cannot shadow it. The
origin/<target>branch has the sametheoretical shadowing exposure against a local branch literally named
origin/<target>, which is narrower still but not zero, and a fix should considerwhether to qualify that return value too.
Scope note
Filed rather than fixed inline in #1112, per that PR's explicit bounds against fixing
adjacent defects in
scripts/local_review.py, and because this predates that PR'schange rather than being introduced by it.