Support for svn status in prompt #2582
Conversation
# ============================== | ||
|
||
# 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` |
faho
Dec 9, 2015
Member
If these aren't supposed to be changed, why are they variables?
If these aren't supposed to be changed, why are they variables?
samdmarshall
Dec 9, 2015
Author
Contributor
It seemed like this was the most efficient way to access them for parsing, I don't like they are global defines but there didn't seem to be a more efficient way of doing this when the helper function they get used in will be called many times.
It seemed like this was the most efficient way to access them for parsing, I don't like they are global defines but there didn't seem to be a more efficient way of doing this when the helper function they get used in will be called many times.
faho
Dec 10, 2015
Member
How about either adding them as local variables to the function they are used in or doing a switch/case there? Or make a list of these characters, another of the names and then map them via index:
set __fish_svn_status_chars A C D I M N R # ...
set __fish_svn_statuses added conflicted deleted ignored modified replaced # ...
echo $__fish_svn_prompt_char_{$__fish_svn_statuses(contains -i -- $status $__fish_svn_status_chars)}
(untested, the parser might barf on that)
How about either adding them as local variables to the function they are used in or doing a switch/case there? Or make a list of these characters, another of the names and then map them via index:
set __fish_svn_status_chars A C D I M N R # ...
set __fish_svn_statuses added conflicted deleted ignored modified replaced # ...
echo $__fish_svn_prompt_char_{$__fish_svn_statuses(contains -i -- $status $__fish_svn_status_chars)}
(untested, the parser might barf on that)
faho
Dec 10, 2015
Member
I don't think setting them as local variables is going to take that much time. In my experience most fishscript is taking the most time in calls to external tools (the hg_prompt was particularly bad about that, to the point that we've added custom code to find the .hg directory). Even though svn seems reasonably quick, it's still a few forks.
My gut tells me that optimization here is achieved by replacing grep
or math
(which forks to bc
), not via defining variables.
If you wish to measure, fish has a nice "-p" option to output profiling data (though do keep in mind that even non-interactive fish will still read all config files).
I don't think setting them as local variables is going to take that much time. In my experience most fishscript is taking the most time in calls to external tools (the hg_prompt was particularly bad about that, to the point that we've added custom code to find the .hg directory). Even though svn seems reasonably quick, it's still a few forks.
My gut tells me that optimization here is achieved by replacing grep
or math
(which forks to bc
), not via defining variables.
If you wish to measure, fish has a nice "-p" option to output profiling data (though do keep in mind that even non-interactive fish will still read all config files).
samdmarshall
Dec 10, 2015
Author
Contributor
Since this gets called 7x in a pretty tight loop i was worried that a local variable setup/teardown would cause a bit of unwanted overhead to an already expensive function call. If you think that is fine, I can make that change but I am concerned about the performance of doing this.
Edit: Ok then I'll give that a try and update the PR.
Since this gets called 7x in a pretty tight loop i was worried that a local variable setup/teardown would cause a bit of unwanted overhead to an already expensive function call. If you think that is fine, I can make that change but I am concerned about the performance of doing this.
Edit: Ok then I'll give that a try and update the PR.
|
||
function __fish_svn_prompt_parse_status --argument flag_status_string --description "helper function that does pretty formatting on svn status" | ||
# iterate over the different status types | ||
for flag_index in (seq (count $__fish_svn_prompt_flag_names)) |
faho
Dec 9, 2015
Member
Why not just iterate over __fish_svn_prompt_flag_names directly? The index is only used to access that.
Why not just iterate over __fish_svn_prompt_flag_names directly? The index is only used to access that.
samdmarshall
Dec 9, 2015
Author
Contributor
It was giving me some trouble with multiple substitution in variable names. Might have been my error, so I will have another look at it.
It was giving me some trouble with multiple substitution in variable names. Might have been my error, so I will have another look at it.
The |
# 2. swap the ":" back to newline characters so we can perform a column based `cut` | ||
# 3. cut out the current column of characters | ||
# 4. remove spaces and newline characters | ||
set -l column_status (echo $svn_status_lines | tr ':' '\n' | cut -c $col | tr -d ' \n') |
faho
Dec 9, 2015
Member
Use printf "%s\n" $svn_status_lines
and you can nix replacing the newlines.
What happens is that fish will split command substitutions on newlines, making every line one argument, which then leads to svn_status_lines becoming a list with each line as one element. When you then hand it to echo
, it'll read the arguments and echo them out separated by spaces. printf won't do that, instead using the format string once per argument.
Use printf "%s\n" $svn_status_lines
and you can nix replacing the newlines.
What happens is that fish will split command substitutions on newlines, making every line one argument, which then leads to svn_status_lines becoming a list with each line as one element. When you then hand it to echo
, it'll read the arguments and echo them out separated by spaces. printf won't do that, instead using the format string once per argument.
samdmarshall
Dec 9, 2015
Author
Contributor
Interesting. OK i will look at changing that. I remember reading there was some funky-ness with handling multi-line strings in a variable in fish shell (and experiencing some of that myself). And came to this code as it "worked". Thanks!
Interesting. OK i will look at changing that. I remember reading there was some funky-ness with handling multi-line strings in a variable in fish shell (and experiencing some of that myself). And came to this code as it "worked". Thanks!
I'll have to test this now. Any nice svn repo to try? |
Heh, the only public one that comes to mind is the llvm repos. This seems most work out of the svn repos I use at work. |
end | ||
|
||
# get the current revision number | ||
set -l repo_revision_number (command svn info | grep "Last Changed Rev: " | sed "s=Last Changed Rev: ==") |
faho
Dec 9, 2015
Member
Hurray for localization - this won't work if your language isn't english. A simple workaround is of course to set -lx LC_ALL C
set -lx LANGUAGE en
(it seems svn will read that, ignoring existing conventions) here, though I'd rather have a less hackish solution.
Hurray for localization - this won't work if your language isn't english. A simple workaround is of course to set -lx LC_ALL C
set -lx LANGUAGE en
(it seems svn will read that, ignoring existing conventions) here, though I'd rather have a less hackish solution.
faho
Dec 9, 2015
Member
Would svnversion
work here?
Would svnversion
work here?
samdmarshall
Dec 9, 2015
Author
Contributor
I did notice there seems to be a function in fish shell for fetching the revision number of a repo but I didn't use it because it also spits out some other strings and I wasn't sure if it was half-implemented or not.
As for the grep strings, I was basing this off the source of the svn command printing out what appear to be english strings.
I did notice there seems to be a function in fish shell for fetching the revision number of a repo but I didn't use it because it also spits out some other strings and I wasn't sure if it was half-implemented or not.
As for the grep strings, I was basing this off the source of the svn command printing out what appear to be english strings.
faho
Dec 9, 2015
Member
I did notice there seems to be a function in fish shell for fetching the revision number
__fish_print_svn_rev? That has the same issue as it does basically the same thing.
As for the grep strings, I was basing this off the source of the svn command printing out what appear to be english strings.
They are english strings if your locale is english. Mine is german, so it'll print german strings (e.g. "Letzte geänderte Rev").
I'm trying to find a portable way to get svn information (i.e. the svn counterpart to git rev-parse
or hg status
), but I'm not coming up with much. svnversion
might fit the bill, but I'm not sure if it prints everything you want here, svnlook
is another, but it works only with repositories, not working copies.
I did notice there seems to be a function in fish shell for fetching the revision number
__fish_print_svn_rev? That has the same issue as it does basically the same thing.
As for the grep strings, I was basing this off the source of the svn command printing out what appear to be english strings.
They are english strings if your locale is english. Mine is german, so it'll print german strings (e.g. "Letzte geänderte Rev").
I'm trying to find a portable way to get svn information (i.e. the svn counterpart to git rev-parse
or hg status
), but I'm not coming up with much. svnversion
might fit the bill, but I'm not sure if it prints everything you want here, svnlook
is another, but it works only with repositories, not working copies.
samdmarshall
Dec 9, 2015
Author
Contributor
Yes, I will look at using svnversion
here and see if it satisfied the needs. Maybe the function should be refactored to use svnversion
then I should call that instead of implementing it twice.
it looks like this might work for using svnversion
:
svnversion | sed -e 's=[^0-9:]*==g' -e 's=^.*:=='
but i'd have to test it out a bit first.
Edit: Now realizing that the second -e
in the sed might not be necessary and be removing desired data to display to the user. I'm ok with removing that second sed expression, if you think that would be locale agnostic.
Yes, I will look at using svnversion
here and see if it satisfied the needs. Maybe the function should be refactored to use svnversion
then I should call that instead of implementing it twice.
it looks like this might work for using svnversion
:
svnversion | sed -e 's=[^0-9:]*==g' -e 's=^.*:=='
but i'd have to test it out a bit first.
Edit: Now realizing that the second -e
in the sed might not be necessary and be removing desired data to display to the user. I'm ok with removing that second sed expression, if you think that would be locale agnostic.
faho
Dec 9, 2015
Member
I'm ok with removing that second sed expression, if you think that would be locale agnostic.
I'm not sure why it wouldn't be - svnversion
for me outputs "255122M", though that's on a fresh checkout of llvm (I don't use svn). It doesn't seem to distinguish between "object modified" and "object missing" (ie. deleted) like svnstatus
does, though, and that's localized as well.
If you'd like to use string, it'd look like svnversion | string replace -r '([0-9]*).*' '$1'
.
Edit: Another way to get the revision seems to be svn info --show-item last-changed-revision
, which seems to purely output the number.
I'm ok with removing that second sed expression, if you think that would be locale agnostic.
I'm not sure why it wouldn't be - svnversion
for me outputs "255122M", though that's on a fresh checkout of llvm (I don't use svn). It doesn't seem to distinguish between "object modified" and "object missing" (ie. deleted) like svnstatus
does, though, and that's localized as well.
If you'd like to use string, it'd look like svnversion | string replace -r '([0-9]*).*' '$1'
.
Edit: Another way to get the revision seems to be svn info --show-item last-changed-revision
, which seems to purely output the number.
samdmarshall
Dec 9, 2015
Author
Contributor
I would like to use string
however since I don't use a development build of fish shell i wanted to avoid maintaining this twice until a formal release is made.
As for the localized status of svn status
the code I looked at from the build of svn apple ships has hardcoded text strings in english for the bits of the output i am attempting to remove. I recognize that isn't sufficient for the numerous platforms that fish shell supports -- I don't know how to approach the problem to tackle this in a more agnostic manner.
I would like to use string
however since I don't use a development build of fish shell i wanted to avoid maintaining this twice until a formal release is made.
As for the localized status of svn status
the code I looked at from the build of svn apple ships has hardcoded text strings in english for the bits of the output i am attempting to remove. I recognize that isn't sufficient for the numerous platforms that fish shell supports -- I don't know how to approach the problem to tackle this in a more agnostic manner.
faho
Dec 10, 2015
Member
As for the localized status of svn status the code I looked at from the build of svn apple ships has hardcoded text strings in english for the bits of the output i am attempting to remove.
I haven't actually gotten any localized output from svn status
- maybe that's not a thing anymore? (It looks like "! CODE_OWNERS.TXT" because I've removed that file to test)
I'm not quite sure how to cause a conflict to happen which might trigger some actual language.
Did you see the suggestion about svn info --show-item last-changed-revision
? (There's also the option of getting xml output, but parsing that in shell is not an enviable task)
As for the localized status of svn status the code I looked at from the build of svn apple ships has hardcoded text strings in english for the bits of the output i am attempting to remove.
I haven't actually gotten any localized output from svn status
- maybe that's not a thing anymore? (It looks like "! CODE_OWNERS.TXT" because I've removed that file to test)
I'm not quite sure how to cause a conflict to happen which might trigger some actual language.
Did you see the suggestion about svn info --show-item last-changed-revision
? (There's also the option of getting xml output, but parsing that in shell is not an enviable task)
samdmarshall
Dec 10, 2015
Author
Contributor
I did see the suggestion, however that doesn't work for my version of svn, (doesn't recognize the --show-item
flag). I think I'm going to go with the svnversion | sed -e 's=[^0-9:]*==g'
command and try to get this PR updated today to reflect the changes we have discussed.
I did see the suggestion, however that doesn't work for my version of svn, (doesn't recognize the --show-item
flag). I think I'm going to go with the svnversion | sed -e 's=[^0-9:]*==g'
command and try to get this PR updated today to reflect the changes we have discussed.
…or changes to _fish_svn_prompt to optimize execution
OK I've updated with changes we discussed, how does it look now? |
This comment has been minimized.
This comment has been minimized.
As I said - this |
Sorry about that, when i tried it out again I think i forgot to add the |
The I'll test the new version a bit more and if it passes I'll merge. |
OK, once a release of fish shell is made with the new |
That last commit changed the time from 255357 to 258585 ( I've managed to get a 20% improvement, though, by removing the `math´ call. |
|
||
# the default separator is empty | ||
set -l prompt_separator "" | ||
for index in (seq (math "$col - $last_column")) |
faho
Dec 10, 2015
Member
Since you're not using $index inside the loop, you can just do seq $last_column $col
. Or am I missing anything?
Since you're not using $index inside the loop, you can just do seq $last_column $col
. Or am I missing anything?
samdmarshall
Dec 10, 2015
Author
Contributor
it looks like it iterates an extra time when doing seq $last_column $col
vs seq (math "$col - $last_column")
it looks like it iterates an extra time when doing seq $last_column $col
vs seq (math "$col - $last_column")
I've merged this now since it's nice code that works and the time is dominated by the Thanks for the nice work and quick response! |
This is to add support for displaying the output of
svn status
in a prompt similar to the waygit
andhg
status get displayed. I tried to match the styling and conventions as best as I could, please let me know if there is any glaring issues with it.Note: I wrote this against fish 2.2.0, so it doesn't take advantage of any of the new string manipulation features as there hasn't been published release that includes support for that yet. I would be happy to update this once a new release is made.