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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jun 17, 2019, 19:13 UTC
Message-ID: <20190617191328.GL31952@szeder.dev>
In-Reply-To: <nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet>

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