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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Aug 7, 2012, 12:41 UTC
Message-ID: <20120807124105.GA913@tgummerer>
In-Reply-To: <7v8vds3jkj.fsf@alter.siamese.dyndns.org>

On 08/05, Junio C Hamano wrote:

Show 19 quoted lines

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

No, index-v5 doesn't have the trailing SHA-1 over the whole file.

Show 6 quoted lines

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

Show 5 quoted lines

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

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;