mbox series

[0/4] In PS1 prompt, make upstream state indicators consistent with other state indicators

Message ID pull.1162.git.1645789446.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series In PS1 prompt, make upstream state indicators consistent with other state indicators | expand

Message

Philippe Blain via GitGitGadget Feb. 25, 2022, 11:44 a.m. UTC 
These patches are about the characters and words that can be configured to
display in the PS1 prompt after the branch name. I've been unable to find a
consistent terminology. I refer to them as follows: [short | long] [type]
state indicator where short is for characters (e.g. ?), long is for words
(e.g. |SPARSE), and type is the type of indicator (e.g. sparse or upstream).
I'd be happy to change the commit messages to a different terminology if
that's preferred.

There are a few inconsistencies with the PS1 prompt upstream state indicator
(GIT_PS1_SHOWUPSTREAM).

 * With GIT_PS1_SHOWUPSTREAM="auto", if there are no other short state
   indicators (e.g. + for staged changes, $ for stashed changes, etc.), the
   upstream state indicator appears adjacent to the branch name (e.g.
   (main=)) instead of being separated by SP or GIT_PS1_STATESEPARATOR (e.g.
   (main =)).
 * If there are long state indicators (e.g. |SPARSE), a short upstream state
   indicator (i.e. GIT_PS1_SHOWUPSTREAM="auto") is to the right of the long
   state indicator (e.g. (main +|SPARSE=)) instead of with the other short
   state indicators (e.g. (main +=|SPARSE)).
 * The long upstream state indicator (e.g. GIT_PS1_SHOWUPSTREAM="verbose")
   is separated from other (short or long) state indicators by a hard-coded
   SP. Other long state indicators are separated by a hard-coded pipe (|).

These patches are to make the upstream state indicators more consistent with
other state indicators.

Justin Donnelly (4):
  git-prompt: rename `upstream` to `upstream_type`
  git-prompt: make upstream state indicator location consistent
  git-prompt: make long upstream state indicator consistent
  git-prompt: put upstream comments together

 contrib/completion/git-prompt.sh | 59 ++++++++++++++++----------------
 1 file changed, 30 insertions(+), 29 deletions(-)


base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1162%2Fjustinrdonnelly%2Fgit-prompt-upstream-consistency-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1162/justinrdonnelly/git-prompt-upstream-consistency-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1162

Comments

Ævar Arnfjörð Bjarmason Feb. 25, 2022, 12:22 p.m. UTC | #1 
On Fri, Feb 25 2022, Justin Donnelly via GitGitGadget wrote:

I couldn't find any glaring issues here on a quick review, just a note.

> These patches are about the characters and words that can be configured to
> display in the PS1 prompt after the branch name. I've been unable to find a
> consistent terminology. I refer to them as follows: [short | long] [type]
> state indicator where short is for characters (e.g. ?), long is for words
> (e.g. |SPARSE), and type is the type of indicator (e.g. sparse or upstream).
> I'd be happy to change the commit messages to a different terminology if
> that's preferred.

I think that terminology is correct, in case you haven't seen it
git-for-each-ref(1) talks about the "short" here as "short",
"trackshort" etc.

> There are a few inconsistencies with the PS1 prompt upstream state indicator
> (GIT_PS1_SHOWUPSTREAM).
>
>  * With GIT_PS1_SHOWUPSTREAM="auto", if there are no other short state
>    indicators (e.g. + for staged changes, $ for stashed changes, etc.), the
>    upstream state indicator appears adjacent to the branch name (e.g.
>    (main=)) instead of being separated by SP or GIT_PS1_STATESEPARATOR (e.g.
>    (main =)).
>  * If there are long state indicators (e.g. |SPARSE), a short upstream state
>    indicator (i.e. GIT_PS1_SHOWUPSTREAM="auto") is to the right of the long
>    state indicator (e.g. (main +|SPARSE=)) instead of with the other short
>    state indicators (e.g. (main +=|SPARSE)).

