Re: [PATCH] t5550: add netrc tests for http 401/403

[Junio C Hamano <gitster@pobox.com>]

Ashlesh Gawande <git@ashlesh.me> writes:

> Signed-off-by: Ashlesh Gawande <git@ashlesh.me>
> ---
> Sending netrc test patches as suggested in: https://lore.kernel.org/git/aPAg3gYwzA9fHCC3@fruit.crustytoothpaste.net

At the conceptual level, I am happy to have tests for features that
we claim to support.  It is a different matter if we want to support
netrc, though ;-).

There are some nits.

> +set_netrc() {

Style.  SP on both sides of ().  I.e.

    set_netrc () {

> +	# $HOME=$TRASH_DIRECTORY
> +	echo "machine $1 login $2 password $3" > $TRASH_DIRECTORY/.netrc

Style.  No space between the redirection operator ">" and
redirection target.

Style.  Enclose the redirection target inside a pair of double
quotes if it involves variable interpolation.  I.e.

	echo ... >"$TRASH_DIRECTORY/.netrc"

> +}
> +
> +clear_netrc() {

Ditto.

> +	rm "$TRASH_DIRECTORY/.netrc"
> +}

Should this fail if .netrc did not exist in the first place, or is
the primary purpose of this helper to ensure the file does not exist
after it returns (in which case it would be desirable not to fail if
the file did not exist when it was called, with "rm -f")?

>  expect_askpass() {

Ditto.

> +test_expect_success 'using credentials from netrc to clone successfully' '
> +	set_askpass wrong &&
> +	set_netrc 127.0.0.1 user@host pass@host &&
> +	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-netrc &&
> +	expect_askpass none
> +'
> +clear_netrc

We try not to run random shell functions outside the test_expect_*
blocks.  A clean-up function like this is better called at the end
of each piece, arranged with the test_when_finished helper.

	test_expect_success 'do random thing' '
		test_when_finished clear_netrc &&
		set_askpass wrong &&
		set_netrc ... &&
		...
	'