diff --git a/test/test_git_pull_resolution.py b/test/test_git_pull_resolution.py new file mode 100644 index 00000000..d5668142 --- /dev/null +++ b/test/test_git_pull_resolution.py @@ -0,0 +1,202 @@ +"""Guard: the update button works on branches without tracking information. + +`git pull --rebase` fails outright on a branch with no upstream: + + There is no tracking information for the current branch. + Please specify which branch you want to rebase against. + +That is easy to land on — checking out a branch by name, restoring a +backup, or following a guide that names one — and the Tools tab reported it +as a bare "Update failed; check logs for details", which the user cannot act +on. resolve_pull_command() falls back to an explicit `origin ` pull +when that remote branch exists, and returns an actionable message when it +does not. + +These tests build real git repositories in a temp dir, so they exercise git's +actual behaviour rather than a mock of it. +""" +import subprocess +from pathlib import Path + +import pytest + +from web_interface.blueprints.api_v3 import ( + checkout_branch, + is_valid_branch_name, + resolve_pull_command, +) + +pytestmark = pytest.mark.skipif( + subprocess.run(['git', '--version'], capture_output=True).returncode != 0, + reason='git not available', +) + + +def _git(*args, cwd): + return subprocess.run(['git', *args], cwd=str(cwd), + capture_output=True, text=True, check=True) + + +@pytest.fixture() +def repos(tmp_path): + """An 'origin' repo with a main branch, and a clone of it.""" + origin = tmp_path / 'origin' + origin.mkdir() + _git('init', '--initial-branch=main', '--bare', cwd=origin) + + work = tmp_path / 'work' + _git('clone', str(origin), str(work), cwd=tmp_path) + _git('config', 'user.email', 'test@example.com', cwd=work) + _git('config', 'user.name', 'Test', cwd=work) + (work / 'README.md').write_text('hello\n') + _git('add', 'README.md', cwd=work) + _git('commit', '-m', 'initial', cwd=work) + _git('push', '-u', 'origin', 'main', cwd=work) + return work + + +def test_branch_with_upstream_uses_a_plain_pull(repos): + args, note, error = resolve_pull_command(str(repos)) + assert error is None + assert args == ['git', 'pull', '--rebase'] + assert note == '' + + +def test_branch_without_upstream_falls_back_to_origin_branch(repos): + """The reported bug: a local branch that also exists on origin.""" + _git('push', 'origin', 'main:audit', cwd=repos) + _git('fetch', 'origin', cwd=repos) + # A branch created this way has no tracking information. + _git('checkout', '-b', 'audit', cwd=repos) + assert subprocess.run(['git', 'rev-parse', '--abbrev-ref', '@{u}'], + cwd=str(repos), capture_output=True).returncode != 0 + + args, note, error = resolve_pull_command(str(repos)) + assert error is None + assert args == ['git', 'pull', '--rebase', 'origin', 'audit'] + assert 'audit' in note + + +def test_pull_fallback_actually_succeeds(repos): + """The fallback command must work, not merely look right.""" + _git('push', 'origin', 'main:audit', cwd=repos) + _git('fetch', 'origin', cwd=repos) + _git('checkout', '-b', 'audit', cwd=repos) + + args, _, error = resolve_pull_command(str(repos)) + assert error is None + done = subprocess.run(args, cwd=str(repos), capture_output=True, text=True) + assert done.returncode == 0, done.stderr + + +def test_local_only_branch_reports_an_actionable_message(repos): + """No upstream and no origin/: say so, don't just fail.""" + _git('checkout', '-b', 'local-experiment', cwd=repos) + args, _, error = resolve_pull_command(str(repos)) + assert args is None + assert error and 'local-experiment' in error + assert 'no origin/local-experiment' in error + + +def test_detached_head_reports_an_actionable_message(repos): + head = subprocess.run(['git', 'rev-parse', 'HEAD'], cwd=str(repos), + capture_output=True, text=True).stdout.strip() + _git('checkout', head, cwd=repos) + args, _, error = resolve_pull_command(str(repos)) + assert args is None + assert error and 'detached HEAD' in error + + +def test_missing_directory_does_not_raise(tmp_path): + """A bad path must return an error, not blow up the request.""" + args, _, error = resolve_pull_command(str(tmp_path / 'nope')) + assert args is None + assert error + + +# ── branch switching ──────────────────────────────────────────────────────── + + +@pytest.mark.parametrize('name', [ + 'main', 'audit', 'feat/thing', 'release-1.2', 'a_b.c', +]) +def test_valid_branch_names_accepted(name): + assert is_valid_branch_name(name) + + +@pytest.mark.parametrize('name', [ + '', ' ', 'a b', 'a;rm -rf /', '--upload-pack=evil', '-x', + 'a..b', 'a\nb', 'x' * 201, 'branch$(whoami)', '../escape', +]) +def test_unsafe_branch_names_rejected(name): + """The value reaches a subprocess argument list, so refuse the exotic.""" + assert not is_valid_branch_name(name) + + +def test_switch_to_remote_only_branch_creates_it_with_tracking(repos): + _git('push', 'origin', 'main:release', cwd=repos) + _git('fetch', 'origin', cwd=repos) + + payload, code = checkout_branch(str(repos), 'release') + assert code == 200 and payload['status'] == 'success', payload + + assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'release' + upstream = subprocess.run(['git', 'rev-parse', '--abbrev-ref', '@{u}'], + cwd=str(repos), capture_output=True, text=True) + assert upstream.stdout.strip() == 'origin/release' + + +def test_switching_attaches_tracking_so_pull_needs_no_fallback(repos): + """The whole point: after switching, a plain `git pull` works.""" + _git('push', 'origin', 'main:audit', cwd=repos) + _git('fetch', 'origin', cwd=repos) + payload, _ = checkout_branch(str(repos), 'audit') + assert payload['status'] == 'success' + + args, note, error = resolve_pull_command(str(repos)) + assert error is None + assert args == ['git', 'pull', '--rebase'] + assert note == '' + + +def test_unknown_branch_is_reported_not_created(repos): + payload, code = checkout_branch(str(repos), 'does-not-exist') + assert code == 404 + assert 'does-not-exist' in payload['message'] + assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'main' + + +def test_local_edits_block_the_switch_and_name_the_files(repos): + _git('push', 'origin', 'main:other', cwd=repos) + _git('fetch', 'origin', cwd=repos) + _git('checkout', '-b', 'other', 'origin/other', cwd=repos) + (repos / 'README.md').write_text('changed on other\n') + _git('add', 'README.md', cwd=repos) + _git('commit', '-m', 'diverge', cwd=repos) + _git('checkout', 'main', cwd=repos) + (repos / 'README.md').write_text('uncommitted local edit\n') + + payload, code = checkout_branch(str(repos), 'other') + assert code == 200 and payload['status'] == 'error' + assert payload['can_retry_with_stash'] is True + assert 'README.md' in payload['detail'] + assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'main' + + +def test_stash_option_lets_the_switch_through_and_keeps_the_work(repos): + """stash=True must switch *and* leave the edit recoverable.""" + _git('push', 'origin', 'main:other', cwd=repos) + _git('fetch', 'origin', cwd=repos) + _git('checkout', '-b', 'other', 'origin/other', cwd=repos) + (repos / 'README.md').write_text('changed on other\n') + _git('add', 'README.md', cwd=repos) + _git('commit', '-m', 'diverge', cwd=repos) + _git('checkout', 'main', cwd=repos) + (repos / 'README.md').write_text('uncommitted local edit\n') + + payload, code = checkout_branch(str(repos), 'other', stash=True) + assert code == 200 and payload['status'] == 'success', payload + assert 'stashed' in payload['message'] + assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'other' + # The edit is not lost — it is on the stash. + assert 'switch to other' in _git('stash', 'list', cwd=repos).stdout diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 56f91a8b..4c80ed07 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1631,6 +1631,161 @@ def get_health(): 'data': {'status': 'unhealthy'} }), 500 +def _git_current_branch(project_dir): + """Current branch name, or '' when detached or git fails.""" + try: + r = subprocess.run(['git', 'branch', '--show-current'], + capture_output=True, text=True, timeout=10, cwd=str(project_dir)) + return r.stdout.strip() if r.returncode == 0 else '' + except (subprocess.TimeoutExpired, OSError): + return '' + + +def _git_upstream(project_dir): + """Configured upstream for the current branch (e.g. 'origin/main'), or ''.""" + try: + r = subprocess.run(['git', 'rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}'], + capture_output=True, text=True, timeout=10, cwd=str(project_dir)) + return r.stdout.strip() if r.returncode == 0 else '' + except (subprocess.TimeoutExpired, OSError): + return '' + + +def _git_remote_branch_exists(project_dir, branch): + """True when origin/ exists locally as a remote-tracking ref.""" + if not branch: + return False + try: + r = subprocess.run( + ['git', 'show-ref', '--verify', '--quiet', f'refs/remotes/origin/{branch}'], + capture_output=True, text=True, timeout=10, cwd=str(project_dir)) + return r.returncode == 0 + except (subprocess.TimeoutExpired, OSError): + return False + + +def resolve_pull_command(project_dir): + """Work out how to pull, for branches with and without an upstream. + + A plain ``git pull --rebase`` fails outright on a branch that has no + upstream ("There is no tracking information for the current branch"), + which is easy to end up on: checking out a branch by name, restoring a + backup, or following an install guide that names one. The update button + then reports a failure the user cannot act on. + + Returns ``(args, note, error)``. When ``origin/`` exists the pull + is made explicit against it, so the update proceeds and the branch is + given tracking information afterwards. + """ + upstream = _git_upstream(project_dir) + if upstream: + return ['git', 'pull', '--rebase'], '', None + + branch = _git_current_branch(project_dir) + if not branch: + return None, '', ( + "This checkout is in a detached HEAD state, so there is no branch " + "to update. Switch to a branch first (Tools -> Switch branch)." + ) + if _git_remote_branch_exists(project_dir, branch): + return ( + ['git', 'pull', '--rebase', 'origin', branch], + f"Branch '{branch}' had no upstream; pulled from origin/{branch} and set it as the upstream.", + None, + ) + return None, '', ( + f"Branch '{branch}' has no upstream and there is no origin/{branch} to " + f"pull from. Use Switch branch to move to a branch that exists on the " + f"remote, or push this one first." + ) + + +_BRANCH_NAME_RE = re.compile(r'[A-Za-z0-9._/-]{1,200}') + + +def is_valid_branch_name(name): + """Accept only plain branch names. + + This value becomes a subprocess argument, so anything exotic is refused + rather than escaped. '..' is excluded because it is range syntax to git. + """ + if not name or not _BRANCH_NAME_RE.fullmatch(name): + return False + return '..' not in name and not name.startswith('-') + + +def checkout_branch(project_dir, target, stash=False): + """Switch the checkout to `target`, returning (payload, http_status). + + Split out of the route so it can be tested against real repositories. + Attaches tracking when the branch exists on origin, so the next + Pull Latest is a plain `git pull` rather than the no-upstream fallback. + """ + target = (target or '').strip() + if not target: + return {'status': 'error', 'message': 'Branch name required'}, 400 + if not is_valid_branch_name(target): + return {'status': 'error', 'message': 'Invalid branch name'}, 400 + + try: + subprocess.run(['git', 'fetch', 'origin', '--prune'], + capture_output=True, text=True, timeout=60, cwd=project_dir) + + local_exists = subprocess.run( + ['git', 'show-ref', '--verify', '--quiet', f'refs/heads/{target}'], + capture_output=True, text=True, timeout=10, cwd=project_dir).returncode == 0 + remote_exists = _git_remote_branch_exists(project_dir, target) + if not local_exists and not remote_exists: + return {'status': 'error', + 'message': f"No branch '{target}' locally or on origin"}, 404 + + # Local edits block a checkout. Pull Latest already stashes for the + # same reason, so offer it here too -- but only when asked, never + # silently: putting someone's edits away unasked is worse than + # refusing the switch. + stash_note = '' + if stash: + stashed = subprocess.run(['git', 'stash', 'push', '-m', f'switch to {target}'], + capture_output=True, text=True, timeout=60, cwd=project_dir) + if stashed.returncode == 0 and 'No local changes' not in stashed.stdout: + stash_note = ' Local changes were stashed (recover them with git stash list).' + + if local_exists: + co = subprocess.run(['git', 'checkout', target], + capture_output=True, text=True, timeout=60, cwd=project_dir) + else: + # -B so a stale local ref does not block the checkout. + co = subprocess.run(['git', 'checkout', '-B', target, f'origin/{target}'], + capture_output=True, text=True, timeout=60, cwd=project_dir) + + if co.returncode != 0: + logger.warning("git checkout %s failed: %s", target, co.stderr) + return { + 'status': 'error', + 'message': f"Could not switch to '{target}'.", + # Keep git's full list of blocking files: naming them is the + # difference between an error the user can act on and one they + # cannot. + 'detail': (co.stderr or '').strip(), + 'can_retry_with_stash': 'would be overwritten by checkout' in (co.stderr or ''), + }, 200 + + if remote_exists: + subprocess.run(['git', 'branch', f'--set-upstream-to=origin/{target}', target], + capture_output=True, text=True, timeout=10, cwd=project_dir) + + logger.info("Switched checkout to branch %s", target) + return { + 'status': 'success', + 'message': f"Now on '{target}'.{stash_note} Use Pull Latest to fetch its newest code.", + }, 200 + except subprocess.TimeoutExpired: + return {'status': 'error', 'message': 'Timed out talking to git'}, 504 + except OSError as exc: + logger.error("checkout_branch failed: %s", exc, exc_info=True) + return {'status': 'error', 'message': 'Could not switch branch'}, 500 + + def get_git_version(project_dir=None): """Get git version information from the repository""" if project_dir is None: @@ -1795,6 +1950,14 @@ def execute_system_action(): # Use PROJECT_ROOT instead of hardcoded path project_dir = str(PROJECT_ROOT) + # Decide how to pull BEFORE stashing. If this checkout cannot be + # updated at all, stashing first would put the user's local changes + # away for an update that was never going to run. + pull_args, upstream_note, pull_error = resolve_pull_command(project_dir) + if pull_error: + logger.warning("git pull not attempted: %s", pull_error) + return jsonify({'status': 'error', 'message': pull_error}) + # Check if there are local changes that need to be stashed # Exclude plugins directory - plugins are separate repos and shouldn't be stashed with base project # Use --untracked-files=no to skip untracked files check (much faster with symlinked plugins) @@ -1849,15 +2012,28 @@ def execute_system_action(): except subprocess.TimeoutExpired: logger.warning("git rev-parse timed out before pull") - # Perform the git pull + # Perform the git pull. Branches without an upstream were given + # an explicit "origin " above so the update still works. result = subprocess.run( - ['git', 'pull', '--rebase'], + pull_args, capture_output=True, text=True, timeout=60, cwd=project_dir ) + # Give the branch tracking information so the next pull is a plain + # `git pull` — otherwise every update repeats the fallback. + if result.returncode == 0 and upstream_note: + branch = _git_current_branch(project_dir) + if branch: + try: + subprocess.run( + ['git', 'branch', f'--set-upstream-to=origin/{branch}', branch], + capture_output=True, text=True, timeout=10, cwd=project_dir) + except (subprocess.TimeoutExpired, OSError) as exc: + logger.debug("could not set upstream for %s: %s", branch, exc) + # Return custom response for git_pull if result.returncode == 0: pull_message = "Code updated successfully." @@ -1865,6 +2041,8 @@ def execute_system_action(): pull_message = f"Code updated successfully. Local changes were automatically stashed.{stash_info}" if result.stdout and "Already up to date" not in result.stdout: pull_message = f"Code updated successfully.{stash_info}" + if upstream_note: + pull_message = f"{pull_message} {upstream_note}" # Keep Python dependencies in sync automatically: if the pull # changed a requirements file, install it now — users updating @@ -1929,12 +2107,25 @@ def execute_system_action(): logger.warning("Post-update plugin purge failed: %s", purge_err) else: logger.warning("git pull failed (returncode=%d): %s", result.returncode, result.stderr) - pull_message = "Update failed; check logs for details" + # Show git's own first line: "check logs" leaves the user with + # nothing to act on, and these failures are usually actionable + # (conflicting local commits, no upstream, network). + detail = next((ln.strip() for ln in (result.stderr or '').splitlines() + if ln.strip()), '') + pull_message = f"Update failed: {detail}" if detail else "Update failed; check logs for details" return jsonify({ 'status': 'success' if result.returncode == 0 else 'error', 'message': pull_message, }) + elif action == 'checkout_branch': + # Switch branches from the Tools tab. Needed because a checkout + # that predates tracking (or a restored backup) can leave the pi + # on a branch the update button cannot pull. + result_payload, http_status = checkout_branch( + str(PROJECT_ROOT), data.get('branch') or '', stash=bool(data.get('stash'))) + return jsonify(result_payload), http_status + elif action == 'restart_display_service': result = subprocess.run(['sudo', 'systemctl', 'restart', 'ledmatrix.service'], capture_output=True, text=True, timeout=10) @@ -2078,18 +2269,64 @@ def get_git_info(): log = subprocess.run([_GIT, 'log', '--oneline', '-5'], capture_output=True, text=True, timeout=10, cwd=d) remote = subprocess.run([_GIT, 'remote', 'get-url', 'origin'], capture_output=True, text=True, timeout=10, cwd=d) + branch_name = branch.stdout.strip() + upstream = _git_upstream(d) return jsonify({ - 'branch': branch.stdout.strip(), + 'branch': branch_name, 'dirty': bool(status.stdout.strip()), 'status': status.stdout.strip(), 'recent_commits': log.stdout.strip() if log.returncode == 0 else '', 'remote_url': _scrub_git_remote_url(remote.stdout.strip()) if remote.returncode == 0 else '', + # Surfaced so the Tools tab can warn before the user clicks Pull + # Latest, rather than after it fails. + 'upstream': upstream, + 'can_pull': bool(upstream) or _git_remote_branch_exists(d, branch_name), }) except Exception as e: logger.error("get_git_info failed: %s", e, exc_info=True) return jsonify({'status': 'error', 'message': 'Failed to get git info'}), 500 +@api_v3.route('/system/git-branches', methods=['GET']) +def get_git_branches(): + """List branches available to switch to, for the Tools tab picker.""" + if not _GIT: + return jsonify({'status': 'error', 'message': 'git not found on this system'}), 503 + d = str(PROJECT_ROOT) + try: + # Refresh remote refs so a branch created since the last fetch shows up. + subprocess.run([_GIT, 'fetch', 'origin', '--prune'], + capture_output=True, text=True, timeout=60, cwd=d) + + local = subprocess.run([_GIT, 'for-each-ref', '--format=%(refname:short)', 'refs/heads'], + capture_output=True, text=True, timeout=15, cwd=d) + remote = subprocess.run([_GIT, 'for-each-ref', '--format=%(refname:short)', 'refs/remotes/origin'], + capture_output=True, text=True, timeout=15, cwd=d) + if local.returncode != 0: + return jsonify({'status': 'error', 'message': 'Could not list branches'}), 500 + + local_names = [b for b in local.stdout.split() if b] + remote_names = [] + for ref in remote.stdout.split() if remote.returncode == 0 else []: + name = ref.split('origin/', 1)[-1] + # origin/HEAD is a symbolic alias, not a branch a user can pick. + if name and name != 'HEAD' and name not in local_names: + remote_names.append(name) + + return jsonify({ + 'status': 'success', + 'current': _git_current_branch(d), + 'upstream': _git_upstream(d), + 'local': sorted(local_names), + 'remote_only': sorted(remote_names), + }) + except subprocess.TimeoutExpired: + return jsonify({'status': 'error', 'message': 'Timed out talking to the remote'}), 504 + except OSError as e: + logger.error("get_git_branches failed: %s", e, exc_info=True) + return jsonify({'status': 'error', 'message': 'Failed to list branches'}), 500 + + @api_v3.route('/hardware/status', methods=['GET']) def get_hardware_status(): """Return LED matrix hardware initialization status written by display_manager at startup.""" diff --git a/web_interface/templates/v3/partials/tools.html b/web_interface/templates/v3/partials/tools.html index b552e93f..f4f7cfdd 100644 --- a/web_interface/templates/v3/partials/tools.html +++ b/web_interface/templates/v3/partials/tools.html @@ -31,6 +31,24 @@
+ +
+
+

