Re: [PATCH] t/test-lib.sh: do not trust $SHELL

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2012, 20:16 UTC
Message-ID: <7vobkx3j9i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0kWJ3ndEMGzyGGMEa+ko3BTShC7_o-HVsXzFCGwRem5Jw@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 14 quoted lines

> Here's a patch.
>
> -- 8< --
> From: Ramkumar Ramachandra <artagnon@gmail.com>
> Date: Sat, 22 Sep 2012 10:25:10 +0530
> Subject: [PATCH] test-lib: do not trust $SHELL
>
> Do not trust $SHELL to be a bourne-compatible shell.  Instead, use the
> Makefile variable $SHELL_PATH.  This fixes a bug: when a test was run
> with --tee and $SHELL was set to ZSH, $PATH on line 479 was not
> getting split due to ZSH not respecting $IFS.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

The part that this starts letting run, which the original "Re-run the command under tee as early as possible" wanted to avoid running, does not affect anything that would affect how we run that tee magic (e.g. "mkdir -p test-results" will still create it directly inside the directory the test script was started in), so I think this patch is safe _for now_.

However, it forces people who need to update earlier parts of this script to be extra careful; it has been true before the patch, and the patch makes it even more so.

I am not opposed to queuing this as an interim solution, but I wonder if we can get rid of that double-launch altogether.

Instead of re-launching the script with its output piped to "tee", can't we do the same by redirecting our standard output to the file in the file, and spawn a "tail -f" that reads from the file and outputs to our original output? Something along the lines of:

        mkdir -p test-results
        tee_base=test-results/$(basename "$0" .sh)

        # empty the file and start "tail -f" on it ...
        : >"$tee_base.out"
        ( tail -f "$tee_base.out" ) &
        tee_pid=$!
	trap 'kill $tee_pid; exit' 0 1 2 3
	# ... and then redirect our output to it
        exec >"$tee_base.out"

and wrap it in a shell helper function that is called from where the parsing of the command line arguments for "--tee" happens, and don't forget to kill $tee_pid when we exit.

Hrm?