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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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...