Re: [PATCH v5 10/10] credential: add WWW-Authenticate header to cred requests
On 2023-01-12 00:48, Ævar Arnfjörð Bjarmason wrote:
Show 24 quoted lines
>
> On Wed, Jan 11 2023, Matthew John Cheetham via GitGitGadget wrote:
>
>> 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.Both good points! Thanks - will take this onboard in next iteration.
Show 21 quoted lines
>> +++ 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'
True; `sed -n -e '/^$/q' -e 'p'` is equivalent here.
Show 20 quoted lines
> 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.
In the next iteration I will move to using write_script; thanks!
Show 21 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).Matthew John Cheetham <mjcheetham@outlook.com>