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