{"thread":{"id":"59323","subject":"[PATCH try2] completion: prompt: use generic colors","startedAt":"2023-02-28T14:59:49Z","lastAt":"2023-03-16T23:21:28Z","messageCount":6,"participants":["Felipe Contreras","Junio C Hamano","Joakim Petersen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472848","messageId":"20230228145934.4182166-1-felipe.contreras@gmail.com","threadId":"59323","inReplyTo":null,"subject":"[PATCH try2] completion: prompt: use generic colors","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-02-28T14:59:34Z","receivedAt":"2023-02-28T14:59:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When the prompt command mode was introduced in 1bfc51ac81 (Allow\n__git_ps1 to be used in PROMPT_COMMAND, 2012-10-10) the assumption was\nthat it was necessary in order to properly add colors to PS1 in bash,\nbut this wasn't true.\n\nIt's true that the \\[ \\] markers add the information needed to properly\ncalculate the width of the prompt, and they have to be added directly to\nPS1, a function returning them doesn't work.\n\nBut that is because bash coverts the \\[ \\] markers in PS1 to \\001 \\002,\nwhich is what readline ultimately needs in order to calculate the width.\n\nWe don't need bash to do this conversion, we can use \\001 \\002\nourselves, and then the prompt command mode is not necessary to display\ncolors.\n\nThis is what functions returning colors are supposed to do [1].\n\n[1] http://mywiki.wooledge.org/BashFAQ/053\n\nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-prompt.sh | 19 +++++++------------\n t/t9903-bash-prompt.sh | 8 ++++----\n 2 files changed, 11 insertions(+), 16 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 57972c2845..76ee4ab1e5 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -100,9 +100,7 @@\n #\n # If you would like a colored hint about the current dirty state, set\n # GIT_PS1_SHOWCOLORHINTS to a nonempty value. The colors are based on\n-# the colored output of \"git status -sb\" and are available only when\n-# using __git_ps1 for PROMPT_COMMAND or precmd in Bash,\n-# but always available in Zsh.\n+# the colored output of \"git status -sb\".\n #\n # If you would like __git_ps1 to do nothing in the case when the current\n # directory is set up to be ignored by git, then set\n@@ -259,12 +257,12 @@ __git_ps1_colorize_gitstring ()\n \t\tlocal c_lblue='%F{blue}'\n \t\tlocal c_clear='%f'\n \telse\n-\t\t# Using \\[ and \\] around colors is necessary to prevent\n+\t\t# Using \\001 and \\002 around colors is necessary to prevent\n \t\t# issues with command line editing/browsing/completion!\n-\t\tlocal c_red='\\[\\e[31m\\]'\n-\t\tlocal c_green='\\[\\e[32m\\]'\n-\t\tlocal c_lblue='\\[\\e[1;34m\\]'\n-\t\tlocal c_clear='\\[\\e[0m\\]'\n+\t\tlocal c_red=$'\\001\\e[31m\\002'\n+\t\tlocal c_green=$'\\001\\e[32m\\002'\n+\t\tlocal c_lblue=$'\\001\\e[1;34m\\002'\n+\t\tlocal c_clear=$'\\001\\e[0m\\002'\n \tfi\n \tlocal bad_color=$c_red\n \tlocal ok_color=$c_green\n@@ -574,11 +572,8 @@ __git_ps1 ()\n \t\tb=\"\\${__git_ps1_branch_name}\"\n \tfi\n \n-\t# NO color option unless in PROMPT_COMMAND mode or it's Zsh\n \tif [ -n \"${GIT_PS1_SHOWCOLORHINTS-}\" ]; then\n-\t\tif [ $pcmode = yes ] || [ -n \"${ZSH_VERSION-}\" ]; then\n-\t\t\t__git_ps1_colorize_gitstring\n-\t\tfi\n+\t\t__git_ps1_colorize_gitstring\n \tfi\n \n \tlocal f=\"$h$w$i$s$u$p\"\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex d459fae655..d667dda654 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -13,10 +13,10 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n \n actual=\"$TRASH_DIRECTORY/actual\"\n-c_red='\\\\[\\\\e[31m\\\\]'\n-c_green='\\\\[\\\\e[32m\\\\]'\n-c_lblue='\\\\[\\\\e[1;34m\\\\]'\n-c_clear='\\\\[\\\\e[0m\\\\]'\n+c_red='\\001\\e[31m\\002'\n+c_green='\\001\\e[32m\\002'\n+c_lblue='\\001\\e[1;34m\\002'\n+c_clear='\\001\\e[0m\\002'\n \n test_expect_success 'setup for prompt tests' '\n \tgit init otherrepo &&\n-- \n2.39.2\n\n"},{"id":"472893","messageId":"xmqq4jr4nu3u.fsf@gitster.g","threadId":"59323","inReplyTo":"20230228145934.4182166-1-felipe.contreras@gmail.com","subject":"Re: [PATCH try2] completion: prompt: use generic colors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-01T19:34:45Z","receivedAt":"2023-03-01T19:34:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> When the prompt command mode was introduced in 1bfc51ac81 (Allow\n> __git_ps1 to be used in PROMPT_COMMAND, 2012-10-10) the assumption was\n> that it was necessary in order to properly add colors to PS1 in bash,\n> but this wasn't true.\n>\n> It's true that the \\[ \\] markers add the information needed to properly\n> calculate the width of the prompt, and they have to be added directly to\n> PS1, a function returning them doesn't work.\n>\n> But that is because bash coverts the \\[ \\] markers in PS1 to \\001 \\002,\n> which is what readline ultimately needs in order to calculate the width.\n>\n> We don't need bash to do this conversion, we can use \\001 \\002\n> ourselves, and then the prompt command mode is not necessary to display\n> colors.\n>\n> This is what functions returning colors are supposed to do [1].\n>\n> [1] http://mywiki.wooledge.org/BashFAQ/053\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> contrib/completion/git-prompt.sh | 19 +++++++------------\n> t/t9903-bash-prompt.sh | 8 ++++----\n> 2 files changed, 11 insertions(+), 16 deletions(-)\n\nComments from those who use colored prompt and who are familiar with\nthe mechansim used to implement this? As I do not use the feature\nat all and haven't been following it, seeing independent support\nwould help the topic.\n\nThanks.\n\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 57972c2845..76ee4ab1e5 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -100,9 +100,7 @@\n> #\n> # If you would like a colored hint about the current dirty state, set\n> # GIT_PS1_SHOWCOLORHINTS to a nonempty value. The colors are based on\n> -# the colored output of \"git status -sb\" and are available only when\n> -# using __git_ps1 for PROMPT_COMMAND or precmd in Bash,\n> -# but always available in Zsh.\n> +# the colored output of \"git status -sb\".\n> #\n> # If you would like __git_ps1 to do nothing in the case when the current\n> # directory is set up to be ignored by git, then set\n> @@ -259,12 +257,12 @@ __git_ps1_colorize_gitstring ()\n> \t\tlocal c_lblue='%F{blue}'\n> \t\tlocal c_clear='%f'\n> \telse\n> -\t\t# Using \\[ and \\] around colors is necessary to prevent\n> +\t\t# Using \\001 and \\002 around colors is necessary to prevent\n> \t\t# issues with command line editing/browsing/completion!\n> -\t\tlocal c_red='\\[\\e[31m\\]'\n> -\t\tlocal c_green='\\[\\e[32m\\]'\n> -\t\tlocal c_lblue='\\[\\e[1;34m\\]'\n> -\t\tlocal c_clear='\\[\\e[0m\\]'\n> +\t\tlocal c_red=$'\\001\\e[31m\\002'\n> +\t\tlocal c_green=$'\\001\\e[32m\\002'\n> +\t\tlocal c_lblue=$'\\001\\e[1;34m\\002'\n> +\t\tlocal c_clear=$'\\001\\e[0m\\002'\n> \tfi\n> \tlocal bad_color=$c_red\n> \tlocal ok_color=$c_green\n> @@ -574,11 +572,8 @@ __git_ps1 ()\n> \t\tb=\"\\${__git_ps1_branch_name}\"\n> \tfi\n> \n> -\t# NO color option unless in PROMPT_COMMAND mode or it's Zsh\n> \tif [ -n \"${GIT_PS1_SHOWCOLORHINTS-}\" ]; then\n> -\t\tif [ $pcmode = yes ] || [ -n \"${ZSH_VERSION-}\" ]; then\n> -\t\t\t__git_ps1_colorize_gitstring\n> -\t\tfi\n> +\t\t__git_ps1_colorize_gitstring\n> \tfi\n> \n> \tlocal f=\"$h$w$i$s$u$p\"\n> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n> index d459fae655..d667dda654 100755\n> --- a/t/t9903-bash-prompt.sh\n> +++ b/t/t9903-bash-prompt.sh\n> @@ -13,10 +13,10 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> . \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n> \n> actual=\"$TRASH_DIRECTORY/actual\"\n> -c_red='\\\\[\\\\e[31m\\\\]'\n> -c_green='\\\\[\\\\e[32m\\\\]'\n> -c_lblue='\\\\[\\\\e[1;34m\\\\]'\n> -c_clear='\\\\[\\\\e[0m\\\\]'\n> +c_red='\\001\\e[31m\\002'\n> +c_green='\\001\\e[32m\\002'\n> +c_lblue='\\001\\e[1;34m\\002'\n> +c_clear='\\001\\e[0m\\002'\n> \n> test_expect_success 'setup for prompt tests' '\n> \tgit init otherrepo &&\n"},{"id":"472899","messageId":"CAMP44s23cvp-YWDN7vzsKQCcWoc43PgURq+J6pwtx0rxOpZLBg@mail.gmail.com","threadId":"59323","inReplyTo":"xmqq4jr4nu3u.fsf@gitster.g","subject":"Re: [PATCH try2] completion: prompt: use generic colors","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-01T20:27:18Z","receivedAt":"2023-03-01T20:27:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Mar 1, 2023 at 1:34 PM Junio C Hamano wrote:\n> Felipe Contreras writes:\n\n> > We don't need bash to do this conversion, we can use \\001 \\002\n> > ourselves, and then the prompt command mode is not necessary to display\n> > colors.\n\n> Comments from those who use colored prompt and who are familiar with\n> the mechansim used to implement this? As I do not use the feature\n> at all and haven't been following it, seeing independent support\n> would help the topic.\n\nAt least in try1 Justin Donnelly reported success [1].\n\nAnd you don't need to use the feature to check that this patch\nobviously works, it's easy to test. All you need to see the original\nproblem is a short (as in width) terminal, or a long enough typed line\n(I type a lot of d's until surpassing the width).\n\nThis prompt should easily show the original problem:\n\nPS1='\\w (\\e[31mfoooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo\\e[m)\n> '\n\nWith the marks proposed by the patch, the problem is gone:\n\nPS1='\\w (\\001\\e[31m\\002foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo\\001\\e[m\\002)\n> '\n\nOf course, in order to test, PROMPT_COMMAND shouldn't override PS1.\n\nCheers.\n\n[1] https://lore.kernel.org/git/CAGTqyRzZ-cOp1C4f30fGFhjH1hh5U137=77pEHp_bmBzNcmTCw@mail.gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"473586","messageId":"9e317aa7-0037-7076-e159-3a70c581ebc1@online.no","threadId":"59323","inReplyTo":"CAMP44s23cvp-YWDN7vzsKQCcWoc43PgURq+J6pwtx0rxOpZLBg@mail.gmail.com","subject":"Re: [PATCH try2] completion: prompt: use generic colors","fromName":"Joakim Petersen","fromEmail":"joak-pet@online.no","sentAt":"2023-03-16T14:11:06Z","receivedAt":"2023-03-16T14:19:14Z","isPatch":true,"sender":{"key":"joak-pet@online.no","avatar":"https://avatars.githubusercontent.com/u/107927072?v=4"},"body":"On 01/03/2023 21:27, Felipe Contreras wrote:\n>>> We don't need bash to do this conversion, we can use \\001 \\002\n>>> ourselves, and then the prompt command mode is not necessary to display\n>>> colors.\n> \n>> Comments from those who use colored prompt and who are familiar with\n>> the mechansim used to implement this? As I do not use the feature\n>> at all and haven't been following it, seeing independent support\n>> would help the topic.\n> \n> At least in try1 Justin Donnelly reported success [1].\n\nI've played around a bit with this patch, and it seems to work as \nadvertised.\n\n"},{"id":"473616","messageId":"xmqq5yb0i9nz.fsf@gitster.g","threadId":"59323","inReplyTo":"9e317aa7-0037-7076-e159-3a70c581ebc1@online.no","subject":"Re: [PATCH try2] completion: prompt: use generic colors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T23:00:32Z","receivedAt":"2023-03-16T23:00:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joakim Petersen writes:\n\n> I've played around a bit with this patch, and it seems to work as\n> advertised.\n\nAs the change itself is so trivial and likely to exhibit problem\nimmediatly if it were not right (e.g. for some version of the shell\nbut not for others), let's queue it.\n\nThanks.\n"},{"id":"473618","messageId":"CAMP44s3eVcfaeQC-BB+F4FE1e7m=fFj6F3pSP-qrb9uPX9zT1A@mail.gmail.com","threadId":"59323","inReplyTo":"xmqq5yb0i9nz.fsf@gitster.g","subject":"Re: [PATCH try2] completion: prompt: use generic colors","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-16T23:21:08Z","receivedAt":"2023-03-16T23:21:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Mar 16, 2023 at 5:00 PM Junio C Hamano wrote:\n>\n> Joakim Petersen writes:\n>\n> > I've played around a bit with this patch, and it seems to work as\n> > advertised.\n>\n> As the change itself is so trivial and likely to exhibit problem\n> immediatly if it were not right (e.g. for some version of the shell\n> but not for others), let's queue it.\n\nFor the record, it's not even the shell, it's the readline library,\nand those markers are present since the initial import in 2011:\n\nhttps://git.savannah.gnu.org/cgit/readline.git/tree/readline.h?id=06cd36cdc90634c88636a6d09230c573078ead0e#n282\n\n#define RL_PROMPT_START_IGNORE '\\001'\n#define RL_PROMPT_END_IGNORE '\\002'\n\n-- \nFelipe Contreras\n"}]}