GitDesktop / Tickets / #151 feat(stash,sync,branches): add stash-and-reapply recovery

Anonymous - 2026-08-07

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 794cc72

Status: ✅ Deploy successful!

Preview URL: https://a27d2af3.gitdesktop.pages.dev

Branch Preview URL: https://feature-stash-and-reapply-re.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Originally posted by: theBGuy

Context for reviewers — deliberate calls, evidence, and disclosures, numbered for reference. (Posted while draft so it precedes the first review.)

What this is. One-click stash → run → reapply recovery when uncommitted changes block a sync operation (#150). A dirty tree that doesn't overlap the incoming change behaves exactly as before — the machinery only engages on git's refusal.

Deliberate calls:

Recovery is reactive for pull and branch updates — it fires only when git actually refuses, so the common non-overlapping case keeps its one-click path (live-verified: a dirty non-overlapping pull runs untouched, no stash created). The branch-switch surface is pre-flight by design: its dialog is the pre-existing switch-with-changes prompt (git checkout's bring-vs-stash semantics make a pre-flight choice the right model there), now carrying a reapply option.

App-level compound, not --autostash. Measured on git 2.51.1: --autostash exits 0 even when the reapply conflicts (the only signal is a printed line; no MERGE_HEAD/rebase state exists, so no conflict UI can key off it), it never stashes untracked files (our stash is --include-untracked), and switch/checkout have no autostash at all.

DIRTY_TREE_MARKERS are measured strings (git 2.51.1, cited at the definition), deliberately disjoint from CONFLICT_MARKERS so real conflicts keep their existing banner recovery. git switch emits "overwritten by checkout" — measured, not assumed.

run_git_with_creds_once returns raw output (non-zero exit ≠ Err): a conflicted merge/pull writes its report to stdout with an empty stderr (measured), and error-shaping at that layer would hand the UI empty payloads. run_git_mutating_with_creds re-shapes to AppError::Git; all 10 existing call sites keep their exact contract (each verified).

Lock semantics in run_git_mutating_with_creds: one continuous repo-lock hold across the injected + ambient attempts (previously two acquisitions), with the index.lock retry wrapping the pair. Worst-case wall clock is structurally unchanged (both old and new are two runner invocations deep per attempt pair).

ReapplyConflicted.conflicted / OpFailedStashKept.inProgress exist because each variant covers two user-distinguishable states — an untracked-file pop abort leaves no unmerged paths (nothing in the changes list to point at), and a failed restore-pop leaves no banner to continue/abort. Copy branches on them; the wire shape is pinned by a serialization test over all variants.

The unchecked "Stash and switch" path now routes through the same compound (StashedOnly): one continuous repo-lock hold with the stash popped back if the switch fails. The old two-step path left the stash stranded on a failed checkout — that delta is an intentional upgrade, not parity.

ConflictBanner's !op branch extension is generic on purpose — it also improves manual git stash pop conflicts, which previously showed a bare count.

No new palette actions — the dialogs are responses, not invocable commands; the existing pull/fetch/update actions route through the recovered paths (hotkey pull verified live).

No oplog wrap — these compounds never reset --hard; a stranded autostash is recoverable via the stash list and the orphaned-stash rescue UI.

MCP surface unchanged — agents keep raw-error semantics and can stash explicitly; changing agent-facing behavior silently felt wrong.

Cancelling the recovery prompt intentionally shows no further error — the dialog itself is the explanation of why the operation didn't run; declining it is an informed choice.

Verification: cargo 1001 passed / 0 failed (20 autostash tests incl. the wire-shape pin), clippy clean, pnpm build + site build green. Live dogfood on a scratch origin+clone: tracked and untracked refusals, both ReapplyConflicted arms, preference-on no-dialog recovery, non-overlap fast path, settings round-trips, switch reapply end-to-end, Enter/Esc keyboard paths.

Disclosures (deferred, each with a home):

The branch menu's plain merge/rebase actions are not wired to this recovery (deliberate scope: the spec records widening the merge dialog as future work; the same markers + backend compound make it a small follow-up). → backlog.

Pre-existing sibling gaps our own sweep found (not introduced here): git_stash_paths_core guards unmerged entries but not a fully staged mid-merge resolve, and git_stash_all_core has no mid-op guard at all. The new compounds guard both. → backlog.

The real-HTTPS credential pull path is contract-verified at all call sites but not live-run (tests use file:// remotes); the first dirty pull against an authenticated remote covers it. OpFailedStashKept's in-progress arm is unit-tested; its banner is pre-existing UI.

Help's Settings → General enumeration doesn't list the two new toggles (that bullet is adjacent to PR [#148]'s territory; the new help section names the setting and its location). → fold in after [#148] merges.

Auto-ticking "Reapply after switching" when re-opening after a bring-changes refusal was considered and deferred — the hint copy names the real control instead; implicit seeding of a persisted preference needs its own design pass. → backlog.

A double-confirm race on the recovery dialog is benign by construction: compounds serialize on the repo lock and the second one hits the mid-op refusal guard.

PR [#148] coexistence: the three shared files (README, help content, git api.ts) are edited additively in disjoint regions; either merge order rebases clean.

Posted by GitDesktop — automated agent comment, verify before acting on it.

Related

Tickets: ~~#148~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Summary

Adds an app-level stash → run → reapply compound (git_pull_autostash / git_merge_autostash / git_switch_autostash) plus the UI choreography that offers it when git refuses a pull, branch update, or switch over uncommitted changes, and refactors run_git_mutating_with_creds into a lock-free run_git_with_creds_once + a locking wrapper so the compounds can run under a single repo-lock hold.

The design is sound and I found nothing blocking: the lock discipline is correct (every inner step uses the lock-free runners, refuse_mid_op gates both the unmerged-index and the staged-mid-op case), settle never drops the stash, the plain-command error shape is preserved for the nothing-stashed path, and the outcome wire shape is pinned by a serialization test. Docs are covered across all four surfaces (README, capabilities.ts, help guide, changelog fragment).

Correctness & feedback

should-fix — src/features/repository/SyncControls.tsx:370 (Pull button icon). The spinner is still gated on pull.isPending alone, but the recovery compound is a second, separate mutation. With Automatically stash and reapply on pull on, clicking Pull runs: plain pull fails fast (git refuses locally, no network) → pullAutostash runs a real git pull under NETWORK_TIMEOUT with no dialog open — so for the whole network round-trip the button group is merely flat/disabled (busy includes recovery.pending) with no progress indicator and no toast. The confirm-prompt path is fine (the dialog holds its own spinner), but the auto path is exactly the case the preference makes routine. Fix: {pull.isPending || recovery.pending ? <Spinner data-icon="inline-start" /> : <ArrowDownIcon data-icon="inline-start" />}. recovery.pending in this component also covers the update-from-upstream recovery, which is launched from the same button group's caret menu, so the Pull button is the right place for it — no second call site needed.

Recorded decisions I'm not re-raising: the branch menu's plain merge/rebase actions staying unwired to the recovery (note 13 — the same DIRTY_TREE_MARKERS fire there, so the inconsistency is real but deliberate and backlogged); the help guide's Settings & updates General enumeration (src/features/help/content.ts:1898) not listing the two new toggles (note 16 — and that bullet already omits auto-fetch, so it isn't claiming exhaustiveness); the real-HTTPS credential pull path being contract-verified rather than live-run (note 15).

Tests

should-fix — src/lib/error-summary.ts:83 (DIRTY_TREE_MARKERS). The entire feature triggers off six hand-measured English strings, and nothing in CI notices if git rewords one — the failure mode is silent: the recovery simply stops being offered and users get today's dead-end toast. There's no frontend test runner, but the Rust suite already provokes these exact refusals with real git; add a test in autostash.rs that runs a blocked pull (tracked and untracked overlap), a blocked pull --rebase (unstaged and staged-index variants), and a blocked switch (tracked and untracked), asserting each stderr.to_lowercase() contains the corresponding literal. Keep the literals duplicated in the test on purpose — that duplication is the canary — and add a comment on DIRTY_TREE_MARKERS pointing at the test so the two lists stay paired.

nit — src-tauri/src/git/remote.rs:106 — nothing pins the refactored run_git_mutating_with_creds' hand-built AppError::Git { code, stderr } shaping; one real-repo test with an empty cred slice and a failing sub-command (e.g. &["merge", "nope"]) asserting AppError::Git with git's own code/stderr would guard the contract the 10 existing call sites rely on.

Nits

README.md, Stash and reapply bullet: "one git refuses outright leaves them safely stashed" is missing a word — "one that git refuses outright…" (the changelog fragment already has the correct phrasing).

src/features/repository/useStashReapplyRecovery.tsx:36-55 — stderrDetails re-implements toastError's Details/Copy action builder verbatim (src/lib/toast.ts:14-29); export the action builder from toast.ts (taking an ErrorPresentation) and call it from both so the two can't drift.

src-tauri/src/git/autostash.rs, refuse_mid_op — the unmerged-index arm says "Can't stash while a merge conflict is in progress" even when the unmerged entries come from a rebase or cherry-pick (that arm is reached first); "…while a conflict is in progress" would cover all three, matching the op-state arm below it.

src/features/repository/SwitchWithChangesDialog.tsx:83,88 — stashPending / bringPending are effectively unobservable: both handlers call setSwitchTarget(null) before starting the mutation, so the dialog is already closed by the time either flag flips. Either drop the props or keep the dialog open until the compound settles (the way StashReapplyDialog does).

src/features/settings/GeneralSection.tsx:114 — the label "Automatically stash and reapply on pull" under-sells the toggle, which also governs Update from upstream and Update branch from (its own description says so); "…on pull and branch updates" would match. Worth settling now since autoStashOnPull is a persisted key that's awkward to rename later.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No genuinely exploitable security vulnerability is introduced by these changes.

What I checked and why it clears: the three new Tauri commands in src-tauri/src/git/autostash.rs are the only new privileged entry points, and every caller-supplied argument reaching an argv is guarded — mode is mapped to a closed set of literal flags (--rebase/--no-rebase/--ff-only), and branch/name/remote go through branches::validate_ref_name, which rejects empty, leading - (option injection), and * ? [ : \ <space> plus ASCII controls; the switch --track {remote}/{name} argv is byte-identical in shape to the pre-existing git_checkout_remote_branch, and merge here uses a stricter validator than git_merge_core's validate_branch_arg. run_git_raw spawns via tokio::process::Command with an argv array (no shell), so no interpreter is involved. The remote.rs split into run_git_with_creds_once / run_git_mutating_with_creds preserves the auth-retry gate (is_auth_class_failure on a non-zero exit) and the injected -c entries are credential.https://<host>.helper=<abs path> — no token material on the command line — so the new AutostashOutcome.stderr/stdout-fallback payload surfaces no secret the plain AppError::Git { stderr } path didn't already. On the frontend, stderr reaches only React text nodes and presentError/toast/clipboard — no dangerouslySetInnerHTML, no URL-scheme sink; the settings spread merges keys the code fully controls into a validated shape.

Copilot's remote.rs:93 claim that &args.iter().map(String::as_str).collect::<Vec<_>>() is a borrowed temporary dropped across .await is wrong: argument temporaries live to the end of the enclosing let statement, and the diff shows this is the identical expression the pre-change code already awaited through run_git_mutating. Not a defect, and not security either way.

Posted by GitDesktop — AI output, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Originally posted by: theBGuy

Round-2 dispositions (GitDesktop AI review + security audit). Everything below is applied in the tree and rides the next push, alongside Copilot's accepted dialog fix.

Should-fixes — both fixed.

Pull-button spinner: now gates on pull.isPending || recovery.pending, your prescribed mechanism — verified that recovery.pending ORs both compounds, so the update-from-upstream recovery is covered from the same button group. One correction to the rationale for the record: the update arm deliberately re-merges with no second fetch (that's why dirty-blocked carries the resolved ref), so "runs a real network pull" is true only of the pull arm; the code comment states the no-dialog rationale without the network claim. Known cosmetic edge, on the record: with the preference on, an update recovery spins the Pull button rather than the caret menu that launched it — the group shares one button row and the caret has no spinner slot; judged acceptable.

Marker canary: added refusal_stderr_still_matches_the_frontend_markers (autostash.rs). All six literals were re-derived by measurement inside the test and matched DIRTY_TREE_MARKERS exactly; they're duplicated in-test deliberately (the duplication is the canary), and DIRTY_TREE_MARKERS now carries the pairing pointer. Two scope notes: four of the six cases are provoked via plain merge/switch (measured: pull --ff-only/--no-rebase reach the same merge machinery and emit identical lines, which keeps those cases network-free — the two rebase variants use a real bare-origin clone), and the canary pins markers-vs-git, not the disjointness-from-CONFLICT_MARKERS claim, which remains comment-asserted.

run_git_mutating_with_creds contract test (your test nit): added mutating_with_creds_surfaces_gits_own_code_and_stderr — empty cred slice, failing merge, asserts AppError::Git with git's own exit code and stderr (measured: merge: nonexistent-ref-xyz - not something we can merge).

Nits — all five fixed.

README missing word — fixed ("one that git refuses outright").

stderrDetails duplication — errorToastAction(presentation) is now exported from toast.ts and used by both toastError and the hook; behavior parity verified (same Details/Copy selection, dialog wiring, clipboard fallback; repo-wide grep shows no other hand-built instance).

Guard message — fixed, and your point generalized one file further: git_stash_paths_core's identical sibling guard in ops.rs carried the same "merge conflict" wording; both now read "while a conflict is in progress".

Dead bringPending/stashPending props — removed from the dialog and call site (they were unobservable before this PR too: both handlers close the dialog before their mutation starts); the dialog's doc comment updated to match.

Setting label — now "Automatically stash and reapply on pull and branch updates" (persisted key unchanged), swept across GeneralSection, README, the changelog fragment, and the help guide.

Security audit: acknowledged, no action needed — and it independently settled Copilot's remote.rs lifetime claim with the same reasoning used to decline it on the thread.

Verification on the batch: cargo 1003 passed / 0 failed, clippy -D warnings clean, tsc -b clean, pnpm build green, scoped biome clean on every touched file.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Both should-fixes and all five nits from the last round are applied and correct in the current diff; the fix hunks themselves (the marker canary, the errorToastAction extraction, the prop removal, the guard-message sweep) are clean, and I found no new problem in them.

Resolved since last review

Pull-button spinner — SyncControls.tsx:370 now gates on pull.isPending || recovery.pending, so the no-dialog auto path shows progress; recovery.pending covers both compounds in that hook instance, so the caret-launched update recovery is covered from the same button.

Marker canary — refusal_stderr_still_matches_the_frontend_markers (autostash.rs) provokes all six refusals with real git and asserts the literals; run_git_raw pins LC_ALL=C, so the lower-cased matching is locale-stable, and the pull --rebase unstaged/staged split is correctly staged (b.txt is tracked from the base commit, then git added for the index variant). DIRTY_TREE_MARKERS carries the pairing pointer back.

run_git_mutating_with_creds contract test — mutating_with_creds_surfaces_gits_own_code_and_stderr pins code + git's own stderr; AppState, temp_base, and init_repo are all in scope in that test module.

README missing word — "one that git refuses outright".

stderrDetails duplication — errorToastAction is exported from toast.ts and used by both toastError and the hook; a repo-wide grep for useErrorDialog.getState().open / label: "Details" now finds only toast.ts. The doc comment moved with the extracted logic and toastError kept an accurate one-liner.

Guard message — "while a conflict is in progress" in both autostash.rs:80 and the sibling ops.rs:626; no stale copy of the old wording survives anywhere (conflict is in progress matches exactly those two lines).

Dead bringPending/stashPending — removed from both the dialog and the BranchSwitcher call site, and the dialog's doc comment updated to match; checkout/checkoutRemote/stashAll all still have live references, so nothing went unused.

Setting label — "Automatically stash and reapply on pull and branch updates" is consistent across GeneralSection.tsx:114, README, the changelog fragment, and help/content.ts (the help copy is line-wrapped, so a naive grep for the phrase misses it — verified against the diff).

Copilot's SwitchWithChangesDialog template-literal — fixed: the reapply arm is JSX interpolation now, so a null target renders empty during the close transition.

Copilot's remote.rs:93 lifetime claim is a false positive and needs no action: the Vec<&str> is an argument-position temporary, so it lives until the end of the enclosing let out = …; statement and outlives the .await.

Recorded decisions still standing, not re-raised: the branch menu's plain merge/rebase actions remain unwired to the recovery (note 13, backlogged) — useMergeBranch at BranchSwitcher.tsx:749 is still the only such call site; and the real-HTTPS credential pull path is contract-verified rather than live-run (note 15).

Leftover polish (non-blocking)

src/features/repository/StashReapplyDialog.tsx — Cancel (and Esc, via onOpenChange) stays enabled while pending: it clears the request and closes the dialog but doesn't stop the in-flight compound, which then reports via toast. Disable it alongside the confirm button (disabled={pending}) and gate onOpenChange on !pending so the dialog can't be dismissed mid-stash.

Nothing further to raise.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Originally posted by: theBGuy

On the one leftover polish item (Cancel/Esc enabled while pending): declining, with the reasoning on record. Close-immediately-report-by-toast is the deliberate house idiom for these flows — the switch dialog in this same PR closes before its compound even starts, and the recovery's completion toast reports into the closed-dialog state (exercised live in the round-0 dogfood). The compound is non-destructive at every exit (worst case: changes safely in the stash) and serialized on the repo lock, so a dismissal can't corrupt anything — while gating onOpenChange on !pending would hold the user inside a modal for up to a network-length pull with no way out and nothing to decide. If we'd rather lock it after all, the right execution is narrower than suggested: block dismissal only across the sub-second stash step (the only window where "mid-stash" is literally true), never across the network op — but as a default, dismissable-with-toast wins.

With that recorded: every review item across all rounds is now fixed, declined-with-evidence, or deferred-with-a-home; the re-review verified the full round-2 batch against the code and raised nothing blocking; CI is green on this head across all eight checks. Merge-ready from this side.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-07

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

feat(stash,sync,branches): add stash-and-reapply recovery

Milestone

Searches

Help

#151 feat(stash,sync,branches): add stash-and-reapply recovery

Git backend

Frontend plumbing

UI

Settings

Documentation

Related

Discussion

Deploying gitdesktop with Cloudflare Pages

Related

Summary

Correctness & feedback

Tests

Nits

Resolved since last review

Leftover polish (non-blocking)

Latest commit:	`794cc72`
Status:	✅ Deploy successful!
Preview URL:	https://a27d2af3.gitdesktop.pages.dev
Branch Preview URL:	https://feature-stash-and-reapply-re.gitdesktop.pages.dev