midx.c: write MIDX filenames to strbuf

To ask for the name of a MIDX and its corresponding .rev file, callers
invoke get_midx_filename() and get_midx_rev_filename(), respectively.
These both invoke xstrfmt(), allocating a chunk of memory which must be
freed later on.

This makes callers in pack-bitmap.c somewhat awkward. Specifically,
midx_bitmap_filename(), which is implemented like:

    return xstrfmt("%s-%s.bitmap",
                   get_midx_filename(midx->object_dir),
                   hash_to_hex(get_midx_checksum(midx)));

this leaks the second argument to xstrfmt(), which itself was allocated
with xstrfmt(). This caller could assign both the result of
get_midx_filename() and the outer xstrfmt() to a temporary variable,
remembering to free() the former before returning. But that involves a
wasteful copy.

Instead, get_midx_filename() and get_midx_rev_filename() take a strbuf
as an output parameter. This way midx_bitmap_filename() can manipulate
and pass around a temporary buffer which it detaches back to its caller.

That allows us to implement the function without copying or open-coding
get_midx_filename() in a way that doesn't leak.

Update the other callers of get_midx_filename() and
get_midx_rev_filename() accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/midx.c b/midx.c
index e529ca76c12dbeefc48287112c5f057ad885f4cc..696a938e720195afbde57064b7b0b5aa1f4fa771 100644 (file)
--- a/midx.c
+++ b/midx.c
@@ -57,15 +57,15 @@ const unsigned char *get_midx_checksum(struct multi_pack_index *m)
        return m->data + m->data_len - the_hash_algo->rawsz;
 }
 
-char *get_midx_filename(const char *object_dir)
+void get_midx_filename(struct strbuf *out, const char *object_dir)
 {
-       return xstrfmt("%s/pack/multi-pack-index", object_dir);
+       strbuf_addf(out, "%s/pack/multi-pack-index", object_dir);
 }
 
-char *get_midx_rev_filename(struct multi_pack_index *m)
+void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
 {
-       return xstrfmt("%s/pack/multi-pack-index-%s.rev",
-                      m->object_dir, hash_to_hex(get_midx_checksum(m)));
+       get_midx_filename(out, m->object_dir);
+       strbuf_addf(out, "-%s.rev", hash_to_hex(get_midx_checksum(m)));
 }
 
 static int midx_read_oid_fanout(const unsigned char *chunk_start,
@@ -89,28 +89,30 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
        size_t midx_size;
        void *midx_map = NULL;
        uint32_t hash_version;
-       char *midx_name = get_midx_filename(object_dir);
+       struct strbuf midx_name = STRBUF_INIT;
        uint32_t i;
        const char *cur_pack_name;
        struct chunkfile *cf = NULL;
 
-       fd = git_open(midx_name);
+       get_midx_filename(&midx_name, object_dir);
+
+       fd = git_open(midx_name.buf);
 
        if (fd < 0)
                goto cleanup_fail;
        if (fstat(fd, &st)) {
-               error_errno(_("failed to read %s"), midx_name);
+               error_errno(_("failed to read %s"), midx_name.buf);
                goto cleanup_fail;
        }
 
        midx_size = xsize_t(st.st_size);
 
        if (midx_size < MIDX_MIN_SIZE) {
-               error(_("multi-pack-index file %s is too small"), midx_name);
+               error(_("multi-pack-index file %s is too small"), midx_name.buf);
                goto cleanup_fail;
        }
 
-       FREE_AND_NULL(midx_name);
+       strbuf_release(&midx_name);
 
        midx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);
        close(fd);
@@ -184,7 +186,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 
 cleanup_fail:
        free(m);
-       free(midx_name);
+       strbuf_release(&midx_name);
        free_chunkfile(cf);
        if (midx_map)
                munmap(midx_map, midx_size);
