t3302-notes-index-expensive.sh and t3419-rebase-patch-id.sh need time in /usr/bin

Hi folks

t3302-notes-index-expensive.sh and t3419-rebase-patch-id.sh need "time " to be in "/usr/bin", however on my system it is in "/bin".

Can't this be checked for?

Bye, Jojo

t3419 sets the t3419-rebase-patch-id.sh prereq based on the availability of /usr/bin/time but calls the binary unconditionally (in debug mode).

Make it run the timing only when the prereq is matched.

---
 t/t3419-rebase-patch-id.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index e70ac10..52af547 100755
@@ -69,7 +69,7 @@ do_tests()
 		git cherry-pick master >/dev/null 2>&1
 	"
 
-	test_debug "
+	test_have_prereq USR_BIN_TIME && test_debug "
 		run git diff master^\!
 	"
 

@@ -77,7 +77,7 @@ do_tests()
 		echo 'file binary' >.gitattributes
 	"
 
-	test_debug "
+	test_have_prereq USR_BIN_TIME && test_debug "
 		run git format-patch --stdout master &&
 		run git format-patch --stdout --ignore-if-in-upstream master
 	"

1.8.0.rc2.304.g9f3ac5c

I do not think we should ship our tests with too many test_debug in the first place (it is something you as a developer who are trying to figure out why your change broke tests can insert into tests). It might be useful to be able to say "sh ./t1234-*.sh -d" and see debug output that somebody else thought that it might be useful, so I wouldn't say we should remove all existing test_debug, but at the same time, if a developer finds existing test_debug does not work for him (either what the debug output gives him is insufficient for his needs, or what the debug command uses is not available on his system), he should be willing to update (and capable of doing so) it to suit his needs. Adding prereq in front of test_debug is simply an act of insanity.

For this particular one, I think this should suffice. If the shell does not have "time" built-in, then timeme can be set to even an empty string, but that is left as an exercise to the readers.

 t/t3419-rebase-patch-id.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index e70ac10..bf2736a 100755
@@ -6,7 +6,11 @@ test_description='git rebase - test patch id computation'
 
 test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_PATCHID_TIMING_TESTS" && test_set_prereq EXPENSIVE
