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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 12, 2023, 08:48 UTC
Message-ID: <230112.86sfgg2kuj.gmgdl@evledraar.gmail.com>
In-Reply-To: <af66d2d2ede2a502f32d74c86f302598c68d1476.1673475190.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

> From: Matthew John Cheetham <mjcheetham@outlook.com>
> [...]
> +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...

Show 12 quoted lines

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

Show 13 quoted lines

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

Show 20 quoted lines

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