{"thread":{"id":"37650","subject":"[PATCH] git-prompt.sh: shorter equal upstream branch name","startedAt":"2014-09-30T15:36:10Z","lastAt":"2014-10-07T20:10:30Z","messageCount":9,"participants":["Julien Carsique","Richard Hansen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250036","messageId":"1412091370-11727-1-git-send-email-jcarsique@nuxeo.com","threadId":"37650","inReplyTo":null,"subject":"[PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Julien Carsique","fromEmail":"julien.carsique@gmail.com","sentAt":"2014-09-30T15:36:10Z","receivedAt":"2014-09-30T15:36:10Z","isPatch":true,"sender":{"key":"julien.carsique@gmail.com","avatar":"https://avatars.githubusercontent.com/u/81140?v=4"},"body":"From: Julien Carsique <julien.carsique@gmail.com>\n\nWhen using the \"name\" option of GIT_PS1_SHOWUPSTREAM to show the upstream\nabbrev name, if the upstream name is the same as the local name, then some\nspace could be saved in the prompt. This is especially needed on long branch\nnames.\n\nReplace the upstream name with the sign '=' if equal to the local one.\nExample:    [master * u= origin/=]$\ninstead of: [master * u= origin/master]$\n\nSigned-off-by: Julien Carsique <julien.carsique@gmail.com>\n---\n contrib/completion/git-prompt.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex c5473dc..a9aba20 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -209,6 +209,13 @@ __git_ps1_show_upstream ()\n \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n \t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n \t\t\t\t--abbrev-ref \"$upstream\" 2>/dev/null)\n+\n+\t\t\t__head=${b##refs/heads/}\n+\t\t\tif [ \"$__head\" = \"${__git_ps1_upstream_name##*/}\" ]; then\n+\t\t\t\t__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}\n+\t\t\tfi\n+\t\t\tunset __head\n+\n \t\t\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n \t\t\t\tp=\"$p \\${__git_ps1_upstream_name}\"\n \t\t\telse\n-- \n2.1.0\n"},{"id":"250049","messageId":"542B1623.2070109@bbn.com","threadId":"37650","inReplyTo":"1412091370-11727-1-git-send-email-jcarsique@nuxeo.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-09-30T20:44:19Z","receivedAt":"2014-09-30T20:44:19Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-09-30 11:36, Julien Carsique wrote:\n> From: Julien Carsique <julien.carsique@gmail.com>\n> \n> When using the \"name\" option of GIT_PS1_SHOWUPSTREAM to show the upstream\n> abbrev name, if the upstream name is the same as the local name, then some\n> space could be saved in the prompt. This is especially needed on long branch\n> names.\n> \n> Replace the upstream name with the sign '=' if equal to the local one.\n> Example:    [master * u= origin/=]$\n> instead of: [master * u= origin/master]$\n\nSeems like a good idea to me.\n\n> \n> Signed-off-by: Julien Carsique <julien.carsique@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh | 7 +++++++\n>  1 file changed, 7 insertions(+)\n\nPlease add some new tests in t/9903-bash-prompt.sh.  In particular:\n  * upstream ref in refs/heads\n  * upstream is git-svn\n  * branch names containing slashes\n\n> \n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index c5473dc..a9aba20 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -209,6 +209,13 @@ __git_ps1_show_upstream ()\n>  \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n>  \t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n>  \t\t\t\t--abbrev-ref \"$upstream\" 2>/dev/null)\n> +\n> +\t\t\t__head=${b##refs/heads/}\n\nTo avoid colliding with other stuff, this variable should either be\nlocal or prefixed with '__git_ps1'.\n\n> +\t\t\tif [ \"$__head\" = \"${__git_ps1_upstream_name##*/}\" ]; then\n\nThis comparison breaks on branches containing a slash (e.g., foo/bar).\n\nAlso, how does this interact with git-svn?  (I don't use git-svn so I'm\nnot very familiar with how it manages refs.)\n\nAssuming remote names can't contain a slash (which I think is true), a\nsafer approach might be parse the full ref and special-case refs/remotes:\n\n    __git_ps1_upstream_name=$(git rev-parse \\\n        --abbrev-ref \"${upstream}\" 2>/dev/null)\n    local tmp\n    tmp=$(git rev-parse --symbolic-full-name \"${upstream}\" 2>/dev/null)\n    case ${tmp} in\n    refs/remotes/*)\n        # todo: can ${b} be something other than refs/heads/* here?\n        [ \"${__git_ps1_upstream_name#*/}\" != \"${b#refs/heads/}\" ] \\\n            || __git_ps1_upstream_name=${__git_ps1_upstream_name%%/*}/\\=\n        ;;\n    esac\n\nAdditional cases could be added to handle git-svn if needed.\n\n> +\t\t\t\t__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}\n\n  * This could break if ${__head} contains any pattern-special\n    characters.\n\n  * While this syntax works in both Bash and Zsh (assuming no\n    pattern-special characters), my preference is to stick to POSIX[1]\n    when possible.  For example, assuming the upstream name is\n    always in refs/remotes (which is not true, but this is an example)\n    and remote names can't contain a '/', you could do this:\n\n        __git_ps1_upstream_name=${__git_ps1_upstream_name%%/*}/\\=\n\n  * I don't think the CodingGuidelines explicitly prohibit long lines\n    for shell code, and this file already contains plenty of long\n    lines, but I really dislike lines longer than 80 characters.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html\n\n> +\t\t\tfi\n> +\t\t\tunset __head\n> +\n>  \t\t\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n>  \t\t\t\tp=\"$p \\${__git_ps1_upstream_name}\"\n>  \t\t\telse\n\n-Richard\n"},{"id":"250051","messageId":"xmqqbnpwrbcw.fsf@gitster.dls.corp.google.com","threadId":"37650","inReplyTo":"1412091370-11727-1-git-send-email-jcarsique@nuxeo.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-30T22:29:19Z","receivedAt":"2014-09-30T22:29:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Julien Carsique <julien.carsique@gmail.com> writes:\n\n> From: Julien Carsique <julien.carsique@gmail.com>\n>\n> When using the \"name\" option of GIT_PS1_SHOWUPSTREAM to show the upstream\n> abbrev name, if the upstream name is the same as the local name, then some\n> space could be saved in the prompt. This is especially needed on long branch\n> names.\n>\n> Replace the upstream name with the sign '=' if equal to the local one.\n> Example:    [master * u= origin/=]$\n> instead of: [master * u= origin/master]$\n>\n> Signed-off-by: Julien Carsique <julien.carsique@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh | 7 +++++++\n>  1 file changed, 7 insertions(+)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index c5473dc..a9aba20 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -209,6 +209,13 @@ __git_ps1_show_upstream ()\n>  \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n>  \t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n>  \t\t\t\t--abbrev-ref \"$upstream\" 2>/dev/null)\n> +\n> +\t\t\t__head=${b##refs/heads/}\n> +\t\t\tif [ \"$__head\" = \"${__git_ps1_upstream_name##*/}\" ]; then\n\nWhen you are on your \"xyz\" branch that was forked off of\n\"refs/remote/origin/xyz/xyz\", $__git_ps1_upstream_name would be\n\"origin/xyz/xyz\" and $__head is \"xyz\" at this point.  The latter\nmatches the former after stripping */ maximally from its front\n(i.e. \"xyz\").  It is unclear if you really want to make these two\nmatch, but as long as you correctly abbreviate you would not lose\ninformation, I would imagine.  I am guessing that you would want to\nsee \"origin/xyz/=\" in such a case, and if you started your \"xyz\" off\nof \"origin/abc/xyz\", you would want \"origin/abc/=\".\n\n> +\t\t\t\t__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}\n\nNow, what does ${__git_ps1_upstream_name/$__head/=} give us?\nThis should not do regexp substitution in the first place, I would\nthink.  You made sure $__head is the tail-matching substring of the\nlonger variable, so you chop that many bytes off of the latter.\n\nIs this new feature something everybody should want unconditionally,\nor are there valid reasons why some people may not want to see this\nabbreviation happen (even if the implementation were not buggy)?\n\n> +\t\t\tfi\n> +\t\t\tunset __head\n\nUnlike __git_ps1_upstream_name, this __head does not have to be\nvisible outside this function.  Wouldn't it be better to declare it\nas local (this is bash prompt and we can afford to step outside\nPOSIX) and there is no need to \"unset\" after use if you did so, no?\n\n>  \t\t\tif [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then\n>  \t\t\t\tp=\"$p \\${__git_ps1_upstream_name}\"\n>  \t\t\telse\n"},{"id":"250052","messageId":"xmqq7g0krb2p.fsf@gitster.dls.corp.google.com","threadId":"37650","inReplyTo":"542B1623.2070109@bbn.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-30T22:35:26Z","receivedAt":"2014-09-30T22:35:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n> Additional cases could be added to handle git-svn if needed.\n\nThanks for a review (everything I omitted above looked good to me).\n\n>> +\t\t\t\t__git_ps1_upstream_name=${__git_ps1_upstream_name/$__head/=}\n>\n>   * This could break if ${__head} contains any pattern-special\n>     characters.\n\n... but I do not think refnames can have *, ? and such so it may not\nbe relevant ;-).\n\n>   * While this syntax works in both Bash and Zsh (assuming no\n>     pattern-special characters), my preference is to stick to POSIX[1]\n>     when possible.\n\nNah.  The existing script is full of bash-isms like local you\nsuggested to add (and other constructs like shell arrays and [[ ]]\ntests, I suspect), and there is no hope to \"fix\" them to stick to\nthe bare-minimum POSIX, and there is no need to do so (isn't this\nbash-prompt script after all?)\n\n>   * I don't think the CodingGuidelines explicitly prohibit long lines\n>     for shell code, and this file already contains plenty of long\n>     lines, but I really dislike lines longer than 80 characters.\n\nYes, I dislike overlong lines, too.  But I also dislike lines that\nare artificially chopped into\nshorter pieces without\ngood reason ;-).\n"},{"id":"250056","messageId":"542B7AF8.6080501@bbn.com","threadId":"37650","inReplyTo":"xmqq7g0krb2p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-10-01T03:54:32Z","receivedAt":"2014-10-01T03:54:32Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-09-30 18:35, Junio C Hamano wrote:\n> Richard Hansen <rhansen@bbn.com> writes:\n>>   * While this syntax works in both Bash and Zsh (assuming no\n>>     pattern-special characters), my preference is to stick to POSIX[1]\n>>     when possible.\n> \n> Nah.  The existing script is full of bash-isms like local you\n> suggested to add (and other constructs like shell arrays and [[ ]]\n> tests, I suspect),\n\nTrue.\n\n> and there is no hope to \"fix\" them to stick to\n> the bare-minimum POSIX,\n\nI don't think it'd be hard to convert it to pure POSIX if there was a\ndesire to do so.  The biggest challenge would be 'local', which would\nrequire subshells or uniquified prefixed global variables.  Both of\nthose are likely to make the code a bit grotesque.\n\n> and there is no need to do so (isn't this\n> bash-prompt script after all?)\n\nIt would be unwise to go to great lengths to avoid Bashisms, but I think\nit would be smart to use POSIX syntax when it is easy to do so.  Rarely\nis it hard or awkward to use POSIX syntax ('local' and arrays are two\nmajor exceptions), so Bashisms like the ${//} expansion in this patch\nare usually unnecessary divergences from a ubiquitous standard.  POSIX\nis a stable foundation, and it's easy to get POSIX shell code to run\nconsistently on all POSIX-like shells.\n\nOne of these days I'll try converting git-prompt.sh to POSIX -- I'm\ncurious to see how bad it would be.\n\n-Richard\n"},{"id":"250126","messageId":"xmqqppebptmj.fsf@gitster.dls.corp.google.com","threadId":"37650","inReplyTo":"542B7AF8.6080501@bbn.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-01T17:49:56Z","receivedAt":"2014-10-01T17:49:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n>> and there is no hope to \"fix\" them to stick to\n>> the bare-minimum POSIX,\n>\n> I don't think it'd be hard to convert it to pure POSIX if there was a\n> desire to do so.\n\nNot necessarily; if you make it so slow to be usable as a prompt\nscript, that is not a \"conversion\".  Bash-isms in the script is\nallowed for a reason, unfortunately.\n\n> It would be unwise to go to great lengths to avoid Bashisms, but I think\n> it would be smart to use POSIX syntax when it is easy to do so.  \n\nIn general, I agree with you. People who know only bash tend to\noveruse bash-isms where they are not necessary, leaving an\nunreadable mess.\n\nFor the specific purpose of Julien's \"if the tail part of this\nstring matches the other string, replace that with an equal sign\",\n${parameter/pattern/string} is a wrong bash-ism to use.  But the\nright solution to count the length of the other string and take a\nsubstring of this string from its beginning would require other\nbash-isms ${#parameter} and ${parameter:offset:length}.\n\nAnd that's fine.\n"},{"id":"250285","messageId":"54340D63.8030507@gmail.com","threadId":"37650","inReplyTo":"xmqqppebptmj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Julien Carsique","fromEmail":"julien.carsique@gmail.com","sentAt":"2014-10-07T15:57:23Z","receivedAt":"2014-10-07T15:57:23Z","isPatch":true,"sender":{"key":"julien.carsique@gmail.com","avatar":"https://avatars.githubusercontent.com/u/81140?v=4"},"body":"Hi,\n\nThank you both for your feedback!\nI'm looking at applying your requests:\n- add tests,\n- variable renaming,\n- use of local,\n- fix multiple issues on string parsing\n- avoid useless bash-isms? Did you agree on the ones I should remove?\n\nI'll send an updated patch asap. Tell me if I forgot something.\n\nRegards,\nJulien\n\nOn 01/10/2014 19:49, Junio C Hamano wrote:\n> Richard Hansen <rhansen@bbn.com> writes:\n>\n>>> and there is no hope to \"fix\" them to stick to\n>>> the bare-minimum POSIX,\n>> I don't think it'd be hard to convert it to pure POSIX if there was a\n>> desire to do so.\n> Not necessarily; if you make it so slow to be usable as a prompt\n> script, that is not a \"conversion\".  Bash-isms in the script is\n> allowed for a reason, unfortunately.\n>\n>> It would be unwise to go to great lengths to avoid Bashisms, but I think\n>> it would be smart to use POSIX syntax when it is easy to do so.  \n> In general, I agree with you. People who know only bash tend to\n> overuse bash-isms where they are not necessary, leaving an\n> unreadable mess.\n>\n> For the specific purpose of Julien's \"if the tail part of this\n> string matches the other string, replace that with an equal sign\",\n> ${parameter/pattern/string} is a wrong bash-ism to use.  But the\n> right solution to count the length of the other string and take a\n> substring of this string from its beginning would require other\n> bash-isms ${#parameter} and ${parameter:offset:length}.\n>\n> And that's fine.\n"},{"id":"250306","messageId":"54344241.60703@bbn.com","threadId":"37650","inReplyTo":"54340D63.8030507@gmail.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-10-07T19:42:57Z","receivedAt":"2014-10-07T19:42:57Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-10-07 11:57, Julien Carsique wrote:\n> Hi,\n> \n> Thank you both for your feedback!\n> I'm looking at applying your requests:\n> - add tests,\n> - variable renaming,\n> - use of local,\n> - fix multiple issues on string parsing\n> - avoid useless bash-isms? Did you agree on the ones I should remove?\n\nI'm guessing the structure of your code will change somewhat when you\naddress the other issues, so I think it may be premature to discuss\nspecific Bashisms right now.  (There aren't any particular Bashisms that\nI think should always be avoided -- I just want people to ponder whether\na particular use of a Bashism is truly preferable to a POSIX-conformant\nalternative.)\n\nWrite the code in the way you think is best, and if I see a good way to\nconvert a Bashism to POSIX I'll let you know.  And feel free to ignore\nme -- I'm a member of the Church of POSIX Conformance while Junio is\nmuch more grounded in reality.  :)\n\n> \n> I'll send an updated patch asap. Tell me if I forgot something.\n\nYour list looks complete to me.  Thank you for contributing!\n\n-Richard\n\n\n> \n> Regards,\n> Julien\n> \n> On 01/10/2014 19:49, Junio C Hamano wrote:\n>> Richard Hansen <rhansen@bbn.com> writes:\n>>\n>>>> and there is no hope to \"fix\" them to stick to\n>>>> the bare-minimum POSIX,\n>>> I don't think it'd be hard to convert it to pure POSIX if there was a\n>>> desire to do so.\n>> Not necessarily; if you make it so slow to be usable as a prompt\n>> script, that is not a \"conversion\".  Bash-isms in the script is\n>> allowed for a reason, unfortunately.\n>>\n>>> It would be unwise to go to great lengths to avoid Bashisms, but I think\n>>> it would be smart to use POSIX syntax when it is easy to do so.  \n>> In general, I agree with you. People who know only bash tend to\n>> overuse bash-isms where they are not necessary, leaving an\n>> unreadable mess.\n>>\n>> For the specific purpose of Julien's \"if the tail part of this\n>> string matches the other string, replace that with an equal sign\",\n>> ${parameter/pattern/string} is a wrong bash-ism to use.  But the\n>> right solution to count the length of the other string and take a\n>> substring of this string from its beginning would require other\n>> bash-isms ${#parameter} and ${parameter:offset:length}.\n>>\n>> And that's fine.\n> \n> \n"},{"id":"250309","messageId":"xmqqiojvej49.fsf@gitster.dls.corp.google.com","threadId":"37650","inReplyTo":"54340D63.8030507@gmail.com","subject":"Re: [PATCH] git-prompt.sh: shorter equal upstream branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-07T20:10:30Z","receivedAt":"2014-10-07T20:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Julien Carsique <julien.carsique@gmail.com> writes:\n\n> Hi,\n>\n> Thank you both for your feedback!\n> I'm looking at applying your requests:\n> - add tests,\n> - variable renaming,\n> - use of local,\n> - fix multiple issues on string parsing\n> - avoid useless bash-isms? Did you agree on the ones I should remove?\n>\n> I'll send an updated patch asap. Tell me if I forgot something.\n\nI just re-read the comments in the thread, and a few things look\nmissing:\n\n - git-svn?\n - conditionally enable this feature?\n\nOther than that the above list looks like a fairly good one.\n\nThanks.\n"}]}