-test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
+if test -x /usr/bin/time
+	timeme=/usr/bin/time
+else
+	timeme=time
+fi
 
 count()
 {

@@ -35,7 +39,7 @@ scramble()
 run()
 {
 	echo \$ "$@"
-	/usr/bin/time "$@" >/dev/null
+	$timeme "$@" >/dev/null
 }
 
 test_expect_success 'setup' '

Currently, lazy prereq tests are run in a subshell which communicates only the exit code to the outer world.

Run it as a subcommand so that variables can be exported to the test environment.

---
I don't think this has any adverse side effects, but I'm begging for another set of eyeballs to have a look. (Test suite passes, of course.)

 t/test-lib-functions.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 8889ba5..e587902 100644
@@ -246,13 +246,15 @@ test_lazy_prereq () {
 test_run_lazy_prereq_ () {
 	script='
 mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
-(
+{
 	cd "$TRASH_DIRECTORY/prereq-test-dir" &&'"$2"'
-)'
+}'
 	say >&3 "checking prerequisite: $1"
 	say >&3 "$script"
+	orig_pwd="$(pwd)"
 	test_eval_ "$script"
 	eval_ret=$?
+	cd "$orig_pwd"
 	rm -rf "$TRASH_DIRECTORY/prereq-test-dir"
 	if test "$eval_ret" = 0; then
 		say >&3 "prerequisite $1 ok"

1.8.0.rc2.304.g9f3ac5c

The lazy prereqs are designed to be used lazily, in any test that he who wrote lazy-prereq did not anticipate. It is run inside a subshell to make it absolutely sure that whatever it does (like use of shell variables, chdir around) will not be able to affect _any_ calling context that is not anticipated by who writes lazy prerequisite.

Please don't do this.

Some test want to use the time command (not the shell builtin) and test for its availability at /usr/bin/time.

Provide a lazy prereq TIME_COMMAND which tests for /usr/bin/time and /bin/time. If any is found, set TEST_COMMAND_PATH to the first match.

---
Rather than iterating over 2 or more hardcoded paths, one could use "test -P time" or allow a make variable TIME_COMMAND_PATH whose executability is checked by the prereq. I really don't know what's best.

 t/test-lib.sh | 7 +++++++
 1 file changed, 7 insertions(+)

index 489bc80..7977c15 100644
@@ -738,6 +738,13 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
 	esac
 '
 
+test_lazy_prereq TIME_COMMAND '
+	for command in /usr/bin/time /bin/time
+	do
+		test -x "$command" && break
+	done && TIME_COMMAND_PATH="$command"
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY

1.8.0.rc2.304.g9f3ac5c

test -P time won't work for me:
test -P: unary operator expected

I do have another one in /usr/local/bin, maybe that could get added too?

Some test want to use the time command (not the shell builtin) and test for its availability at /usr/bin/time.

Provide a lazy prereq TIME_COMMAND which tests for $TEST_COMMAND_PATH, which can be set from config.mak. It defaults to /usr/bin/time.

---
Here's what the make variable version could look like. 1/4 and 4/4 would stay as is, 2/4 could be dropped.

 t/Makefile    | 5 +++--
 t/test-lib.sh | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

index 88e289f..52b4039 100644
@@ -13,6 +13,7 @@ TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove
 DEFAULT_TEST_TARGET ?= test
+TIME_COMMAND_PATH ?= /usr/bin/time
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

@@ -27,11 +28,11 @@ test: pre-clean $(TEST_LINT)
 	$(MAKE) aggregate-results-and-cleanup
 
 prove: pre-clean $(TEST_LINT)
-	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove ***"; GIT_CONFIG=.git/config TIME_COMMAND_PATH=$(TIME_COMMAND_PATH) $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):
-	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+	@echo "*** $@ ***"; GIT_CONFIG=.git/config TIME_COMMAND_PATH=$(TIME_COMMAND_PATH) '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 
 pre-clean:
 	$(RM) -r test-results

index 489bc80..173eb13 100644
@@ -738,6 +738,10 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
 	esac
 '
 
+test_lazy_prereq TIME_COMMAND '
+	test -x "$TIME_COMMAND_PATH"
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY

1.8.0.rc2.304.g9f3ac5c

An alternative way to suppress the builtin meaning is to quote it, like \time.

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Hi Andreas. I hope you don't mind my nitpickiness, but ...

... to be 100% precise, this quoting trick works because 'time' is a shell keyword, rather than a builtin:

    $ type time
    time is a shell keyword

Regards,
Stefano

Am 16.10.2012 17:07, schrieb Michael J Gruber:

This avoids the builtin:

command time $that_command

It works for bash, ksh, zsh, and dash (where the latter doesn't have it as builtin).

-- Hannes

"command time" works but I think that is because it is not a built-in ;-)

Here is what I read in bash(1):

command [-pVv] command [arg ...] Run command with args
suppressing the normal shell function lookup. Only builtin commands or commands found in the PATH are executed.

Taken together with this from "COMMAND EXECUTION":

If the command name contains no slashes, the shell attempts to locate it. If there exists a shell function by that name, that function is invoked as described above in FUNCTIONS. If the name does not match a function, the shell searches for it in the list of shell builtins. If a match is found, that builtin is invoked.

If the name is neither a shell function nor a builtin, and contains no slashes, bash searches each element of the PATH for a directory containing an executable file by that name.

I suspect "command printf 'a b c\n'" would not use $HOME/bin/printf even when I have $HOME/bin early in my $PATH (nor /usr/bin/printf for that matter).

Joachim Schmitz venit, vidit, dixit 16.10.2012 16:13:

Yikes.

If we introduce a make variable TIME_COMMAND_PATH we can even get rid of 2/4 (but have to change Makefile or t/Makefile).

I don't mind too much. /usr/bin/time and /bin/time should be enough.

As we have t/perf these days, I suspect that we should aim to remove these uses of /usr/bin/time in the main testsuite instead.

The one in 3419 was "run this command, and while at it run it under 'time'" but it is only inside test_debug and I do not think it gives anything more than curiosity values.

The one in 3302 is used to compare two runs (one without and one with notes tree), so it has a little more value than just curiosity, but its value inside the main test suite is still highly dubious. It does not have any "under this value the test passes" criteria.

When the performance of having to look up notes tree really matters, it shouldn't be done inside the main test suite that is designed to be runnable unattended and the only check the humans do is to see their "ok/fail" output.

In short, what you would get out of /usr/bin/time simply is not a good match inside the context of these unit tests.

Use the TIME_COMMAND prereq in both tests so that time from several paths can be used.

---
 t/t3302-notes-index-expensive.sh | 5 ++---
 t/t3419-rebase-patch-id.sh       | 7 +++----
 2 files changed, 5 insertions(+), 7 deletions(-)

index e35d781..3d9f37e 100755
@@ -9,7 +9,6 @@ test_description='Test commit notes index (expensive!)'
 
 test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_NOTES_TIMING_TESTS" && test_set_prereq EXPENSIVE
-test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
 
 create_repo () {
 	number_of_commits=$1

@@ -96,7 +95,7 @@ time_notes () {
 	for mode in no-notes notes
 	do
 		echo $mode
-		/usr/bin/time "$SHELL_PATH" ../time_notes $mode $1
+		"$TIME_COMMAND_PATH" "$SHELL_PATH" ../time_notes $mode $1
 	done
 }
 

@@ -113,7 +112,7 @@ do_tests () {
 
 	test_expect_success $pr 'notes work' "test_notes $count"
 
-	test_expect_success USR_BIN_TIME,$pr 'notes timing with /usr/bin/time' "time_notes 100"
+	test_expect_success TIME_COMMAND,$pr 'notes timing with a time command' "time_notes 100"
 
 	test_expect_success $pr 'teardown / cd ..' 'cd ..'
 }

index 52af547..c7d90a6 100755
@@ -6,7 +6,6 @@ test_description='git rebase - test patch id computation'
 
 test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_PATCHID_TIMING_TESTS" && test_set_prereq EXPENSIVE
-test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
 
 count()
 {

@@ -35,7 +34,7 @@ scramble()
 run()
 {
 	echo \$ "$@"
-	/usr/bin/time "$@" >/dev/null
+	"$TIME_COMMAND_PATH" "$@" >/dev/null
 }
 
 test_expect_success 'setup' '

@@ -69,7 +68,7 @@ do_tests()
 		git cherry-pick master >/dev/null 2>&1
 	"
 
-	test_have_prereq USR_BIN_TIME && test_debug "
+	test_have_prereq TIME_COMMAND && test_debug "
 		run git diff master^\!
 	"
 

@@ -77,7 +76,7 @@ do_tests()
 		echo 'file binary' >.gitattributes
 	"
 
-	test_have_prereq USR_BIN_TIME && test_debug "
+	test_have_prereq TIME_COMMAND && test_debug "
 		run git format-patch --stdout master &&
 		run git format-patch --stdout --ignore-if-in-upstream master
 	"

1.8.0.rc2.304.g9f3ac5c

This mini series enables the use of the lazy prereq framework for tests which need a (non-builtin) time command.

1/4 is a bugfix
2/4 allows lazy prereq test scripts to set variables for later use 3/4 implements the prereq
4/4 uses it

This should solve the problem reported by Joachim Schmitz.

3/4 could be done in various different ways, see the comments there.

Michael J Gruber (4):
t3419-rebase-patch-id: heed USR_BIN_TIME prereq
test-lib: allow variable export from lazy prereq tests
test-lib: provide lazy TIME_COMMAND prereq t3302,t3419: use the TIME_COMMAND prereq

 t/t3302-notes-index-expensive.sh | 5 ++---
 t/t3419-rebase-patch-id.sh       | 7 +++----
 t/test-lib-functions.sh          | 6 ++++--
 t/test-lib.sh                    | 7 +++++++
 4 files changed, 16 insertions(+), 9 deletions(-)

1.8.0.rc2.304.g9f3ac5c