diff --git a/controller/lib/scripts.py b/controller/lib/scripts.py index 7785c9ca..a9aa43fa 100644 --- a/controller/lib/scripts.py +++ b/controller/lib/scripts.py @@ -78,17 +78,20 @@ def get_current_branch() -> Optional[str]: Get the current git branch of /opt/wrolpi. Returns: - Branch name, or None if unable to determine + Branch name ('release' on a detached HEAD), or None if unable to determine """ try: + # Not `rev-parse --abbrev-ref HEAD`: it reports "heads/release" when a stale + # `release` tag exists alongside the branch, breaking origin/{branch} lookups. result = subprocess.run( - ["git", "-C", "/opt/wrolpi", "rev-parse", "--abbrev-ref", "HEAD"], + ["git", "-C", "/opt/wrolpi", "branch", "--show-current"], capture_output=True, text=True, timeout=5, ) if result.returncode == 0: - return result.stdout.strip() + # A detached HEAD prints nothing; upgrades come from `release` by default. + return result.stdout.strip() or "release" except (subprocess.TimeoutExpired, subprocess.SubprocessError, FileNotFoundError): pass return None diff --git a/controller/test/test_scripts.py b/controller/test/test_scripts.py index ecbb7fde..29a42c7e 100644 --- a/controller/test/test_scripts.py +++ b/controller/test/test_scripts.py @@ -7,6 +7,7 @@ from unittest import mock from controller.lib.scripts import ( AVAILABLE_SCRIPTS, + get_current_branch, get_script_output, get_script_status, list_available_scripts, @@ -14,6 +15,44 @@ from controller.lib.scripts import ( ) +class TestGetCurrentBranch: + """Tests for get_current_branch function.""" + + def test_returns_branch_name(self): + """Should return the branch name from `git branch --show-current`. + + `git rev-parse --abbrev-ref HEAD` must not be used: it reports "heads/release" when a + stale `release` tag exists alongside the `release` branch. + """ + mock_result = mock.MagicMock() + mock_result.returncode = 0 + mock_result.stdout = "release\n" + + with mock.patch("subprocess.run", return_value=mock_result) as mock_run: + assert get_current_branch() == "release" + cmd = mock_run.call_args[0][0] + assert "--show-current" in cmd + assert "--abbrev-ref" not in cmd + + def test_detached_head_defaults_to_release(self): + """A detached HEAD (empty output) should default to 'release'.""" + mock_result = mock.MagicMock() + mock_result.returncode = 0 + mock_result.stdout = "\n" + + with mock.patch("subprocess.run", return_value=mock_result): + assert get_current_branch() == "release" + + def test_git_failure_returns_none(self): + """A failing git command should return None.""" + mock_result = mock.MagicMock() + mock_result.returncode = 128 + mock_result.stdout = "" + + with mock.patch("subprocess.run", return_value=mock_result): + assert get_current_branch() is None + + class TestAvailableScripts: """Tests for AVAILABLE_SCRIPTS constant.""" diff --git a/wrolpi/test/test_upgrade.py b/wrolpi/test/test_upgrade.py index 2215e388..21ef1ba7 100644 --- a/wrolpi/test/test_upgrade.py +++ b/wrolpi/test/test_upgrade.py @@ -1,6 +1,7 @@ """ Tests for the WROLPi upgrade system. """ +import subprocess from unittest.mock import patch, MagicMock import pytest @@ -69,10 +70,22 @@ class TestGitFunctions: assert get_current_branch() is None def test_get_current_branch_detached_head(self): - """A detached HEAD (abbrev-ref prints the literal "HEAD") is not a branch.""" + """A detached HEAD (`git branch --show-current` prints nothing) defaults to 'release'.""" mock_result = MagicMock() mock_result.returncode = 0 - mock_result.stdout = b'HEAD\n' + mock_result.stdout = b'\n' + mock_path = MagicMock() + mock_path.is_dir.return_value = True + + with patch('wrolpi.upgrade.PROJECT_DIR', mock_path), \ + patch('subprocess.run', return_value=mock_result): + assert get_current_branch() == 'release' + + def test_get_current_branch_command_failed(self): + """A failing git command (e.g. not a repository) returns None.""" + mock_result = MagicMock() + mock_result.returncode = 128 + mock_result.stdout = b'' mock_path = MagicMock() mock_path.is_dir.return_value = True @@ -80,6 +93,29 @@ class TestGitFunctions: patch('subprocess.run', return_value=mock_result): assert get_current_branch() is None + def test_get_current_branch_with_ambiguous_tag(self, test_directory): + """A tag with the same name as the branch must not change the reported branch name. + + `git rev-parse --abbrev-ref HEAD` reports "heads/release" when a `release` tag also + exists, which breaks the origin/{branch} comparison in check_for_update. + """ + repo = test_directory / 'repo' + repo.mkdir() + env = {'GIT_AUTHOR_NAME': 'test', 'GIT_AUTHOR_EMAIL': 'test@example.com', + 'GIT_COMMITTER_NAME': 'test', 'GIT_COMMITTER_EMAIL': 'test@example.com'} + for cmd in ( + ['git', 'init', '--initial-branch=release'], + ['git', 'commit', '--allow-empty', '-m', 'one'], + ['git', 'commit', '--allow-empty', '-m', 'two'], + # Tag an old commit with the same name as the branch, like the stale + # `release` tag found on real WROLPi installs. + ['git', 'tag', 'release', 'HEAD~1'], + ): + subprocess.run(cmd, cwd=repo, check=True, capture_output=True, env=env) + + with patch('wrolpi.upgrade.PROJECT_DIR', repo): + assert get_current_branch() == 'release' + def test_get_local_commit(self): """get_local_commit returns the short commit hash.""" mock_result = MagicMock() diff --git a/wrolpi/upgrade.py b/wrolpi/upgrade.py index 40138de8..81950c92 100644 --- a/wrolpi/upgrade.py +++ b/wrolpi/upgrade.py @@ -49,26 +49,24 @@ def get_current_branch() -> str | None: """ Get the current git branch name (e.g., 'release', 'master'). - Returns None if unable to determine. + Defaults to 'release' on a detached HEAD. Returns None if unable to determine. """ if not PROJECT_DIR.is_dir(): return None try: + # Not `rev-parse --abbrev-ref HEAD`: it reports "heads/release" when a stale + # `release` tag exists alongside the branch, breaking origin/{branch} lookups. result = subprocess.run( - ['git', 'rev-parse', '--abbrev-ref', 'HEAD'], + ['git', 'branch', '--show-current'], cwd=str(PROJECT_DIR), capture_output=True, timeout=10, ) if result.returncode == 0: branch = result.stdout.decode().strip() - # A detached HEAD makes `--abbrev-ref HEAD` print the literal - # "HEAD" (not a branch). Comparing origin/HEAD — the remote's - # default branch — against it yields a bogus "commits behind" - # count and a false "update available", so report no branch. - if branch and branch != 'HEAD': - return branch + # A detached HEAD prints nothing; upgrades come from `release` by default. + return branch or 'release' except Exception as e: logger.error('Failed to get current branch', exc_info=e) diff --git a/wrolpi/version.py b/wrolpi/version.py index 6a0acf0c..8e16cddf 100644 --- a/wrolpi/version.py +++ b/wrolpi/version.py @@ -17,10 +17,13 @@ def git_revision(): def git_branch(): try: - cmd = ('git', 'rev-parse', '--abbrev-ref', 'HEAD') + # Not `rev-parse --abbrev-ref HEAD`: it reports "heads/release" when a stale + # `release` tag exists alongside the branch. + cmd = ('git', 'branch', '--show-current') branch = subprocess.check_output(cmd, stderr=subprocess.PIPE) branch = branch.decode().strip() - return branch + # A detached HEAD prints nothing. + return branch or 'HEAD' except Exception: return 'unknown'