feat(gix-stash): new crate — push / pop / list MVP#2603
feat(gix-stash): new crate — push / pop / list MVP#2603mxaddict (mxaddict) wants to merge 11 commits into
Conversation
New plumbing crate placeholder for git-stash workflows. push() / pop() currently return Error::NotImplemented; full implementation follows in subsequent commits. Crate layout mirrors gix-blame: lib.rs re-exports, push/ and pop/ modules each with their own Options/Outcome/Error types. The plumbing takes lower-level handles (index, ODB, ref store, worktree path) rather than gix::Repository so it stays in the plumbing layer; gix porcelain will wrap it as Repository::stash_push / stash_pop. Wired into the workspace and exposed through gix as the gated `stash` feature, matching the blame/worktree-stream pattern. Reference: crate-status.md gix-stash checklist. Co-authored-by: Claude <noreply@anthropic.com>
Third MVP op — walks refs/stash reflog and returns entries newest-first (matching git stash list output order). Entry carries index, commit oid, reflog message, and Unix timestamp; returns Vec<Entry> wrapped in Outcome. Empty Outcome when refs/stash is unborn. Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Push: build WIP tree from actual working-tree file content (not index OIDs) so unstaged modifications are captured; reset WT to HEAD after stash commit via gix_worktree_state::checkout; remove untracked files from disk when include_untracked is set. Pop: perform 3-way merge (base=stash parent[0] tree, ours=current HEAD tree, theirs=stash WIP tree) via gix_merge::tree; write merged result to WT; restore untracked files from parent[2] on clean merge; leave refs/stash intact on conflict. Context structs are now generic over Objects (Find + FindHeader + Write + Send + Clone) matching gix-blame/gix-merge conventions. Remove stash_tree and base_commit from pop::Outcome; pop now does the merge itself. Add gix-validate and gix-path to direct deps. Co-authored-by: Claude <noreply@anthropic.com>
Cover all three operations with bash fixture scripts and Rust tests. Fix NoLocalChanges guard in push to compare WIP/index trees against HEAD rather than checking for an empty index. - list: 3 tests (newest-first ordering, empty repo, positive timestamps) - push: 6 tests (unstaged capture, staged capture, clean-WT bug→fix, include_untracked flag, exclude_untracked flag, empty-repo doc) - pop: 5 tests (clean apply + ref drop, multi-stash stack, no-stash error, untracked restore, conflict keeps ref intact) Co-authored-by: Claude <noreply@anthropic.com>
Manual initial entry per gitoxide convention — cargo-smart-release will auto-augment future releases with commit-statistics sections. Documents the MVP scope (list / push / pop), behaviour, errors, and known limitations carried over from the implementation TODOs. Co-authored-by: Claude <noreply@anthropic.com>
mxaddict (mxaddict)
force-pushed
the
feat/gix-stash
branch
from
0a4696b to
551d6d1
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a4696b822
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| source: e, | ||
| })?; | ||
| } | ||
| std::fs::write(&entry_path, blob.data).map_err(|e| super::Error::RestoreUntracked { |
There was a problem hiding this comment.
When restoring parent[2] during pop, this write unconditionally truncates any existing file at the target path. If the user created the same untracked filename after stashing, pop will silently overwrite their current file and then continue toward dropping refs/stash, which can irreversibly lose data instead of surfacing a restore conflict and keeping the stash entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3dd66fd. Pre-flight scan via collect_restore_targets_recursive checks try_exists() on every parent[2] entry before any write. If any target already exists, set had_conflicts=true and bail — refs/stash is preserved, nothing is written. New test pop_returns_conflict_when_untracked_target_exists covers it.
| let content = std::fs::read(&abs_path).map_err(|e| Error::ReadFile { | ||
| path: abs_path.clone(), | ||
| source: e, | ||
| })?; | ||
| let blob_oid = odb | ||
| .write_buf(gix_object::Kind::Blob, &content) | ||
| .map_err(Error::WriteBlob)?; |
There was a problem hiding this comment.
For untracked symlinks, this reads bytes via std::fs::read, which follows the symlink and stores the target file's content, not the symlink target path. Later pop interprets that blob as a link target, so restored symlinks point to garbage paths (or fail), breaking stash round-trips for untracked symlink entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e3b420c. collect_untracked now branches on symlink_metadata().file_type().is_symlink() — for symlinks, reads target via std::fs::read_link, converts to bytes via gix_path::into_bstr, and stores with EntryKind::Link. The tracked-files path in write_wip_tree already did this correctly. New test push_captures_untracked_symlink_target covers it.
| if !has_wt_changes && !has_index_changes && !options.include_untracked { | ||
| return Err(Error::NoLocalChanges); |
There was a problem hiding this comment.
This guard skips NoLocalChanges whenever include_untracked is true, even if the untracked snapshot later turns out empty. On a clean repository, push with include_untracked=true will still create no-op stash/index commits, polluting the stash stack instead of behaving like a no-op with no new stash entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e3b420c. Guard reordered: build WIP, index, and pending-untracked trees first; then compare each against head_tree (or the empty-tree OID for untracked). Returns NoLocalChanges only when all three are vacuous, so -u on a clean repo no longer pollutes the stack. New test push_returns_no_local_changes_with_include_untracked_when_nothing_to_save covers it.
| let mut head_index = | ||
| gix_index::State::from_tree(&head_tree, objects, gix_validate::path::component::Options::default())?; | ||
| let should_interrupt = std::sync::atomic::AtomicBool::new(false); | ||
| gix_worktree_state::checkout( | ||
| &mut head_index, |
There was a problem hiding this comment.
The implementation always checks out head_tree after writing the stash, and Options::keep_index is never consulted. That means staged changes are removed from the working tree even when callers request keep_index, violating the option’s documented contract and breaking staged/unstaged split workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e3b420c. reset_tree = if options.keep_index { index_tree_oid } else { head_tree } — reuses the index tree already written for parent[1], no extra ODB writes. New test push_with_keep_index_preserves_staged_changes_in_wt verifies staged content remains on disk after push.
Before writing any blob from parent[2] during pop, scan all target paths for pre-existing files. If any would be clobbered, mark had_conflicts=true, leave refs/stash intact, and skip all parent[2] writes — preventing silent data loss when the user created a file at the same path after stashing. Co-authored-by: Claude <noreply@anthropic.com>
In collect_untracked, symlinks were read with std::fs::read which follows the link and reads the pointed-at file's content. Use std::fs::read_link instead, convert the OsStr target to bytes via gix_path::into_bstr, and store the result as an EntryKind::Link blob so the stash faithfully records the link target string, not the content of the target file. Also re-evaluate NoLocalChanges after building all three trees: build the untracked tree eagerly when include_untracked=true and include it in the no-changes check so a clean repo with include_untracked=true returns Err(NoLocalChanges) instead of writing an empty stash commit. Honour the keep_index option in the post-stash worktree checkout: when keep_index=true reset the working tree to the index tree (staged changes remain on disk) rather than unconditionally resetting to HEAD. Co-authored-by: Claude <noreply@anthropic.com>
|
Thanks chatgpt-codex-connector — all 4 review comments addressed in 3dd66fd + e3b420c. Added 4 new tests covering each fix; suite now 18/18.
The two original P1+P2 fixes got bundled into one commit because they all live in |
|
To use Codex here, create a Codex account and connect to github. |
…mlink restore by cfg cargo machete flagged gix-dir, gix-trace, smallvec as unused; remove them from the manifest (they were copy-paste from the gix-blame template but never imported). cargo doc rejected the intra-doc links [push] / [pop] / [list] as ambiguous because each name is both a function (re-exported in lib.rs) and a module. Disambiguate with [push()] / [pop()] / [list()] to target the function re-exports. cargo test on windows-latest / windows-11-arm / macos failed at the build step because std::os::unix::fs::symlink doesn't exist on Windows. cfg-gate the symlink call: unix uses std::os::unix::fs::symlink, windows uses std::os::windows::fs::symlink_file, other targets get an Unsupported io::Error. The pre-flight try_exists scan already short-circuits before any writes when a target conflict is detected, so a partial restore on Windows symlink failure only happens when the file system itself rejects the call (e.g. insufficient privileges). Co-authored-by: Claude <noreply@anthropic.com>
|
CI failures from the previous run addressed in b6c5aad:
Local validation before push:
|
|
Heads-up: the 3 remaining All gix-stash jobs are green; the failure looks like a runner-image |
|
Happy to open a separate PR to investigate the |
… lands cargo publish rejects a manifest whose declared gix features (`stash`, `merge`) do not exist on the version published to crates.io. The mxaddict/gitoxide fork adds those features; stock gix 0.83.0 does not. Until GitoxideLabs/gitoxide#2603 lands and we repoint the gix dep back to crates.io, krypt-core and krypt-cli cannot be published. krypt-platform + krypt-pkg continue to publish (no gix dep). The binary still ships via AUR / brew / scoop / GH Releases (built from the fork in the build job).
1 participant
Adds a new
gix-stashplumbing crate implementing the three coregit stashoperations:
push,pop,list. Wires it through the top-levelgixcrateas the gated
stashfeature, mirroring theblame/worktree-streampattern.
I have an end-to-end consumer running this on real dotfile repos
(see "End-to-end validation" below), so this is not speculative API
shopping — the surface here is shaped by an actual integration.
What it adds
list(refs)&gix_ref::file::Store -> Result<Outcome>refs/stashreflog newest-first. Returns emptyOutcome(not an error) when the ref is unborn — matchesgit stash liston a stash-free repo.push(ctx, head_commit, head_tree, head_branch, options)Context<Objects>with refs/odb/index/worktree/committer/checkout-optionsgix_worktree_state::checkout. Optionally captures + removes untracked files intoparent[2].pop(ctx, head_tree, options)Contextshape +head_tree: ObjectIdgix_merge::tree(base = stashparent[0]tree, ours = current HEAD tree, theirs = stash WIP tree). On clean merge: writes via checkout, restoresparent[2]untracked files, drops the ref. On conflict: setshad_conflicts = true, writes conflict markers, leavesrefs/stashintact — matchinggit stash popsemantics.Implementation notes
git stashwrites: a 2- or 3-parentmerge commit at
refs/stash, with the reflog providing the stack(`stash@{0}` is current ref, `stash@{1}` is reflog[-2], etc).
gix-index,gix-object,gix-ref,gix-actor,gix-dir,gix-diff(with
blob),gix-merge,gix-worktree-state, plus the usualgix-hash/gix-date/gix-trace. Same shape asgix-blame.pushcaptures the working-tree content for tracked files (readingeach file off disk + writing a fresh blob), not just the index — this is
important because the headline use case for
git stashis "save myunstaged edits".
Contexttypes are generic over `Objects: Find + FindHeader + Write + Sendso the same handle (e.g.gix::Repository::objects`) satisfies thewhole bound stack.
Test coverage
14 integration tests, all in
gix-stash/tests/stash/, usinggix_testtools::scripted_fixture_*with bash fixture scripts that builddeterministic repos via real
git(then we operate on them with the gixplumbing). Coverage:
list: empty repo, populated stack newest-first, timestamp sanitypush: captures unstaged WT mod, captures staged change in parent[1],NoLocalChangeson clean WT,include_untrackedon/off, WT actuallyresets to HEAD post-push
pop: clean apply round-trip, multi-stash advances ref (doesn't drop),NoStashwhen unborn, parent[2] untracked restoration, conflict pathleaves
refs/stashintactThe tests caught a real bug during dev: an early version of
pushhad aNoLocalChangesguard checkingindex.entries().is_empty()instead ofdiffing trees vs HEAD — which meant any non-empty index would "succeed"
into producing a stash whose tree was identical to HEAD. Tests forced a
fix that compares both WIP and index trees against
head_tree.End-to-end validation
This isn't tested in isolation — it's already wired into a real
dotfiles manager: kryptic-sh/krypt#44.
Krypt's `update` command auto-stashes a dirty working tree before pulling
the dotfiles repo, then auto-pops afterwards.
Smoke test against a real repo:
```
$ git status
M tracked.txt
$ krypt update
INFO auto-stashed working tree before krypt update stash=1f3baab… repo=…
INFO auto-stash restored cleanly after krypt update stash=1f3baab… repo=…
stash: auto-stashed (restored after pull)
pull: up to date
link: wrote: 0
$ git status
M tracked.txt # exactly the same modification
$ git stash list
# empty — stash was popped + dropped cleanly
$ cat tracked.txt
modified-by-smoke # the user's edit survived the roundtrip
```
Krypt also exposes a
--no-stashopt-out that errors immediately on dirtyWT — both code paths exercised.
Known limitations (carried forward as
TODO(gix-stash):in code)These are MVP cuts, not bugs:
checkout_options::filters/attributesare empty. Callerswiring this into porcelain (the
gixRepositorylayer, or downstreamconsumers like krypt) need to populate them so smudge/clean filters and
gitattributes run during the worktree write. The plumbing is correct;
filter setup is the porcelain's responsibility per the pattern
established by
gix-worktree-state.OID rather than recorded as a deletion in the WIP tree. A pop of such a
stash won't restore the deletion.
merged tree.
Not in this PR
Deferred for follow-up PRs (some likely reviewer requests):
apply(pop without drop),drop,show,branchoperationsgix::Repository(`stash_push` / `stash_pop` /`stash_list`) — currently only plumbing is exposed
cargo-smart-releasemetadata polish for the new crateHappy to roll any of these into this PR if preferred — they're scoped out
mostly for review-size reasons.
AI assistance disclosure
Per CONTRIBUTING.md:
as the implementing agent on behalf of mxaddict (@mxaddict).
test suite were AI-generated under human review at each step. Two bugs
surfaced and were fixed during review: (1) initial
pushwas buildingthe WIP tree from the index instead of the working tree, masking
unstaged modifications — caught during code review and fixed; (2) the
NoLocalChangesguard was checking index-empty rather than tree-equal,caught by the integration tests.
Co-authored-by: Claude <noreply@anthropic.com>trailers pergitoxide convention. Authorship stays with mxaddict (@mxaddict); Claude is credited as
co-author on every commit in the series.
Closes (functional half of): kryptic-sh/krypt#44