Re: [PATCH 1/7] refs: include committer info in `ref_update` struct

> On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak <karthik.188@gmail.com> 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,