Re: [PATCHv3 02/11] git-p4: test debug macro

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 18, 2011, 21:48 UTC
Message-ID: <7vwr9to8kk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20111218140633.GB16487@padd.com>

Pete Wyckoff <pw@padd.com> writes:

Show 7 quoted lines

> Call this from a test to have it pause and wait for you to
> investigate.  It prints out its current directory and the
> P4 environment variables.  It waits for ctrl-c before continuing
> the test.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>
> ---

...

Show 21 quoted lines

> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index a870f9a..4c30960 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -72,3 +72,34 @@ kill_p4d() {
>  cleanup_git() {
>  	rm -rf "$git"
>  }
> +
> +#
> +# This is a handy tool when developing or debugging tests.  Use
> +...
> +debug() {
> +	echo "*** Debug me, hit ctrl-c when done.  Useful shell commands:"
> +	echo cd \"$(pwd)\"
> +	echo export P4PORT=$P4PORT P4CLIENT=$P4CLIENT
> +	trap "echo" INT
> +	sleep $((3600 * 24 * 30))
> +	trap - INT
> +}
> +

You may have found this handy yourself, but I would rather not to see it here in the current form for multiple reasons.

 - Why "ctrl-c"? You are not even spawning shell from here but are having
   the user interact with this state in the middle of a test from another
   shell, no?

   Why not "When done, type <RET>" and have a "read junk" or something
   instead? That would be a lot simpler and you do not have to worry about
   portability with many lines of comments.

   An alternative is to spawn an interactive shell here, and change the
   "Debug me" comment to say "ctrl-d when done".

 - This is not linked to the generic "debug" option "txxxx-name.sh -d".
   Shouldn't you be extending test_debug so that it can go interactive,
   give customized comments&insns (i.e. "cd $here" may be useful for test
   scripts outside testing p4, but "P4PORT=..." would not be, so the user
   of test_debug in t9800-git-p4.* needs customizability of the message).

Also could we please rename p4 related tests in t/t98* series so that it is clear that they are about git-p4 from "ls t/" output (i.e. have them all have "git-p4" in their names)?