On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> If there is no `committer_info`
> provided, the reference backends default to using
> `git_committer_info(0)`. The field itself cannot be set to
> `git_committer_info(0)` since the values are dynamic and must be
> obtained right when the reflog is being committed.
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> struct strbuf sb = STRBUF_INIT;
> int ret = 0;
>
> + if (!committer)
> + committer = git_committer_info(0);
It looks like this is where we obtain the value "right when the reflog
is being committed".
> +
> strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);
> if (msg && *msg) {
> strbuf_addch(&sb, '\t');
> @@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> }
>
> static int files_log_ref_write(struct files_ref_store *refs,
> - const char *refname, const struct object_id *old_oid,
> - const struct object_id *new_oid, const char *msg,
> + const char *refname,
> + const struct object_id *old_oid,
> + const struct object_id *new_oid,
> + const char *committer_info, const char *msg,
> int flags, struct strbuf *err)
> {
> int logfd, result;
> @@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs,
>
> if (logfd < 0)
> return 0;
> - result = log_ref_write_fd(logfd, old_oid, new_oid,
> - git_committer_info(0), msg);
> + result = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg);
Here we just pass the committer_info to the above function.
> if (result) {
> struct strbuf sb = STRBUF_INIT;
> int save_errno = errno;
> @@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs,
> files_assert_main_repository(refs, "commit_ref_update");
>
> clear_loose_ref_cache(refs);
> - if (files_log_ref_write(refs, lock->ref_name,
> - &lock->old_oid, oid,
> + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL,
> logmsg, flags, err)) {
Here we don't have the info so we pass NULL.
> char *old_msg = strbuf_detach(err, NULL);
> strbuf_addf(err, "cannot update the ref '%s': %s",
> @@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs,
> if (head_ref && (head_flag & REF_ISSYMREF) &&
> !strcmp(head_ref, lock->ref_name)) {
> struct strbuf log_err = STRBUF_INIT;
> - if (files_log_ref_write(refs, "HEAD",
> - &lock->old_oid, oid,
> + if (files_log_ref_write(refs, "HEAD", &lock->old_oid,
> + oid, git_committer_info(0),
Here we don't have the info either, so I think we should also pass
NULL. It would then be computed "right when the reflog is being
committed" in the above function. No?
> logmsg, flags, &log_err)) {
> error("%s", log_err.buf);
> strbuf_release(&log_err);
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
It is not your fault but write_transaction_table() does the following
right at the beginning of the function:
committer_info = git_committer_info(0);
if (split_ident_line(&committer_ident, committer_info,
strlen(committer_info)))
BUG("failed splitting committer info");
but then 'committer_ident' is only used in the hunk you are changing:
> if (create_reflog) {
> + struct ident_split c;
> +
> ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
> log = &logs[logs_nr++];
> memset(log, 0, sizeof(*log));
>
> - fill_reftable_log_record(log, &committer_ident);
> + if (u->committer_info) {
> + if (split_ident_line(&c, u->committer_info,
> + strlen(u->committer_info)))
> + BUG("failed splitting committer info");
> + } else {
I would think it would be more efficient to only compute
'committer_ident' here, right before we use it if needed. Or is there
something I am missing?
> + c = committer_ident;
> + }
> +
> + fill_reftable_log_record(log, &c);
> log->update_index = ts;
> log->refname = xstrdup(u->refname);
> memcpy(log->value.update.new_hash,
