Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Hi Junio, Junio C Hamano wrote: Ramkumar Ramachandra artag...@gmail.com writes: Here's a patch. -- 8 -- From: Ramkumar Ramachandra artag...@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 artag...@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. I see you've queued it in `pu` after rewriting the commit message. 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? Good idea. I'll write a patch to do this once the interim solution graduates to `master`. Ram -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Ramkumar Ramachandra artag...@gmail.com writes: Here's a patch. -- 8 -- From: Ramkumar Ramachandra artag...@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 artag...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
On Sat, Sep 22, 2012 at 02:22:46AM +0530, Ramkumar Ramachandra wrote: Replace $SHELL with an explicit `/bin/sh`, as some shells do not support all the features used in the script. For example, ZSH does not respect IFS, which is used in line 478. I don't think that is the right thing to do. The point of SHELL is to point at a bourne-compatible shell. On some systems, the main reason to set it is that /bin/sh is _broken_, and we are trying to avoid it. A bigger question is: why are you setting SHELL=zsh in the first place? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Jeff King p...@peff.net writes: A bigger question is: why are you setting SHELL=zsh in the first place? SHELL is set to the login shell by default. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote: I don't think that is the right thing to do. The point of SHELL is to point at a bourne-compatible shell. On some systems, the main reason to set it is that /bin/sh is _broken_, and we are trying to avoid it. But you're only avoiding it in the --tee/ --va* codepath. In the normal codepath, you're stuck with /bin/sh anyway. No, the #!-header is only information. When you run make test we actually invoke the shell ourselves using $SHELL_PATH. A bigger question is: why are you setting SHELL=zsh in the first place? I use ZSH as my primary shell, so SHELL is set to zsh when I run tests. How can we trust $SHELL to be a bourne-compatible shell? Ah, my fault. I was thinking we overrode $SHELL along with $SHELL_PATH, but we do not. The correct patch is to stop using $SHELL, but not to switch to a manual /bin/sh. It should use $SHELL_PATH instead, which is how you tell git your path to a sane bourne shell. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
On Fri, Sep 21, 2012 at 11:08:34PM +0200, Andreas Schwab wrote: Jeff King p...@peff.net writes: A bigger question is: why are you setting SHELL=zsh in the first place? SHELL is set to the login shell by default. Yeah, sorry, I was thinking this was coming from our $SHELL_PATH Makefile variable, and that he was setting that. The real solution is to properly use $SHELL_PATH instead of $SHELL. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Hi Peff, Jeff King wrote: On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote: I don't think that is the right thing to do. The point of SHELL is to point at a bourne-compatible shell. On some systems, the main reason to set it is that /bin/sh is _broken_, and we are trying to avoid it. But you're only avoiding it in the --tee/ --va* codepath. In the normal codepath, you're stuck with /bin/sh anyway. No, the #!-header is only information. When you run make test we actually invoke the shell ourselves using $SHELL_PATH. My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the Makefile. Which shell is it supposed to point to? If you're proposing to use a variable that's only set in the Makefile in the test, you're not allowing users to run the test as a standalone- that's not a good change, is it? Ram -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Ramkumar Ramachandra artag...@gmail.com writes: My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the Makefile. Which shell is it supposed to point to? Inside a makefile the variable SHELL is special in that it is never imported from the environment. If not set it defaults to /bin/sh. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Ramkumar Ramachandra artag...@gmail.com writes: Hi Peff, Jeff King wrote: On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote: I don't think that is the right thing to do. The point of SHELL is to point at a bourne-compatible shell. On some systems, the main reason to set it is that /bin/sh is _broken_, and we are trying to avoid it. But you're only avoiding it in the --tee/ --va* codepath. In the normal codepath, you're stuck with /bin/sh anyway. No, the #!-header is only information. When you run make test we actually invoke the shell ourselves using $SHELL_PATH. My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the Makefile. Which shell is it supposed to point to? SHELL_PATH is always supposed to point to a Bourne that can be used to run POSIXy shell scripts. I think the fallback you pointed out above assumes that the majority of people who type make use Bourne compatibles as their $SHELL and the default is to help the majority. It may not hurt to add a note to INSTALL for people who use $SHELL that is not Bourne (csh and zsh users, but there may be others) that they need to set SHELL_PATH, of course. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
On Sat, Sep 22, 2012 at 03:04:50AM +0530, Ramkumar Ramachandra wrote: No, the #!-header is only information. When you run make test we actually invoke the shell ourselves using $SHELL_PATH. My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the Makefile. Which shell is it supposed to point to? Something bourne compatible. If you are on a sane system, /bin/sh is fine (and is the default). If you're proposing to use a variable that's only set in the Makefile in the test, you're not allowing users to run the test as a standalone- that's not a good change, is it? It gets written to GIT-BUILD-OPTIONS, which should get pulled in by test-lib.sh. There may be an ordering problem with the command line parsing though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Hi Junio,
Junio C Hamano wrote:
The reference to ${SHELL-/bin/sh} in the test need to be updated to
SHELL_PATH as Peff suggested in the other subthread.
For that, the entire block needs to be moved down to come after `.
GIT_BUILD_DIR=$TEST_DIRECTORY/..`. Is this okay?
diff --git a/t/test-lib.sh b/t/test-lib.sh
index dfa86e4..2284b8b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,22 +15,6 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see http://www.gnu.org/licenses/ .
-# if --tee was passed, write the output not only to the terminal, but
-# additionally to the file test-results/$BASENAME.out, too.
-case $GIT_TEST_TEE_STARTED, $* in
-done,*)
- # do not redirect again
- ;;
-*' --tee '*|*' --va'*)
- mkdir -p test-results
- BASE=test-results/$(basename $0 .sh)
- (GIT_TEST_TEE_STARTED=done ${SHELL-sh} $0 $@ 21;
-echo $? $BASE.exit) | tee $BASE.out
- test $(cat $BASE.exit) = 0
- exit
- ;;
-esac
-
# Keep the original TERM for say_color
ORIGINAL_TERM=$TERM
@@ -54,6 +38,22 @@ GIT_BUILD_DIR=$TEST_DIRECTORY/..
. $GIT_BUILD_DIR/GIT-BUILD-OPTIONS
export PERL_PATH SHELL_PATH
+# if --tee was passed, write the output not only to the terminal, but
+# additionally to the file test-results/$BASENAME.out, too.
+case $GIT_TEST_TEE_STARTED, $* in
+done,*)
+ # do not redirect again
+ ;;
+*' --tee '*|*' --va'*)
+ mkdir -p test-results
+ BASE=test-results/$(basename $0 .sh)
+ (GIT_TEST_TEE_STARTED=done ${SHELL_PATH} $0 $@ 21;
+echo $? $BASE.exit) | tee $BASE.out
+ test $(cat $BASE.exit) = 0
+ exit
+ ;;
+esac
+
# For repeatability, reset the environment to known value.
LANG=C
LC_ALL=C
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Ramkumar Ramachandra artag...@gmail.com writes:
Junio C Hamano wrote:
The reference to ${SHELL-/bin/sh} in the test need to be updated to
SHELL_PATH as Peff suggested in the other subthread.
For that, the entire block needs to be moved down to come after `.
GIT_BUILD_DIR=$TEST_DIRECTORY/..`. Is this okay?
Have you tested it with --tee (or valgrind) and does that work?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t/test-lib.sh: do not trust $SHELL
Hi again,
On Sat, Sep 22, 2012 at 10:22 AM, Junio C Hamano gits...@pobox.com wrote:
Ramkumar Ramachandra artag...@gmail.com writes:
Junio C Hamano wrote:
The reference to ${SHELL-/bin/sh} in the test need to be updated to
SHELL_PATH as Peff suggested in the other subthread.
For that, the entire block needs to be moved down to come after `.
GIT_BUILD_DIR=$TEST_DIRECTORY/..`. Is this okay?
Have you tested it with --tee (or valgrind) and does that work?
Yes, it works.
Ram
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
