Derrick Stolee <stolee@xxxxxxxxx> writes:
> +#define MIDX_HASH_LEN 20
> +#define MIDX_MIN_SIZE (MIDX_HEADER_SIZE + MIDX_HASH_LEN)
>
> static char *get_midx_filename(const char *object_dir)
> {
> return xstrfmt("%s/pack/multi-pack-index", object_dir);
> }
>
> +struct multi_pack_index *load_multi_pack_index(const char *object_dir)
> +{
> + struct multi_pack_index *m = NULL;
> + int fd;
> + struct stat st;
> + size_t midx_size;
> + void *midx_map = NULL;
> + uint32_t hash_version;
> + char *midx_name = get_midx_filename(object_dir);
> +
> + fd = git_open(midx_name);
> +
> + if (fd < 0) {
> + error_errno(_("failed to read %s"), midx_name);
> + FREE_AND_NULL(midx_name);
> + return NULL;
> + }
> + if (fstat(fd, &st)) {
> + error_errno(_("failed to read %s"), midx_name);
> + FREE_AND_NULL(midx_name);
> + close(fd);
> + return NULL;
> + }
> +
> + midx_size = xsize_t(st.st_size);
> +
> + if (midx_size < MIDX_MIN_SIZE) {
> + close(fd);
> + error(_("multi-pack-index file %s is too small"), midx_name);
> + goto cleanup_fail;
> + }
> +
> + FREE_AND_NULL(midx_name);
Error handling in the above part looks a bit inconsistent. I first
thought that the earlier ones manually clean up and leave because
jumping to cleanup_fail would need a successfully opened fd and
successfully mmapped midx_map, but the above "goto" forces
cleanup_fail: to munmap NULL and close an already closed fd.
I wonder if it is simpler to do
cleanup_fail:
/* no need to check for NULL when freeing */
free(m);
free(midx_name);
if (midx_map)
munmap(midx_map, midx_size);
if (0 <= fd)
close(fd);
return NULL;
and have all of the above error codepath to jump there.
> + midx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +
> + m = xcalloc(1, sizeof(*m) + strlen(object_dir) + 1);
> + strcpy(m->object_dir, object_dir);
> + m->data = midx_map;
> +
> + m->signature = get_be32(m->data);
> + if (m->signature != MIDX_SIGNATURE) {
> + error(_("multi-pack-index signature 0x%08x does not match signature 0x%08x"),
> + m->signature, MIDX_SIGNATURE);
> + goto cleanup_fail;
> + }
> +
> + m->version = m->data[4];
> + if (m->version != MIDX_VERSION) {
> + error(_("multi-pack-index version %d not recognized"),
> + m->version);
> + goto cleanup_fail;
> + }
> +
> + hash_version = m->data[5];
Is there a good existing example to show a better way to avoid these
hard-coded constants that describe/define the file format?
> + if (hash_version != MIDX_HASH_VERSION) {
> + error(_("hash version %u does not match"), hash_version);
> + goto cleanup_fail;
> + }
> + m->hash_len = MIDX_HASH_LEN;
> +
> + m->num_chunks = *(m->data + 6);
By the way, this mixture of m->data[4] and *(m->data + 6) is even
worse. You could do get_be32(&8[m->data]) if you want to irritate
readers even more ;-)
> + m->num_packs = get_be32(m->data + 8);
> +
> + return m;
> +
> +cleanup_fail:
> + FREE_AND_NULL(m);
> + FREE_AND_NULL(midx_name);
> + munmap(midx_map, midx_size);
> + close(fd);
> + return NULL;
> +}
> +
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 8622a7cdce..0372704c96 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -3,9 +3,19 @@
> test_description='multi-pack-indexes'
> . ./test-lib.sh
>
> +midx_read_expect() {
"midx_read_expect () {", i.e. SP on both sides of (), please.
> + cat >expect <<- EOF
"<<-\EOF", i.e. make it easy for readers to spot that there is no
funny substitutions happening in the here-doc body.
> + header: 4d494458 1 0 0
> + object_dir: .
> + EOF
> + test-tool read-midx . >actual &&
> + test_cmp expect actual
> +}
> +
> test_expect_success 'write midx with no packs' '
> git multi-pack-index --object-dir=. write &&
> - test_path_is_file pack/multi-pack-index
> + test_path_is_file pack/multi-pack-index &&
> + midx_read_expect
> '
>
> test_done
