fix(cli): restore visible git fetch progress and defer autostash until update confirmed (#3523) - #62962
Open
sahilthakur456111-stack wants to merge 1 commit into
Conversation
…l update confirmed (NousResearch#3523) Regression from NousResearch#3492 introduced two user-visible issues in `hermes update`: 1. `git fetch` was run with `capture_output=True`, swallowing git's progress output (object enumeration, remote branch listing). 2. `git stash push` ran BEFORE we knew whether there was an update to apply. On an up-to-date tree with local changes, every `hermes update` call would autostash-and-pop for nothing — cluttering the stash list and triggering an interactive restore prompt. Fix: - Switch the user-facing fetch to `capture_output=False` so progress reaches the terminal. Re-run with `capture_output=True` only on failure so we can still pattern-match stderr for network/auth errors. - Move the `git status` + `git stash push` block to AFTER the `git rev-list HEAD..origin/main --count` check. If the count is 0, no stash is created and we exit cleanly. - The race where someone else pulls between our check and our stash is still handled by the post-stash `commit_count == 0` block. Tests: - `test_3523_no_stash_when_up_to_date_with_local_changes`: verifies no `git stash push` is called when up to date. - `test_3523_fetch_stderr_visible_on_success`: verifies the user-facing fetch uses `capture_output=False`. - `test_3523_stash_still_happens_when_update_available`: verifies the fix didn't break the normal update-with-local-changes path.
Contributor
|
Thanks for isolating two real Problems
Suggested changes
Automated hermes-sweeper review. |
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3523
Symptom
Regression from #3492 introduced two user-visible issues in
hermes update:Silent git fetch —
git fetchran withcapture_output=True, swallowing git's progress output (object enumeration, remote branch listing). Users saw only the literal string "→ Fetching updates..." with no indication anything was happening.Autostash on every run —
git stash pushran BEFORE we knew whether there was an update to apply. On an up-to-date tree with local changes, everyhermes updatecall would:hermes-update-autostash-<timestamp>…for nothing. Clutters the stash list, breaks
--yescron updates, and prompts in non-interactive contexts.Repro
Expected: update completes silently (or with progress), no stash prompt.
Actual: "⚠ Local changes were stashed before updating." → "Restore local changes now? [Y/n]" prompt → stash restored.
Root Cause
Fix
1. Visible fetch progress
Switch the user-facing fetch to
capture_output=False. On failure, re-run withcapture_output=Trueso we can still pattern-match stderr for network/auth errors (the existingCould not resolve host,Authentication failed, etc. logic is preserved).2. Deferred stash
Move
_stash_local_changes_if_neededto AFTER therev-listcount check. Ifcommit_count == 0, we exit cleanly without ever creating a stash. The race condition where someone else pulls between our check and our stash is still handled correctly by the post-stashcommit_count == 0block.3. Removed redundant rev-list re-run
The original code ran
rev-listtwice (once before the stash, once after). Sincegit stashdoesn't move HEAD, the second call was redundant. Removed.Tests Added
Three new tests in
tests/hermes_cli/test_cmd_update.py:test_3523_no_stash_when_up_to_date_with_local_changes— assertsgit stash pushis NOT called when up to date, even with dirty treetest_3523_fetch_stderr_visible_on_success— asserts user-facing fetch hascapture_output=Falsetest_3523_stash_still_happens_when_update_available— asserts stash still happens when update IS available (regression guard for the deferred-stash fix)All 30 tests pass (27 existing + 3 new).
Checklist
maintest_cmd_update.pypasscapture_outputdoesn't affect what gets cachedHERMES_*env vars