glo:: use forgit::diff when entering a commit

[carlfriedrich]

When using glo (forgit::log) we can press Enter to show the changeset of the selected commit. The presentation, however, is rendered using the basic git diff command. We have a much more user friendly diff command gd (forgit::diff) right in forgit, though. Use this instead.

Fixes #217.

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

Type of change

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

Test environment

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

[cjappl]

Looks reasonable enough to me! Thanks for the help, leaving to wfxr to approve officially

[wfxr]

Looks like something went wrong paring the commit hash?

[carlfriedrich]

@wfxr Thanks for your feedback. I did not verify my changes in zsh in the first place, so I'm sorry that it didn't work.

After some checking it turns out that my (rather unrelated) quotation changes from \" to ' are causing the problem. I always prefer using different quotes over escaping, so I changed this on the run, but obviously zsh interprets this differently than bash.

I updated my branch with the quotation changes reverted. Should work in zsh now.

[carlfriedrich]

I just updated my branch again to synchronize the quotation changes to the fish version. Should be ready to go now.

[wfxr]

@carlfriedrich Now the preview window works but when I hit enter it crashes:

With some background processes:

[carlfriedrich]

@wfxr Ah, I see. Obviously it tries to use bash for the function call to forgit::diff. What's your $SHELL environment variable set to?

[wfxr]

@wfxr Ah, I see. Obviously it tries to use bash for the function call to forgit::diff. What's your $SHELL environment variable set to?

@carlfriedrich /opt/homebrew/bin/zsh

[wfxr]

@carlfriedrich Maybe you should capture the printed commit hash from forgit::log and then call forgit::diff with it.

[carlfriedrich]

@wfxr Well, that's exactly what I'm doing. Strange. Can't reproduce this here. Can you try with a clean shell environment?

env -i TERM=xterm-color SHELL=/opt/homebrew/bin/zsh /opt/homebrew/bin/zsh
source forgit.plugin.zsh
glo

Does it happen there as well?

[wfxr]

@wfxr Well, that's exactly what I'm doing. Strange. Can't reproduce this here. Can you try with a clean shell environment?

env -i TERM=xterm-color SHELL=/opt/homebrew/bin/zsh /opt/homebrew/bin/zsh
source forgit.plugin.zsh
glo

Does it happen there as well?

@carlfriedrich Still the same.

Maybe you should capture the printed commit hash from forgit::log and then call forgit::diff with it.

I meant:

forgit::log() {
    # ....
    commit_hash=(eval "git log $graph --color=always --format='$log_format' $* $forgit_emojify" |
        FZF_DEFAULT_OPTS="$opts" fzf | capture_commit_hash)
    # call forgit::diff with $commit_hash
}

BTW remove enter keybind for fzf opts so that the selected line will be printed when hit enter.

[wfxr]

@carlfriedrich Would you mind if I add a commit to this pr?

[carlfriedrich]

@wfxr Give me a few minutes, I am fixing it right now. Your proposal above would not be the same, because the log routine would quit after exiting from the first diff.

[carlfriedrich]

@wfxr Sorry, took a bit longer that I thought. Had to double-check my test environment (turned out that it still wasn't working correctly) and had a business call in between. However, now I am confident that it works in all three shells, at least on my machine. :-) Can you check again?

[wfxr]

@carlfriedrich Still crashes:

I'm using zsh-defer to load most of the plugins including forgit in my zshrc. Maybe the plugin is not ready when calling forgit::diff?

[carlfriedrich]

@wfxr Ah, I wasn't aware of zsh-defer until now. Yes, in this case forgit is obviously not loaded yet when forgit::diff is called.

Different to bash, unfortunately zsh does not support function exporting, so there's no way to pass the functions of the current environment to a new subshell. Starting an interactive shell explicitly, which loads .zshrc and along with that forgit again, seems to be the only way to have forgit functions in a subshell.

Is there a way to either

• bypass zsh-defer loading or
• wait until defered loading is completed?

[carlfriedrich]

@wfxr Even though I do not find it ideal, I have added a wait loop if the forgit functions are not found immediately. Can you check this?

Better and cleaner way to do this would be transferring all the forgit functions to an actual executable script, so that they do not have to be loaded to the environment in the first place, but are available via the PATH. That would require some refactoring, though, and I assume that wouldn't be what zsh calls a "plugin" anymore, since we would have more than one file, correct?

[carlfriedrich]

@wfxr I found a better solution: instead of a waiting loop, do not rely on forgit being loaded via .bashrc / .zshrc at all, but reload it explicitly instead. This seems far more reasonable and should work in both bash and zsh. Can you check again?

[carlfriedrich]

@wfxr Sorry for spam. :-) I just re-added the -i flag for spawning an interactive shell, because otherwise user defined forgit variable settings would be lost for the subshell. If these get loaded deferred as well, I wouldn't know how to work around this, though. Can you check if this works now for you?

[wfxr]

Hi @carlfriedrich Thanks for your effort for this PR. Now I can jump into the forgit::diff.

https://user-images.githubusercontent.com/6105425/179879898-04420b87-7613-4033-afc4-03c92f1559f1.mp4

However there still some issues.

1. When hit enter the following message appears at the top of the window:

