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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2026, 10:20 UTC
Message-ID: <xmqqjyxvjb4c.fsf@gitster.g>
In-Reply-To: <20260106093451.748761-1-git@ashlesh.me>

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.

Show 7 quoted lines

> +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 ... &&
		...
	'