refs/reftable: stop micro-optimizing refname allocations on copy

When copying refs, we execute `write_copy_table()` to write the new
table. As the names are given to us via `arg->newname` and
`arg->oldname`, respectively, we optimize away some allocations by
assigning those fields to the reftable records we are about to write
directly, without duplicating them. This requires us to cast the input
to `char *` pointers as they are in fact constant strings. Later on, we
then unset the refname for all of the records before calling
`reftable_log_record_release()` on them.

We also do this when assigning the "HEAD" constant, but here we do not
cast because its type is `char[]` by default. It's about to be turned
into `const char *` though once we enable `-Wwrite-strings` and will
thus cause another warning.

It's quite dubious whether this micro-optimization really helps. We're
about to write to disk anyway, which is going to be way slower than a
small handful of allocations. Let's drop the optimization altogther and
instead copy arguments to simplify the code and avoid the future warning
with `-Wwrite-strings`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 1af86bbdec9ddd816bc7166d77f8043d9730b273..e77faa2b9d13b196a593caeb706480900c35c57d 100644 (file)
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1340,10 +1340,10 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
         * old reference.
         */
        refs[0] = old_ref;
-       refs[0].refname = (char *)arg->newname;
+       refs[0].refname = xstrdup(arg->newname);
        refs[0].update_index = creation_ts;
        if (arg->delete_old) {
-               refs[1].refname = (char *)arg->oldname;
+               refs[1].refname = xstrdup(arg->oldname);
                refs[1].value_type = REFTABLE_REF_DELETION;
                refs[1].update_index = deletion_ts;
        }
@@ -1366,7 +1366,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
                ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
                memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
                fill_reftable_log_record(&logs[logs_nr], &committer_ident);
-               logs[logs_nr].refname = (char *)arg->newname;
+               logs[logs_nr].refname = xstrdup(arg->newname);
                logs[logs_nr].update_index = deletion_ts;
                logs[logs_nr].value.update.message =
                        xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
@@ -1387,7 +1387,13 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
                if (append_head_reflog) {
                        ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
                        logs[logs_nr] = logs[logs_nr - 1];
-                       logs[logs_nr].refname = "HEAD";
+                       logs[logs_nr].refname = xstrdup("HEAD");
+                       logs[logs_nr].value.update.name =
+                               xstrdup(logs[logs_nr].value.update.name);
+                       logs[logs_nr].value.update.email =
+                               xstrdup(logs[logs_nr].value.update.email);
+                       logs[logs_nr].value.update.message =
+                               xstrdup(logs[logs_nr].value.update.message);
                        logs_nr++;
                }
        }
@@ -1398,7 +1404,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
        ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
        memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
        fill_reftable_log_record(&logs[logs_nr], &committer_ident);
-       logs[logs_nr].refname = (char *)arg->newname;
+       logs[logs_nr].refname = xstrdup(arg->newname);
        logs[logs_nr].update_index = creation_ts;
        logs[logs_nr].value.update.message =
                xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
@@ -1430,7 +1436,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
                 */
                ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
                logs[logs_nr] = old_log;
-               logs[logs_nr].refname = (char *)arg->newname;
+               logs[logs_nr].refname = xstrdup(arg->newname);
                logs_nr++;
 
                /*
@@ -1439,7 +1445,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
                if (arg->delete_old) {
                        ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
                        memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
-                       logs[logs_nr].refname = (char *)arg->oldname;
+                       logs[logs_nr].refname = xstrdup(arg->oldname);
                        logs[logs_nr].value_type = REFTABLE_LOG_DELETION;
                        logs[logs_nr].update_index = old_log.update_index;
                        logs_nr++;
@@ -1462,13 +1468,11 @@ done:
        reftable_iterator_destroy(&it);
        string_list_clear(&skip, 0);
        strbuf_release(&errbuf);
-       for (i = 0; i < logs_nr; i++) {
-               if (!strcmp(logs[i].refname, "HEAD"))
-                       continue;
-               logs[i].refname = NULL;
+       for (i = 0; i < logs_nr; i++)
                reftable_log_record_release(&logs[i]);
-       }
        free(logs);
+       for (i = 0; i < ARRAY_SIZE(refs); i++)
+               reftable_ref_record_release(&refs[i]);
        reftable_ref_record_release(&old_ref);
        reftable_log_record_release(&old_log);
        return ret;