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

Thomas Gummerer [off-list ref] writes:

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?

On 08/05, Junio C Hamano wrote:

Thomas Gummerer [off-list ref] writes:

quoted

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?

No, index-v5 doesn't have the trailing SHA-1 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.

v5 still has the num-entries word, but at a different position.
The +20 however would still be wrong, because of the missing
SHA-1 over the file.

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?

This part is called even before we know what version of the index
we will read, and before the file is mmaped.  The best solution
i think is to drop the check and just call verify_hdr, since it will 
check the checksum anyway and detect the error, while not having
a big cost on a index file that is very small.

quoted

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