Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 21, 2017, 01:24 UTC
Message-ID: <xmqqtvxojufd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171120212223.wquyxbmz34foynrk@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 13 quoted lines

> +test_expect_success 'set up ssh wrapper' '
> +	cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \
> +		"$TRASH_DIRECTORY/ssh$X" &&
> +	GIT_SSH="$TRASH_DIRECTORY/ssh$X" &&
> +	export GIT_SSH &&
> +	export TRASH_DIRECTORY &&
> +	>"$TRASH_DIRECTORY"/ssh-output
> +'
>  
>  copy_ssh_wrapper_as () {
>  	cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" &&
> +	test_when_finished "rm -f ${1%$X}$X" &&
>  	GIT_SSH="${1%$X}$X" &&

As we can clearly see in the context, this is not a new issue, but I find the users of this helper that benefit from the "${1%$X}$X" magic somewhat iffy.

There are callers of this helper that pass "somedir/plink" and "somedir/plink.exe", but behind these callers that _think_ they are testing the variant with and without the trailing ".exe", the helper always add ".exe" (after stripping an existing one) on $X=.exe platforms, ending up in testing the same thing twice. On platforms with $X='', testing two different command names may have "some" value, but I wonder if it is cleaner to use a much less magical "$1$X" here, and skip the test with a caller that gives ".exe" variant using a test prerequisite on $X=.exe platforms to avoid redundant tests?

This is totally outside the scope of this series; I mention this only because this may be a possible #leftoverbits.

Thanks.