Re: [Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 26, 2024, 11:51 UTC
Message-ID: <CAP8UFD38-LngmtQ3cm-GNsPH1eRv4=-SjgVhfn+twzR2NnuNwQ@mail.gmail.com>
In-Reply-To: <ZgK0Ap8CokOqWT7v@tanuki>

On Tue, Mar 26, 2024 at 12:39 PM Patrick Steinhardt <ps@pks.im> wrote:

>
> On Thu, Feb 29, 2024 at 06:40:03AM +0100, Achu Luma wrote:

Show 11 quoted lines

> > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> > index 482a1e58a4..7bfbb75c9b 100644
> > --- a/t/helper/test-tool.c
> > +++ b/t/helper/test-tool.c
> > @@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
> >       { "serve-v2", cmd__serve_v2 },
> >       { "sha1", cmd__sha1 },
>
> I'm a bit surprised that the `cmd_sha1` subcommand still exists. I would
> have expected it to be removed as part of this commit, as well, judging
> by the commit message.

I think that's because it is used in t0013-sha1dc.sh not just t0015-hash.sh, and porting t0013-sha1dc.sh is complex because it relies on `test-tool sha1` dying when it is used on a file created to contain the known sha1 attack.

But I agree this should be better explained in the commit message.

> In any case I think it would be easier to review if the "sha1" and
> "sha256" refactorings were split out into two separate patches.

Thanks for your review!