I think it would really help to in each commit message have a
before/after comparison of the relevant PS1 output that's being changed.

I'm not sure how to readthis example. So before we said "main +=|SPARSE"
but now we'll say "main +|SPARSE=", but without sparse we'll say
"main="?

Aren't both of those harder to read than they need to be, shouldn't it
be closer to:

    main= |SPARSE

Or:

    main= |+SPARSE

Or:

    main= +|SPARSE

I can't recall what the "+" there is (if any).

I.e. the "=" refers to the ahead/behind state of "main, it seems odd in
both versions of your example that we're splitting it off from "main"
because we have "SPARSE" too.

But maybe I'm missing something...
Justin Donnelly Feb. 27, 2022, 12:32 a.m. UTC | #2 
Thanks for the feedback. Comments interleaved below.

On Fri, Feb 25, 2022 at 7:26 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Fri, Feb 25 2022, Justin Donnelly via GitGitGadget wrote:
>
> I couldn't find any glaring issues here on a quick review, just a note.
>
> > These patches are about the characters and words that can be configured to
> > display in the PS1 prompt after the branch name. I've been unable to find a
> > consistent terminology. I refer to them as follows: [short | long] [type]
> > state indicator where short is for characters (e.g. ?), long is for words
> > (e.g. |SPARSE), and type is the type of indicator (e.g. sparse or upstream).
> > I'd be happy to change the commit messages to a different terminology if
> > that's preferred.
>
> I think that terminology is correct, in case you haven't seen it
> git-for-each-ref(1) talks about the "short" here as "short",
> "trackshort" etc.
>
> > There are a few inconsistencies with the PS1 prompt upstream state indicator
> > (GIT_PS1_SHOWUPSTREAM).
> >
> >  * With GIT_PS1_SHOWUPSTREAM="auto", if there are no other short state
> >    indicators (e.g. + for staged changes, $ for stashed changes, etc.), the
> >    upstream state indicator appears adjacent to the branch name (e.g.
> >    (main=)) instead of being separated by SP or GIT_PS1_STATESEPARATOR (e.g.
> >    (main =)).
> >  * If there are long state indicators (e.g. |SPARSE), a short upstream state
> >    indicator (i.e. GIT_PS1_SHOWUPSTREAM="auto") is to the right of the long
> >    state indicator (e.g. (main +|SPARSE=)) instead of with the other short
> >    state indicators (e.g. (main +=|SPARSE)).
>
> I think it would really help to in each commit message have a
> before/after comparison of the relevant PS1 output that's being changed.

I agree that a before/after comparison would probably make it easier
to understand. Maybe some examples without upstream (for a baseline to
compare against) and a table that shows before/after for upstream.

`__git_ps1` examples without upstream:
(main)
(main %)
(main *%)
(main|SPARSE)
(main %|SPARSE)
(main *%|SPARSE)
(main|SPARSE|REBASE 1/2)
(main %|SPARSE|REBASE 1/2)

Of note:
1. If there are short state indicators, they appear together after the
branch name and separated from it by `SP` or `GIT_PS1_STATESEPARATOR`.
2. If there are long state indicators, they appear after short state
indicators if there are any, or after the branch name if there are no
short state indicators. Each long state indicator begins with a pipe
(`|`) as a separator.

Patch 2 before/after:
| Before           | After            |
| ---------------- | ---------------- |
| (main=)          | (main =)         |
| (main|SPARSE=)   | (main =|SPARSE)  |
| (main %|SPARSE=) | (main %=|SPARSE) |