@@ -1115,7 +1117,7 @@ static int write_midx_internal(const char *object_dir,
                               const char *refs_snapshot,
                               unsigned flags)
 {
-       char *midx_name;
+       struct strbuf midx_name = STRBUF_INIT;
        unsigned char midx_hash[GIT_MAX_RAWSZ];
        uint32_t i;
        struct hashfile *f = NULL;
@@ -1130,10 +1132,10 @@ static int write_midx_internal(const char *object_dir,
        /* Ensure the given object_dir is local, or a known alternate. */
        find_odb(the_repository, object_dir);
 
-       midx_name = get_midx_filename(object_dir);
-       if (safe_create_leading_directories(midx_name))
+       get_midx_filename(&midx_name, object_dir);
+       if (safe_create_leading_directories(midx_name.buf))
                die_errno(_("unable to create leading directories of %s"),
-                         midx_name);
+                         midx_name.buf);
 
        if (!packs_to_include) {
                /*
@@ -1367,7 +1369,7 @@ static int write_midx_internal(const char *object_dir,
                pack_name_concat_len += MIDX_CHUNK_ALIGNMENT -
                                        (pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
 
-       hold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);
+       hold_lock_file_for_update(&lk, midx_name.buf, LOCK_DIE_ON_ERROR);
        f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 
        if (ctx.nr - dropped_packs == 0) {
@@ -1404,9 +1406,9 @@ static int write_midx_internal(const char *object_dir,
                ctx.pack_order = midx_pack_order(&ctx);
 
        if (flags & MIDX_WRITE_REV_INDEX)
-               write_midx_reverse_index(midx_name, midx_hash, &ctx);
+               write_midx_reverse_index(midx_name.buf, midx_hash, &ctx);
        if (flags & MIDX_WRITE_BITMAP) {
-               if (write_midx_bitmap(midx_name, midx_hash, &ctx,
+               if (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,
                                      refs_snapshot, flags) < 0) {
                        error(_("could not write multi-pack bitmap"));
                        result = 1;
@@ -1435,7 +1437,7 @@ cleanup:
        free(ctx.entries);
        free(ctx.pack_perm);
        free(ctx.pack_order);
-       free(midx_name);
+       strbuf_release(&midx_name);
 
        return result;
 }
@@ -1499,20 +1501,22 @@ static void clear_midx_files_ext(const char *object_dir, const char *ext,
 
 void clear_midx_file(struct repository *r)
 {
-       char *midx = get_midx_filename(r->objects->odb->path);
+       struct strbuf midx = STRBUF_INIT;
+
+       get_midx_filename(&midx, r->objects->odb->path);
 
        if (r->objects && r->objects->multi_pack_index) {
                close_midx(r->objects->multi_pack_index);
                r->objects->multi_pack_index = NULL;
        }
 
-       if (remove_path(midx))
-               die(_("failed to clear multi-pack-index at %s"), midx);
+       if (remove_path(midx.buf))
+               die(_("failed to clear multi-pack-index at %s"), midx.buf);
 
        clear_midx_files_ext(r->objects->odb->path, ".bitmap", NULL);
        clear_midx_files_ext(r->objects->odb->path, ".rev", NULL);
 
-       free(midx);
+       strbuf_release(&midx);
 }
 
 static int verify_midx_error;
@@ -1565,12 +1569,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
        if (!m) {
                int result = 0;
                struct stat sb;
-               char *filename = get_midx_filename(object_dir);
-               if (!stat(filename, &sb)) {
+               struct strbuf filename = STRBUF_INIT;
+
+               get_midx_filename(&filename, object_dir);
+
+               if (!stat(filename.buf, &sb)) {
                        error(_("multi-pack-index file exists, but failed to parse"));
                        result = 1;
                }
-               free(filename);
+               strbuf_release(&filename);
                return result;
        }
 

diff --git a/midx.h b/midx.h
index 6e32297fa3aaf0edeea852c5760e328b3ed5d061..b7d79a515c63a73422755737d789b0c2febe22a9 100644 (file)
--- a/midx.h
+++ b/midx.h
@@ -48,8 +48,8 @@ struct multi_pack_index {
 #define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)
 
 const unsigned char *get_midx_checksum(struct multi_pack_index *m);
-char *get_midx_filename(const char *object_dir);
-char *get_midx_rev_filename(struct multi_pack_index *m);
+void get_midx_filename(struct strbuf *out, const char *object_dir);
+void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m);
 
 struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);
 int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);

diff --git a/pack-bitmap.c b/pack-bitmap.c
index f47a0a7db4dd30a6450b3e59e0ec463c5578da39..3f603425c9bbe2c8dae53e344458459108e6390a 100644 (file)
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -292,9 +292,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 
 char *midx_bitmap_filename(struct multi_pack_index *midx)
 {
-       return xstrfmt("%s-%s.bitmap",
-                      get_midx_filename(midx->object_dir),
-                      hash_to_hex(get_midx_checksum(midx)));
+       struct strbuf buf = STRBUF_INIT;
+
+       get_midx_filename(&buf, midx->object_dir);
+       strbuf_addf(&buf, "-%s.bitmap", hash_to_hex(get_midx_checksum(midx)));
+
+       return strbuf_detach(&buf, NULL);
 }
 
 char *pack_bitmap_filename(struct packed_git *p)
@@ -324,10 +327,12 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
        }
 
        if (bitmap_git->pack || bitmap_git->midx) {
+               struct strbuf buf = STRBUF_INIT;
+               get_midx_filename(&buf, midx->object_dir);
                /* ignore extra bitmap file; we can only handle one */
-               warning("ignoring extra bitmap file: %s",
-                       get_midx_filename(midx->object_dir));
+               warning("ignoring extra bitmap file: %s", buf.buf);
                close(fd);
+               strbuf_release(&buf);
                return -1;
        }
 

diff --git a/pack-revindex.c b/pack-revindex.c
index 0e4a31d9db9e71aa67ac6812241b064f01bb02a2..70d0fbafcbf954e33b58e6fb79dd173282f59eea 100644 (file)
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -296,14 +296,14 @@ int load_pack_revindex(struct packed_git *p)
 
 int load_midx_revindex(struct multi_pack_index *m)
 {
-       char *revindex_name;
+       struct strbuf revindex_name = STRBUF_INIT;
        int ret;
        if (m->revindex_data)
                return 0;
 
-       revindex_name = get_midx_rev_filename(m);
+       get_midx_rev_filename(&revindex_name, m);
 
-       ret = load_revindex_from_disk(revindex_name,
+       ret = load_revindex_from_disk(revindex_name.buf,
                                      m->num_objects,
                                      &m->revindex_map,
                                      &m->revindex_len);
@@ -313,7 +313,7 @@ int load_midx_revindex(struct multi_pack_index *m)
        m->revindex_data = (const uint32_t *)((const char *)m->revindex_map + RIDX_HEADER_SIZE);
 
 cleanup:
-       free(revindex_name);
+       strbuf_release(&revindex_name);
        return ret;
 }