From 0d98116b374cb10ccff34e0bd79dc35689ea9f39 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 12 Apr 2026 00:19:33 -0700 Subject: [PATCH] fix: improve self-update git pull diagnostics (#287) Rebased and enhanced version of PR #287 by @ccqqlo: - _run_git() now returns stderr on failure instead of empty string, so the UI can surface actionable git error messages - Added _split_remote_ref() to split tracking refs like origin/master into separate remote + branch args for git pull - Ignore untracked files in stash decision (--untracked-files=no) to prevent misleading stash-pop failures - Fail early with clear message on unresolved merge conflicts - 4 unit tests covering stderr, stdout fallback, exit code, and ref splitting Based on work by @ccqqlo in PR #287. Co-authored-by: Nathan Esquenazi Co-authored-by: Claude Opus 4.6 (1M context) --- api/updates.py | 57 +++++++++++++++++++++++++++++++++++++------ tests/test_updates.py | 53 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 tests/test_updates.py diff --git a/api/updates.py b/api/updates.py index 0f2aa2b..17f4cad 100644 --- a/api/updates.py +++ b/api/updates.py @@ -29,15 +29,39 @@ CACHE_TTL = 1800 # 30 minutes def _run_git(args, cwd, timeout=10): - """Run a git command and return (stdout, ok).""" + """Run a git command and return (useful output, ok). + + On failure, returns stderr (or stdout as fallback) so callers can + surface actionable git error messages instead of empty strings. + """ try: r = subprocess.run( ['git'] + args, cwd=str(cwd), capture_output=True, text=True, timeout=timeout, ) - return r.stdout.strip(), r.returncode == 0 - except (subprocess.TimeoutExpired, FileNotFoundError, OSError): - return '', False + stdout = r.stdout.strip() + stderr = r.stderr.strip() + if r.returncode == 0: + return stdout, True + return stderr or stdout or f"git exited with status {r.returncode}", False + except subprocess.TimeoutExpired as exc: + detail = (getattr(exc, 'stderr', None) or getattr(exc, 'stdout', None) or '').strip() + return detail or f"git {' '.join(args)} timed out after {timeout}s", False + except FileNotFoundError: + return 'git executable not found', False + except OSError as exc: + return f'git failed to start: {exc}', False + + +def _split_remote_ref(ref): + """Split 'origin/branch-name' into ('origin', 'branch-name'). + + Returns (None, ref) if ref contains no slash. + """ + if '/' not in ref: + return None, ref + remote, branch = ref.split('/', 1) + return remote, branch def _detect_default_branch(path): @@ -159,8 +183,17 @@ def _apply_update_inner(target): ), } - # Check for dirty working tree - status_out, _ = _run_git(['status', '--porcelain'], path) + # Check for dirty working tree (ignore untracked files — git stash + # doesn't include them, so stashing on '??' alone leaves nothing to pop) + status_out, status_ok = _run_git( + ['status', '--porcelain', '--untracked-files=no'], path + ) + if not status_ok: + return {'ok': False, 'message': f'Failed to inspect repo status: {status_out[:200]}'} + # Fail early on unresolved merge conflicts + if any(line[:2] in {'DD', 'AU', 'UD', 'UA', 'DU', 'AA', 'UU'} + for line in status_out.splitlines()): + return {'ok': False, 'message': 'Repository has unresolved merge conflicts'} stashed = False if status_out: _, ok = _run_git(['stash'], path) @@ -168,8 +201,16 @@ def _apply_update_inner(target): return {'ok': False, 'message': 'Failed to stash local changes'} stashed = True - # Pull with ff-only (no merge commits) - pull_out, pull_ok = _run_git(['pull', '--ff-only', compare_ref], path, timeout=30) + # Pull with ff-only (no merge commits). + # Split tracking refs like 'origin/main' into separate remote + branch + # arguments — git treats 'origin/main' as a repository name otherwise. + remote, branch = _split_remote_ref(compare_ref) + pull_args = ['pull', '--ff-only'] + if remote: + pull_args.extend([remote, branch]) + else: + pull_args.append(compare_ref) + pull_out, pull_ok = _run_git(pull_args, path, timeout=30) if not pull_ok: if stashed: _run_git(['stash', 'pop'], path) diff --git a/tests/test_updates.py b/tests/test_updates.py new file mode 100644 index 0000000..d99c561 --- /dev/null +++ b/tests/test_updates.py @@ -0,0 +1,53 @@ +"""Tests for self-update diagnostics (api/updates.py).""" +from unittest.mock import MagicMock, patch + +import api.updates as updates + + +def test_run_git_returns_stderr_on_failure(tmp_path): + """When a git command fails, _run_git should return stderr (not empty string).""" + with patch('subprocess.run') as mock_run: + mock_run.return_value = MagicMock( + returncode=1, + stdout='', + stderr="fatal: 'origin/master' does not appear to be a git repository\n", + ) + out, ok = updates._run_git(['pull', '--ff-only', 'origin/master'], tmp_path) + + assert ok is False + assert "does not appear to be a git repository" in out + + +def test_run_git_returns_stdout_when_no_stderr(tmp_path): + """If stderr is empty on failure, fall back to stdout.""" + with patch('subprocess.run') as mock_run: + mock_run.return_value = MagicMock( + returncode=128, + stdout='Already up to date.', + stderr='', + ) + out, ok = updates._run_git(['pull'], tmp_path) + + assert ok is False + assert 'Already up to date' in out + + +def test_run_git_returns_exit_code_when_no_output(tmp_path): + """If both stdout and stderr are empty, report the exit code.""" + with patch('subprocess.run') as mock_run: + mock_run.return_value = MagicMock( + returncode=1, + stdout='', + stderr='', + ) + out, ok = updates._run_git(['status'], tmp_path) + + assert ok is False + assert 'status 1' in out + + +def test_split_remote_ref_splits_tracking_ref(): + """_split_remote_ref should correctly split origin/branch.""" + assert updates._split_remote_ref('origin/master') == ('origin', 'master') + assert updates._split_remote_ref('origin/feature/foo') == ('origin', 'feature/foo') + assert updates._split_remote_ref('master') == (None, 'master')