Thanks for the feedback. Comments interleaved below. On Fri, Feb 25, 2022 at 7:26 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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...
