threads / patch / 38225

patchgit-prompt: preserve command exit status

Subject: [PATCH] git-prompt: preserve command exit status

## tl;dr

9 messages between Dec 22, 2014 and Jan 14, 2015. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Tony Finch· Dec 22, 2014, 14:30 UTC · lore

Signed-off-by: Tony Finch <dot@dotat.at>
---
 contrib/completion/git-prompt.sh | 4 ++++
 1 file changed, 4 insertions(+)

Show changes to contrib/completion/git-prompt.sh +4 −0

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· Dec 22, 2014, 17:19 UTC · re: Tony Finch · lore

Re: [PATCH] git-prompt: preserve command exit status

Tony Finch <dot@dotat.at> writes:

Show 25 quoted lines

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

Tony Finch· Dec 22, 2014, 18:09 UTC · re: Junio C Hamano · lore

[PATCH v2] git-prompt: preserve value of $? inside shell prompt

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 :-)

Show changes to contrib/completion/git-prompt.sh +4 −0

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· Dec 22, 2014, 19:58 UTC · re: Tony Finch · lore

Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt

Tony Finch <dot@dotat.at> writes:

Show 26 quoted lines

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

Show 20 quoted lines

> 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· Dec 22, 2014, 22:18 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt

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.

SZEDER Gábor· Jan 13, 2015, 23:57 UTC · re: Tony Finch · lore

Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt

Hi,

Quoting Tony Finch <dot@dotat.at>:

Show 51 quoted lines

> 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

Tony Finch· Jan 14, 2015, 10:05 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt

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.

Tony Finch· Jan 14, 2015, 10:06 UTC · re: SZEDER Gábor · lore

[PATCH] git-prompt: preserve value of $? in all cases

Signed-off-by: Tony Finch <dot@dotat.at>
---
 contrib/completion/git-prompt.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to contrib/completion/git-prompt.sh +4 −4

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· Jan 14, 2015, 12:10 UTC · re: Tony Finch · lore

Re: [PATCH] git-prompt: preserve value of $? in all cases

Hi,

Quoting Tony Finch <dot@dotat.at>:

Show 55 quoted lines

> 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