Closed dljsjr closed 3 months ago
I can address the lint failures when I’m back on a keyboard.
Sent up a fixup commit to address the shellcheck lints, I'll squash it down before merge once approved :)
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 😀
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:
@carlfriedrich I made some adjustments:
-0
defaults to enabled across the board and the behavior is now configurable via a FORGIT-prefixed variable, mentioned in the README. Example:
@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.
@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.
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: @.***>
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:
fish
export FZF_TMUX=1
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
@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.
@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.
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.
Use bash -c
in the preview
commands.
@cjappl I've made the proposed change only for ga
on this branch for right now; let me know if that works.
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!
@cjappl Guess this is my excuse to give fish
a try for the first time in like, 8 years :)
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: @.***>
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.
@dljsjr Shall we reopen this? What are the chances that you'll get back to this?
Check list
Description
Overview
The meat of this change is to inspect whether or not we are in a
tmux
pane, and if so, we check theFZF_TMUX
andFZF_TMUX_OPTS
variables. If those are set, we usefzf-tmux
instead offzf
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 aboveOnly hardcode
-0
/--exit-0
forfzf
, as it causes flicker w/ tmux popupReplace all invocations of
fzf
withdo_fzf
Type of change
Test environment