Message ID | 3a41ad889cc33a1fc0414b8f14af6438b49c88ee.1723886761.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Commit | b732e08671f037373f615a6d8509da2dbc476322 |
Headers | show |
Series | git-prompt: support more shells v3 | expand |
On Sat, Aug 17, 2024 at 5:26 AM Avi Halachmi (:avih) via GitGitGadget <gitgitgadget@gmail.com> wrote: > The issues which this commit fixes are unlikely to be broken > in real life, but the fixes improve correctness, and would prevent > bugs in some uncommon cases, such as weird IFS values. > > Listing some portability guideline here for future reference. s/guideline/guidelines/ > I'm leaving it to someone else to decide whether to include > it in the file itself, place is as a new file, or not. perhaps: s/is as/it as/ > "Simple command" (POSIX term) is assignment[s] and/or command [args]. > Examples: > foo=bar # one assignment > foo=$bar x=y # two assignments > foo bar # command, no assignments > x=123 foo bar # one assignment and a command > > The assignments part is not IFS-split or glob-expanded. > > The command+args part does get IFS field split and glob expanded, > but only at unquoted expanded/substituted parts. > > In the command+args part, expanded/substituted values must be quoted. > (the commands here are "[" and "local"): > Good: [ "$mode" = yes ]; local s="*" x="$y" e="$?" z="$(cmd ...)" > Bad: [ $mode = yes ]; local s=* x=$y e=$? z=$(cmd...) This new explanation in v3 is a helpful addition. > The arguments to "local" do look like assignments, but they're not > the assignment part of a simple command. they're at the command part. either: s/they're/They're/ or: s/. they're/; they're/ I doubt that any of the above extremely minor commit message botches is worth a reroll.
On Saturday, August 17, 2024 at 12:38:23 PM GMT+3, Eric Sunshine <sunshine@sunshineco.com> wrote: > On Sat, Aug 17, 2024 at 5:26 AM Avi Halachmi (:avih) via GitGitGadget >> Listing some portability guideline here for future reference. > >> s/guideline/guidelines/ Right. >> I'm leaving it to someone else to decide whether to include >> it in the file itself, place is as a new file, or not. > > perhaps: s/is as/it as/ Indeed. >> "Simple command" (POSIX term) is assignment[s] and/or command [args]. >> Examples: >> ... > > This new explanation in v3 is a helpful addition. Thanks. >> The arguments to "local" do look like assignments, but they're not >> the assignment part of a simple command. they're at the command part. > > either: s/they're/They're/ > or: s/. they're/; they're/ Indeed. Thanks for the comments. > I doubt that any of the above extremely minor commit message botches > is worth a reroll. I'm not the one to judge that, but I'm OK to push as many new versions as deemed required/preferred. For now I'm waiting for someone to say whether I should do that or not... and if yes - when. I was trying to wait few days for more comments on v2 (perhaps like yours), but I noticed that v2 was already was just integrated into "seen", so I posted v3 to address the existing comments on v2. This is my 1st patch to "git", so still finding my feet with the procedures. avih
avih <avihpit@yahoo.com> writes: > I was trying to wait few days for more comments on v2 (perhaps > like yours), but I noticed that v2 was already was just integrated > into "seen", so I posted v3 to address the existing comments on v2. Please consider that it is just like being on the list and nothing else to be in "seen". It merely is another place some patches I've "seen" are published, to help those of you who find "git fetch && git log -pW origin/master..origin/topic" a more convenient way to review the changes. This is outlined in the note I send out occasionally. https://lore.kernel.org/git/xmqqmslewwpo.fsf@gitster.g/ If you think that v2 needs a few more days' exposure to receive more feedback from reviewers, and that v3 might be incomplete before waiting for their feedback, just saying so as a response to the "What's cooking" message is a very effective way to make sure I'll wait for an updated iteration. Such a comment on individual topics is *not* limited to the author of the topic, e.g. https://lore.kernel.org/git/owlyil264yew.fsf@fine.c.googlers.com/ is an example (but the particular example was sent a bit too late---such a "wait, don't merge, an update is coming" needs to come before the topic is merged down to 'next'. https://lore.kernel.org/git/13f08ce5-f036-f769-1ba9-7d47b572af28@gmail.com/ is an example of a reviewer sending a "stop, I have an improvement to suggest on this one I'll send soon" on a topic by somebody else. https://lore.kernel.org/git/Zr2_4sNu56_frqlr@tanuki/ is an example of the author saying "this topic of mine should be complete already", which is a bit less usual and takes some finesse to avoid sounding too self-promoting (this particular example does so well).
On Saturday, August 17, 2024 at 07:28:44 PM GMT+3, Junio C Hamano <gitster@pobox.com> wrote: > avih <avihpit@yahoo.com> writes: >> I was trying to wait few days for more comments on v2 (perhaps >> like yours), but I noticed that v2 was already was just integrated >> into "seen", so I posted v3 to address the existing comments on v2. > > Please consider that it is just like being on the list and nothing > else to be in "seen". It merely is another place some patches I've > "seen" are published, to help those of you who find "git fetch && > git log -pW origin/master..origin/topic" a more convenient way to > review the changes. This is outlined in the note I send out > occasionally. > > https://lore.kernel.org/git/xmqqmslewwpo.fsf@gitster.g/ Thanks for the time and info. That's a useful intro, and I now do see it says this: until a topic is merged to "next", updates to it is expected by replacing the patch(es) in the topic with an improved version > If you think that v2 needs a few more days' exposure to receive more > feedback from reviewers, and that v3 might be incomplete before > waiting for their feedback, just saying so as a response to the > "What's cooking" message is a very effective way to make sure I'll > wait for an updated iteration. Such a comment on individual topics > is *not* limited to the author of the topic, e.g. > > https://lore.kernel.org/git/owlyil264yew.fsf@fine.c.googlers.com/ > > is an example ... Thanks. I replied few times that requested changes will be included in v3, so I thought it's apparent that v3 is sill to come, but in retrospect, when you replied to [PATCH v2 0/8]: > I've read the series and they looked all sensible. Will queue but > I'd appreciate a second set of eyes before marking it for 'next'. Then I should have replied with something along those lines: Please wait for v3 with requested non-critical changes (typos in comment and commit message) before moving it to "next", if at all. or even sending the same message once I noticed it was merged into "seen", instead of posting v3 out of "panic" that it boarded the train without all the requested changes applied, yes? If yes, then here's heads-up that there's still another non-critical requested change to arrive in v4 (commit message wording), which I'll send in few days, to allow for further comments to arrive. Because resending the whole series (is that "reroll"?) for every minor typo or wording change feels noisy and inappropriate to me. Thanks again for the time and info. avih
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 4781261f868..5d7f236fe48 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -246,7 +246,7 @@ __git_ps1_show_upstream () if [ -n "$count" ] && [ -n "$name" ]; then __git_ps1_upstream_name=$(git rev-parse \ --abbrev-ref "$upstream_type" 2>/dev/null) - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then + if [ "$pcmode" = yes ] && [ "$ps1_expanded" = yes ]; then upstream="$upstream \${__git_ps1_upstream_name}" else upstream="$upstream ${__git_ps1_upstream_name}" @@ -278,12 +278,12 @@ __git_ps1_colorize_gitstring () local c_lblue=$'\001\e[1;34m\002' local c_clear=$'\001\e[0m\002' fi - local bad_color=$c_red - local ok_color=$c_green + local bad_color="$c_red" + local ok_color="$c_green" local flags_color="$c_lblue" local branch_color="" - if [ $detached = no ]; then + if [ "$detached" = no ]; then branch_color="$ok_color" else branch_color="$bad_color" @@ -360,7 +360,7 @@ __git_sequencer_status () __git_ps1 () { # preserve exit status - local exit=$? + local exit="$?" local pcmode=no local detached=no local ps1pc_start='\u@\h:\w ' @@ -379,7 +379,7 @@ __git_ps1 () ;; 0|1) printf_format="${1:-$printf_format}" ;; - *) return $exit + *) return "$exit" ;; esac @@ -427,7 +427,7 @@ __git_ps1 () rev_parse_exit_code="$?" if [ -z "$repo_info" ]; then - return $exit + return "$exit" fi local short_sha="" @@ -449,7 +449,7 @@ __git_ps1 () [ "$(git config --bool bash.hideIfPwdIgnored)" != "false" ] && git check-ignore -q . then - return $exit + return "$exit" fi local sparse="" @@ -499,7 +499,7 @@ __git_ps1 () case "$ref_format" in files) if ! __git_eread "$g/HEAD" head; then - return $exit + return "$exit" fi case $head in @@ -597,10 +597,10 @@ __git_ps1 () fi fi - local z="${GIT_PS1_STATESEPARATOR-" "}" + local z="${GIT_PS1_STATESEPARATOR- }" b=${b##refs/heads/} - if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then + if [ "$pcmode" = yes ] && [ "$ps1_expanded" = yes ]; then __git_ps1_branch_name=$b b="\${__git_ps1_branch_name}" fi @@ -612,7 +612,7 @@ __git_ps1 () local f="$h$w$i$s$u$p" local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}" - if [ $pcmode = yes ]; then + if [ "$pcmode" = yes ]; then if [ "${__git_printf_supports_v-}" != yes ]; then gitstring=$(printf -- "$printf_format" "$gitstring") else @@ -623,5 +623,5 @@ __git_ps1 () printf -- "$printf_format" "$gitstring" fi - return $exit + return "$exit" }