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

From: Justin Donnelly <justinrdonnelly@gmail.com>
Date: Feb 27, 2022, 00:32 UTC
Message-ID: <CAGTqyRwTEdwut4HKD2=MaBfG_tZqN_TjGPAjChzmjBubC0-wuQ@mail.gmail.com>
In-Reply-To: <220225.86czjb874f.gmgdl@evledraar.gmail.com>

Thanks for the feedback. Comments interleaved below.

On Fri, Feb 25, 2022 at 7:26 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 33 quoted lines

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

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.

Show 20 quoted lines

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

Show 7 quoted lines

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