wfxr / forgit

:zzz: A utility tool powered by fzf for using git interactively.
MIT License
4.32k stars 136 forks source link

scmpuff/scmbreeze file number shortcuts no longer work #280

Closed tekumara closed 1 year ago

tekumara commented 1 year ago

Check list

Environment info

Problem / Steps to reproduce

241 (commit 4995c614e484a5a4636ffcea6e949dd7bd80bdac) broke integration with scmpuff/scmbreeze.

Prior to #241 it was possible to use scmpuff file number shortcuts, eg:

❯ gs
# On branch: master  |  [*] => $e*
#
➤ Changes not staged for commit
#
#       modified:  [1] README.md
#
❯ forgit::add 1
# On branch: master  |  [*] => $e*
#
➤ Changes to be committed
#
#       modified:  [1] README.md
#
M  README.md

This now errors:

❯ gs
# On branch: master  |  [*] => $e*
#
➤ Changes to be committed
#
#       modified:  [1] README.md
#
❯ forgit::add 1
fatal: pathspec '1' did not match any files
Nothing to add.
cjappl commented 1 year ago

Cool tools! I didn't know they existed.

We are open to PRs if you want to take a crack at fixing this :)

carlfriedrich commented 1 year ago

Hi @tekumara, thanks for the note and sorry for the late reply. Are you sure that it is 4995c614e484a5a4636ffcea6e949dd7bd80bdac which breaks the scmpuff support? This commit just moves code to a different place without changing any implementation, so it surprises me that it induces such a change in behavior. Since I don't use any of these tools, can you maybe double check if this is really the breaking commit?

tekumara commented 1 year ago

yeh when I did a git bisect I landed on https://github.com/wfxr/forgit/commit/4995c614e484a5a4636ffcea6e949dd7bd80bdac as the first bad commit.

If I check out 4995c61~1 (ie: the commit before 4995c614) it works but 4995c61 doesn't.

carlfriedrich commented 1 year ago

@tekumara Thanks for verifying. Do I get it right that scmpuff works by sourcing something into the current shell? I.e. it won't work when you invoke a new shell, unless you source it there as well? That would explain the change, because since https://github.com/wfxr/forgit/commit/4995c614e484a5a4636ffcea6e949dd7bd80bdac the forgit code runs in an invoked bash script instead of in the current shell.

tekumara commented 1 year ago

ah yes, I do a eval "$(scmpuff init -s)" in my shell scripts.

carlfriedrich commented 1 year ago

@tekumara Thanks for confirming.

As far as I can see, scmpuff overrides the git command by defining a git() function in the current shell. This is what I see from the output of scmpuff init -s:

SCMPUFF_GIT_CMD=${SCMPUFF_GIT_CMD:-"$(\which git)"}
export SCMPUFF_GIT_CMD

function git() {
  case $1 in
    commit|blame|log|rebase|merge)
      scmpuff exec -- "$SCMPUFF_GIT_CMD" "$@";;
    checkout|diff|rm|reset|restore)
      scmpuff exec --relative -- "$SCMPUFF_GIT_CMD" "$@";;
    add)
      scmpuff exec -- "$SCMPUFF_GIT_CMD" "$@"
      scmpuff_status;;
    *)
      "$SCMPUFF_GIT_CMD" "$@";;
  esac
}

With the new way forgit is implemented I don't see any straight-forward solution to support integration with scmpuff again, unfortunately, as forgit calls git directly without spawning an interactive shell.

How important is it for you to have the two tools work together?

One thing I can think of without having to patch forgit would be to encapsulate the above code in a shell script, name it git and put it in the PATH somewhere before the original git command, so that scmpuff is also used in non-interactive shells. Would that possibly work for you?

tekumara commented 1 year ago

Thanks for the explanation, that makes sense!

Sounds like there is no easy fix, and it's not too big a deal for me. If it becomes an issue I'll try the PATH suggestion.

carlfriedrich commented 1 year ago

Alright, then I'm gonna close this. Feel free to open a new issue if something comes up again.