* [PATCH] git-prompt: preserve command exit status @ 2014-12-22 14:30 Tony Finch 2014-12-22 17:19 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Tony Finch @ 2014-12-22 14:30 UTC (permalink / raw) To: git Signed-off-by: Tony Finch <dot@dotat.at> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] git-prompt: preserve command exit status 2014-12-22 14:30 [PATCH] git-prompt: preserve command exit status Tony Finch @ 2014-12-22 17:19 ` Junio C Hamano 2014-12-22 18:09 ` [PATCH v2] git-prompt: preserve value of $? inside shell prompt Tony Finch 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2014-12-22 17:19 UTC (permalink / raw) To: Tony Finch; +Cc: git Tony Finch <dot@dotat.at> writes: > Signed-off-by: Tony Finch <dot@dotat.at> > --- > 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... ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] git-prompt: preserve value of $? inside shell prompt 2014-12-22 17:19 ` Junio C Hamano @ 2014-12-22 18:09 ` Tony Finch 2014-12-22 19:58 ` Junio C Hamano 2015-01-13 23:57 ` SZEDER Gábor 0 siblings, 2 replies; 9+ messages in thread From: Tony Finch @ 2014-12-22 18:09 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 <dot@dotat.at> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt 2014-12-22 18:09 ` [PATCH v2] git-prompt: preserve value of $? inside shell prompt Tony Finch @ 2014-12-22 19:58 ` Junio C Hamano 2014-12-22 22:18 ` Tony Finch 2015-01-13 23:57 ` SZEDER Gábor 1 sibling, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2014-12-22 19:58 UTC (permalink / raw) To: Tony Finch; +Cc: git Tony Finch <dot@dotat.at> 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 <dot@dotat.at> > --- > 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 > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt 2014-12-22 19:58 ` Junio C Hamano @ 2014-12-22 22:18 ` Tony Finch 0 siblings, 0 replies; 9+ messages in thread From: Tony Finch @ 2014-12-22 22:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> 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 <dot@dotat.at> 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt 2014-12-22 18:09 ` [PATCH v2] git-prompt: preserve value of $? inside shell prompt Tony Finch 2014-12-22 19:58 ` Junio C Hamano @ 2015-01-13 23:57 ` SZEDER Gábor 2015-01-14 10:05 ` Tony Finch 2015-01-14 10:06 ` [PATCH] git-prompt: preserve value of $? in all cases Tony Finch 1 sibling, 2 replies; 9+ messages in thread From: SZEDER Gábor @ 2015-01-13 23:57 UTC (permalink / raw) To: Tony Finch; +Cc: git, Junio C Hamano Hi, Quoting Tony Finch <dot@dotat.at>: > 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 <dot@dotat.at> > --- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt 2015-01-13 23:57 ` SZEDER Gábor @ 2015-01-14 10:05 ` Tony Finch 2015-01-14 10:06 ` [PATCH] git-prompt: preserve value of $? in all cases Tony Finch 1 sibling, 0 replies; 9+ messages in thread From: Tony Finch @ 2015-01-14 10:05 UTC (permalink / raw) To: SZEDER Gábor; +Cc: git, Junio C Hamano [-- Attachment #1: Type: TEXT/PLAIN, Size: 552 bytes --] SZEDER Gábor <szeder@ira.uka.de> 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 <dot@dotat.at> 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] git-prompt: preserve value of $? in all cases 2015-01-13 23:57 ` SZEDER Gábor 2015-01-14 10:05 ` Tony Finch @ 2015-01-14 10:06 ` Tony Finch 2015-01-14 12:10 ` SZEDER Gábor 1 sibling, 1 reply; 9+ messages in thread From: Tony Finch @ 2015-01-14 10:06 UTC (permalink / raw) To: git; +Cc: SZEDER Gábor, Junio C Hamano Signed-off-by: Tony Finch <dot@dotat.at> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] git-prompt: preserve value of $? in all cases 2015-01-14 10:06 ` [PATCH] git-prompt: preserve value of $? in all cases Tony Finch @ 2015-01-14 12:10 ` SZEDER Gábor 0 siblings, 0 replies; 9+ messages in thread From: SZEDER Gábor @ 2015-01-14 12:10 UTC (permalink / raw) To: Tony Finch; +Cc: git, Junio C Hamano Hi, Quoting Tony Finch <dot@dotat.at>: > Signed-off-by: Tony Finch <dot@dotat.at> > --- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-14 12:10 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-22 14:30 [PATCH] git-prompt: preserve command exit status Tony Finch 2014-12-22 17:19 ` Junio C Hamano 2014-12-22 18:09 ` [PATCH v2] git-prompt: preserve value of $? inside shell prompt Tony Finch 2014-12-22 19:58 ` Junio C Hamano 2014-12-22 22:18 ` Tony Finch 2015-01-13 23:57 ` SZEDER Gábor 2015-01-14 10:05 ` Tony Finch 2015-01-14 10:06 ` [PATCH] git-prompt: preserve value of $? in all cases Tony Finch 2015-01-14 12:10 ` SZEDER Gábor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).