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

> 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 <g...@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
+       timeme=time
@@ -35,7 +39,7 @@ scramble()
        echo \$ "$@"
-       /usr/bin/time "$@" >/dev/null
+       $timeme "$@" >/dev/null
 test_expect_success 'setup' '
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

Reply via email to