* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.