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

[Christian Couder <christian.couder@gmail.com>]

On Sun, Jun 9, 2019 at 11:21 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Sun, Jun 09, 2019 at 10:24:55PM +0200, Christian Couder wrote:
> > On Sun, Jun 9, 2019 at 11:23 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > >
> > > New Perl dependencies always make Dscho sad... :)
> >
> > Yeah, I was not sure how to do it properly in shell so I was hoping I
> > would get suggestions about this. Thanks for looking at this!
> >
> > I could have hardcoded the values as it is done in t0011-hashmap.sh,
> > but I thought it was better to find a function that does he job.
>
> Well, I'm fine with hardcoding the expected hash values (in network
> byte order) as well, because then we won't add another git process
> upstream of a pipe that would pop up during audit later...

Ok, I think I will do that then.

> > > So, 'test oidmap' from the previous patch prints the value we want to
> > > check with:
> > >
> > >     printf("%u\n", sha1hash(oid.hash));
> >
> > Yeah, I did it this way because "test-hashmap.c" does the same kind of
> > thing to print hashes:
> >
> >             printf("%u %u %u %u\n",
> >                    strhash(p1), memhash(p1, strlen(p1)),
> >                    strihash(p1), memihash(p1, strlen(p1)));
> >
> > > First, since object ids inherently make more sense as hex values, it
> > > would be more appropriate to print that hash with the '%x' format
> > > specifier,
> >
> > I would be ok with that, but then I think it would make sense to also
> > print hex values in "test-hashmap.c".
> >
> > > and then we wouldn't need Perl's hex() anymore, and thus
> > > could swap the order of the first four bytes in oidmap's hash without
> > > relying on Perl, e.g. with:
> > >
> > >   sed -e 's/^\(..\)\(..\)\(..\)\(..\).*/\4\3\2\1/'
> > >
> > > Second, and more importantly, the need for swapping the byte order
> > > indicates that this test would fail on big-endian systems, I'm afraid.
> > > So I think we need an additional bswap32() on the printing side,
> >
> > Ok, but then shouldn't we also use bswap32() in "test-hashmap.c"?
>
> No.  The two test scripts/helpers work with different hashes.  t0011
> and 'test-hashmap.c' uses the various FNV-1-based hash functions
> (strhash(), memhash(), ...) to calculate an unsigned int hash of the
> items stored in the hashmap, therefore their hashes will be the same
> regardless of endianness.

I see. Thanks for explaining that.

>  In an oidmap, however, the hash is simply
> the first four bytes of the object id as an unsigned int as is,

Yeah, I had realized that.

Thanks,
Christian.