Branch

+

Choose which branch this pi follows. Switching attaches tracking, so Pull Latest works afterwards.

+
+
+ + +
+
+ +
@@ -467,6 +485,14 @@
`; } + if (d.upstream) { + html += `

tracking ${escHtml(d.upstream)}

`; + } else if (d.can_pull) { + html += `

No upstream set; Pull Latest will use origin/${escHtml(d.branch || '')} and set it.

`; + } else { + html += `

No upstream and no matching branch on origin — Pull Latest cannot run. Switch to a branch that exists on the remote.

`; + } + if (d.remote_url) { html += `

${escHtml(d.remote_url)}

`; } @@ -479,6 +505,84 @@ }); } + // ── branch picker ───────────────────────────────────────────────────── + // A pi can end up on a branch with no tracking information (checked out by + // name, restored from a backup), where `git pull` refuses to run. Being + // able to see and change the branch from here beats needing SSH. + + function loadBranches() { + const sel = document.getElementById('branch-select'); + if (!sel) return; + + fetch('/api/v3/system/git-branches') + .then(r => r.ok ? r.json() : r.json().then(d => Promise.reject(d.message || `HTTP ${r.status}`))) + .then(d => { + if (d.status === 'error') { + sel.innerHTML = ``; + sel.disabled = true; + return; + } + sel.innerHTML = ''; + const add = (name, suffix) => { + const o = document.createElement('option'); + o.value = name; + o.textContent = name + (suffix || ''); + if (name === d.current) o.selected = true; + sel.appendChild(o); + }; + (d.local || []).forEach(b => add(b, b === d.current ? ' (current)' : '')); + // Remote-only branches are checked out on demand. + (d.remote_only || []).forEach(b => add(b, ' (remote)')); + if (!sel.options.length) add('', 'no branches found'); + }) + .catch(err => { + sel.innerHTML = ``; + sel.disabled = true; + }); + } + + window.checkoutBranch = function(stash) { + const sel = document.getElementById('branch-select'); + const branch = sel && sel.value; + if (!branch) return; + + setBusy('btn-checkout-branch', true); + const el = document.getElementById('result-checkout-branch'); + if (el) el.classList.add('hidden'); + + fetch('/api/v3/system/action', { + method: 'POST', + headers: {'Content-Type': 'application/json'}, + body: JSON.stringify({action: 'checkout_branch', branch: branch, stash: !!stash}) + }) + .then(r => r.json().catch(() => ({status: 'error', message: `HTTP ${r.status}`}))) + .then(d => { + const ok = d.status === 'success'; + // Show git's own list of blocking files, then offer the single + // action that clears it. Stashing is never done unasked. + showResult('result-checkout-branch', ok, d.message || '', d.detail || ''); + if (!ok && d.can_retry_with_stash && el) { + const retry = document.createElement('div'); + retry.className = 'mt-2 flex items-center gap-2'; + const label = document.createElement('span'); + label.className = 'text-xs text-gray-700'; + label.textContent = 'Stash these changes and switch anyway?'; + const btn = document.createElement('button'); + btn.className = 'inline-flex items-center px-2 py-1 border border-gray-300 text-xs font-medium rounded text-gray-700 bg-white hover:bg-gray-50'; + btn.textContent = 'Stash and switch'; + btn.onclick = function() { window.checkoutBranch(true); }; + retry.appendChild(label); + retry.appendChild(btn); + el.appendChild(retry); + } + // Both panels describe the checkout, so refresh them together. + loadGitInfo(); + loadBranches(); + }) + .catch(err => showResult('result-checkout-branch', false, String(err))) + .finally(() => setBusy('btn-checkout-branch', false)); + }; + // ── power supply diagnostics panel ──────────────────────────────────────── // Reuses the same SSE stream (window.statsSource, set up in base.html) // that already drives the header badge/banner and Overview card, instead @@ -810,6 +914,7 @@ // Load on first render; HTMX will have already swapped us in by this point. loadGitInfo(); + loadBranches(); // Plugin health: initial load + periodic refresh. Guard against duplicate // timers if this partial is re-swapped in by HTMX; the handler re-resolves