Re: [PATCH 06/28] t0000: annotate with SHA1 prerequisite

From: Martin Ågren <martin.agren@gmail.com>
Date: May 8, 2018, 18:28 UTC
Message-ID: <CAN0heSq8EYxVx9Qpb==KgeBVT3AXtgK6f+CQyK_kNUXZHMMnLA@mail.gmail.com>
In-Reply-To: <20180507234058.GJ953644@genre.crustytoothpaste.net>

On 8 May 2018 at 01:40, brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 22 quoted lines

> On Mon, May 07, 2018 at 12:24:13PM +0200, Martin Ågren wrote:
>> This could be more centralized at the top of the file, more likely to be
>> noticed during a `make test` and easier to adapt in the future. Maybe
>> something like this at the top of the file:
>>
>> if hash for storage is SHA-1 then
>>         knowntree=7bb943559a305bdd6bdee2cef6e5df2413c3d30a
>>         path0=f87290f8eb2cbbea7857214459a0739927eab154
>>         ....
>> else
>>         skip_all='unknown hash, but you really need to expand this test'
>>         # or even error out!
>> fi
>
> As I mentioned in an earlier email, I plan to set an environment
> variable for the algorithms in use and then do something like:
>
>   test "$tree" = "$(test-tool hash-helper --output known-tree)"
>
> where "known-tree" is some key we can use to look up the SHA-1 or
> NewHash value, and we've specified we want the output format (as opposed
> to input format or on-disk format).

My first thought was, can't we introduce such a helper already now? It would not check with the environment, but would always output SHA-1 values. Thinking about it some more, maybe the exact usage/interface would be a bit volatile in the beginning, making it just as good an idea to gain some more experience and feeling first.

Show 15 quoted lines

>> When we add NewHash, we get to add an "else if". Otherwise, we'd be
>> plugging in NewHash without testing some very basic stuff.
>>
>> Ok, so `skip_all` is quite hard, but skipping certain basic tests also
>> feels really shaky. Adding a new hash algo without adapting this test
>> should be a no-go, IMHO.
>
> I agree that this test needs to be updated for NewHash, but since we
> haven't decided what that's going to be, it makes sense during
> development to still test the rest of the code if possible so that we
> know what does and doesn't work.
>
> This is a first step in making it obvious what doesn't work, and when we
> know what the data is supposed to be, we can adjust it by fixing the
> tests so that it works in all cases.

A lingering feeling is, how do we find all these tests that we "hide" (for lack of a better word)? Maybe introduce some standardized comment keyword. Or use a more grep-friendly prereq-name. Or, if all occurances of "SHA1" (outside of t????-*sha*) are expected to go away real soon now, maybe this is not an issue.

Anyway, thanks for putting up with me, and thanks a big lot for driving this major undertaking forward.

Martin