Christian Couder <christian.couder@xxxxxxxxx> writes:
> 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?
>
Indeed, passing NULL should be sufficient here, good catch.
>> 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?
>
It would if there wasn't a loop. Since we loop over multiple updates,
computing committer_ident for each would end up being expensive. So it
is done before the loop starts.
>> + c = committer_ident;
>> + }
>> +
>> + fill_reftable_log_record(log, &c);
>> log->update_index = ts;
>> log->refname = xstrdup(u->refname);
>> memcpy(log->value.update.new_hash,
Attachment:
signature.asc
Description: PGP signature
