Re: [PATCH 2/3] t: add t0016-oidmap.sh

From: Jeff King <peff@peff.net>
Date: Jun 13, 2019, 19:02 UTC
Message-ID: <20190613190229.GB27217@sigill.intra.peff.net>
In-Reply-To: <20190613175236.GF31952@szeder.dev>

On Thu, Jun 13, 2019 at 07:52:36PM +0200, SZEDER Gábor wrote:

Show 14 quoted lines

> > At the point where we are normalizing byte order of the hashes, I have
> > to wonder: why do we care about testing the hash value in the first
> > place? We care that oidmap can store and retrieve values, and that it
> > performs well. But as long as it does those things, I don't think
> > anybody cares if it uses the first 4 bytes of the sha1 or the last 4.
> > 
> > I know there are testing philosophies that go to this level of
> > white-box testing, but I don't think we usually do in Git. A unit
> > test of oidmap's externally visible behavior seems like the right
> > level to me.
> 
> That's a good point...  but then why does 't0011-hashmap.sh' do it in
> the first place?  As far as I understood this t0016 mainly follows
> suit of t0011.

I'd make the same argument against t0011. :)

I think there it at least made a little more sense because we truly are hashing ourselves, rather than just copying out some sha1 bytes. But I think I'd still argue that if I updated strhash() to use a different hash, I should not have to be updating t0011 to change out the hashes.

-Peff