Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
Hi,
Thank you both for your feedback!
I'm looking at applying your requests:
- add tests,
- variable renaming,
- use of local,
- fix multiple issues on string parsing
- avoid useless bash-isms? Did you agree on the ones I should remove?
I'll send an updated patch asap. Tell me if I forgot something.
Regards,
Julien
On 01/10/2014 19:49, Junio C Hamano wrote:
Richard Hansen rhan...@bbn.com writes:
and there is no hope to fix them to stick to
the bare-minimum POSIX,
I don't think it'd be hard to convert it to pure POSIX if there was a
desire to do so.
Not necessarily; if you make it so slow to be usable as a prompt
script, that is not a conversion. Bash-isms in the script is
allowed for a reason, unfortunately.
It would be unwise to go to great lengths to avoid Bashisms, but I think
it would be smart to use POSIX syntax when it is easy to do so.
In general, I agree with you. People who know only bash tend to
overuse bash-isms where they are not necessary, leaving an
unreadable mess.
For the specific purpose of Julien's if the tail part of this
string matches the other string, replace that with an equal sign,
${parameter/pattern/string} is a wrong bash-ism to use. But the
right solution to count the length of the other string and take a
substring of this string from its beginning would require other
bash-isms ${#parameter} and ${parameter:offset:length}.
And that's fine.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
On 2014-10-07 11:57, Julien Carsique wrote:
Hi,
Thank you both for your feedback!
I'm looking at applying your requests:
- add tests,
- variable renaming,
- use of local,
- fix multiple issues on string parsing
- avoid useless bash-isms? Did you agree on the ones I should remove?
I'm guessing the structure of your code will change somewhat when you
address the other issues, so I think it may be premature to discuss
specific Bashisms right now. (There aren't any particular Bashisms that
I think should always be avoided -- I just want people to ponder whether
a particular use of a Bashism is truly preferable to a POSIX-conformant
alternative.)
Write the code in the way you think is best, and if I see a good way to
convert a Bashism to POSIX I'll let you know. And feel free to ignore
me -- I'm a member of the Church of POSIX Conformance while Junio is
much more grounded in reality. :)
I'll send an updated patch asap. Tell me if I forgot something.
Your list looks complete to me. Thank you for contributing!
-Richard
Regards,
Julien
On 01/10/2014 19:49, Junio C Hamano wrote:
Richard Hansen rhan...@bbn.com writes:
and there is no hope to fix them to stick to
the bare-minimum POSIX,
I don't think it'd be hard to convert it to pure POSIX if there was a
desire to do so.
Not necessarily; if you make it so slow to be usable as a prompt
script, that is not a conversion. Bash-isms in the script is
allowed for a reason, unfortunately.
It would be unwise to go to great lengths to avoid Bashisms, but I think
it would be smart to use POSIX syntax when it is easy to do so.
In general, I agree with you. People who know only bash tend to
overuse bash-isms where they are not necessary, leaving an
unreadable mess.
For the specific purpose of Julien's if the tail part of this
string matches the other string, replace that with an equal sign,
${parameter/pattern/string} is a wrong bash-ism to use. But the
right solution to count the length of the other string and take a
substring of this string from its beginning would require other
bash-isms ${#parameter} and ${parameter:offset:length}.
And that's fine.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
Julien Carsique julien.carsi...@gmail.com writes: Hi, Thank you both for your feedback! I'm looking at applying your requests: - add tests, - variable renaming, - use of local, - fix multiple issues on string parsing - avoid useless bash-isms? Did you agree on the ones I should remove? I'll send an updated patch asap. Tell me if I forgot something. I just re-read the comments in the thread, and a few things look missing: - git-svn? - conditionally enable this feature? Other than that the above list looks like a fairly good one. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
Richard Hansen rhan...@bbn.com writes:
and there is no hope to fix them to stick to
the bare-minimum POSIX,
I don't think it'd be hard to convert it to pure POSIX if there was a
desire to do so.
Not necessarily; if you make it so slow to be usable as a prompt
script, that is not a conversion. Bash-isms in the script is
allowed for a reason, unfortunately.
It would be unwise to go to great lengths to avoid Bashisms, but I think
it would be smart to use POSIX syntax when it is easy to do so.
In general, I agree with you. People who know only bash tend to
overuse bash-isms where they are not necessary, leaving an
unreadable mess.
For the specific purpose of Julien's if the tail part of this
string matches the other string, replace that with an equal sign,
${parameter/pattern/string} is a wrong bash-ism to use. But the
right solution to count the length of the other string and take a
substring of this string from its beginning would require other
bash-isms ${#parameter} and ${parameter:offset:length}.
And that's fine.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-prompt.sh: shorter equal upstream branch name
From: Julien Carsique julien.carsi...@gmail.com
When using the name option of GIT_PS1_SHOWUPSTREAM to show the upstream
abbrev name, if the upstream name is the same as the local name, then some
space could be saved in the prompt. This is especially needed on long branch
names.
Replace the upstream name with the sign '=' if equal to the local one.
Example:[master * u= origin/=]$
instead of: [master * u= origin/master]$
Signed-off-by: Julien Carsique julien.carsi...@gmail.com
---
contrib/completion/git-prompt.sh | 7 +++
1 file changed, 7 insertions(+)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c5473dc..a9aba20 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -209,6 +209,13 @@ __git_ps1_show_upstream ()
if [[ -n $count -n $name ]]; then
__git_ps1_upstream_name=$(git rev-parse \
--abbrev-ref $upstream 2/dev/null)
+
+ __head=${b##refs/heads/}
+ if [ $__head = ${__git_ps1_upstream_name##*/} ];
then
+
__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}
+ fi
+ unset __head
+
if [ $pcmode = yes ] [ $ps1_expanded = yes ]; then
p=$p \${__git_ps1_upstream_name}
else
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
On 2014-09-30 11:36, Julien Carsique wrote:
From: Julien Carsique julien.carsi...@gmail.com
When using the name option of GIT_PS1_SHOWUPSTREAM to show the upstream
abbrev name, if the upstream name is the same as the local name, then some
space could be saved in the prompt. This is especially needed on long branch
names.
Replace the upstream name with the sign '=' if equal to the local one.
Example:[master * u= origin/=]$
instead of: [master * u= origin/master]$
Seems like a good idea to me.
Signed-off-by: Julien Carsique julien.carsi...@gmail.com
---
contrib/completion/git-prompt.sh | 7 +++
1 file changed, 7 insertions(+)
Please add some new tests in t/9903-bash-prompt.sh. In particular:
* upstream ref in refs/heads
* upstream is git-svn
* branch names containing slashes
diff --git a/contrib/completion/git-prompt.sh
b/contrib/completion/git-prompt.sh
index c5473dc..a9aba20 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -209,6 +209,13 @@ __git_ps1_show_upstream ()
if [[ -n $count -n $name ]]; then
__git_ps1_upstream_name=$(git rev-parse \
--abbrev-ref $upstream 2/dev/null)
+
+ __head=${b##refs/heads/}
To avoid colliding with other stuff, this variable should either be
local or prefixed with '__git_ps1'.
+ if [ $__head = ${__git_ps1_upstream_name##*/} ];
then
This comparison breaks on branches containing a slash (e.g., foo/bar).
Also, how does this interact with git-svn? (I don't use git-svn so I'm
not very familiar with how it manages refs.)
Assuming remote names can't contain a slash (which I think is true), a
safer approach might be parse the full ref and special-case refs/remotes:
__git_ps1_upstream_name=$(git rev-parse \
--abbrev-ref ${upstream} 2/dev/null)
local tmp
tmp=$(git rev-parse --symbolic-full-name ${upstream} 2/dev/null)
case ${tmp} in
refs/remotes/*)
# todo: can ${b} be something other than refs/heads/* here?
[ ${__git_ps1_upstream_name#*/} != ${b#refs/heads/} ] \
|| __git_ps1_upstream_name=${__git_ps1_upstream_name%%/*}/\=
;;
esac
Additional cases could be added to handle git-svn if needed.
+
__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}
* This could break if ${__head} contains any pattern-special
characters.
* While this syntax works in both Bash and Zsh (assuming no
pattern-special characters), my preference is to stick to POSIX[1]
when possible. For example, assuming the upstream name is
always in refs/remotes (which is not true, but this is an example)
and remote names can't contain a '/', you could do this:
__git_ps1_upstream_name=${__git_ps1_upstream_name%%/*}/\=
* I don't think the CodingGuidelines explicitly prohibit long lines
for shell code, and this file already contains plenty of long
lines, but I really dislike lines longer than 80 characters.
[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
+ fi
+ unset __head
+
if [ $pcmode = yes ] [ $ps1_expanded = yes ]; then
p=$p \${__git_ps1_upstream_name}
else
-Richard
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
Julien Carsique julien.carsi...@gmail.com writes:
From: Julien Carsique julien.carsi...@gmail.com
When using the name option of GIT_PS1_SHOWUPSTREAM to show the upstream
abbrev name, if the upstream name is the same as the local name, then some
space could be saved in the prompt. This is especially needed on long branch
names.
Replace the upstream name with the sign '=' if equal to the local one.
Example:[master * u= origin/=]$
instead of: [master * u= origin/master]$
Signed-off-by: Julien Carsique julien.carsi...@gmail.com
---
contrib/completion/git-prompt.sh | 7 +++
1 file changed, 7 insertions(+)
diff --git a/contrib/completion/git-prompt.sh
b/contrib/completion/git-prompt.sh
index c5473dc..a9aba20 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -209,6 +209,13 @@ __git_ps1_show_upstream ()
if [[ -n $count -n $name ]]; then
__git_ps1_upstream_name=$(git rev-parse \
--abbrev-ref $upstream 2/dev/null)
+
+ __head=${b##refs/heads/}
+ if [ $__head = ${__git_ps1_upstream_name##*/} ];
then
When you are on your xyz branch that was forked off of
refs/remote/origin/xyz/xyz, $__git_ps1_upstream_name would be
origin/xyz/xyz and $__head is xyz at this point. The latter
matches the former after stripping */ maximally from its front
(i.e. xyz). It is unclear if you really want to make these two
match, but as long as you correctly abbreviate you would not lose
information, I would imagine. I am guessing that you would want to
see origin/xyz/= in such a case, and if you started your xyz off
of origin/abc/xyz, you would want origin/abc/=.
+
__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}
Now, what does ${__git_ps1_upstream_name/$__head/=} give us?
This should not do regexp substitution in the first place, I would
think. You made sure $__head is the tail-matching substring of the
longer variable, so you chop that many bytes off of the latter.
Is this new feature something everybody should want unconditionally,
or are there valid reasons why some people may not want to see this
abbreviation happen (even if the implementation were not buggy)?
+ fi
+ unset __head
Unlike __git_ps1_upstream_name, this __head does not have to be
visible outside this function. Wouldn't it be better to declare it
as local (this is bash prompt and we can afford to step outside
POSIX) and there is no need to unset after use if you did so, no?
if [ $pcmode = yes ] [ $ps1_expanded = yes ]; then
p=$p \${__git_ps1_upstream_name}
else
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
Richard Hansen rhan...@bbn.com writes:
Additional cases could be added to handle git-svn if needed.
Thanks for a review (everything I omitted above looked good to me).
+
__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}
* This could break if ${__head} contains any pattern-special
characters.
... but I do not think refnames can have *, ? and such so it may not
be relevant ;-).
* While this syntax works in both Bash and Zsh (assuming no
pattern-special characters), my preference is to stick to POSIX[1]
when possible.
Nah. The existing script is full of bash-isms like local you
suggested to add (and other constructs like shell arrays and [[ ]]
tests, I suspect), and there is no hope to fix them to stick to
the bare-minimum POSIX, and there is no need to do so (isn't this
bash-prompt script after all?)
* I don't think the CodingGuidelines explicitly prohibit long lines
for shell code, and this file already contains plenty of long
lines, but I really dislike lines longer than 80 characters.
Yes, I dislike overlong lines, too. But I also dislike lines that
are artificially chopped into
shorter pieces without
good reason ;-).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt.sh: shorter equal upstream branch name
On 2014-09-30 18:35, Junio C Hamano wrote:
Richard Hansen rhan...@bbn.com writes:
* While this syntax works in both Bash and Zsh (assuming no
pattern-special characters), my preference is to stick to POSIX[1]
when possible.
Nah. The existing script is full of bash-isms like local you
suggested to add (and other constructs like shell arrays and [[ ]]
tests, I suspect),
True.
and there is no hope to fix them to stick to
the bare-minimum POSIX,
I don't think it'd be hard to convert it to pure POSIX if there was a
desire to do so. The biggest challenge would be 'local', which would
require subshells or uniquified prefixed global variables. Both of
those are likely to make the code a bit grotesque.
and there is no need to do so (isn't this
bash-prompt script after all?)
It would be unwise to go to great lengths to avoid Bashisms, but I think
it would be smart to use POSIX syntax when it is easy to do so. Rarely
is it hard or awkward to use POSIX syntax ('local' and arrays are two
major exceptions), so Bashisms like the ${//} expansion in this patch
are usually unnecessary divergences from a ubiquitous standard. POSIX
is a stable foundation, and it's easy to get POSIX shell code to run
consistently on all POSIX-like shells.
One of these days I'll try converting git-prompt.sh to POSIX -- I'm
curious to see how bad it would be.
-Richard
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
