Re: [PATCH 1/7] refs: include committer info in `ref_update` struct
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Dec 10, 2024, 16:51 UTC
- Message-ID
- <CAP8UFD3FXtbuHH1G==rR00_O7MBjqQiw_qzGHPVLK4P13RRtjQ@mail.gmail.com>
- In-Reply-To
- <20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f@gmail.com>
On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak <karthik.188@gmail.com> wrote:
Show 5 quoted lines
> 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.
Show 10 quoted lines
> 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".
Show 24 quoted lines
> +
> 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.
Show 11 quoted lines
> 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.
Show 10 quoted lines
> 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);Show 5 quoted lines
> 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:
Show 13 quoted lines
> 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?
Show 7 quoted lines
> + c = committer_ident; > + } > + > + fill_reftable_log_record(log, &c); > log->update_index = ts; > log->refname = xstrdup(u->refname); > memcpy(log->value.update.new_hash,