feat: Use fzf-tmux if fzf options are set

[dljsjr]

Check list

• [x] I have performed a self-review of my code
• [x] I have commented my code in hard-to-understand areas
• [x] I have made corresponding changes to the documentation

Description

Overview

The meat of this change is to inspect whether or not we are in a tmux pane, and if so, we check the FZF_TMUX and FZF_TMUX_OPTS variables. If those are set, we use fzf-tmux instead of fzf and we forward the appropriate options.

This resolves #303

Changes

• Add a do_fzf function for abstracting fzf invocation based on shell state as outlined above

• Only hardcode -0/--exit-0 for fzf, as it causes flicker w/ tmux popup

• Replace all invocations of fzf with do_fzf

Type of change

• [ ] Bug fix
• [x] New feature
• [ ] Refactor
• [ ] Breaking change
• [ ] Documentation change

Test environment

• Shell
  • [x] bash
  • [x] zsh
  • [ ] fish
• OS
  • [x] Linux
  • [x] Mac OS X
  • [ ] Windows
  • [ ] Others:

[dljsjr]

I can address the lint failures when I’m back on a keyboard.

[dljsjr]

Sent up a fixup commit to address the shellcheck lints, I'll squash it down before merge once approved :)

[cjappl]

Don't stress about squashing, we use "squash and merge"

I'll hopefully get to test out and review soon! I've never heard about tmux-fzf but it sounds cool.

Thanks for the contribution, we will be back to you with comments/approval etc when we can 😀

[dljsjr]

I've never heard about tmux-fzf but it sounds cool.

For a heavy tmux user that uses a lot of panes/splits, the pop-up mode is invaluable:

[dljsjr]

@carlfriedrich I made some adjustments:

1. -0 defaults to enabled across the board and the behavior is now configurable via a FORGIT-prefixed variable, mentioned in the README. Example:
2. Made the splitting and sharing of the regular options and the tmux options much more explicit
3. Adjusted the README as requested

[dljsjr]

@carlfriedrich changes made and branch is rebased on top of latest upstream master. Also reworded the first commit to reflect the refactors made during review.

[carlfriedrich]

@dljsjr Great, looks good code-wise for me now. Thanks a lot for your contribution!

I Did not do any functional tests, though, since I do not use tmux. @cjappl Do you want to do any more testing or review? Otherwise I will merge this shortly.

[cjappl]

Sure. Let me test with and without this plugin and make sure it’s all good! I’ll do that today, and merge if everything looks good.

On Wed, May 10, 2023 at 11:21 PM, Tim @.***(mailto:On Wed, May 10, 2023 at 11:21 PM, Tim < wrote:

@.***(https://github.com/dljsjr) Great, looks good code-wise for me now. Thanks a lot for your contribution!

I Did not do any functional tests, though, since I do not use tmux. @.***(https://github.com/cjappl) Do you want to do any more testing or review? Otherwise I will merge this shortly.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[cjappl]

So far most commands are working well! However ga comes up with this when executing with the new fzf tmux, in fish

Steps to repro:

1. Use fish
2. Make a change to some file
3. export FZF_TMUX=1
4. ga

This does not happen without step 3 (turning on this new FZF_TMUX). I won't be able to fully dig in today to triage. Let me know if you have any "aha's" and I'm happy to test again @dljsjr

[dljsjr]

@cjappl Hmm, interesting. I didn't modify any of that code, and I don't use fish.

~~I could probably get it set up and do a bisect. I did rebase this change on top of the latest origin/master after my last round of revisions so it also could have pulled in a regression from upstream.~~

[dljsjr]

@cjappl do you have anything in your .tmux.conf that forces a specific shell type, such as the macos reattach to user namespace stuff for pasteboard cooperation?

I think what's going on here is that the spawned tmux pane is executing things in fish instead of bash but I'm not 100% sure why yet. It's definitely not a regression, just something that's unique to the tmux configuration, and I'm not 100% sure why just right now.

Update

This is an oddity with tmux. After digging in, it kinda makes sense:

Any new panes/windows/splits/popups started inside of a tmux session will use the same $SHELL environment that the server was started with. So even though we export SHELL in such a way to "force" things to be bash, because the tmux server was forked from a fish shell, all new attachments to that server will use the same shell. Even though git-forgit is running in a bash context, the commands that get forwarded to --preview will be running in the context of the shell where fzf is running. That's the source of the error above.

Proposed Solution

Use bash -c in the preview commands.

[dljsjr]

@cjappl I've made the proposed change only for ga on this branch for right now; let me know if that works.

[cjappl]

Unfortunately no dice, similar issue

I think the best option I'd recommend is you install and test it on fish. Otherwise, I can do the work on fish at some point in the future but it may take a bit for me to get to it!

[dljsjr]

@cjappl Guess this is my excuse to give fish a try for the first time in like, 8 years :)

[cjappl]

Haha! It’s a blessing and a curse :)

Appreciate you being willing to chase it down. Shout if you need a test subject, happy to run anything you come up with.

On Mon, May 15, 2023 at 9:16 AM, Douglas Stephen @.***(mailto:On Mon, May 15, 2023 at 9:16 AM, Douglas Stephen < wrote:

@.***(https://github.com/cjappl) Guess this is my excuse to give fish a try for the first time in like, 8 years :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[carlfriedrich]

@dljsjr Shall we reopen this? What are the chances that you'll get back to this?