# [PATCH] __git_ps1: Don't kill shell if user types `set -e` 5 messages from 2017-04-16 to 2017-05-04. Participants: Tom "Ravi" Hale, Junio C Hamano, Tom Hale, Ævar Arnfjörð Bjarmason. Thread: https://gitlist.dev/t/45709 ## Tom "Ravi" Hale, 2017-04-16 08:28 Subject: [PATCH] __git_ps1: Don't kill shell if user types `set -e` Message-ID: URL: https://gitlist.dev/e/b344d0c3-c8b8-da27-79f6-ae750be6830a%40hale.ee ``` If a user types `set -e` in an interactive shell, and is using __git_ps1 to set their prompt, the shell will die if the current directory isn't inside a git repository. This is because `set -e` instructs the shell to exit upon a command returning a non-zero exit status, and the following command exits with status 128: repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ --is-bare-repository --is-inside-work-tree \ --short HEAD 2>/dev/null)" I couldn't write a test to illustrate this - the test harness seems to be preventing `set -e` from having an effect. Signed-off-by: Tom "Ravi" Hale From 7867768f4516559c129ed9eb07ae29c36a6c9367 Mon Sep 17 00:00:00 2001 From: "Tom \"Ravi\" Hale" Date: Sun, 16 Apr 2017 14:10:10 +0700 Subject: [PATCH] __git_ps1: Don't kill shell if user types `set -e` Ensure that `set +e` is in effect inside __git_ps1 to prevent any commands exiting non-zero from killing an interactive shell in which the user has run `set -e`. --- 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 97eacd7..15a0b74 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -304,6 +304,10 @@ __git_ps1 () local ps1pc_start='\u@\h:\w ' local ps1pc_end='\$ ' local printf_format=' (%s)' + # Prevent any non-zero exit statuses from killing an interactive shell in the + # case that a user types `set -e` + local -- - # Shadow shell options in $- (restored upon return) + set +e case "$#" in 2|3) pcmode=yes -- 2.11.1 ``` ## Junio C Hamano, 2017-04-17 04:24 Subject: Re: [PATCH] __git_ps1: Don't kill shell if user types `set -e` Message-ID: URL: https://gitlist.dev/e/xmqq60i3k6ed.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` "Tom \"Ravi\" Hale" writes: > If a user types `set -e` in an interactive shell, and is using __git_ps1 > to set > their prompt, the shell will die if the current directory isn't inside a git > repository. > > This is because `set -e` instructs the shell to exit upon a command > returning a non-zero exit status, and the following command exits with > status 128: > > repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ > --is-bare-repository --is-inside-work-tree \ > --short HEAD 2>/dev/null)" Hmph. So the fix would be something like this? repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ --is-bare-repository --is-inside-work-tree \ - --short HEAD 2>/dev/null)" + --short HEAD 2>/dev/null || :)" I am too afraid to ask what a user would try to achieve by doing the "set -e" thing in an interactive session. It is understandable (I am not saying I think it is necessarily a good idea) to do so in a script, but for an interactive shell? ``` ## Tom Hale, 2017-05-04 08:26 Subject: Re: [PATCH] __git_ps1: Don't kill shell if user types `set -e` Message-ID: URL: https://gitlist.dev/e/c8879806-4e43-d033-bd69-918e730e2d05%40hale.ee In-Reply-To: ``` On 17/04/17 11:24, Junio C Hamano wrote: > "Tom \"Ravi\" Hale" writes: > > > If a user types `set -e` in an interactive shell, and is using __git_ps1 > > to set > > their prompt, the shell will die if the current directory isn't inside a git > > repository. > > > Hmph. So the fix would be something like this? > > repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ > --is-bare-repository --is-inside-work-tree \ > - --short HEAD 2>/dev/null)" > + --short HEAD 2>/dev/null || :)" Nope, that would cause the next line rev_parse_exit_code="$?" to always be assigned 0. I believe the patch we're after is attached. -- Cheers, Tom From 74fa2e2fa058f89605cf0b2774ad8e9df981cf86 Mon Sep 17 00:00:00 2001 From: "Tom \"Ravi\" Hale" Date: Sun, 16 Apr 2017 14:15:14 +0700 Subject: [PATCH] Prevent non-zero exit if outside a repository --- contrib/completion/git-prompt.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 97eacd7..d0d890c 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -361,8 +361,8 @@ __git_ps1 () local repo_info rev_parse_exit_code repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ --is-bare-repository --is-inside-work-tree \ - --short HEAD 2>/dev/null)" - rev_parse_exit_code="$?" + --short HEAD 2>/dev/null)" && + rev_parse_exit_code="$?" || : # Don't die if "set -e" if [ -z "$repo_info" ]; then return $exit -- 2.12.2 ``` ## Ævar Arnfjörð Bjarmason, 2017-05-04 09:16 Subject: Re: [PATCH] __git_ps1: Don't kill shell if user types `set -e` Message-ID: URL: https://gitlist.dev/e/CACBZZX6XdwND2-7cY0zp8%2BK_uh60tn-i_r9V7HAe2X-a-RJeQA%40mail.gmail.com In-Reply-To: ``` On Thu, May 4, 2017 at 10:26 AM, Tom Hale wrote: > > On 17/04/17 11:24, Junio C Hamano wrote: >> "Tom \"Ravi\" Hale" writes: >> >> > If a user types `set -e` in an interactive shell, and is using __git_ps1 >> > to set >> > their prompt, the shell will die if the current directory isn't inside a git >> > repository. >> > >> Hmph. So the fix would be something like this? >> >> repo_info="$(git rev-parse --git-dir --is-inside-git-dir \ >> --is-bare-repository --is-inside-work-tree \ >> - --short HEAD 2>/dev/null)" >> + --short HEAD 2>/dev/null || :)" > > Nope, that would cause the next line > rev_parse_exit_code="$?" > to always be assigned 0. > > I believe the patch we're after is attached. I see how this would fix things, but this just seems like a rather crazy thing for us to support, we can take this patch, but it's going to take quite some maintenance burden to ensure that this code is set -e clean going forward, and I don't think we should take a patch like this without some general support in the regression tests to ensure that the completion code is set -e clean, otherwise this is going to very easily regress the next time someone not aware of this patches it. What's the real use-case here? If you do "set -e" in your shell you also get e.g.: $ ls -l blah ls: cannot access blah: No such file or directory === Command terminated with exit status 2 (Thu May 4 11:16:03 2017) === I.e. any little failure will terminate your shell, are you actually running a shell like this? For what purpose? ``` ## Tom Hale, 2017-05-04 09:35 Subject: Re: [PATCH] __git_ps1: Don't kill shell if user types `set -e` Message-ID: <60ea6f0e-aa27-9a2c-54fd-9c14db9f028c@hale.ee> URL: https://gitlist.dev/e/60ea6f0e-aa27-9a2c-54fd-9c14db9f028c%40hale.ee In-Reply-To: ``` On 04/05/17 16:16, Ævar Arnfjörð Bjarmason wrote: > What's the real use-case here? If you do "set -e" in your shell you > also get e.g.: > > $ ls -l blah > ls: cannot access blah: No such file or directory > === Command terminated with exit status 2 (Thu May 4 11:16:03 2017) === > > I.e. any little failure will terminate your shell, are you actually > running a shell like this? For what purpose? If I want to copy and paste a list of commands and have the execution stop at a failure, I type "bash" then "set -e", then paste the commands. It saves me creating a script file and then removing it later. -- Tom Hale ```