Re: [PATCH v7 00/12] Enhance credential helper protocol to include auth headers

From: Victoria Dye <vdye@github.com>
To: Matthew John Cheetham via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: "Derrick Stolee" <derrickstolee@github.com>,
	"Lessley Dennington" <lessleydennington@gmail.com>,
	"Matthew John Cheetham" <mjcheetham@outlook.com>,
	"M Hickford" <mirth.hickford@gmail.com>,
	"Jeff Hostetler" <git@jeffhostetler.com>,
	"Glen Choo" <chooglen@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v7 00/12] Enhance credential helper protocol to include auth headers
Date: Tue, 24 Jan 2023 09:30:02 -0800	[thread overview]
Message-ID: <e57c1ca3-c21c-db41-a386-e5887f46055c@github.com> (raw)
In-Reply-To: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com>

Matthew John Cheetham via GitGitGadget wrote:
> Updates in v6
> =============
> 
>  * Clarify the change to make logging optional in the check_dead_children()
>    function during libification of daemon.c.
> 
>  * Fix missing pointer dereference bugs identified in libification of child
>    process handling functions for daemon.c.
> 
>  * Add doc comments to child process handling function declarations in the
>    daemon-utils.h header.
> 
>  * Align function parameter names with variable names at callsites for
>    libified daemon functions.
> 
>  * Re-split out the test-http-server test helper commits in to smaller
>    patches: error response handling, request parsing, http-backend
>    pass-through, simple authentication, arbitrary header support.
> 
>  * Call out auth configuration file format for test-http-server test helper
>    and supported options in commit messages, as well as a test to exercise
>    and demonstrate these options.
> 
>  * Permit auth.token and auth.challenge to appear in any order; create the
>    struct auth_module just-in-time as options for that scheme are read. This
>    simplifies the configuration authoring of the test-http-server test
>    helper.
> 
>  * Update tests to use auth.allowAnoymous in the patch that introduces the
>    new test helper option.
> 
>  * Drop the strvec_push_nodup() commit and update the implementation of HTTP
>    request header line folding to use xstrdup and strvec_pop and _pushf.
> 
>  * Use size_t instead of int in credential.c when iterating over the struct
>    strvec credential members. Also drop the not required const and cast from
>    the full_key definition and free.
> 
>  * Replace in-tree test-credential-helper-reply.sh test cred helper script
>    with the lib-credential-helper.sh reusable 'lib' test script and shell
>    functions to configure the helper behaviour.
> 
>  * Leverage sed over the while read $line loop in the test credential helper
>    script.
> 
> 
> Updates in v7
> =============
> 
>  * Address several whitespace and arg/param list alignment issues.
> 
>  * Rethink the test-http-helper worker-mode error and result enum to be more
>    simple and more informative to the nature of the error.
> 
>  * Use uintmax_t to store the Content-Length of a request in the helper
>    test-http-server. Maintain a bit flag to store if we received such a
>    header.
> 
>  * Return a "400 Bad Request" HTTP response if we fail to parse the request
>    in the test-http-server.
> 
>  * Add test case to cover request message parsing in test-http-server.
> 
>  * Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in
>    get_auth_module.
> 
>  * Correctly free the split strbufs created in the header parsing loop in
>    test-http-server.
> 
>  * Avoid needless comparison > 0 for unsigned types.
> 
>  * Always set optional outputs to NULL if not present in test helper config
>    value handling.
> 
>  * Remove an accidentally commented-out test cleanup line for one test case
>    in t5556.
I've re-read the patches in this version; all of my comments from v5 have
been addressed, and the additional updates w.r.t. other reviewer feedback
all look good as well. At this point, I think the series is ready for
'next'.

Thanks!