Feature: add forgit show command

[carlfriedrich]

Use the new command in git log enter. This fixes the display of diffs for merge commits.

Fixes #416.

Check list

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

Description

Type of change

• Bug fix
• New feature
• Refactor
• Breaking change
• Documentation change

Test environment

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

[carlfriedrich]

Not sure about the fish completions, we don't have them for forgit diff either. @cjappl or @sandr01d do you know more about this?

[cjappl]

Not sure about the fish completions, we don't have them for forgit diff either. @cjappl or @sandr01d do you know more about this?

Hmm, good find. Would you file a ticket for this and assign it to me and I can fix it when I get time? I can confirm git diff does not behave with completions on fish.

[wfxr]

LGTM.

[sandr01d]

Looking forward to see this feature merged, I definitely need this 😃
In addition to things I've pointed out in the code, there are two things I've noticed:

1. The show command has not been added to conf.d/forgit.plugin.fish and as a result does not work in fish. Might the issue you were having with fish completions be related to this?
2. Completions for zsh are missing. If I remember correctly, you're not a zsh user @carlfriedrich, so if you don't want to go through the hassle of setting up zsh on your system I'd offer to implement a zsh completion and add it to this PR. Just let me know.

[carlfriedrich]

Thanks for your reviews everyone, especially for the good finds @sandr01d.

1. The show command has not been added to conf.d/forgit.plugin.fish and as a result does not work in fish. Might the issue you were having with fish completions be related to this?

I am sorry, I simply forgot this. Updated it in the new commit version. However, I did not test the fish completions, I just did not add anything in the fish completions file. I was looking for something to copy from forgit diff, but then noticed that that's missing as well.

2. Completions for zsh are missing. If I remember correctly, you're not a zsh user @carlfriedrich, so if you don't want to go through the hassle of setting up zsh on your system I'd offer to implement a zsh completion and add it to this PR. Just let me know.

Good point, I added them in the new commit version, but haven't tested them. Can you give them a quick test?

Hmm, good find. Would you file a ticket for this and assign it to me and I can fix it when I get time? I can confirm git diff does not behave with completions on fish.

@cjappl Yep, thanks a lot, I will do so.

[sandr01d]

