Skip to content

internal/cmd: git_wrapper: add git stash push to wrapped subcommands#160

Open
christoph-heiss wants to merge 2 commits into
mroth:mainfrom
christoph-heiss:cmd/wrap-git-stash
Open

internal/cmd: git_wrapper: add git stash push to wrapped subcommands#160
christoph-heiss wants to merge 2 commits into
mroth:mainfrom
christoph-heiss:cmd/wrap-git-stash

Conversation

@christoph-heiss

Copy link
Copy Markdown
Contributor

Pretty much as it says on the tin.

git stash push (optionally) also takes path(s) for its arguments, so translate them.

`git stash push` (optionally) also takes path(s) for its arguments, so
translate them.

Signed-off-by: Christoph Heiss <christoph@c8h4.io>
@christoph-heiss

Copy link
Copy Markdown
Contributor Author

@mroth Mind taking a look?

@mroth

mroth commented Jun 16, 2026

Copy link
Copy Markdown
Owner

@christoph-heiss sorry for the delay, let me take a look.

Cover both code paths the stash wrapper introduces, across bash/zsh/fish:
- `git stash push <n>` expands the numeric arg to the right path
- bare `git stash` / `git stash pop` pass through unchanged (no expansion)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mroth

mroth commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Nice, well-scoped change — translating only stash push is exactly right, since for pop/apply/drop/show a number is a stash index (git stash pop 1), not a file number, so expanding it would be actively wrong.

One non-blocking suggestion: that reasoning is the whole point of the push-only branch, but it's currently implicit. A one-line comment would help a future reader (and stop someone from "helpfully" widening the case):

In git_wrapper.sh:

    stash)
      # Only `stash push` takes pathspecs; for pop/apply/drop/show a number is a
      # stash index, not a file number, so pass those through untranslated.
      case ${2-:} in
        push)
          scmpuff exec --relative -- "$SCMPUFF_GIT_CMD" "$@";;
        *)
          "$SCMPUFF_GIT_CMD" "$@";;
      esac
      ;;

…and the equivalent above the switch $argv[2] in git_wrapper.fish.

Otherwise this looks good. I pushed a couple of integration tests to the branch to lock in the behavior across bash/zsh/fish — one for git stash push <n> expanding to the right path, and one verifying bare git stash / git stash pop pass through unchanged (which also confirms the empty-$argv[2] path is fine in fish). Take a look and let me know what you think.

(Review drafted by by Claude Opus but manually reviewed by @mroth )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants