Re: [PATCH v3 06/24] multi-pack-index: load into memory
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 9, 2018, 19:08 UTC
- Message-ID
- <xmqq36wsdy75.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180706005321.124643-7-dstolee@microsoft.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 23 quoted lines
> +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)
> + goto cleanup_fail;
> + if (fstat(fd, &st)) {
> + error_errno(_("failed to read %s"), midx_name);
> + goto cleanup_fail;
> + }
> +
> + midx_size = xsize_t(st.st_size);
> +
> + if (midx_size < MIDX_MIN_SIZE) {
> + close(fd);With the use of "do things normally and jump to cleanup-fail label" pattern, I think you do not want the close() here (unless you also assign -1 to fd yourself, but that is a pointless workaround). Another goto we see above after fstat() failure correctly omits it.
Show 5 quoted lines
> + error(_("multi-pack-index file %s is too small"), midx_name);
> + goto cleanup_fail;
> + }
> +
> + FREE_AND_NULL(midx_name);This correctly calls free-and-null not just free (otherwise we'd break the cleanup-fail procedure below), which is good.
> + 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);
Hmph, I thought we had FLEX_ALLOC_*() convenience functions exactly for doing things like this more safely.
Show 30 quoted lines
> + m->fd = fd;
> + m->data = midx_map;
> + m->data_len = midx_size;
> +
> + 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[MIDX_BYTE_FILE_VERSION];
> + if (m->version != MIDX_VERSION) {
> + error(_("multi-pack-index version %d not recognized"),
> + m->version);
> + goto cleanup_fail;
> + }
> +
> + hash_version = m->data[MIDX_BYTE_HASH_VERSION];
> + 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[MIDX_BYTE_NUM_CHUNKS];
> +
> + m->num_packs = get_be32(m->data + MIDX_BYTE_NUM_PACKS);
> +
> + return m;> +cleanup_fail: > + /* no need to check for NULL when freeing */
I wonder who the target reader of this comment is. We certainly are not in the business of C language newbies.
If this _were_ a commit that looked like this:
- if (ptr) - free(ptr); + /* no need to check for NULL when freeing */ + free(ptr);
then it might be more understandable, but it still is wrong (such a comment does not help understanding the new code, which is the only thing the people who read the comment sees, without knowing what was there previously---it belongs to the commit log message as a rationale to make that change).
Show 9 quoted lines
> diff --git a/midx.h b/midx.h > index dbdbe9f873..2d83dd9ec1 100644 > --- a/midx.h > +++ b/midx.h > @@ -1,6 +1,10 @@ > #ifndef __MIDX_H__ > #define __MIDX_H__ > > +struct multi_pack_index;
I actually was quite surprised that this struct is defined in object-store.h and not here. It feels the other way around.
The raw_object_store needs to know that such an in-core structure might exist as an optional feature in an object store, but as an optional feature, I suspect that it has a pointer to an instance of multi_pack_index, instead of embedding the struct itself in it, so I would have expected to see an "I am only telling you that there is a struct with this name, but I am leaving it opaque as you do not have any business looking inside the struct yourself. You only need to be aware of the type's existence and a pointer to it so that you can call helpers that know what's inside and that should be sufficient for your needs." decl like this in object-store.h and instead an actual implementation in here.