| Message ID | 20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add reflog support to `git refs migrate` | expand |
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,
Christian Couder <christian.couder@gmail.com> writes: > 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? > 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,
diff --git a/refs.c b/refs.c index 762f3e324d59c60cd4f05c2f257e54de8deb00e5..f003e51c6bf5229bfbce8ce61ffad7cdba0572e0 100644 --- a/refs.c +++ b/refs.c @@ -1151,6 +1151,7 @@ void ref_transaction_free(struct ref_transaction *transaction) for (i = 0; i < transaction->nr; i++) { free(transaction->updates[i]->msg); + free(transaction->updates[i]->committer_info); free((char *)transaction->updates[i]->new_target); free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); 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); + 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); 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)) { 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), logmsg, flags, &log_err)) { error("%s", log_err.buf); strbuf_release(&log_err); @@ -2969,7 +2972,8 @@ static int parse_and_write_reflog(struct files_ref_store *refs, } if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, - &update->new_oid, update->msg, update->flags, err)) { + &update->new_oid, update->committer_info, + update->msg, update->flags, err)) { char *old_msg = strbuf_detach(err, NULL); strbuf_addf(err, "cannot update the ref '%s': %s", diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 58aa56d1b27c85d606ed7c8c0d908e4b87d1066b..0fd95cdacd99e4a728c22f5286f6b3f0f360c110 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -113,6 +113,7 @@ struct ref_update { void *backend_data; unsigned int type; char *msg; + char *committer_info; /* * If this ref_update was split off of a symref update via 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 } 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 { + c = committer_ident; + } + + fill_reftable_log_record(log, &c); log->update_index = ts; log->refname = xstrdup(u->refname); memcpy(log->value.update.new_hash,
The reference backends obtain the committer information from `git_committer_info(0)` when adding a reflog. The upcoming patches introduce support for migrating reflogs between the reference backends. This requires an interface to creating reflogs, including custom committer information. Add a new field `committer_info` to the `ref_update` struct, which is then used by the reference backends. 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. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- refs.c | 1 + refs/files-backend.c | 22 +++++++++++++--------- refs/refs-internal.h | 1 + refs/reftable-backend.c | 12 +++++++++++- 4 files changed, 26 insertions(+), 10 deletions(-)