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

Rebuild terlar prompt as a configuration of fish_git_prompt #7918

Closed
wants to merge 4 commits into from

Conversation

faho
Copy link
Member

@faho faho commented Apr 10, 2021

Description

The terlar sample prompt uses its own git prompt implementation called __terlar_git_prompt, which was always a bit cheesy and meant that it was harder to adjust than other prompts.

This rebuilds that function as a wrapper around fish_git_prompt that just sets some variables.

To do so it adds two features to fish_git_prompt to achieve parity:

  • The clean state char will now be used even in non-informative mode, it just won't be set by default then (this uses the actual informative mode setting, not even the informative chars setting)
  • The branch can now be colored differently when it is dirty or something is staged

Note that this doesn't remove the __terlar_git_prompt function because that would break every configuration that currently uses the terlar prompt, which is quite awkward. If we wanted to remove it we would have to figure out how to tell users to switch. (which isn't too much to ask, all they'd have to do is copy the sample again!)

Part of #7884.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho faho added this to the fish 3.3.0 milestone Apr 10, 2021
@faho faho marked this pull request as draft April 13, 2021 15:43
@faho
Copy link
Member Author

faho commented Apr 13, 2021

(needs some more testing)

@@ -46,6 +46,8 @@ echo
#CHECK: (newbranch|…1)
set -e __fish_git_prompt_show_informative_status

set -e __fish_git_prompt_show_informative_status
Copy link
Member

Choose a reason for hiding this comment

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

Huh deleting it twice won't trigger another handler, right? Unless there is a spooky universal variables around...

(Re the commit message: the variable handler behavior for variable overrides seems correct as far as I can tell)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh this is a rebase artifact (I did this for #7926 as well).

(Re the commit message: the variable handler behavior for variable overrides seems correct as far as I can tell)

The problem is that the handler only triggers when the override goes into scope, not also when it goes out of scope. So since we disable informative mode stuff when the variable is erased, we don't notice when it's used as an override.

See:

> set -l foo bar
> function on-foo --on-variable foo; echo on foo: $argv; echo foo is $foo; end
> foo=banana echo bar
on foo: VARIABLE SET foo
foo is banana
bar

See how the handler is never triggered for foo being "bar" again? That's the problem here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure how this should act, just saying that it gets in the way here.

Copy link
Member

@krobelus krobelus Apr 13, 2021

Choose a reason for hiding this comment

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

That sounds correct, because the override acts just like begin; set -l foo banana; ...; end

Copy link
Member Author

Choose a reason for hiding this comment

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

That's consistent, I'm not sure it's correct.

Maybe we should fire the variable handlers when it goes out of scope? Or would that massively slow down block closing?

Copy link
Member

@krobelus krobelus Apr 18, 2021

Choose a reason for hiding this comment

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

I guess this problem has just never come up before because variable handlers are primarily used on global/universal variables.
Firing the handlers seems useful here - for running the git prompt with different configurations via variable overrides.
For this use case it's enough to fire handlers when a variable, that shadows another variables, goes out of scope; not when a local-only variable dies.

(Ideally we don't need the caching logic of ___fish_git_prompt_init, but it's probably a worthwhile optimization).

@krobelus
Copy link
Member

Sounds great! I think it's okay if there are minor differences.

This is a neat feature of the terlar git prompt
And then only define it in informative mode.
This removes the awkward secondary logic.

Note that we still ship a function called `__terlar_git_prompt`
because people who picked the prompt will still be calling it - we
don't update the prompt.
The terlar prompt uses `|branch`, this is visually quite important.
@faho
Copy link
Member Author

faho commented Jun 6, 2021

I'm not currently working on this - there's some annoying subtleties that I don't have the right frame of mind to work on atm, so I'm moving it out of the 3.3 milestone.

@faho faho modified the milestones: fish 3.3.0, fish-future Jun 6, 2021
@faho
Copy link
Member Author

faho commented Aug 4, 2021

Yeah, closing for now. If I ever get back to this I can just reopen it, no need to keep the draft around.

@faho faho closed this Aug 4, 2021
@zanchey zanchey removed this from the fish-future milestone Aug 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants