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

[Junio C Hamano <gitster@pobox.com>]

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

> 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?

> @@ -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;