Thanks for the updates @carlfriedrich. Fish is working now (except completions, but we can handle this in #418, so no objections from my side). Zsh completions are working fine too.
There is one additional thing I've noticed while testing: the current implementation does not work well with tags. The reason for this is that git provides some additional information when showing a tag. E.g. git show --pretty="" --name-status 24.10.0 produces the following output:

tag 24.10.0
Tagger: github-actions <[email protected]>

Release 24.10.0
M	README.md
M	bin/git-forgit
M	completions/_git-forgit
M	completions/git-forgit.bash
M	completions/git-forgit.fish
M	conf.d/forgit.plugin.fish
M	forgit.plugin.zsh

While this probably is an edge case, we should be able to filter out the additional information quite easily.

[carlfriedrich]

Thanks for the updates @carlfriedrich. Fish is working now (except completions, but we can handle this in #418, so no objections from my side). Zsh completions are working fine too. There is one additional thing I've noticed while testing: the current implementation does not work well with tags. The reason for this is that git provides some additional information when showing a tag. E.g. git show --pretty="" --name-status 24.10.0 produces the following output:

@sandr01d Good catch, thank you! I updated the call producing the file list to use the commit object instead of the tag. I kept the original call for the preview and enter commands, though. IMO if you call git show on a tag you actually might want to see the tag information, so now it is displayed above the diff for each file. Are you fine with that?

[carlfriedrich]

Thinking more about this I am asking myself whether people might be interested in the commit message as well when calling forgit show. Should we display it somehow? Maybe implement a preview toggle so that we can switch between the commit message and the file diff (using transform action)?

[sandr01d]

I kept the original call for the preview and enter commands, though. IMO if you call git show on a tag you actually might want to see the tag information, so now it is displayed above the diff for each file. Are you fine with that?

Yes, absolutely. Having the tag information displayed is a big plus IMO.

Thinking more about this I am asking myself whether people might be interested in the commit message as well when calling forgit show. Should we display it somehow? Maybe implement a preview toggle so that we can switch between the commit message and the file diff (using transform action)?

I think this is a great idea! Wasn't aware of the transform action — what a neat feature. We'd have to check for the installed fzf version though. I suspect a lot of users running versions older than 0.45.0.

[carlfriedrich]

@sandr01d So I gave it a shot and added a keybinding to toggle between the commit message and the commit diff. Works pretty well IMO and I already see myself using it a lot.

I also added a border to the preview window indicating what is currently shown. And furthermore I added a border displaying the keybindings.

I think maybe the latter might trigger a more general discussion (as I already suggested someting similar in #259) and maybe we might split that part out to a new MR.

I did not yet check the fzf version, yet (we need at least 0.49.0 now). I wanted to discuss how we continue on this, first. Maybe we should merge the show feature as it was before the new commit (because it already worked before) and add everything else in one or more follow-ups.

Let me know what you think and how you like the proposal.

[sandr01d]

I find this super useful, well done @carlfriedrich! See my comments below regarding implementation details.
I think moving the second commit to a new PR would probably be best. E.g., I'm not a big fan of the bottom border and would prefer to have the keybindings displayed without it - but don't know if fzf supports this. With a new PR, we could also implement showing the existing keybindings of other functions (editing files with diff and add).

[carlfriedrich]

@sandr01d Thanks a lot for your new review. Really helpful! I updated the second commit according to your comments and removed the keybinding help text. Will file a new PR for that eventually.

I also changed the keybinding from ctrl-t to alt-t, because we also use alt-e for edit. Ok for you?

[sandr01d]

I also changed the keybinding from ctrl-t to alt-t, because we also use alt-e for edit. Ok for you?

Yeah, makes sense to keep the keybinding consistent with what we already have.

Once the opened thread above is resolved the only thing missing would be checking for the fzf version, so we don't break the function for users of older fzf versions.

[carlfriedrich]

@sandr01d Thanks again. Please check if the quotes are fine for your and resolve the thread in case.

Concerning the fzf version check: do you have an opinion what we do when the version is below 0.49.0? Should we exit forgit with an error immediately or should we just disable the features we cannot use with older versions?
I would prefer the former, as partially disabling features would bloat the code, and basically everybody should be able to install a recent fzf version. There might be lazy people relying on old versions in their Ubuntu's apt repo (Ubuntu 24.04 packages fzf 0.46.1), but is that a reason not to move on with forgit? I mean, these people still can install old versions of forgit working with their fzf version as well.
WDYT?

Also pinging @cjappl and @wfxr for an opinion.

[sandr01d]

Concerning the fzf version check: do you have an opinion what we do when the version is below 0.49.0? Should we exit forgit with an error immediately or should we just disable the features we cannot use with older versions?
I would prefer the former, as partially disabling features would bloat the code, and basically everybody should be able to install a recent fzf version. There might be lazy people relying on old versions in their Ubuntu's apt repo (Ubuntu 24.04 packages fzf 0.46.1), but is that a reason not to move on with forgit? I mean, these people still can install old versions of forgit working with their fzf version as well.
WDYT?

This is a tough one. I agree that introducing version checks for specific features of fzf would make maintenance difficult if we decide to implement similar features like the one in question for other functions (which is something I'd like to see). My concern is that there might be some users in restricted environments that are not able to install a different version of fzf than their distributions package manager provides. We had a similar case with #328. I wonder how many users would be negatively affected by such a change. Maybe it would be worth to open a discussion and start a poll on which distributions/versions people use? Not sure whether we'd see many responses, but might be worth a try.
I am currently leaning more towards requiring a newer version of fzf for forgit in general, because I think we should not hold back development. Would be interested to know what the other maintainers think.

[cjappl]

Sorry for the delay! Thanks for pinging :)

Concerning the fzf version check: do you have an opinion what we do when the version is below 0.49.0? Should we exit forgit with an error immediately or should we just disable the features we cannot use with older versions?
I would prefer the former, as partially disabling features would bloat the code, and basically everybody should be able to install a recent fzf version. There might be lazy people relying on old versions in their Ubuntu's apt repo (Ubuntu 24.04 packages fzf 0.46.1), but is that a reason not to move on with forgit? I mean, these people still can install old versions of forgit working with their fzf version as well.
WDYT?

Generally in agreement with everything said here. I think we should do a (possibly major) version bump of forgit, and take in the new fzf.

Should we exit forgit with an error immediately

This is my preference. I am against bloating the code with fzf version check

I mean, these people still can install old versions of forgit working with their fzf version as well.

This is the key I think. This is why we version our code, so people can fall back to an older one. Do we have to do a major version number bump as this is a backwards incompatible change? (That would be my first thought)

I think one of the possible good things here is that forgit is not really used as a CI/CD tool, but on devs machines directly where they have more control. I think this probably means we can be more aggressive moving forward than something crucial to many people's build pipelines.

If we get many many reports we can always roll back and remove the feature, and I will eat my words :)

[carlfriedrich]

@sandr01d @cjappl Thanks for your opinions!

My concern is that there might be some users in restricted environments that are not able to install a different version of fzf than their distributions package manager provides.

I don't see that, actually. Linux is flexible and fzf is distributed as a single binary. Everybody should be able to put it in their home directory under something like ~/.local/bin and add that to their PATH (that's how I have it installed, it's actually part of my dotfiles).
The most user-friendly way is possibly to use fzf's own install script, which is documented in their README and also installs it into the user's home dir. We could point to those instructions in our README as well.

We had a similar case with #328. I wonder how many users would be negatively affected by such a change.

This was actually about git, which is a more common tool to have installed via the package manager than fzf I would argue.

Do we have to do a major version number bump as this is a backwards incompatible change? (That would be my first thought)

Since we don't use semantic versioning, there is no such thing as major versions in forgit. I think we should highlight this in the release notes and add it to the README, and we should make sure that the new version dependency is correctly modeled in the AUR and brew recipes.

If that's fine with you then, I would implement the hard version check.

[sandr01d]

Hey @carlfriedrich, sorry for the delay.

If that's fine with you then, I would implement the hard version check.

Yes, please go ahead.

[carlfriedrich]

@sandr01d @cjappl I have implemented the version check and added it to the README as well as a separate commit, which will appear in the release notes. Can you check if that works for you? Any suggestions for improvements?

[cjappl]

LGTM - good work

[sandr01d]

Just one minor thing

[sandr01d]

I prepared restricting fzf to version >= 0.49.0 for the AUR packages. @cjappl can you handle brew?

[cjappl]

I prepared restricting fzf to version >= 0.49.0 for the AUR packages. @cjappl can you handle brew?

Yes, but I'm traveling until 1/29. I can handle it then, or someone else can dive in if they want to before then! I'll set a reminder for myself, just ping me if anyone gets there first.

[sandr01d]

All set from my side, thanks for all the work @carlfriedrich!

[cjappl]

I prepared restricting fzf to version >= 0.49.0 for the AUR packages. @cjappl can you handle brew?

This is prepared for a PR and ready to go. When this review is submitted I will put it up for review in the homebrew repo. They are usually very prompt in approving and merging

[carlfriedrich]

@sandr01d @cjappl Thanks for preparing AUR and brew. This will land in forgit 25.02.0, which will be automatically released on 2025-02-01.

[cjappl]

Put up for review here for brew:

Homebrew/homebrew-core#205866