Re: [PATCH v5 06/10] test-http-server: add simple authentication

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2023, 21:06 UTC
Message-ID: <xmqq8ri616ww.fsf@gitster.g>
In-Reply-To: <2a0b5f3f-7ab2-bc9e-76ac-93a52b4d32d0@github.com>

Victoria Dye <vdye@github.com> writes:

Show 8 quoted lines

>> +static int split_auth_param(const char *str, char **scheme, char **val, int required_val)
>> +{
>> ...
>> +}
>
> There's nothing really *new* in these functions in this iteration, just code
> moved from the option parsing/handling in 'cmd_main()' into dedicated
> functions. Looks good!

> ...
>
> I completely missed the "fall-through" comment in my last review [1], as you
> kindly pointed out [2]. ;) Given that, this makes sense to me.

Show 5 quoted lines

>> +		*wr = send_http_error(STDOUT_FILENO, 401, "Unauthorized", -1,
>> +				      &hdrs, *wr);
>
> The "extra_headers" configuration is new, and helps make the test server
> more flexible.

This is not limited to this single review message, but it is good to see "this part of the patch is good because ..." explicitly stated. I wish more people did so, in addition to pointing out what needs to be improved.

Thanks.