Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Matthieu Moy <git@matthieu-moy.fr>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
Date: Mon, 17 Jun 2019 21:25:22 +0200	[thread overview]
Message-ID: <20190617192522.GM31952@szeder.dev> (raw)
In-Reply-To: <20190617191328.GL31952@szeder.dev>

[resend to Matthieu's current email address]

On Mon, Jun 17, 2019 at 09:13:28PM +0200, SZEDER Gábor wrote:
> On Fri, Jun 14, 2019 at 08:42:29PM +0200, Johannes Schindelin wrote:
> 
> > The `SHELL` thing here is important, as t/t3404-rebase-interactive.sh sets
> > this to the empty value explicitly:
> > 
> > https://github.com/git/git/blob/v2.22.0/t/t3404-rebase-interactive.sh#L63-L68
> 
> Hmm, hang on for a sec.  Why does it set SHELL to empty?
> 
> So t3404 sets SHELL to the empty value since cd035b1cef
> (rebase -i: add exec command to launch a shell command, 2010-08-10),
> and the in-test comment highlighted on the above link says:
> 
>   # "exec" commands are run with the user shell by default, but this may
>   # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
>   # to create a file. Unsetting SHELL avoids such non-portable behavior
>   # in tests. It must be exported for it to take effect where needed.
>   SHELL=
> 
> Furthermore, the corresponding documentation added in cd035b1cef
> says the following:
> 
>   The "exec" command launches the command in a shell (the one specified
>   in `$SHELL`, or the default shell if `$SHELL` is not set)
> 
> IOW if SHELL is not set/empty, then it will run the default '/bin/sh',
> but who knows what it might be, there's no guarantee that it's a sane
> POSIX shell.  That's why we have "Define SHELL_PATH to a POSIX shell
> if your /bin/sh is broken." very near to the top of our Makefile.
> 
> So while setting SHELL to the empty value might indeed avoid
> non-portable behavior when the user in general prefers a non-POSIX
> shell, it wouldn't help if '/bin/sh' is broken.  For that it should be
> set to $SHELL_PATH (or perhaps $TEST_SHELL_PATH nowadays...).
> 
> Though cd035b1cef is now close to 9 years old, plenty of time for
> somebody to run into this issue in the wild and speak up...
>