Re: [PATCH 5/6] Allow to use crc32 as a lighter checksum on index
- From
Shawn Pearce <spearce@spearce.org>
- Date
- Feb 7, 2012, 03:17 UTC
- Message-ID
- <CAJo=hJvSyhv8EUh=6ROotc3Q=zQo7vbww_ShQJP3tf1T7s889g@mail.gmail.com>
- In-Reply-To
- <1328507319-24687-5-git-send-email-pclouds@gmail.com>
2012/2/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
Show 13 quoted lines
> if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
> return error("bad signature");
> - if (hdr->hdr_version != htonl(2) &&
> - hdr->hdr_version != htonl(3) &&
> - hdr->hdr_version != htonl(4))
> + if (hdr->hdr_version == htonl(2) ||
> + hdr->hdr_version == htonl(3))
> + do_crc = 0;
> + else if (hdr->hdr_version == htonl(4)) {
> + struct ext_cache_header *ehdr = (struct ext_cache_header *)hdr;
> + do_crc = ntohl(ehdr->hdr_flags) & CACHE_F_CRC;
> + }
> + elseIck. Ick. Ick. Please $DEITY no.
When it comes to data integrity codes in Git... PICK ONE AND STICK WITH IT.
If CRC-32 is good enough to protect the index content such that disk corruption is probably detectable with it, lets just switch to CRC-32 in index version 4. Don't make it optional with a new header field that wasn't there in version 3 and is now only able to accept 32 bits of flags before we have to go and create index version 5. We already have a cache extension system available with extension codes in the footer of the index file. We don't need YET ANOTHER EXTENSION SYSTEM.
If CRC-32 is not good enough, and we don't want to trust it (or really, YOU don't want to trust it) please do not then go and propose that a less knowledgeable user should switch to CRC-32 "because it is faster". If we don't want to rely on the error detection of CRC-32, then we should be using SHA-1. Or SHA-256.
I haven't really put a lot of thought into this. But I suspect CRC-32 is sufficient on the index file, until it gets so big that the probability of a bit flip going undetected is too high due to the size of the file, but then we are into the "huge" index size range that has you trying to swap out SHA-1 for CRC-32 because SHA-1 is too slow. Uhm no.
CRC-32 may be good enough, we use it inside of the pack-objects when doing repacking locally and don't want to inflate objects to check SHA-1, but do want to try and detect a random bit flip caused by a broken file copier. Thus far its held up well there. Given the very transient nature of the index file (and how it can be mostly rebuilt from a tree object and the working directory), CRC-32 might be good enough. But please pick one.