Re: [PATCH/RFC v2 01/16] Modify cache_header to prepare for other index formats

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 6, 2012, 01:17 UTC
Message-ID: <7v8vds3jkj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1344203353-2819-2-git-send-email-t.gummerer@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> writes:

Show 11 quoted lines

> diff --git a/read-cache.c b/read-cache.c
> index 2f8159f..5d61d92 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1433,7 +1446,7 @@ int read_index_from(struct index_state *istate, const char *path)
>  
>  	errno = EINVAL;
>  	mmap_size = xsize_t(st.st_size);
> -	if (mmap_size < sizeof(struct cache_header) + 20)
> +	if (mmap_size < sizeof(struct cache_version_header) + 20)
>  		die("index file smaller than expected");

At the design level, I have a large problem with this change. I understand that you wanted to make sure that some versions can lack the num-entries word in the header, but then what is the point of keeping that "+20" here? Are all versions of the file format still required to have the 20-byte trailing SHA-1 sum over the whole file?

	Side note: I am actually fine with that "sum at the end"
	requirement, but then it needs to be documented what are
	assumed to be unomittable and why.

        I also do not see why v5 *needs* to drop the num-entries
        word from the header in the first place.

At the practical level, we used to error out, upon seeing a file that claims to be v2 in the header but is too small to hold the version header, the number of entries word and the trailing SHA-1 sum. We no longer do this and happily call verify_hdr() in the following code even when the file is too small, no?

Show 7 quoted lines

> @@ -1442,11 +1455,13 @@ int read_index_from(struct index_state *istate, const char *path)
>  		die_errno("unable to map index file");
>  
>  	hdr = mmap;
> +	hdr_v2 =  mmap + sizeof(*hdr);
>  	if (verify_hdr(hdr, mmap_size) < 0)
>  		goto unmap;