Skip to content

Improve handling of branch references#92

Merged
jorio merged 1 commit intojorio:masterfrom
stdout-se:symlinks
Mar 24, 2026
Merged

Improve handling of branch references#92
jorio merged 1 commit intojorio:masterfrom
stdout-se:symlinks

Conversation

@stdout-se
Copy link
Copy Markdown

porcelain: fix listall_remote_branches symrefs and in_gitdir symlink path

  • listall_remote_branches: handle symbolic refs (e.g. repo tool refs/remotes/m/master) and skip stale symrefs without raising
  • in_gitdir: resolve paths so symlinked repo roots don't trigger ValueError (is_relative_to resolved vs unresolved)

Add tests in test_porcelain.py.

Resolves #91

@jorio
Copy link
Copy Markdown
Owner

jorio commented Mar 6, 2026 via email

@jorio
Copy link
Copy Markdown
Owner

jorio commented Mar 18, 2026

Thank you for the patch + tests, and thank you for your patience. Here are my notes:

How should we handle duplicates after resolving? For example, if m/master is symbolic and points to origin/master, then origin/master currently shows up twice (causing weirdness in the upstream context menu).

Ultimately, we're only showing direct refs in real remotes, so how about skipping all symbolic refs altogether, for now?


Skip references without a branch name (e.g., refs/remotes/git-svn from git svn clone)

Skip references that don't match any known remote (e.g., stale refs from deleted remotes)

Could you please add tests to cover these code paths, so we don't inadvertently break this behavior in the future?


Suggestion: the big 'for' loop could iterate over self.references.iterator() instead of self.listall_references(). This gets us real Reference objects instead of strings, and we can get rid of the additional lookup into self.references in the loop body.

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.
@stdout-se
Copy link
Copy Markdown
Author

Good feedback. I did an update and added some more tests. I guess the branches could be sorted as well, but maybe that's a job for the presentation layer and not listall_remote_branches?

@jorio jorio merged commit aa2a4ff into jorio:master Mar 24, 2026
13 of 14 checks passed
@stdout-se stdout-se deleted the symlinks branch March 25, 2026 08:25
@jorio
Copy link
Copy Markdown
Owner

jorio commented Mar 25, 2026

but maybe that's a job for the presentation layer and not listall_remote_branches?

I agree, the upstream menu should probably honor the RefSort setting (Settings -> General -> Sort branches & tags). For now this only applies to the sidebar, but I should factor out SidebarModel.populateRefNodeTree so that the upstream menu can benefit from it, too.

Thank you for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Opening "repo" tool repos gives errors

2 participants