On Sun, Jun 9, 2019 at 11:23 AM SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote:
>
> On Sun, Jun 09, 2019 at 06:49:06AM +0200, Christian Couder wrote:
> > +
> > +test_oidmap() {
> > + echo "$1" | test-tool oidmap $3 > actual &&
> > + echo "$2" > expect &&
>
> Style nit: space between redirection op and filename.
Thanks for spotting this. It's fixed in my current version.
> > +test_oidhash() {
> > + git rev-parse "$1" | perl -ne 'print hex("$4$3$2$1") . "\n" if m/^(..)(..)(..)(..).*/;'
>
> 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.
> 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"?
By the way it seems that we use ntohl() or htonl() instead of
bswap32() in the source code.
> and then could further simplify 'test_oidhash':
>
> diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
> index 0ba122a264..4177912f9a 100644
> --- a/t/helper/test-oidmap.c
> +++ b/t/helper/test-oidmap.c
> @@ -51,7 +51,7 @@ int cmd__oidmap(int argc, const char **argv)
>
> /* print hash of oid */
> if (!get_oid(p1, &oid))
> - printf("%u\n", sha1hash(oid.hash));
> + printf("%x\n", bswap32(sha1hash(oid.hash)));
> else
> printf("Unknown oid: %s\n", p1);
>
> diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh
> index 3a8e8bdb3d..9c0d88a316 100755
> --- a/t/t0016-oidmap.sh
> +++ b/t/t0016-oidmap.sh
> @@ -22,10 +22,10 @@ test_expect_success 'setup' '
> '
>
> test_oidhash() {
> - git rev-parse "$1" | perl -ne 'print hex("$4$3$2$1") . "\n" if m/^(..)(..)(..)(..).*/;'
> + git rev-parse "$1" | cut -c1-8
> }
>
> -test_expect_success PERL 'hash' '
> +test_expect_success 'hash' '
Yeah, I agree that it seems better to me this way.
