Re: [PATCH v5 02/17] pack-mtimes: support reading .mtimes files

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

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/pack-mtimes.c b/pack-mtimes.c
> index 46ad584af1..0e0aafdcb0 100644
> --- a/pack-mtimes.c
> +++ b/pack-mtimes.c
> @@ -1,3 +1,4 @@
> +#include "git-compat-util.h"
>  #include "pack-mtimes.h"
>  #include "object-store.h"
>  #include "packfile.h"
> @@ -7,12 +8,10 @@ static char *pack_mtimes_filename(struct packed_git *p)
>  	size_t len;
>  	if (!strip_suffix(p->pack_name, ".pack", &len))
>  		BUG("pack_name does not end in .pack");
> -	/* NEEDSWORK: this could reuse code from pack-revindex.c. */
>  	return xstrfmt("%.*s.mtimes", (int)len, p->pack_name);
>  }
>
>  #define MTIMES_HEADER_SIZE (12)
> -#define MTIMES_MIN_SIZE (MTIMES_HEADER_SIZE + (2 * the_hash_algo->rawsz))
>
>  struct mtimes_header {
>  	uint32_t signature;
> @@ -26,10 +25,9 @@ static int load_pack_mtimes_file(char *mtimes_file,
>  {
>  	int fd, ret = 0;
>  	struct stat st;
> -	void *data = NULL;
> -	size_t mtimes_size;
> +	uint32_t *data = NULL;
> +	size_t mtimes_size, expected_size;
>  	struct mtimes_header header;
> -	uint32_t *hdr;
>
>  	fd = git_open(mtimes_file);
>
> @@ -44,21 +42,16 @@ static int load_pack_mtimes_file(char *mtimes_file,
>
>  	mtimes_size = xsize_t(st.st_size);
>
> -	if (mtimes_size < MTIMES_MIN_SIZE) {
> +	if (mtimes_size < MTIMES_HEADER_SIZE) {
>  		ret = error(_("mtimes file %s is too small"), mtimes_file);
>  		goto cleanup;
>  	}
>
> -	if (mtimes_size - MTIMES_MIN_SIZE != st_mult(sizeof(uint32_t), num_objects)) {
> -		ret = error(_("mtimes file %s is corrupt"), mtimes_file);
> -		goto cleanup;
> -	}
> +	data = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);
>
> -	data = hdr = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);
> -
> -	header.signature = ntohl(hdr[0]);
> -	header.version = ntohl(hdr[1]);
> -	header.hash_id = ntohl(hdr[2]);
> +	header.signature = ntohl(data[0]);
> +	header.version = ntohl(data[1]);
> +	header.hash_id = ntohl(data[2]);

So, instead of assuming that the size of the file is cast in stone
to match the size the current implementation happens to give and
reject a file from a future version, we check the header first to
give a more readable error when we see a version of the file that
we do not understand.

Makes sense.

At least, "here is a small fixup!" should have been accompanied by a
brief explanation to say something like that, i.e. why a fixup is
needed, what shortcoming in the original it is meant to address,
etc.

Will queue between 2/17 and 3/17 without squashing (yet).

Thanks.

>  	if (header.signature != MTIMES_SIGNATURE) {
>  		ret = error(_("mtimes file %s has unknown signature"), mtimes_file);
> @@ -77,13 +70,23 @@ static int load_pack_mtimes_file(char *mtimes_file,
>  		goto cleanup;
>  	}
>
> +
> +	expected_size = MTIMES_HEADER_SIZE;
> +	expected_size = st_add(expected_size, st_mult(sizeof(uint32_t), num_objects));
> +	expected_size = st_add(expected_size, 2 * (header.hash_id == 1 ? GIT_SHA1_RAWSZ : GIT_SHA256_RAWSZ));
> +
> +	if (mtimes_size != expected_size) {
> +		ret = error(_("mtimes file %s is corrupt"), mtimes_file);
> +		goto cleanup;
> +	}
> +
>  cleanup:
>  	if (ret) {
>  		if (data)
>  			munmap(data, mtimes_size);
>  	} else {
>  		*len_p = mtimes_size;
> -		*data_p = (const uint32_t *)data;
> +		*data_p = data;
>  	}
>
>  	close(fd);
> diff --git a/pack-mtimes.h b/pack-mtimes.h
> index 38ddb9f893..cc957b3e85 100644
> --- a/pack-mtimes.h
> +++ b/pack-mtimes.h
> @@ -8,8 +8,19 @@
>
>  struct packed_git;
>
> +/*
> + * Loads the .mtimes file corresponding to "p", if any, returning zero
> + * on success.
> + */
>  int load_pack_mtimes(struct packed_git *p);
>
> +/* Returns the mtime associated with the object at position "pos" (in
> + * lexicographic/index order) in pack "p".
> + *
> + * Note that it is a BUG() to call this function if either (a) "p" does
> + * not have a corresponding .mtimes file, or (b) it does, but it hasn't
> + * been loaded
> + */
>  uint32_t nth_packed_mtime(struct packed_git *p, uint32_t pos);
>
>  #endif
> --
> 2.36.1.94.gb0d54bedca
>
> --- >8 ---