Subject: Re: [PATCH v5 10/10] credential: add WWW-Authenticate header to cred requests

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 12 Jan 2023 09:48:43 +0100

On Wed, Jan 11 2023, Matthew John Cheetham via GitGitGadget wrote:

> From: Matthew John Cheetham <[email-masked]>
> [...]
> +static void credential_write_strvec(FILE *fp, const char *key,
> +				    const struct strvec *vec)
> +{
> +	int i = 0;
> +	const char *full_key = xstrfmt("%s[]", key);
> +	for (; i < vec->nr; i++) {
> +		credential_write_item(fp, full_key, vec->v[i], 0);

Style: Don't mismatch types if there's no good reason. Use "size_t i" here, also let's do:

	for (size_t i = 0; ....

I.e. no reason to declare it earlier.

> +	}
> +	free((void*)full_key);

Just don't add a "const" to that "full_key" and skip the cast with
free() here.

> +++ b/t/helper/test-credential-helper-replay.sh

I see to my surprise that we have one existing *.sh helper in that
directory, but in any case...

> @@ -0,0 +1,14 @@
> +cmd=$1
> +teefile=$cmd-actual.cred
> +catfile=$cmd-response.cred
> +rm -f $teefile
> +while read line;
> +do
> +	if test -z "$line"; then
> +		break;
> +	fi
> +	echo "$line" >> $teefile
> +done

It looks like you're re-inventing "sed" here, isn't this whole loop just

	sed -n -e '/^$/q' -n 'p'

And then you can skip the "rm" before, as you could just clobber the
thing.

> +if test "$cmd" = "get"; then
> +	cat $catfile
> +fi
> diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
> index 65105a5a6a9..a8dbee6ca40 100755
> --- a/t/t5556-http-auth.sh
> +++ b/t/t5556-http-auth.sh
> @@ -27,6 +27,8 @@ PID_FILE="$TRASH_DIRECTORY"/pid-file.pid
>  SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log
>  
>  PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
> +CREDENTIAL_HELPER="$GIT_BUILD_DIR/t/helper/test-credential-helper-replay.sh" \
> +	&& export CREDENTIAL_HELPER

...(continued from above): Let's just use write_script() here or
whatever, i.e. no reason to make this a global script, it's just used in
this one test, so it can set it up.
>  
>  test_expect_success 'setup repos' '
>  	test_create_repo "$REPO_DIR" &&
> @@ -92,15 +94,279 @@ start_http_server () {
>  
>  per_test_cleanup () {
>  	stop_http_server &&
> -	rm -f OUT.*
> +	rm -f OUT.* &&
> +	rm -f *.cred &&
> +	rm -f auth.config
>  }
>  
>  test_expect_success 'http auth anonymous no challenge' '
>  	test_when_finished "per_test_cleanup" &&
> -	start_http_server &&
> +
> +	cat >auth.config <<-EOF &&
> +	[auth]
> +	    allowAnonymous = true

Mixed tab/space. Use "\t" not 4x " " (ditto below).