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

fix Subversion prompt #7278

Merged
merged 4 commits into from Aug 26, 2020
Merged

fix Subversion prompt #7278

merged 4 commits into from Aug 26, 2020

Conversation

chref
Copy link
Contributor

@chref chref commented Aug 24, 2020

  • after switching to "string match", some SVN status symbols need
    proper escaping
  • the __fish_svn_prompt_flag_names list was missing
    "versioned_obstructed" and was therefore not in line with
    the symbols from __fish_svn_prompt_chars
  • when checking for individual SVN status symbols, use
    "string match -e" to handle the case where multiple different
    symbols appear in one status column
  • use "sort -u" before merging all symbols from a column into
    one line

Fixes #6715

- after switching to "string match", some SVN status symbols need
  proper escaping
- the __fish_svn_prompt_flag_names list was missing
  "versioned_obstructed" and was therefore not in line with
  the symbols from __fish_svn_prompt_chars
- when checking for individual SVN status symbols, use
  "string match -e" to handle the case where multiple different
  symbols appear in one status column
- use "sort -u" before merging all symbols from a column into
  one line

Fixes #6715
@@ -70,15 +70,15 @@ set -g __fish_svn_prompt_char_token_broken_color --bold magenta
function __fish_svn_prompt_parse_status --argument flag_status_string --description "helper function that does pretty formatting on svn status"
# SVN status symbols
# Do not change these! These are the expected characters that are output from `svn status`, they are taken from `svn help status`
set -l __fish_svn_prompt_chars A C D I M R X \? ! ~ L + S K O T B
set -l __fish_svn_prompt_chars A C D I M R X \\\? ! \~ L + S K O T B
Copy link
Member

Choose a reason for hiding this comment

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

The ? should be quoted, not escaped - currently the ? can be disabled via the "qmark-noglob" feature flag (which will be turned on and eventually removed in future), so how many backslashes you need depends on the setting.

Quoting it should always work.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if quoting even works - string match will change behavior depending on status test-feature qmark-noglob.

~> fish_features= fish -c "string match '?' x"
x
~> fish_features=qmark-noglob fish -c "string match '?' x"
~ [1]> 

string match --regex or contains should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I wasn't aware of the qmark-noglob feature. And no, just quoting it isn't enough.
So maybe this isn't the best way to go ...
Can contains be used on a string? I assumed it was meant for lists.

Copy link
Member

Choose a reason for hiding this comment

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

Can contains be used on a string? I assumed it was meant for lists.

Yep, I believe that

if test (count (string match $__fish_svn_prompt_chars[$flag_index] -- $flag_status_string)) -eq 1

can be just

if contains $__fish_svn_prompt_chars[$flag_index] -- $flag_status_string

assuming that $flag_status_string is a list, otherwise even test x = y could work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be just

if contains $__fish_svn_prompt_chars[$flag_index] -- $flag_status_string

assuming that $flag_status_string is a list,

It is not :-)

otherwise even test x = y could work.

Won't work either, because, like I described below, we need to test if x is a substring of y (or more specifically, if the character x is contained in the string y).

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, then I'd use

string match -rq -- (string escape --style=regex -- $__fish_svn_prompt_chars[$flag_index]) $flag_status_string

Copy link
Member

Choose a reason for hiding this comment

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

$flag_status_string should probably just be a list. These are single characters, so I don't see why we join them.

# iterate over the different status types
for flag_type in $__fish_svn_prompt_flag_names
# resolve the name of the variable for the character representing the current status type
set -l flag_index (contains -i $flag_type $__fish_svn_prompt_flag_names)
# check to see if the status string for this column contains the character representing the current status type
if test (count (string match $__fish_svn_prompt_chars[$flag_index] -- $flag_status_string)) -eq 1
if test (count (string match -e $__fish_svn_prompt_chars[$flag_index] -- $flag_status_string)) -eq 1
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this works?

$flag_status_string is received from

__fish_svn_prompt_parse_status $column_status

And $column_status is one element (because of tr -d ' \n'), so this would only ever succeed if this is the only status character.

That doesn't seem correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um, no. $column_status can be, for example, 'AM', if you have one file added and one file modified.
This will effectively result in string match -e A -- AM and string match -e M -- AM, which will both succeed. Without -e, they will not. In both cases the number of matches is 1 (although checking the count seems a bit unnecessary, but I wanted to keep my changes minimal).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, right, "--entire" adds the wildcards at both ends.

I always forget the semantics of string's glob mode (and tbh we should have just gone full-regex).

Using regex matching will prevent different match behaviour
depending on qmark-noglob feature.
Also, counting the resulting matches is unnecessary.
Make $column_status a list be not removing newlines from SVN status
output. This makes checking for the individual status types within
a column easier because it doesn't require regex matching.
# check that the character count is not zero (this would indicate that there are status flags in this column)
if [ (count $column_status) -ne 0 ]
# check that the column status list does not only contain an empty element (if it does, this column is empty)
if [ (count $column_status) -gt 1 -o -n $column_status[1] ]
Copy link
Member

Choose a reason for hiding this comment

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

test -n always needs to have the variable quoted.

Our test (or [) bultin follows POSIX, and that allows usage like test string, which does the same thing as test -n string. Unfortunately it does that by saying that test with any one argument needs to always be true, so test -n, test -d etc are all true.

(yes, this is awkward, sorry)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Odd -- because it works ;-)
But just to be sure, I'll add quotes.

@zanchey zanchey added this to the fish 3.2.0 milestone Aug 25, 2020
@faho faho added the bug Something that's not working as intended label Aug 26, 2020
@faho faho merged commit 81d87d1 into fish-shell:master Aug 26, 2020
@faho
Copy link
Member

faho commented Aug 26, 2020

Merged, thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fish SVN prompt regression in 3.1
4 participants