Re: [PATCH 1/4] t3419-rebase-patch-id: heed USR_BIN_TIME prereq

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2012, 16:32 UTC
Message-ID: <7vd30itm2k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <b04898886f0c7ed9d943aef0ce6e047f10c97d76.1350387132.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 6 quoted lines

> 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.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

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(-)

diff --git i/t/t3419-rebase-patch-id.sh w/t/t3419-rebase-patch-id.sh
index e70ac10..bf2736a 100755
--- i/t/t3419-rebase-patch-id.sh
+++ w/t/t3419-rebase-patch-id.sh
@@ -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' '