# [PATCH] git-prompt: preserve command exit status 9 messages from 2014-12-22 to 2015-01-14. Participants: Tony Finch, Junio C Hamano, SZEDER Gábor. Thread: https://gitlist.dev/t/38225 ## Tony Finch, 2014-12-22 14:30 Subject: [PATCH] git-prompt: preserve command exit status Message-ID: URL: https://gitlist.dev/e/alpine.LSU.2.00.1412221429490.28934%40hermes-1.csi.cam.ac.uk ``` Signed-off-by: Tony Finch --- contrib/completion/git-prompt.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index c5473dc..5fe69d0 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -288,6 +288,7 @@ __git_eread () # In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true __git_ps1 () { + local exit=$? local pcmode=no local detached=no local ps1pc_start='\u@\h:\w ' @@ -511,4 +512,7 @@ __git_ps1 () else printf -- "$printf_format" "$gitstring" fi + + # preserve exit status + return $exit } -- 2.1.0.rc1.12.g1e9b79d ``` ## Junio C Hamano, 2014-12-22 17:19 Subject: Re: [PATCH] git-prompt: preserve command exit status Message-ID: URL: https://gitlist.dev/e/xmqqa92fbo0j.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Tony Finch writes: > Signed-off-by: Tony Finch > --- > contrib/completion/git-prompt.sh | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh > index c5473dc..5fe69d0 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -288,6 +288,7 @@ __git_eread () > # In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true > __git_ps1 () > { > + local exit=$? > local pcmode=no > local detached=no > local ps1pc_start='\u@\h:\w ' > @@ -511,4 +512,7 @@ __git_ps1 () > else > printf -- "$printf_format" "$gitstring" > fi > + > + # preserve exit status > + return $exit > } Hmmmm. I thought "The patch trivially makes sense! Why didn't anybody notice this before?!?", but then noticed that I never suffered from an obvious consequence from the current lack of the exit-code-preserving: : gitster git.git/master; echo "<$PS1>" <: \h \W$(__git_ps1 "/%s"); > : gitster git.git/master; false : gitster git.git/master; echo $? 1 And it does not seem that it is needed, at least for my use pattern: : gitster git.git/master; ps1func () { echo "What Now: "; exit 8; } : gitster git.git/master; PS1='$(ps1func)' What Now: echo $? 0 What Now: (exit 6) What Now: echo $? 6 Also it does not seem that it is needed for the other style to use PROMPT_COMMAND: : gitster git.git/master; sh $ . contrib/completion/git-prompt.sh $ PROMPT_COMMAND='__git_ps1 "\h \W" "; "' gitster git.git (master); (exit 13) gitster git.git (master); echo $? 13 gitster git.git (master); true gitster git.git (master); echo $? 0 So, what are you fixing? In other words, please describe how it fails in the log message. Puzzled... ``` ## Tony Finch, 2014-12-22 18:09 Subject: [PATCH v2] git-prompt: preserve value of $? inside shell prompt Message-ID: URL: https://gitlist.dev/e/alpine.LSU.2.00.1412221808110.2546%40hermes-1.csi.cam.ac.uk In-Reply-To: ``` If you have a prompt which displays the command exit status, __git_ps1 without this change corrupts it, although it has the correct value in the parent shell: ~/src/git (master) 0 $ set | grep ^PS1 PS1='\w$(__git_ps1) $? \$ ' ~/src/git (master) 0 $ false ~/src/git (master) 0 $ echo $? 1 ~/src/git (master) 0 $ There is a slightly ugly workaround: ~/src/git (master) 0 $ set | grep ^PS1 PS1='\w$(x=$?; __git_ps1; exit $x) $? \$ ' ~/src/git (master) 0 $ false ~/src/git (master) 1 $ This change makes the workaround unnecessary. Signed-off-by: Tony Finch --- contrib/completion/git-prompt.sh | 4 ++++ 1 file changed, 4 insertions(+) I hope that explains it properly :-) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index c5473dc..5fe69d0 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -288,6 +288,7 @@ __git_eread () # In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true __git_ps1 () { + local exit=$? local pcmode=no local detached=no local ps1pc_start='\u@\h:\w ' @@ -511,4 +512,7 @@ __git_ps1 () else printf -- "$printf_format" "$gitstring" fi + + # preserve exit status + return $exit } -- 2.2.1.68.g56d9796 ``` ## Junio C Hamano, 2014-12-22 19:58 Subject: Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt Message-ID: URL: https://gitlist.dev/e/xmqqsig78nim.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Tony Finch writes: > If you have a prompt which displays the command exit status, > __git_ps1 without this change corrupts it, although it has > the correct value in the parent shell: > > ~/src/git (master) 0 $ set | grep ^PS1 > PS1='\w$(__git_ps1) $? \$ ' > ~/src/git (master) 0 $ false > ~/src/git (master) 0 $ echo $? > 1 > ~/src/git (master) 0 $ > > There is a slightly ugly workaround: > > ~/src/git (master) 0 $ set | grep ^PS1 > PS1='\w$(x=$?; __git_ps1; exit $x) $? \$ ' > ~/src/git (master) 0 $ false > ~/src/git (master) 1 $ > > This change makes the workaround unnecessary. > > Signed-off-by: Tony Finch > --- > contrib/completion/git-prompt.sh | 4 ++++ > 1 file changed, 4 insertions(+) > > I hope that explains it properly :-) Yes. I wouldn't have spent 20 minutes experimenting with various hypothetical use cases if the above were there in the first place. Thanks. Will queue. > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh > index c5473dc..5fe69d0 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -288,6 +288,7 @@ __git_eread () > # In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true > __git_ps1 () > { > + local exit=$? > local pcmode=no > local detached=no > local ps1pc_start='\u@\h:\w ' > @@ -511,4 +512,7 @@ __git_ps1 () > else > printf -- "$printf_format" "$gitstring" > fi > + > + # preserve exit status > + return $exit > } ``` ## Tony Finch, 2014-12-22 22:18 Subject: Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt Message-ID: URL: https://gitlist.dev/e/alpine.LSU.2.00.1412222216120.26823%40hermes-1.csi.cam.ac.uk In-Reply-To: ``` Junio C Hamano wrote: > > Yes. I wouldn't have spent 20 minutes experimenting with various > hypothetical use cases if the above were there in the first place. Sorry for wasting your time, and thanks for reviewing the patch. (I am so used to having $? in my prompt it took me ages to find and explain the problem too... Sigh!) Tony. -- f.anthony.n.finch http://dotat.at/ Trafalgar: Easterly 6 to gale 8 far southeast, otherwise northeasterly veering southeasterly 4 or 5. Slight or moderate, occasionally rough at first in south. Occasional drizzle. Good, occasionally moderate. ``` ## SZEDER Gábor, 2015-01-13 23:57 Subject: Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt Message-ID: <20150114005726.Horde.idyLC0Or9SvaghEN_N_pRg1@webmail.informatik.kit.edu> URL: https://gitlist.dev/e/20150114005726.Horde.idyLC0Or9SvaghEN_N_pRg1%40webmail.informatik.kit.edu In-Reply-To: ``` Hi, Quoting Tony Finch : > If you have a prompt which displays the command exit status, > __git_ps1 without this change corrupts it, although it has > the correct value in the parent shell: > > ~/src/git (master) 0 $ set | grep ^PS1 > PS1='\w$(__git_ps1) $? \$ ' > ~/src/git (master) 0 $ false > ~/src/git (master) 0 $ echo $? > 1 > ~/src/git (master) 0 $ > > There is a slightly ugly workaround: > > ~/src/git (master) 0 $ set | grep ^PS1 > PS1='\w$(x=$?; __git_ps1; exit $x) $? \$ ' > ~/src/git (master) 0 $ false > ~/src/git (master) 1 $ > > This change makes the workaround unnecessary. > > Signed-off-by: Tony Finch > --- > contrib/completion/git-prompt.sh | 4 ++++ > 1 file changed, 4 insertions(+) > > I hope that explains it properly :-) > > diff --git a/contrib/completion/git-prompt.sh > b/contrib/completion/git-prompt.sh > index c5473dc..5fe69d0 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -288,6 +288,7 @@ __git_eread () > # In this mode you can request colored hints using > GIT_PS1_SHOWCOLORHINTS=true > __git_ps1 () > { > + local exit=$? > local pcmode=no > local detached=no > local ps1pc_start='\u@\h:\w ' > @@ -511,4 +512,7 @@ __git_ps1 () > else > printf -- "$printf_format" "$gitstring" > fi > + > + # preserve exit status > + return $exit > } > -- > 2.2.1.68.g56d9796 Makes sense, but the patch doesn't cover all cases, because __git_ps1() can exit early, off the top of my head without actually having a git clone at hand to look at the code, if: * pwd is not in a git repo, which is a quite common case to worry about. * .git/HEAD becomes unreadable while __git_ps1() is being executed. It's an unlikely race condition so I wouldn't worry much about it, but for consistency's sake I think it's better to return $? there as well. Best, Gábor ``` ## Tony Finch, 2015-01-14 10:05 Subject: Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt Message-ID: URL: https://gitlist.dev/e/alpine.LSU.2.00.1501141003400.23307%40hermes-1.csi.cam.ac.uk In-Reply-To: <20150114005726.Horde.idyLC0Or9SvaghEN_N_pRg1@webmail.informatik.kit.edu> ``` SZEDER Gábor wrote: > > Makes sense, but the patch doesn't cover all cases, because > __git_ps1() can exit early Thanks for looking at the patch. I feel quite silly for missing the other return points :-( Follow-up patch on the way... Tony. -- f.anthony.n.finch http://dotat.at/ Faeroes, Southeast Iceland: Cyclonic for a time in Faeroes, otherwise northeasterly 6 to gale 8, increasing gale 8 to storm 10. Very rough or high. Rain, snow or wintry showers. Moderate or poor, occasionally very poor. ``` ## Tony Finch, 2015-01-14 10:06 Subject: [PATCH] git-prompt: preserve value of $? in all cases Message-ID: URL: https://gitlist.dev/e/alpine.LSU.2.00.1501141005560.23307%40hermes-1.csi.cam.ac.uk In-Reply-To: <20150114005726.Horde.idyLC0Or9SvaghEN_N_pRg1@webmail.informatik.kit.edu> ``` Signed-off-by: Tony Finch --- contrib/completion/git-prompt.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 3c3fc6d..3e70e74 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -288,6 +288,7 @@ __git_eread () # In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true __git_ps1 () { + # preserve exit status local exit=$? local pcmode=no local detached=no @@ -303,7 +304,7 @@ __git_ps1 () ;; 0|1) printf_format="${1:-$printf_format}" ;; - *) return + *) return $exit ;; esac @@ -355,7 +356,7 @@ __git_ps1 () #In PC mode PS1 always needs to be set PS1="$ps1pc_start$ps1pc_end" fi - return + return $exit fi local short_sha @@ -416,7 +417,7 @@ __git_ps1 () if [ $pcmode = yes ]; then PS1="$ps1pc_start$ps1pc_end" fi - return + return $exit fi # is it a symbolic ref? b="${head#ref: }" @@ -513,6 +514,5 @@ __git_ps1 () printf -- "$printf_format" "$gitstring" fi - # preserve exit status return $exit } -- 2.2.1.68.g56d9796 ``` ## SZEDER Gábor, 2015-01-14 12:10 Subject: Re: [PATCH] git-prompt: preserve value of $? in all cases Message-ID: <20150114131006.Horde._hnEBDLPm_RUjO-IJlS9dw1@webmail.informatik.kit.edu> URL: https://gitlist.dev/e/20150114131006.Horde._hnEBDLPm_RUjO-IJlS9dw1%40webmail.informatik.kit.edu In-Reply-To: ``` Hi, Quoting Tony Finch : > Signed-off-by: Tony Finch > --- > contrib/completion/git-prompt.sh | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/contrib/completion/git-prompt.sh > b/contrib/completion/git-prompt.sh > index 3c3fc6d..3e70e74 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -288,6 +288,7 @@ __git_eread () > # In this mode you can request colored hints using > GIT_PS1_SHOWCOLORHINTS=true > __git_ps1 () > { > + # preserve exit status > local exit=$? > local pcmode=no > local detached=no > @@ -303,7 +304,7 @@ __git_ps1 () > ;; > 0|1) printf_format="${1:-$printf_format}" > ;; > - *) return > + *) return $exit > ;; > esac > > @@ -355,7 +356,7 @@ __git_ps1 () > #In PC mode PS1 always needs to be set > PS1="$ps1pc_start$ps1pc_end" > fi > - return > + return $exit > fi > > local short_sha > @@ -416,7 +417,7 @@ __git_ps1 () > if [ $pcmode = yes ]; then > PS1="$ps1pc_start$ps1pc_end" > fi > - return > + return $exit > fi > # is it a symbolic ref? > b="${head#ref: }" > @@ -513,6 +514,5 @@ __git_ps1 () > printf -- "$printf_format" "$gitstring" > fi > > - # preserve exit status > return $exit > } > -- > 2.2.1.68.g56d9796 Thanks for the quick turnaround, looks good to me. I didn't remember the early return in the second hunk. I wonder whether we could test this behavior... but how could we set $? and pass it to __git_ps1()? Junio, as far as I can judge from the last What's cooking and the relevant patch emails on Gmane, this patch will have a textual conflict with the first patch in 'rh/hide-prompt-in-ignored-directory'. While the conflict is trivial (maybe git would even be able to resolve it by itself?), the second patch in that series adds yet another early return to __git_ps1(). Please be sure to add 'return $exit' when merging. Best, Gábor ```