Patch 3 before/after:
| Before                          | After                           |
| ------------------------------- | ------------------------------- |
| (main u=)                       | (main|u=)                       |
| (main u= origin/main)           | (main|u= origin/main)           |
| (main u+1)                      | (main|u+1)                      |
| (main u+1 origin/main)          | (main|u+1 origin/main)          |
| (main % u=)                     | (main %|u=)                     |
| (main % u= origin/main)         | (main %|u= origin/main)         |
| (main % u+1)                    | (main %|u+1)                    |
| (main % u+1 origin/main)        | (main %|u+1 origin/main)        |
| (main|SPARSE u=)                | (main|SPARSE|u=)                |
| (main|SPARSE u= origin/main)    | (main|SPARSE|u= origin/main)    |
| (main|SPARSE u+1)               | (main|SPARSE|u+1)               |
| (main|SPARSE u+1 origin/main)   | (main|SPARSE|u+1 origin/main)   |
| (main %|SPARSE u=)              | (main %|SPARSE|u=)              |
| (main %|SPARSE u= origin/main)  | (main %|SPARSE|u= origin/main)  |
| (main %|SPARSE u+1)             | (main %|SPARSE|u+1)             |
| (main %|SPARSE u+1 origin/main) | (main %|SPARSE|u+1 origin/main) |

