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

From: Ashlesh Gawande <git@ashlesh.me>
Date: Jan 6, 2026, 11:47 UTC
Message-ID: <168f4c53-4f33-42e3-b3c9-b44ab101da94@ashlesh.me>
In-Reply-To: <xmqqjyxvjb4c.fsf@gitster.g>

On 1/6/26 15:50, Junio C Hamano wrote:

Show 10 quoted lines

> 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.

Thanks for the quick review! I think Brian also suggested getting rid of netrc in the future. I have sent v2 to address your comments.

Show 27 quoted lines

>
>> +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")?

Yes, the primary purpose is to just clear the file as it might potentially break other tests (added -f in v2).

Show 22 quoted lines

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