Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FORGIT_STASH_SHOW_PREVIEW_GIT_OPTS #389

Closed
wants to merge 1 commit into from

Conversation

ccoVeille
Copy link
Contributor

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

FORGIT_STASH_SHOW_PREVIEW_GIT_OPTS variable can be used to tune the rendering of a stash preview.

The following example allows listing the content of the stash before opening it.

FORGIT_STASH_SHOW_PREVIEW_GIT_OPTS="--patch-with-stat --stat-count=10"

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:

Copy link
Collaborator

@sandr01d sandr01d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @ccoVeille! Just some minor changes to do here. The code itself works fine for me and I find the ability of having the diff stat on top of the preview very neat. The example you provided will go straight to my dotfiles once this is merged 😉

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Apr 22, 2024

Thanks for your contribution @ccoVeille! Just some minor changes to do here. The code itself works fine for me and I find the ability of having the diff stat on top of the preview very neat. The example you provided will go straight to my dotfiles once this is merged 😉

Do you think, l I should add it to the documentation the same way FORGIT_CHECKOUT_BRANCH_BRANCH_GIT_OPTS is explained ?

FORGIT_STASH_SHOW_PREVIEW_GIT_OPTS="--patch-with-stat --stat-count=10"

otherwise you can merge as this

@sandr01d
Copy link
Collaborator

Do you think, l I should add it to the documentation the same way FORGIT_CHECKOUT_BRANCH_BRANCH_GIT_OPTS is explained ?

Yes, that sounds useful 👍

This variable can be used to tune the rendering of the stash preview.

The following example allows listing the content of the stash before
opening it.

FORGIT_STASH_SHOW_PREVIEW_GIT_OPTS="--patch-with-stat --stat-count=10"
@ccoVeille
Copy link
Contributor Author

@sandr01d pushed back with the changes in README.md

@ccoVeille
Copy link
Contributor Author

Please relaunch the CI as it looks like a false positive due to apt

@carlfriedrich
Copy link
Collaborator

I would suggest to wait for #391 before merging this (see my comment there).

@carlfriedrich
Copy link
Collaborator

@sandr01d @ccoVeille For your information (because that seems to be similar to what you want to achieve here): you can override preview functions by defining your own and then include it in the according FORGIT_*_FZF_OPTS variable.

I am using this by defining a custom preview function for forgit logs and including that in my FORGIT_STASH_FZF_OPTS variable. It includes the diff stat in a dimmed representation, see a screenshot here:

grafik

This works already without introducing new variables, so might this eventually be an alternative way to go for you?

@ccoVeille
Copy link
Contributor Author

ccoVeille commented Apr 24, 2024

My issue is that I cannot achieve what I want with the _FZF variables.

I don't want to affect the git command that is launched of the left, but the git command that are displayed on the right, the preview.

Right now, there is no way to do it.

Edited: I will have a look

@ccoVeille
Copy link
Contributor Author

@sandr01d @ccoVeille For your information (because that seems to be similar to what you want to achieve here): you can override preview functions by defining your own and then include it in the according FORGIT_*_FZF_OPTS variable.

I am using this by defining a custom preview function for forgit logs and including that in my FORGIT_STASH_FZF_OPTS variable. It includes the diff stat in a dimmed representation, see a screenshot here:

grafik

This works already without introducing new variables, so might this eventually be an alternative way to go for you?

I find your solution too complex. I mean you reimplemented somehow the internals of forgit.

It's OK for an advanced shell user, but for a random dev who want to add a parameter to a command. Your code works, someone could copy paste it, but I don't think they would be able to maintain it.

@carlfriedrich
Copy link
Collaborator

I don't want to affect the git command that is launched of the left, but the git command that are displayed on the right, the preview.

@ccoVeille Yes, I got that. Take a look at the links in my previous comment, those point to my dotfiles where I configure forgit similarly to what (I think) you want to achieve here. It works with the current forgit code base.

The point to understand is: instead of configuring the preview function, I replace it with my own (using fzf's --preview option). This gives you full freedom over any forgit preview, without having to add more variables to the forgit code.

@carlfriedrich
Copy link
Collaborator

@ccoVeille Ah sorry, our comments overlapped. I agree that it's not a simple solution for everyone. However, maybe we could add an examples section to the README for use cases like this? Might be better than introducing more variables for things that already can be configured. Not sure about that myself, yet, though.

Curious to hear opinions from @wfxr @sandr01d @cjappl.

@sandr01d
Copy link
Collaborator

sandr01d commented Apr 25, 2024

Thanks for the insights @carlfriedrich. While the solution you provided is certainly an interesting way to solve this, I think replacing preview functions with injected code goes beyond configuration since it requires familiarity with the inner workings of forgit – at least to a certain extend. I think adding environment variables to handle this would simplify the process of customizing the preview quite a bit (while of course adding complexity to the project). I lean more towards adding such variables if we can manage to reduce boiler plate to a minimum.
I agree that the changes from this PR and those proposed in #391 should go into a single PR.

@ccoVeille
Copy link
Contributor Author

OK for me.

@ccoVeille
Copy link
Contributor Author

For records, I don't think that I'll be able to work on it in the next days.

@ccoVeille
Copy link
Contributor Author

For records, I don't think that I'll be able to work on it in the next days.

For records, I'm still alive, but we got a new kid, so well, I'm pretty busy 😄 for now

@ccoVeille
Copy link
Contributor Author

Replaced by

@ccoVeille ccoVeille closed this Jul 17, 2024
@ccoVeille ccoVeille deleted the forgit-stash-preview branch July 17, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants