Re: [PATCH v2 0/4] Strengthen fsck checks for submodule URLs

From: Jeff King <peff@peff.net>
Date: Jan 20, 2024, 00:51 UTC
Message-ID: <20240120005136.GB117170@coredump.intra.peff.net>
In-Reply-To: <xmqqmst2sdn0.fsf@gitster.g>

On Thu, Jan 18, 2024 at 10:24:51AM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> >  * Patch 1 moves 'check_submodule_url()' to a public location so that it can
> >    be used outside of 'fsck.c'.
> >  * Patch 2 removes the obsolete/never-used code in 'test-tool submodule
> >    check-name' handling names provided on the command line.
> >  * Patch 3 adds a 'check-url' mode to 'test-tool submodule', calling the
> >    now-public 'check_submodule_url()' method on a given URL, and adds new
> >    tests checking valid and invalid submodule URLs.
> >  * Patch 4 replaces the 'credential_from_url_gently()' check with
> >    'url_normalize()' followed by 'url_decode()' and an explicit check for
> >    newlines (to preserve the newline handling added in 07259e74ec1 (fsck:
> >    detect gitmodules URLs with embedded newlines, 2020-03-11)).
> 
> Nicely done.  I'll wait for a few days to see if anybody else has
> reaction but after reading the patches myself, my inclination is to
> suggest merging it to 'next'.

It all looks good to me to go to 'next'.

After simplifying the input handling in patch 2, I probably would not have bothered with the abstracted interface in patch 3 (and instead just repeated the few lines of boilerplate, since there's so much already). Mostly just because function pointers in C often make reading and debugging more annoying. But I don't think it's a very big deal either way in this instance.

-Peff