fix(cli): avoid unnecessary stash and show fetch progress in hermes update - #20112
fix(cli): avoid unnecessary stash and show fetch progress in hermes update#20112liuhao1024 wants to merge 1 commit into
Conversation
1 similar comment
There was a problem hiding this comment.
Pull request overview
Fixes two regressions in hermes update introduced by #3492: (1) an unnecessary stash/restore cycle (and confusing prompt) when running on main with no updates, and (2) git fetch progress output being silently swallowed by capture_output=True.
Changes:
- Defer
_stash_local_changes_if_needed()on the main-branch path until aftercommit_countis computed, so stashing only happens when there are actual updates to pull. - After a successful
git fetch, print captured stderr lines so the user sees git's progress messages. - Add three regression tests in
test_update_autostash.pycovering both fixes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hermes_cli/main.py | Moves main-branch stash call after commit-count check; prints fetch stderr after success. |
| tests/hermes_cli/test_update_autostash.py | Adds 3 regression tests for no-stash on already-up-to-date, stash on updates, and visible fetch progress. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating two real hermes update regressions: current main still captures successful fetch stderr at hermes_cli/main.py:9570-9575 and stashes before the same-branch count check at hermes_cli/main.py:9650-9668.
Problems
- The new same-branch
auto_stash_ref = Nonepath removes protection from fork updates whereorigin/mainis current but upstream is ahead. Current main invokes_sync_with_upstream_if_needed()before the no-update return (hermes_cli/main.py:9671-9673), and that helper can rungit pull --ff-only upstream main(hermes_cli/main.py:6879-6884). Dirty edits that overlap upstream changes can therefore block this update path instead of being safely stashed. - Fetch stderr is still captured, then printed after fetch completion. This makes progress visible eventually but not live.
Suggested changes
- Keep autostash protection for a possible upstream fast-forward, and add a dirty-fork/upstream-ahead regression test.
- Stream fetch progress while preserving the existing failure diagnostics.
Automated hermes-sweeper review.
| and not assume_yes | ||
| and (gateway_mode or (sys.stdin.isatty() and sys.stdout.isatty())) | ||
| ) | ||
| auto_stash_ref = None |
There was a problem hiding this comment.
This removes the pre-count stash for fork checkouts too: when origin/main is current but upstream/main is ahead, the no-update branch invokes _sync_with_upstream_if_needed(), which can run git pull --ff-only upstream main. Preserve a stash before that possible fast-forward or add a prior upstream availability check and stash only when it reports an update.
…pdate Two fixes for regressions after NousResearch#3492: 1. Show git fetch progress (sent to stderr) instead of discarding it 2. Defer _stash_local_changes_if_needed to after commit_count > 0 check, avoiding pointless stash/restore cycles when the checkout is current Fixes NousResearch#3523
77d657e to
8c69049
Compare
What does this PR do?
Fixes two regressions in
hermes updateintroduced after #3492:Root Cause
In
_cmd_update_impl, the stash was called unconditionally at the top of the "already on target branch" path, before therev-list --countcheck that determines whether updates exist. This meant evenhermes updateon a clean checkout with no upstream changes would stash and immediately restore.Fix
fetch_result.stderrlines after a successful fetch so the user sees progress._stash_local_changes_if_neededto after thecommit_count > 0check, so the stash is only created when there are actual commits to pull.This is a rebased version of the original PR — only the stash/fetch fix is included.
Related Issue
Fixes #3523
Type of Change
Changes Made
hermes_cli/main.py: Added fetch progress display after successfulgit fetchhermes_cli/main.py: Moved_stash_local_changes_if_neededcall to after thecommit_count > 0check in the same-branch pathHow to Test
python3 -m pytest tests/hermes_cli/test_update_autostash.py -q— should pass (33 tests)hermes updateon a checkout that is already up to date — should NOT print any stash messageshermes updatewhen updates are available — should show fetch progress lines, then stash if neededChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A