Derrick Stolee <[email protected]> writes:
> +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.
> + 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.
> + 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).
> 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.
