patch, 7 partsrefs: add reflog support to `git refs migrate`

> 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);

> +
>         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),

> 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,

> 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,

>
> When refs are updated in the files-backend, a lock is obtained for the
> corresponding file path. This is the case even for reflogs, i.e. a lock
> is obtained on the reference path instead of the reflog path. This
> works, since generally, reflogs are updated alongside the ref.
>
> The upcoming patches will add support for reflog updates in ref
> transaction. This means, in a particular transaction we want to have ref
> updates and reflog updates. For refs, in a given transaction there can
> only be one update. But, we can theoretically have multiple reflog
> updates in a given transaction.

> @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>                         goto out;
>         }
>
> -       ret = lock_raw_ref(refs, update->refname, mustexist,
> -                          affected_refnames,
> -                          &lock, &referent,
> -                          &update->type, err);
> -       if (ret) {
> -               char *reason;
> +       lock = strmap_get(&backend_data->ref_locks, update->refname);
> +       if (lock) {
> +               lock->count = lock->count + 1;

> +       } else {
> +               ret = lock_raw_ref(refs, update->refname, mustexist,
> +                                  affected_refnames,
> +                                  &lock, &referent,
> +                                  &update->type, err);
> +               if (ret) {
> +                       char *reason;
> +
> +                       reason = strbuf_detach(err, NULL);
> +                       strbuf_addf(err, "cannot lock ref '%s': %s",
> +                                   ref_update_original_update_refname(update), reason);
> +                       free(reason);
> +                       goto out;
> +               }
>
> -               reason = strbuf_detach(err, NULL);
> -               strbuf_addf(err, "cannot lock ref '%s': %s",
> -                           ref_update_original_update_refname(update), reason);
> -               free(reason);
> -               goto out;
> +               strmap_put(&backend_data->ref_locks, update->refname, lock);
>         }
>
>         update->backend_data = lock;

> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> When refs are updated in the files-backend, a lock is obtained for the
>> corresponding file path. This is the case even for reflogs, i.e. a lock
>> is obtained on the reference path instead of the reflog path. This
>> works, since generally, reflogs are updated alongside the ref.
>>
>> The upcoming patches will add support for reflog updates in ref
>> transaction. This means, in a particular transaction we want to have ref
>> updates and reflog updates. For refs, in a given transaction there can
>> only be one update. But, we can theoretically have multiple reflog
>> updates in a given transaction.
>
> Nit: Giving an example might help understand where multiple reflog
> updates can happen in a given transaction. Alternatively pointing to
> an existing doc that contains such an example or explanations might
> help too.
>

>> @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>                         goto out;
>>         }
>>
>> -       ret = lock_raw_ref(refs, update->refname, mustexist,
>> -                          affected_refnames,
>> -                          &lock, &referent,
>> -                          &update->type, err);
>> -       if (ret) {
>> -               char *reason;
>> +       lock = strmap_get(&backend_data->ref_locks, update->refname);
>> +       if (lock) {
>> +               lock->count = lock->count + 1;
>
> Nit:
>               lock->count++;
>

>
> When refs are updated in the files-backend, a lock is obtained for the
> corresponding file path. This is the case even for reflogs, i.e. a lock
> is obtained on the reference path instead of the reflog path. This
> works, since generally, reflogs are updated alongside the ref.
>
> The upcoming patches will add support for reflog updates in ref
> transaction. This means, in a particular transaction we want to have ref
> updates and reflog updates. For refs, in a given transaction there can
> only be one update.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -71,6 +71,8 @@ struct ref_lock {
>         char *ref_name;
>         struct lock_file lk;
>         struct object_id old_oid;
> +       /* count keeps track of users of the lock */
> +       unsigned int count;

> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> When refs are updated in the files-backend, a lock is obtained for the
>> corresponding file path. This is the case even for reflogs, i.e. a lock
>> is obtained on the reference path instead of the reflog path. This
>> works, since generally, reflogs are updated alongside the ref.
>>
>> The upcoming patches will add support for reflog updates in ref
>> transaction. This means, in a particular transaction we want to have ref
>> updates and reflog updates. For refs, in a given transaction there can
>> only be one update.
>
> Maybe something like: "For a given ref in a given transaction there
> can be at most one update."
>

>> But, we can theoretically have multiple reflog
>> updates in a given transaction.
>
> And: "But we can theoretically have multiple reflog updates for a
> given ref in a given transaction."
>

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -71,6 +71,8 @@ struct ref_lock {
>>         char *ref_name;
>>         struct lock_file lk;
>>         struct object_id old_oid;
>> +       /* count keeps track of users of the lock */
>> +       unsigned int count;
>
> Nit: maybe the following is a bit better:
>
>       unsigned int count; /* track users of the lock (ref update +
> reflog updates) */

>
> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,
> the refname of the update is verified for:
>
>   - Ensuring it is not a pseudoref.
>   - Checking the refname format.
>
> These checks are also be needed in a following commit where the function

> to add reflog updates to the transaction is introduced. Extract the code
> out into a new static function.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(
>         return update;
>  }
>
> +static int transaction_refname_verification(const char *refname,
> +                                           const struct object_id *new_oid,
> +                                           unsigned int flags,
> +                                           struct strbuf *err)

> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,
>> the refname of the update is verified for:
>>
>>   - Ensuring it is not a pseudoref.
>>   - Checking the refname format.
>>
>> These checks are also be needed in a following commit where the function
>
> s/are also be needed/will also be needed/
>

>> to add reflog updates to the transaction is introduced. Extract the code
>> out into a new static function.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs.c | 43 ++++++++++++++++++++++++++++---------------
>>  1 file changed, 28 insertions(+), 15 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(
>>         return update;
>>  }
>>
>> +static int transaction_refname_verification(const char *refname,
>> +                                           const struct object_id *new_oid,
>> +                                           unsigned int flags,
>> +                                           struct strbuf *err)
>
> We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while
> I couldn't find any 'yyy_verification()' function, so it might be
> better to name it 'transaction_refname_valid()' or maybe
> 'transaction_refname_ok()'.
>

>> +       ret = transaction_refname_verification(refname, new_oid, flags, err);
>> +       if (ret)
>> +               return ret;
>
> Then the above could be just:
>
>        if (!transaction_refname_valid(refname, new_oid, flags, err))
>                return -1;

> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
> > diff --git a/refs.c b/refs.c
> > index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(
> >         return update;
> >  }
> >
> > +static int transaction_refname_verification(const char *refname,
> > +                                           const struct object_id *new_oid,
> > +                                           unsigned int flags,
> > +                                           struct strbuf *err)
> 
> We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while
> I couldn't find any 'yyy_verification()' function, so it might be
> better to name it 'transaction_refname_valid()' or maybe
> 'transaction_refname_ok()'.
> 
> Also I think it should probably return a bool so 1 if the refname is
> valid and 0 otherwise, unless we have plans in the future to follow
> different code paths depending on the different ways it is not valid.
> 
> > +       ret = transaction_refname_verification(refname, new_oid, flags, err);
> > +       if (ret)
> > +               return ret;
> 
> Then the above could be just:
> 
>        if (!transaction_refname_valid(refname, new_oid, flags, err))
>                return -1;

>
> Introduce a new function `ref_transaction_update_reflog`, for clients to
> add a reflog update to a transaction. While the existing function
> `ref_transaction_update` also allows clients to add a reflog entry, this
> function does a few things more, It:
>   - Enforces that only a reflog entry is added and does not update the
>   ref itself.
>   - Allows the users to also provide the committer information. This
>   means clients can add reflog entries with custom committer
>   information.

> A follow up commit will utilize this function to add reflog support to
> `git refs migrate`.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c                  | 89 +++++++++++++++++++++++++++++++++++++------------
>  refs.h                  | 12 +++++++
>  refs/files-backend.c    | 48 +++++++++++++++-----------
>  refs/refs-internal.h    | 16 +++++----
>  refs/reftable-backend.c |  6 ++--
>  5 files changed, 122 insertions(+), 49 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)
>         free(transaction);
>  }
>
> -struct ref_update *ref_transaction_add_update(
> -               struct ref_transaction *transaction,
> -               const char *refname, unsigned int flags,
> -               const struct object_id *new_oid,
> -               const struct object_id *old_oid,
> -               const char *new_target, const char *old_target,
> -               const char *msg)
> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,
> +                                             const char *refname,
> +                                             unsigned int flags,
> +                                             const struct object_id *new_oid,
> +                                             const struct object_id *old_oid,
> +                                             const char *new_target,
> +                                             const char *old_target,
> +                                             const char *committer_info,

> +                                             const char *msg)
>  {
>         struct ref_update *update;
>
> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(
>                 oidcpy(&update->new_oid, new_oid);
>         if ((flags & REF_HAVE_OLD) && old_oid)
>                 oidcpy(&update->old_oid, old_oid);
> -       if (!(flags & REF_SKIP_CREATE_REFLOG))
> +       if (!(flags & REF_SKIP_CREATE_REFLOG)) {
> +               if (committer_info) {
> +                       struct strbuf sb = STRBUF_INIT;
> +                       strbuf_addstr(&sb, committer_info);
> +                       update->committer_info = strbuf_detach(&sb, NULL);

> +               }
> +
>                 update->msg = normalize_reflog_message(msg);
> +       }
>
>         return update;
>  }
> @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update(
>  static int transaction_refname_verification(const char *refname,
>                                             const struct object_id *new_oid,
>                                             unsigned int flags,
> +                                           unsigned int reflog,
>                                             struct strbuf *err)
>  {
>         if (flags & REF_SKIP_REFNAME_VERIFICATION)
>                 return 0;
>
>         if (is_pseudo_ref(refname)) {
> -               strbuf_addf(err, _("refusing to update pseudoref '%s'"),
> -                           refname);
> +               if (reflog)
> +                       strbuf_addf(err, _("refusing to update reflog for pseudoref '%s'"),
> +                                   refname);
> +               else
> +                       strbuf_addf(err, _("refusing to update pseudoref '%s'"),
> +                                   refname);

>                 return -1;
>         } else if ((new_oid && !is_null_oid(new_oid)) ?
>                  check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
>                  !refname_is_safe(refname)) {
> -               strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
> -                           refname);
> +               if (reflog)
> +                       strbuf_addf(err, _("refusing to update reflog with bad name '%s'"),
> +                                   refname);
> +               else
> +                       strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
> +                                   refname);

>  int ref_transaction_create(struct ref_transaction *transaction,
> -                          const char *refname,
> -                          const struct object_id *new_oid,
> -                          const char *new_target,
> -                          unsigned int flags, const char *msg,
> -                          struct strbuf *err)
> +                          const char *refname, const struct object_id *new_oid,
> +                          const char *new_target, unsigned int flags,
> +                          const char *msg, struct strbuf *err)

> index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction,
>                            unsigned int flags, const char *msg,
>                            struct strbuf *err);
>
> +/*
> + * Similar to `ref_transaction_update`, but this function is only for adding
> + * a reflog updates.

>> diff --git a/refs.c b/refs.c
>> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)
>>         free(transaction);
>>  }
>>
>> -struct ref_update *ref_transaction_add_update(
>> -               struct ref_transaction *transaction,
>> -               const char *refname, unsigned int flags,
>> -               const struct object_id *new_oid,
>> -               const struct object_id *old_oid,
>> -               const char *new_target, const char *old_target,
>> -               const char *msg)
>> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,
>> +                                             const char *refname,
>> +                                             unsigned int flags,
>> +                                             const struct object_id *new_oid,
>> +                                             const struct object_id *old_oid,
>> +                                             const char *new_target,
>> +                                             const char *old_target,
>> +                                             const char *committer_info,
>
> This change (adding a 'const char *committer_info' argument to
> ref_transaction_add_update()) is not described in the commit message
> and it requires a number of changes to the callers of this function,
> so I think it might want to be in its own preparatory commit before
> this one.
>

>> +                                             const char *msg)
>>  {
>>         struct ref_update *update;
>>
>> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(
>>                 oidcpy(&update->new_oid, new_oid);
>>         if ((flags & REF_HAVE_OLD) && old_oid)
>>                 oidcpy(&update->old_oid, old_oid);
>> -       if (!(flags & REF_SKIP_CREATE_REFLOG))
>> +       if (!(flags & REF_SKIP_CREATE_REFLOG)) {
>> +               if (committer_info) {
>> +                       struct strbuf sb = STRBUF_INIT;
>> +                       strbuf_addstr(&sb, committer_info);
>> +                       update->committer_info = strbuf_detach(&sb, NULL);
>
> Maybe:
>                       update->committer_info = xstrdup(committer_info);
>

>> +               }
>> +
>>                 update->msg = normalize_reflog_message(msg);
>> +       }
>>
>>         return update;
>>  }
>> @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update(
>>  static int transaction_refname_verification(const char *refname,
>>                                             const struct object_id *new_oid,
>>                                             unsigned int flags,
>> +                                           unsigned int reflog,
>>                                             struct strbuf *err)
>>  {
>>         if (flags & REF_SKIP_REFNAME_VERIFICATION)
>>                 return 0;
>>
>>         if (is_pseudo_ref(refname)) {
>> -               strbuf_addf(err, _("refusing to update pseudoref '%s'"),
>> -                           refname);
>> +               if (reflog)
>> +                       strbuf_addf(err, _("refusing to update reflog for pseudoref '%s'"),
>> +                                   refname);
>> +               else
>> +                       strbuf_addf(err, _("refusing to update pseudoref '%s'"),
>> +                                   refname);
>
> Maybe:
>
>               const char *what = reflog ? "reflog for pseudoref" : "pseudoref";
>               strbuf_addf(err, _("refusing to update %s '%s'"), what, refname);
>

>>                 return -1;
>>         } else if ((new_oid && !is_null_oid(new_oid)) ?
>>                  check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
>>                  !refname_is_safe(refname)) {
>> -               strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
>> -                           refname);
>> +               if (reflog)
>> +                       strbuf_addf(err, _("refusing to update reflog with bad name '%s'"),
>> +                                   refname);
>> +               else
>> +                       strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
>> +                                   refname);
>
> Maybe:
>
>               const char *what = reflog ? "reflog with bad name" :
> "ref with bad name";
>               strbuf_addf(err, _("refusing to update %s '%s'"), what, refname);
>

>>                 return -1;
>>         }
>
> [...]
>
>>  int ref_transaction_create(struct ref_transaction *transaction,
>> -                          const char *refname,
>> -                          const struct object_id *new_oid,
>> -                          const char *new_target,
>> -                          unsigned int flags, const char *msg,
>> -                          struct strbuf *err)
>> +                          const char *refname, const struct object_id *new_oid,
>> +                          const char *new_target, unsigned int flags,
>> +                          const char *msg, struct strbuf *err)
>
> This looks like a wrapping or indenting only change. If you really
> want to do it, it should probably be in its own preparatory commit.
>

>> index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>                            unsigned int flags, const char *msg,
>>                            struct strbuf *err);
>>
>> +/*
>> + * Similar to `ref_transaction_update`, but this function is only for adding
>> + * a reflog updates.
>
> "a reflog update" or "reflog updates".

> diff --git a/refs.c b/refs.c
> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)
>  	free(transaction);
>  }
>  
> -struct ref_update *ref_transaction_add_update(
> -		struct ref_transaction *transaction,
> -		const char *refname, unsigned int flags,
> -		const struct object_id *new_oid,
> -		const struct object_id *old_oid,
> -		const char *new_target, const char *old_target,
> -		const char *msg)
> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,
> +					      const char *refname,
> +					      unsigned int flags,
> +					      const struct object_id *new_oid,
> +					      const struct object_id *old_oid,
> +					      const char *new_target,
> +					      const char *old_target,
> +					      const char *committer_info,
> +					      const char *msg)
>  {
>  	struct ref_update *update;
>  

> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(
>  		oidcpy(&update->new_oid, new_oid);
>  	if ((flags & REF_HAVE_OLD) && old_oid)
>  		oidcpy(&update->old_oid, old_oid);
> -	if (!(flags & REF_SKIP_CREATE_REFLOG))
> +	if (!(flags & REF_SKIP_CREATE_REFLOG)) {
> +		if (committer_info) {
> +			struct strbuf sb = STRBUF_INIT;
> +			strbuf_addstr(&sb, committer_info);
> +			update->committer_info = strbuf_detach(&sb, NULL);

> @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  		}
>  
>  		/*
> -		 * packed-refs don't support symbolic refs and root refs, so we
> -		 * have to queue these references via the loose transaction.
> +		 * packed-refs don't support symbolic refs, root refs and reflogs,
> +		 * so we have to queue these references via the loose transaction.
>  		 */
> -		if (update->new_target || is_root_ref(update->refname)) {
> +		if (update->new_target ||
> +		    is_root_ref(update->refname) ||
> +		    (update->flags & REF_LOG_ONLY)) {
>  			if (!loose_transaction) {
>  				loose_transaction = ref_store_transaction_begin(&refs->base, 0, err);
>  				if (!loose_transaction) {

> On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)
>>  	free(transaction);
>>  }
>>
>> -struct ref_update *ref_transaction_add_update(
>> -		struct ref_transaction *transaction,
>> -		const char *refname, unsigned int flags,
>> -		const struct object_id *new_oid,
>> -		const struct object_id *old_oid,
>> -		const char *new_target, const char *old_target,
>> -		const char *msg)
>> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,
>> +					      const char *refname,
>> +					      unsigned int flags,
>> +					      const struct object_id *new_oid,
>> +					      const struct object_id *old_oid,
>> +					      const char *new_target,
>> +					      const char *old_target,
>> +					      const char *committer_info,
>> +					      const char *msg)
>>  {
>>  	struct ref_update *update;
>>
>
> I'd personally avoid reindenting this block. It's somewhat-common
> practice to not align all arguments with the opening brace when the line
> would become too long. The reindents also distract a bit from the actual
> changes done in other places further down.
>

>> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(
>>  		oidcpy(&update->new_oid, new_oid);
>>  	if ((flags & REF_HAVE_OLD) && old_oid)
>>  		oidcpy(&update->old_oid, old_oid);
>> -	if (!(flags & REF_SKIP_CREATE_REFLOG))
>> +	if (!(flags & REF_SKIP_CREATE_REFLOG)) {
>> +		if (committer_info) {
>> +			struct strbuf sb = STRBUF_INIT;
>> +			strbuf_addstr(&sb, committer_info);
>> +			update->committer_info = strbuf_detach(&sb, NULL);
>
> Can't we simplify this via `xstrdup()`?
>

>> @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>>  		}
>>
>>  		/*
>> -		 * packed-refs don't support symbolic refs and root refs, so we
>> -		 * have to queue these references via the loose transaction.
>> +		 * packed-refs don't support symbolic refs, root refs and reflogs,
>> +		 * so we have to queue these references via the loose transaction.
>>  		 */
>> -		if (update->new_target || is_root_ref(update->refname)) {
>> +		if (update->new_target ||
>> +		    is_root_ref(update->refname) ||
>> +		    (update->flags & REF_LOG_ONLY)) {
>>  			if (!loose_transaction) {
>>  				loose_transaction = ref_store_transaction_begin(&refs->base, 0, err);
>>  				if (!loose_transaction) {
>
> Makes sense. While we already had REF_LOG_ONLY beforehand, it was only
> used in very specific cases and thus the support implemented by the
> backends is lacking. And given that the packed-ref backend does not
> support reflogs we have to queue these up via the loose backend.
>
> Patrick

> @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>         struct ident_split committer_ident = {0};
>         size_t logs_nr = 0, logs_alloc = 0, i;
>         const char *committer_info;
> +       struct strintmap logs_ts;

>         int ret = 0;
>
>         committer_info = git_committer_info(0);
> @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>
>         QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);
>
> +       strintmap_init(&logs_ts, ts);

>         reftable_writer_set_limits(writer, ts, ts);
>
>         for (i = 0; i < arg->updates_nr; i++) {
> @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>
>                         if (create_reflog) {
>                                 struct ident_split c;
> +                               uint64_t update_index;
>
>                                 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
>                                 log = &logs[logs_nr++];
> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>                                 }
>
>                                 fill_reftable_log_record(log, &c);
> -                               log->update_index = ts;
> +
> +                               update_index = strintmap_get(&logs_ts, u->refname);
> +                               log->update_index = update_index;
> +                               strintmap_set(&logs_ts, u->refname, update_index+1);

> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> The reference transaction only allows a update for a given reference to
>
> s/a update/an update/
>
> or: s/a update/a single update/

>> avoid conflicts. This, however, isn't an issue for reflogs. There are no
>> conflicts to be resolved in reflogs and when migrating reflogs between
>> backends we'd have multiple reflog entries for the same refname.
>
>
>> @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>>         struct ident_split committer_ident = {0};
>>         size_t logs_nr = 0, logs_alloc = 0, i;
>>         const char *committer_info;
>> +       struct strintmap logs_ts;
>
> Here a comment might help explain what logs_ts is used for.
>

>>         int ret = 0;
>>
>>         committer_info = git_committer_info(0);
>> @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>>
>>         QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);
>>
>> +       strintmap_init(&logs_ts, ts);
>
> I am not sure I understand what logs_ts is used for and why its
> default value is set to ts.
>

>>         reftable_writer_set_limits(writer, ts, ts);
>>
>>         for (i = 0; i < arg->updates_nr; i++) {
>> @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>>
>>                         if (create_reflog) {
>>                                 struct ident_split c;
>> +                               uint64_t update_index;
>>
>>                                 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
>>                                 log = &logs[logs_nr++];
>> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>>                                 }
>>
>>                                 fill_reftable_log_record(log, &c);
>> -                               log->update_index = ts;
>> +
>> +                               update_index = strintmap_get(&logs_ts, u->refname);
>> +                               log->update_index = update_index;
>> +                               strintmap_set(&logs_ts, u->refname, update_index+1);
>
> s/update_index+1/update_index + 1/
>
> Also is the 'update_index' var really needed or could we just do:
>
>                                log->update_index =
> strintmap_get(&logs_ts, u->refname);
>                                strintmap_set(&logs_ts, u->refname,
> log->update_index + 1);
>
> ?
>

> The reference transaction only allows a update for a given reference to
> avoid conflicts. This, however, isn't an issue for reflogs. There are no
> conflicts to be resolved in reflogs and when migrating reflogs between
> backends we'd have multiple reflog entries for the same refname.
> 
> So allow multiple reflog updates within a single transaction. Also the
> reflog creation logic isn't exposed to the end user. While this might
> change in the future, currently, this reduces the scope of issues to
> think about.
> 
> This is required to add reflog migration support to `git refs migrate`
> which currently doesn't support it.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c    | 15 +++++++++++----
>  refs/reftable-backend.c | 16 +++++++++++++---
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  
>  	update->backend_data = lock;
>  
> +	if (update->flags & REF_LOG_ONLY)
> +		goto out;
> +
>  	if (update->type & REF_ISSYMREF) {
>  		if (update->flags & REF_NO_DEREF) {
>  			/*

> @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  
>  	/* Fail if a refname appears more than once in the transaction: */
>  	for (i = 0; i < transaction->nr; i++)
> -		string_list_append(&affected_refnames,
> -				   transaction->updates[i]->refname);
> +		if (!(transaction->updates[i]->flags & REF_LOG_ONLY))
> +			string_list_append(&affected_refnames,
> +					   transaction->updates[i]->refname);
>  	string_list_sort(&affected_refnames);
>  	if (ref_update_reject_duplicates(&affected_refnames, err)) {
>  		ret = TRANSACTION_GENERIC_ERROR;

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>  				}
>  
>  				fill_reftable_log_record(log, &c);
> -				log->update_index = ts;
> +
> +				update_index = strintmap_get(&logs_ts, u->refname);
> +				log->update_index = update_index;
> +				strintmap_set(&logs_ts, u->refname, update_index+1);

> On Mon, Dec 09, 2024 at 12:07:20PM +0100, Karthik Nayak wrote:
>> The reference transaction only allows a update for a given reference to
>> avoid conflicts. This, however, isn't an issue for reflogs. There are no
>> conflicts to be resolved in reflogs and when migrating reflogs between
>> backends we'd have multiple reflog entries for the same refname.
>>
>> So allow multiple reflog updates within a single transaction. Also the
>> reflog creation logic isn't exposed to the end user. While this might
>> change in the future, currently, this reduces the scope of issues to
>> think about.
>>
>> This is required to add reflog migration support to `git refs migrate`
>> which currently doesn't support it.
>
> Nit: the second half of this sentence starting with "which currently..."
> feels rather pointless, as it's implicit in the first half. I'd just
> drop it.
>

>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/files-backend.c    | 15 +++++++++++----
>>  refs/reftable-backend.c | 16 +++++++++++++---
>>  2 files changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>
>>  	update->backend_data = lock;
>>
>> +	if (update->flags & REF_LOG_ONLY)
>> +		goto out;
>> +
>>  	if (update->type & REF_ISSYMREF) {
>>  		if (update->flags & REF_NO_DEREF) {
>>  			/*
>
> Hm. Does this mean that we don't lock at all for REF_LOG_ONLY updates?
> Reflogs themselves have no lockfile, so isn't it mandatory that we lock
> the corresponding ref like we used to do? Otherwise I cannot see how we
> avoid two concurrent writers to the same reflog.
>

>> @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>>
>>  	/* Fail if a refname appears more than once in the transaction: */
>>  	for (i = 0; i < transaction->nr; i++)
>> -		string_list_append(&affected_refnames,
>> -				   transaction->updates[i]->refname);
>> +		if (!(transaction->updates[i]->flags & REF_LOG_ONLY))
>> +			string_list_append(&affected_refnames,
>> +					   transaction->updates[i]->refname);
>>  	string_list_sort(&affected_refnames);
>>  	if (ref_update_reject_duplicates(&affected_refnames, err)) {
>>  		ret = TRANSACTION_GENERIC_ERROR;
>
> This on the other hand is sensible -- having multiple REF_LOG_ONLY
> transactions queued is fine.
>
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>>  				}
>>
>>  				fill_reftable_log_record(log, &c);
>> -				log->update_index = ts;
>> +
>> +				update_index = strintmap_get(&logs_ts, u->refname);
>> +				log->update_index = update_index;
>> +				strintmap_set(&logs_ts, u->refname, update_index+1);
>
> So we're now tracking update indices via another map in order to ensure
> that the update index will be increased if we have multiple reflog
> entries for the same refname. Can we avoid that overhead by instead just
> having a global update index counter that increases for every single
> reflog entry, regardless of whether we have multiple ones queued up for
> the same reference?
>
> I guess the result would be kind of weird as a single transaction with
> multiple ref updates would now always contain N different update
> indices. Maybe there's an alternative that allows us to reduce the cost,
> like only doing this for REF_LOG_ONLY updates?
>
> I'm mostly being careful because this here is the hot loop of writing
> refs, so I don't want to regress performance.

> @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
>  }
>  
>  struct migration_data {
> +	unsigned int index;
>  	struct ref_store *old_refs;
>  	struct ref_transaction *transaction;
>  	struct strbuf *errbuf;

> @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  	 *   1. Set up a new temporary directory and initialize it with the new
>  	 *      format. This is where all refs will be migrated into.
>  	 *
> -	 *   2. Enumerate all refs and write them into the new ref storage.
> -	 *      This operation is safe as we do not yet modify the main
> +	 *   2. Enumerate all refs and reflogs and write them into the new ref
> +	 *      storage. This operation is safe as we do not yet modify the main

> On Mon, Dec 09, 2024 at 12:07:21PM +0100, Karthik Nayak wrote:
>> @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
>>  }
>>
>>  struct migration_data {
>> +	unsigned int index;
>>  	struct ref_store *old_refs;
>>  	struct ref_transaction *transaction;
>>  	struct strbuf *errbuf;
>
> Calling this `reflog_index` might be a bit easier for context.
>

>> @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>>  	 *   1. Set up a new temporary directory and initialize it with the new
>>  	 *      format. This is where all refs will be migrated into.
>>  	 *
>> -	 *   2. Enumerate all refs and write them into the new ref storage.
>> -	 *      This operation is safe as we do not yet modify the main
>> +	 *   2. Enumerate all refs and reflogs and write them into the new ref
>> +	 *      storage. This operation is safe as we do not yet modify the main
>
> I'd rather move this into a third step, as it is separate from the ref
> enumeration.
>
> Patrick
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The `git refs migrate` command was introduced in
>> 25a0023f28 (builtin/refs: new command to migrate ref storage formats,
>> 2024-06-06) to support migrating from one reference backend to another.
>
> This topic pass the tests standalone for me locally, but seems to
> fail 1460.17 and 1460.31 when merged to 'seen'.  I'll push out the
> integration result tonight; it would be very much appreciated if you
> can help find if there are semantic (or otherwise) mismerges that
> are causing this breakage.
>

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> The `git refs migrate` command was introduced in
>>> 25a0023f28 (builtin/refs: new command to migrate ref storage formats,
>>> 2024-06-06) to support migrating from one reference backend to another.
>>
>> This topic pass the tests standalone for me locally, but seems to
>> fail 1460.17 and 1460.31 when merged to 'seen'.  I'll push out the
>> integration result tonight; it would be very much appreciated if you
>> can help find if there are semantic (or otherwise) mismerges that
>> are causing this breakage.
>>
>
> I see. I can reproduce it on 'seen' as you mentioned. Will debug and get
> back to you on this. Thanks for letting me know.