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 <nesquena@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

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
+        stdout = r.stdout.strip()
-    except (subprocess.TimeoutExpired, FileNotFoundError, OSError):
+        stderr = r.stderr.strip()
-        return '', False
+        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
+    # Check for dirty working tree (ignore untracked files — git stash
-    status_out, _ = _run_git(['status', '--porcelain'], path)
+    # 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 with ff-only (no merge commits).
-    pull_out, pull_ok = _run_git(['pull', '--ff-only', compare_ref], path, timeout=30)
+    # 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)

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')