Note: These tables are inspired by [Markdown Guide extended
syntax](https://www.markdownguide.org/extended-syntax/#tables), but I
didn't wrap the PS1 prompt text in backticks or escape the pipe
because I thought that would make it more confusing. In short, they're
meant to be viewed as (monospaced font) text, not Markdown.

>
>
> I'm not sure how to readthis example. So before we said "main +=|SPARSE"
> but now we'll say "main +|SPARSE=", but without sparse we'll say
> "main="?
>
> Aren't both of those harder to read than they need to be, shouldn't it
> be closer to:
>
>     main= |SPARSE
>
> Or:
>
>     main= |+SPARSE
>
> Or:
>
>     main= +|SPARSE
>
> I can't recall what the "+" there is (if any).


`+` is for staged changes (if `GIT_PS1_SHOWDIRTYSTATE` is a nonempty
value). So it's not directly related to upstream, but the addition of
another short state indicator changes things.

>
>
> I.e. the "=" refers to the ahead/behind state of "main, it seems odd in
> both versions of your example that we're splitting it off from "main"
> because we have "SPARSE" too.
>
> But maybe I'm missing something...
Ævar Arnfjörð Bjarmason Feb. 27, 2022, 10:19 a.m. UTC | #3 
On Sat, Feb 26 2022, Justin Donnelly wrote:

> Thanks for the feedback. Comments interleaved below.
>
> On Fri, Feb 25, 2022 at 7:26 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Fri, Feb 25 2022, Justin Donnelly via GitGitGadget wrote:
>>
>> I couldn't find any glaring issues here on a quick review, just a note.
>>
>> > These patches are about the characters and words that can be configured to
>> > display in the PS1 prompt after the branch name. I've been unable to find a
>> > consistent terminology. I refer to them as follows: [short | long] [type]
>> > state indicator where short is for characters (e.g. ?), long is for words
>> > (e.g. |SPARSE), and type is the type of indicator (e.g. sparse or upstream).
>> > I'd be happy to change the commit messages to a different terminology if
>> > that's preferred.
>>
>> I think that terminology is correct, in case you haven't seen it
>> git-for-each-ref(1) talks about the "short" here as "short",
>> "trackshort" etc.
>>
>> > There are a few inconsistencies with the PS1 prompt upstream state indicator
>> > (GIT_PS1_SHOWUPSTREAM).
>> >
>> >  * With GIT_PS1_SHOWUPSTREAM="auto", if there are no other short state
>> >    indicators (e.g. + for staged changes, $ for stashed changes, etc.), the
>> >    upstream state indicator appears adjacent to the branch name (e.g.
>> >    (main=)) instead of being separated by SP or GIT_PS1_STATESEPARATOR (e.g.
>> >    (main =)).
>> >  * If there are long state indicators (e.g. |SPARSE), a short upstream state
>> >    indicator (i.e. GIT_PS1_SHOWUPSTREAM="auto") is to the right of the long
>> >    state indicator (e.g. (main +|SPARSE=)) instead of with the other short
>> >    state indicators (e.g. (main +=|SPARSE)).
>>
>> I think it would really help to in each commit message have a
>> before/after comparison of the relevant PS1 output that's being changed.
>
> I agree that a before/after comparison would probably make it easier
> to understand. Maybe some examples without upstream (for a baseline to
> compare against) and a table that shows before/after for upstream.
>
> `__git_ps1` examples without upstream:
> (main)
> (main %)
> (main *%)
> (main|SPARSE)
> (main %|SPARSE)
> (main *%|SPARSE)
> (main|SPARSE|REBASE 1/2)
> (main %|SPARSE|REBASE 1/2)
>
> Of note:
> 1. If there are short state indicators, they appear together after the
> branch name and separated from it by `SP` or `GIT_PS1_STATESEPARATOR`.
> 2. If there are long state indicators, they appear after short state
> indicators if there are any, or after the branch name if there are no
> short state indicators. Each long state indicator begins with a pipe
> (`|`) as a separator.
>
> Patch 2 before/after:
> | Before           | After            |
> | ---------------- | ---------------- |
> | (main=)          | (main =)         |
> | (main|SPARSE=)   | (main =|SPARSE)  |
> | (main %|SPARSE=) | (main %=|SPARSE) |
>
> Patch 3 before/after:
> | Before                          | After                           |
> | ------------------------------- | ------------------------------- |
> | (main u=)                       | (main|u=)                       |
> | (main u= origin/main)           | (main|u= origin/main)           |
> | (main u+1)                      | (main|u+1)                      |
> | (main u+1 origin/main)          | (main|u+1 origin/main)          |
> | (main % u=)                     | (main %|u=)                     |
> | (main % u= origin/main)         | (main %|u= origin/main)         |
> | (main % u+1)                    | (main %|u+1)                    |
> | (main % u+1 origin/main)        | (main %|u+1 origin/main)        |
> | (main|SPARSE u=)                | (main|SPARSE|u=)                |
> | (main|SPARSE u= origin/main)    | (main|SPARSE|u= origin/main)    |
> | (main|SPARSE u+1)               | (main|SPARSE|u+1)               |
> | (main|SPARSE u+1 origin/main)   | (main|SPARSE|u+1 origin/main)   |
> | (main %|SPARSE u=)              | (main %|SPARSE|u=)              |
> | (main %|SPARSE u= origin/main)  | (main %|SPARSE|u= origin/main)  |
> | (main %|SPARSE u+1)             | (main %|SPARSE|u+1)             |
> | (main %|SPARSE u+1 origin/main) | (main %|SPARSE|u+1 origin/main) |
>
> Note: These tables are inspired by [Markdown Guide extended
> syntax](https://www.markdownguide.org/extended-syntax/#tables), but I
> didn't wrap the PS1 prompt text in backticks or escape the pipe
> because I thought that would make it more confusing. In short, they're
> meant to be viewed as (monospaced font) text, not Markdown.

Thanks. These comparisons are really useful & would be nice to work into
relevant commit messages in a re-roll.

I withdraw any suggestions about making this "main|SPARSE|u=" instead of
"main=|SPARSE|u" or whatever. I think such a thing might still make
sense, but it's clearly unrelated to the improvements you're making
here.

>>
>>
>> I'm not sure how to readthis example. So before we said "main +=|SPARSE"
>> but now we'll say "main +|SPARSE=", but without sparse we'll say
>> "main="?
>>
>> Aren't both of those harder to read than they need to be, shouldn't it
>> be closer to:
>>
>>     main= |SPARSE
>>
>> Or:
>>
>>     main= |+SPARSE
>>
>> Or:
>>
>>     main= +|SPARSE
>>
>> I can't recall what the "+" there is (if any).
>
>
> `+` is for staged changes (if `GIT_PS1_SHOWDIRTYSTATE` is a nonempty
> value). So it's not directly related to upstream, but the addition of
> another short state indicator changes things.

Thanks!

>>
>>
>> I.e. the "=" refers to the ahead/behind state of "main, it seems odd in
>> both versions of your example that we're splitting it off from "main"
>> because we have "SPARSE" too.
>>
>> But maybe I'm missing something...