Skip to content

Commit aa2a4ff

Browse files
Andreas Nilsson Strömstdout-se
authored andcommitted
Improve handling of branch references
porcelain: fix listall_remote_branches symrefs and in_gitdir symlink path - listall_remote_branches: skip symbolic refs (e.g. repo tool refs/remotes/m/master), refs without branch a name and stale symrefs - in_gitdir: resolve paths so symlinked repo roots don't trigger ValueError (is_relative_to resolved vs unresolved) Add tests in test_porcelain.py.
1 parent 6bf85b7 commit aa2a4ff

2 files changed

Lines changed: 150 additions & 6 deletions

File tree

gitfourchette/porcelain.py

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -758,10 +758,10 @@ def in_gitdir(self, path: str, common: bool = True) -> str:
758758
assert not _isabs(path)
759759

760760
parent = self.commondir if common else self.path
761-
761+
parent_resolved = _Path(parent).resolve()
762762
p = _Path(parent, path).resolve()
763763

764-
if not p.is_relative_to(parent):
764+
if not p.is_relative_to(parent_resolved):
765765
raise ValueError("Won't resolve absolute path outside gitdir")
766766

767767
return str(p)
@@ -903,13 +903,13 @@ def listall_remote_branches(self, value_style: _typing.Literal["strip", "shortha
903903
for remote in self.remotes:
904904
names[remote.name] = []
905905

906-
for refname in self.listall_references():
907-
prefix, shorthand = RefPrefix.split(refname)
906+
for ref in self.references.iterator():
907+
prefix, shorthand = RefPrefix.split(ref.name)
908908

909909
if prefix != RefPrefix.REMOTES:
910910
continue
911911

912-
if refname.endswith("/HEAD"):
912+
if ref.name.endswith("/HEAD"):
913913
# Skip refs/remotes/*/HEAD (the remote's default branch).
914914
# The ref file (.git/refs/remotes/*/HEAD) is created ONCE when first cloning the repository,
915915
# and it's never updated again automatically, even if the default branch has changed on the remote.
@@ -918,13 +918,25 @@ def listall_remote_branches(self, value_style: _typing.Literal["strip", "shortha
918918
# See: https://stackoverflow.com/questions/8839958
919919
continue
920920

921+
# Skip symbolic refs (e.g., repo tool's refs/remotes/m/master -> origin/master).
922+
# We only show direct refs in real remotes; resolving would duplicate the target
923+
# (e.g. origin/master would appear twice in the upstream context menu).
924+
if ref.type == ReferenceType.SYMBOLIC:
925+
continue
926+
921927
remote_name, branch_name = split_remote_branch_shorthand(shorthand)
928+
# Skip references without a branch name (e.g., refs/remotes/git-svn from git svn clone)
929+
if not branch_name:
930+
continue
931+
# Skip references that don't match any known remote (e.g., stale refs from deleted remotes)
932+
if remote_name not in names:
933+
continue
922934
if value_style == "strip":
923935
value = branch_name
924936
elif value_style == "shorthand":
925937
value = shorthand
926938
elif value_style == "refname":
927-
value = refname
939+
value = ref.name
928940
else:
929941
raise NotImplementedError(f"unsupported value_style {value_style}")
930942
names[remote_name].append(value)

test/test_porcelain.py

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
# -----------------------------------------------------------------------------
2+
# Copyright (C) 2026 Iliyas Jorio.
3+
# This file is part of GitFourchette, distributed under the GNU GPL v3.
4+
# For full terms, see the included LICENSE file.
5+
# -----------------------------------------------------------------------------
6+
7+
"""
8+
Unit tests for gitfourchette.porcelain (Repo, listall_remote_branches, in_gitdir).
9+
"""
10+
11+
from pathlib import Path
12+
13+
import pytest
14+
15+
from .util import unpackRepo, RepoContext, WINDOWS
16+
17+
18+
def testListallRemoteBranchesWithSymbolicRef(tempDir):
19+
"""
20+
Repo.listall_remote_branches() must not raise when the repo contains
21+
symbolic refs (e.g. from the "repo" tool: refs/remotes/m/master -> refs/remotes/origin/master).
22+
Right-clicking such a branch in the sidebar would previously cause a stack trace.
23+
"""
24+
wd = unpackRepo(tempDir)
25+
# Create a symbolic ref like repo tool: refs/remotes/<manifest-name>/<branch>
26+
# pointing at refs/remotes/origin/<branch>. "m" is not a configured remote.
27+
refs_m_dir = Path(wd.rstrip("/")) / ".git" / "refs" / "remotes" / "m"
28+
refs_m_dir.mkdir(parents=True, exist_ok=True)
29+
(refs_m_dir / "master").write_text("ref: refs/remotes/origin/master\n")
30+
31+
with RepoContext(wd) as repo:
32+
# Must not raise KeyError or similar
33+
result = repo.listall_remote_branches()
34+
assert "origin" in result
35+
assert "master" in result["origin"]
36+
37+
38+
def testListallRemoteBranchesSymbolicRefToOriginMasterNoDuplicate(tempDir):
39+
"""
40+
A symbolic ref pointing at origin/master (e.g. m/master -> origin/master)
41+
must not cause origin/master to appear twice in the results.
42+
"""
43+
wd = unpackRepo(tempDir)
44+
refs_m_dir = Path(wd.rstrip("/")) / ".git" / "refs" / "remotes" / "m"
45+
refs_m_dir.mkdir(parents=True, exist_ok=True)
46+
(refs_m_dir / "master").write_text("ref: refs/remotes/origin/master\n")
47+
48+
with RepoContext(wd) as repo:
49+
result = repo.listall_remote_branches()
50+
result_shorthand = repo.listall_remote_branches(value_style="shorthand")
51+
assert result["origin"].count("master") == 1
52+
assert result_shorthand["origin"].count("origin/master") == 1
53+
54+
55+
def testListallRemoteBranchesWithStaleSymbolicRef(tempDir):
56+
"""
57+
Stale symbolic refs (pointing to a ref that no longer exists) must be
58+
skipped without raising.
59+
"""
60+
wd = unpackRepo(tempDir)
61+
refs_m_dir = Path(wd.rstrip("/")) / ".git" / "refs" / "remotes" / "m"
62+
refs_m_dir.mkdir(parents=True, exist_ok=True)
63+
(refs_m_dir / "master").write_text("ref: refs/remotes/origin/nonexistent\n")
64+
65+
with RepoContext(wd) as repo:
66+
result = repo.listall_remote_branches()
67+
# Should still have origin's branches; stale symref is skipped
68+
assert set(result.keys()) == {"origin"}
69+
assert "master" in result["origin"]
70+
71+
72+
def testListallRemoteBranchesSkipsRefWithoutBranchName(tempDir):
73+
"""
74+
Refs without a branch name (e.g. refs/remotes/git-svn from git svn clone)
75+
must be skipped. split_remote_branch_shorthand yields empty branch_name
76+
for such refs.
77+
"""
78+
wd = unpackRepo(tempDir)
79+
git_dir = Path(wd.rstrip("/")) / ".git"
80+
with RepoContext(wd) as repo:
81+
oid = str(repo.head_commit_id)
82+
# refs/remotes/git-svn has no "/branch" part -> branch_name is ""
83+
(git_dir / "refs" / "remotes" / "git-svn").write_text(oid + "\n")
84+
85+
with RepoContext(wd) as repo:
86+
result = repo.listall_remote_branches()
87+
# git-svn is not a known remote; even if it were, it has no branch name.
88+
# We must still see origin's branches and must not include git-svn.
89+
assert "origin" in result
90+
assert "git-svn" not in result
91+
assert "master" in result["origin"]
92+
93+
94+
def testListallRemoteBranchesSkipsStaleRefsFromDeletedRemote(tempDir):
95+
"""
96+
Refs that belong to a remote no longer in the config (e.g. after
97+
git remote remove) must be skipped so we don't show stale data.
98+
"""
99+
wd = unpackRepo(tempDir)
100+
git_dir = Path(wd.rstrip("/")) / ".git"
101+
with RepoContext(wd) as repo:
102+
oid = str(repo.head_commit_id)
103+
# Create refs/remotes/deletedremote/master but do not add "deletedremote" as a remote
104+
deleted_dir = git_dir / "refs" / "remotes" / "deletedremote"
105+
deleted_dir.mkdir(parents=True, exist_ok=True)
106+
(deleted_dir / "master").write_text(oid + "\n")
107+
108+
with RepoContext(wd) as repo:
109+
result = repo.listall_remote_branches()
110+
# Only configured remotes (origin) appear; deletedremote is skipped
111+
assert set(result.keys()) == {"origin"}
112+
assert "deletedremote" not in result
113+
assert "master" in result["origin"]
114+
115+
116+
@pytest.mark.skipif(WINDOWS, reason="symlinks are flaky on Windows")
117+
def testInGitdirWithSymlinkedRepo(tempDir):
118+
"""
119+
Repo.in_gitdir() must not raise ValueError when the repo path is a symlink.
120+
Previously is_relative_to(parent) failed because the resolved path was
121+
compared against the unresolved (symlink) parent.
122+
"""
123+
wd = unpackRepo(tempDir)
124+
real_path = Path(wd.rstrip("/")).resolve()
125+
link_path = Path(tempDir.name) / "repo-link"
126+
link_path.symlink_to(real_path)
127+
repo_path = str(link_path) + "/"
128+
129+
with RepoContext(repo_path) as repo:
130+
# Must not raise ValueError("Won't resolve absolute path outside gitdir")
131+
config_path = repo.in_gitdir("config", common=True)
132+
assert config_path.endswith("config")

0 commit comments

Comments
 (0)