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

From: SZEDER Gábor <[email-masked]>
Date: Mon, 17 Jun 2019 21:13:28 +0200

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