2. I found I cannot see the detailed changes through the preview window anymore, which makes it difficult to quickly browse changes.

3. As the video shows the screen will be messed up every first time I hit enter in the sub shell.

IMO entering a interactive shell in a fzf keybinding is too tricky which will cause some unexpected behaviors we don't known even if the above problems were solved. The other thing I concern is there are a log of guys who use oh-my-zsh or enabled many plugins take quite a few seconds to load the shell. This change will result in forgit::log with little usability.

So I tend to keep it simple and stupid.

[carlfriedrich]

@wfxr Thanks a lot for your detailed feedback. Really appreciate your responsiveness and will to check my changes.

Hi @carlfriedrich Thanks for your effort for this PR. Now I can jump into the forgit::diff.

Yeah, finally! :-)

However there still some issues.

1. When hit enter the following message appears at the top of the window:

Okay, I have no idea where that comes from. :-/

2. I found I cannot see the detailed changes through the preview window anymore, which makes it difficult to quickly browse changes.

That was a deliberate change. I like the preview short and overview-like, then being able to enter the change to see the detailed diff. I understand, however, that this is personal preference and doesn't meet everyone's taste.

3. As the video shows the screen will be messed up every first time I hit enter in the sub shell.

I have no idea about that either, and cannot reproduce it unfortunately.

IMO entering a interactive shell in a fzf keybinding is too tricky which will cause some unexpected behaviors we don't known even if the above problems were solved. The other thing I concern is there are a log of guys who use oh-my-zsh or enabled many plugins take quite a few seconds to load the shell. This change will result in forgit::log with little usability.

I see your point here. Obviously your zsh setup is already so much different from my test environment that it causes issues with this change, so there will probably be more issues with other setups.

I just wanted to bring something upstream that I am using in bash for quite a few months now, so that other people can benefit from it. However, I didn't expect it to be incompatible with zsh. Too bad. :-/

One last option would be to make this feature bash-only, but I assume that you would like to keep the features for all shells on par?

And one last general question for the bigger picture: I am not familiar with the zsh / oh-my-zsh plugin system. Is there a way to have a plugin that adds its folder to the PATH, so that we could have forgit in form of an executable script instead of env functions? Would that be an option? Because IMO we could use forgit feature chaining in more scenarios than just with glo -> gd (e.g. cherry picking by first interactively selecting the source branch instead of having to pass the source branch as an argument (that is so un-forgit-y :-))), which would make forgit much more user-friendly. And with forgit being an executable script we wouldn't have the issues concerning the functions being unknown to subshells.

[carlfriedrich]

So here's another shot for integrating forgit::diff into forgit::log, similar to the cherry-pick implementation of #216 and just like proposed by @wfxr above. This implementation does not mess with subshells in the enter command anymore and makes use of the actual fzf output.

I do not find it ideal, though. After quitting the diff, a new fzf instance is started, so the previous selection is gone. This makes it hard to browse through subsequent commit diffs. Unfortunately fzf does not provide a way to pre-select a line.

@wfxr @cjappl What do you think? On one hand I think this is an improvement concerning usability, because forgit's diff function provides a better (and more consistent) user experience than a plain git diff. On the other hand the resetting log window after each diff is a step back imo.

[wfxr]

@carlfriedrich I tested this implementation. As you said, both improvements and flaws are significant. How about making it an optional feature to satisfy users who like this feature more without affecting others?

[carlfriedrich]

With the changes of #241 we have finally a proper way to do this. Since the new commit was actually part of #241 during testing and approved along with those changes, I am directly merging this as well.

[nathanchance]

For what it's worth, this messes with the ability to view larger commit messages in forgit::log, which means I cannot use this much for my Linux kernel work :/ is there a way for this feature to coexist with that ability?

[carlfriedrich]

@nathanchance I'm sorry that this disrupts your workflow.

You can bring back the old behaviour by defining a custom enter command and adding it to your FORGIT_LOG_FZF_OPTS variable before sourcing forgit:

export FORGIT_LOG_ENTER_CMD="echo {} | grep -Eo '[a-f0-9]+' | head -1 | tr -d '[:space:]' | xargs -I% git show --color=always % | LESS='-Rc' less"
export FORGIT_LOG_FZF_OPTS="--bind='enter:execute($FORGIT_LOG_ENTER_CMD)'"

But may I ask why the new behavior actually prevents you from viewing the commit message? Don't you have it displayed in your preview window? FYI: You can scroll within forgit's preview window by defining fzf keybindings in FORGIT_FZF_DEFAULT_OPTS. forgit already has defined standard values for this (Down: Alt+J / Alt+N, Up: Alt+K / Alt+P).

Would one of these two options work for you?

[nathanchance]

@nathanchance I'm sorry that this disrupts your workflow.

No worries!

You can scroll within forgit's preview window by defining fzf keybindings in FORGIT_FZF_DEFAULT_OPTS. forgit already has defined standard values for this (Down: Alt+J / Alt+N, Up: Alt+K / Alt+P).

Aha, I did not know about being able to scroll the preview window! That will work just fine for this, thanks a lot for pointing it out!

[carlfriedrich]

@nathanchance Great, thanks for your feedback!