Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts

3 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:07:24

Nguyễn Thái Ngọc Duy  [off-list ref] writes:
 Let's hope there will be no third report about this commit..
Hmm, why does this additional test fail only under prove but pass
without it?
quoted hunk
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index f91bbcf..19539fc 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -87,6 +87,33 @@ test_expect_success 'plain nested in bare through aliased command' '
 	check_config bare-ancestor-aliased.git/plain-nested/.git false unset
 '
 
+test_expect_success 'No extra GIT_* on alias scripts' '
+	cat <<-\EOF >expected &&
+	GIT_ATTR_NOSYSTEM
+	GIT_AUTHOR_EMAIL
+	GIT_AUTHOR_NAME
+	GIT_COMMITTER_EMAIL
+	GIT_COMMITTER_NAME
+	GIT_CONFIG_NOSYSTEM
+	GIT_EXEC_PATH
+	GIT_MERGE_AUTOEDIT
+	GIT_MERGE_VERBOSITY
+	GIT_PREFIX
+	GIT_TEMPLATE_DIR
+	GIT_TEXTDOMAINDIR
+	GIT_TRACE_BARE
+	EOF
+	cat <<-\EOF >script &&
+	#!/bin/sh
+	env | grep GIT_ | sed "s/=.*//" | sort >actual
This is more about coding discipline than style, but piping grep
output to sed is wasteful.  "sed -ne '/^GIT_/s/=.*//p'" or something
like that, perhaps?

I wondered what happens if the user has an unrelated stray variable
whose name happens to begin with GIT_ in her environment, but it
turns out that we cleanse them in test-lib.sh fairly early, so that
would be fine.  You need to tighten your "grep" pattern, though.
+	exit 0
+	EOF
+	chmod 755 script &&
+	git config alias.script \!./script &&
+	( mkdir sub && cd sub && git script ) &&
+	test_cmp expected actual
+'
+
 test_expect_success 'plain with GIT_WORK_TREE' '
 	mkdir plain-wt &&
 	test_must_fail env GIT_WORK_TREE="$(pwd)/plain-wt" git init plain-wt

Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts

From: Duy Nguyen <hidden>
Date: 2016-06-15 23:07:24

On Mon, Dec 7, 2015 at 7:54 PM, Junio C Hamano [off-list ref] wrote:
Nguyễn Thái Ngọc Duy  [off-list ref] writes:
quoted
 Let's hope there will be no third report about this commit..
Hmm, why does this additional test fail only under prove but pass
without it?
It passes with prove for me. Some mysterious variable leaks through somehow?
quoted
+     env | grep GIT_ | sed "s/=.*//" | sort >actual
This is more about coding discipline than style, but piping grep
output to sed is wasteful.  "sed -ne '/^GIT_/s/=.*//p'" or something
like that, perhaps?
OK will fix.
I wondered what happens if the user has an unrelated stray variable
whose name happens to begin with GIT_ in her environment, but it
turns out that we cleanse them in test-lib.sh fairly early, so that
would be fine.  You need to tighten your "grep" pattern, though.
OK
-- 
Duy

Re: [PATCH 3/2] git.c: make sure we do not leak GIT_* to alias scripts

From: Jeff King <hidden>
Date: 2016-06-15 23:07:24

On Tue, Dec 08, 2015 at 05:55:20PM +0100, Duy Nguyen wrote:
On Mon, Dec 7, 2015 at 7:54 PM, Junio C Hamano [off-list ref] wrote:
quoted
Nguyễn Thái Ngọc Duy  [off-list ref] writes:
quoted
 Let's hope there will be no third report about this commit..
Hmm, why does this additional test fail only under prove but pass
without it?
It passes with prove for me. Some mysterious variable leaks through somehow?
It fails for me when run via "make" (with prove or without) but not as
"./t0001-init.sh". Looks like extra variables from my config.mak leak
through:

  $ make t0001-init.sh GIT_TEST_OPTS="-v -i"
  [...]
  --- expected    2015-12-08 17:18:06.304699181 +0000
  +++ actual      2015-12-08 17:18:06.312699180 +0000
  @@ -9,5 +9,9 @@
   GIT_MERGE_VERBOSITY
   GIT_PREFIX
   GIT_TEMPLATE_DIR
  +GIT_TEST_GIT_DAEMON
  +GIT_TEST_HTTPD
  +GIT_TEST_OPTS
   GIT_TEXTDOMAINDIR
   GIT_TRACE_BARE
  +MAKEFLAGS
  not ok 6 - No extra GIT_* on alias scripts

Any GIT_TEST_* is allowed through by test-lib.sh.

For the MAKEFLAGS one, I suspect it's hitting

  MAKEFLAGS=GIT_TEST_OPTS=...

You should probably anchor your regex. :)

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help