{"thread":{"id":"62616","subject":"[PATCH 0/7] refs: add reflog support to `git refs migrate`","startedAt":"2024-12-09T11:07:24Z","lastAt":"2024-12-12T14:52:06Z","messageCount":30,"participants":["Karthik Nayak","Junio C Hamano","Christian Couder","karthik nayak","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"508855","messageId":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":null,"subject":"[PATCH 0/7] refs: add reflog support to `git refs migrate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:14Z","receivedAt":"2024-12-09T11:07:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `git refs migrate` command was introduced in\n25a0023f28 (builtin/refs: new command to migrate ref storage formats,\n2024-06-06) to support migrating from one reference backend to another.\n\nOne limitation of the feature was that it didn't support migrating\nrepositories which contained reflogs. This isn't a requirement on the\nserver side as repositories are stored as bare repositories (which do\nnot contain any reflogs). Clients however generally use reflogs and\nuntil now couldn't use the `git refs migrate` command to migrate their\nrepositories to the new reftable format.\n\nOne of the issues for adding reflog support is that the ref transactions\ndon't support reflogs additions:\n 1. While there is REF_LOG_ONLY flag, there is no function to utilize\n the flag and add reflogs.\n 2. reference backends generally sort the updates by the refname. This\n wouldn't work for reflogs which need to ensure that they maintain the\n order of creation.\n 3. In the files backend, reflog entries are added by obtaining locks\n on the refs themselves. This means each update in the transaction, will\n obtain a ref_lock. This paradigm fails to accompany the fact that there\n could be multiple reflog updates for a refname in a single transaction.\n 4. The backends check for duplicate entries, which doesn't make sense\n in the context of adding multiple reflogs for a given refname.\n\nWe overcome these issue we make the following changes:\n - Update the ref_update structure to also include the committer\n information. Using this, we can add a new function which only adds\n reflog updates to the transaction.\n - Add an index field to the ref_update structure, this will help order\n updates in pre-defined order, this fixes #2.\n - While the ideal fix for #3 would be to actually introduce reflog\n locks, this wouldn't be possible without breaking backward\n compatibility. So we add a count field to the existing ref_lock. With\n this, multiple reflog updates can share a single ref_lock.\n\nOverall, this series is a bit more involved, and I would appreciate it\nif it receives a bit more scrutiny.\n\nSigned-off-by: Karthik Nayak \n---\nKarthik Nayak (7):\n refs: include committer info in `ref_update` struct\n refs: add `index` field to `struct ref_udpate`\n refs/files: add count field to ref_lock\n refs: extract out refname verification in transactions\n refs: introduce the `ref_transaction_update_reflog` function\n refs: allow multiple reflog entries for the same refname\n refs: add support for migrating reflogs\n\n Documentation/git-refs.txt | 2 -\n refs.c | 204 ++++++++++++++++++++++++++++++++-------------\n refs.h | 12 +++\n refs/files-backend.c | 144 ++++++++++++++++++++------------\n refs/refs-internal.h | 24 ++++--\n refs/reftable-backend.c | 47 +++++++++--\n t/t1460-refs-migrate.sh | 73 +++++++++++-----\n 7 files changed, 360 insertions(+), 146 deletions(-)\n---\n\n\n\n--- \n\nbase-commit: e66fd72e972df760a53c3d6da023c17adfc426d6\nchange-id: 20241111-320-git-refs-migrate-reflogs-a53e3a6cffc9\n\nThanks\n- Karthik\n\n"},{"id":"508856","messageId":"20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 1/7] refs: include committer info in `ref_update` struct","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:15Z","receivedAt":"2024-12-09T11:07:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reference backends obtain the committer information from\n`git_committer_info(0)` when adding a reflog. The upcoming patches\nintroduce support for migrating reflogs between the reference backends.\nThis requires an interface to creating reflogs, including custom\ncommitter information.\n\nAdd a new field `committer_info` to the `ref_update` struct, which is\nthen used by the reference backends. If there is no `committer_info`\nprovided, the reference backends default to using\n`git_committer_info(0)`. The field itself cannot be set to\n`git_committer_info(0)` since the values are dynamic and must be\nobtained right when the reflog is being committed.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 1 +\n refs/files-backend.c | 22 +++++++++++++---------\n refs/refs-internal.h | 1 +\n refs/reftable-backend.c | 12 +++++++++++-\n 4 files changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 762f3e324d59c60cd4f05c2f257e54de8deb00e5..f003e51c6bf5229bfbce8ce61ffad7cdba0572e0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1151,6 +1151,7 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tfree(transaction->updates[i]->msg);\n+\t\tfree(transaction->updates[i]->committer_info);\n \t\tfree((char *)transaction->updates[i]->new_target);\n \t\tfree((char *)transaction->updates[i]->old_target);\n \t\tfree(transaction->updates[i]);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret = 0;\n \n+\tif (!committer)\n+\t\tcommitter = git_committer_info(0);\n+\n \tstrbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n \tif (msg && *msg) {\n \t\tstrbuf_addch(&sb, '\\t');\n@@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n }\n \n static int files_log_ref_write(struct files_ref_store *refs,\n-\t\t\t const char *refname, const struct object_id *old_oid,\n-\t\t\t const struct object_id *new_oid, const char *msg,\n+\t\t\t const char *refname,\n+\t\t\t const struct object_id *old_oid,\n+\t\t\t const struct object_id *new_oid,\n+\t\t\t const char *committer_info, const char *msg,\n \t\t\t int flags, struct strbuf *err)\n {\n \tint logfd, result;\n@@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs,\n \n \tif (logfd < 0)\n \t\treturn 0;\n-\tresult = log_ref_write_fd(logfd, old_oid, new_oid,\n-\t\t\t\t git_committer_info(0), msg);\n+\tresult = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg);\n \tif (result) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tint save_errno = errno;\n@@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n \tfiles_assert_main_repository(refs, \"commit_ref_update\");\n \n \tclear_loose_ref_cache(refs);\n-\tif (files_log_ref_write(refs, lock->ref_name,\n-\t\t\t\t&lock->old_oid, oid,\n+\tif (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL,\n \t\t\t\tlogmsg, flags, err)) {\n \t\tchar *old_msg = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n@@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t !strcmp(head_ref, lock->ref_name)) {\n \t\t\tstruct strbuf log_err = STRBUF_INIT;\n-\t\t\tif (files_log_ref_write(refs, \"HEAD\",\n-\t\t\t\t\t\t&lock->old_oid, oid,\n+\t\t\tif (files_log_ref_write(refs, \"HEAD\", &lock->old_oid,\n+\t\t\t\t\t\toid, git_committer_info(0),\n \t\t\t\t\t\tlogmsg, flags, &log_err)) {\n \t\t\t\terror(\"%s\", log_err.buf);\n \t\t\t\tstrbuf_release(&log_err);\n@@ -2969,7 +2972,8 @@ static int parse_and_write_reflog(struct files_ref_store *refs,\n \t}\n \n \tif (files_log_ref_write(refs, lock->ref_name, &lock->old_oid,\n-\t\t\t\t&update->new_oid, update->msg, update->flags, err)) {\n+\t\t\t\t&update->new_oid, update->committer_info,\n+\t\t\t\tupdate->msg, update->flags, err)) {\n \t\tchar *old_msg = strbuf_detach(err, NULL);\n \n \t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 58aa56d1b27c85d606ed7c8c0d908e4b87d1066b..0fd95cdacd99e4a728c22f5286f6b3f0f360c110 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -113,6 +113,7 @@ struct ref_update {\n \tvoid *backend_data;\n \tunsigned int type;\n \tchar *msg;\n+\tchar *committer_info;\n \n \t/*\n \t * If this ref_update was split off of a symref update via\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\t}\n \n \t\t\tif (create_reflog) {\n+\t\t\t\tstruct ident_split c;\n+\n \t\t\t\tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \t\t\t\tlog = &logs[logs_nr++];\n \t\t\t\tmemset(log, 0, sizeof(*log));\n \n-\t\t\t\tfill_reftable_log_record(log, &committer_ident);\n+\t\t\t\tif (u->committer_info) {\n+\t\t\t\t\tif (split_ident_line(&c, u->committer_info,\n+\t\t\t\t\t\t\t strlen(u->committer_info)))\n+\t\t\t\t\t\tBUG(\"failed splitting committer info\");\n+\t\t\t\t} else {\n+\t\t\t\t\tc = committer_ident;\n+\t\t\t\t}\n+\n+\t\t\t\tfill_reftable_log_record(log, &c);\n \t\t\t\tlog->update_index = ts;\n \t\t\t\tlog->refname = xstrdup(u->refname);\n \t\t\t\tmemcpy(log->value.update.new_hash,\n\n-- \n2.47.1\n\n"},{"id":"508858","messageId":"20241209-320-git-refs-migrate-reflogs-v1-2-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 2/7] refs: add `index` field to `struct ref_udpate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:16Z","receivedAt":"2024-12-09T11:07:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reftable backend, sorts its updates by refname before applying them,\nthis ensures that the references are stored sorted. When migrating\nreflogs from one backend to another, the order of the reflogs must be\nmaintained. Add a new `index` field to the `ref_update` struct to\nfacilitate this.\n\nThis field is used in the reftable backend's sort comparison function\n`transaction_update_cmp`, to ensure that indexed fields maintain their\norder.\n\nSigned-off-by: Karthik Nayak \n---\n refs/refs-internal.h | 7 +++++++\n refs/reftable-backend.c | 13 +++++++++++--\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 0fd95cdacd99e4a728c22f5286f6b3f0f360c110..f5c733d099f0c6f1076a25f4f77d9d5eb345ec87 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -115,6 +115,13 @@ struct ref_update {\n \tchar *msg;\n \tchar *committer_info;\n \n+\t/*\n+\t * The index overrides the default sort algorithm. This is needed\n+\t * when migrating reflogs and we want to ensure we carry over the\n+\t * same order.\n+\t */\n+\tunsigned int index;\n+\n \t/*\n \t * If this ref_update was split off of a symref update via\n \t * split_symref_update(), then this member points at that\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e882602487c66261d586a94101bb1b4e9a2ed60e..c008f20be719fec3af6a8f81c821cb9c263764d7 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1279,8 +1279,17 @@ static int reftable_be_transaction_abort(struct ref_store *ref_store UNUSED,\n \n static int transaction_update_cmp(const void *a, const void *b)\n {\n-\treturn strcmp(((struct reftable_transaction_update *)a)->update->refname,\n-\t\t ((struct reftable_transaction_update *)b)->update->refname);\n+\tstruct reftable_transaction_update *update_a = (struct reftable_transaction_update *)a;\n+\tstruct reftable_transaction_update *update_b = (struct reftable_transaction_update *)b;\n+\n+\t/*\n+\t * If there is an index set, it should take preference (default is 0).\n+\t * This ensures that updates with indexes are sorted amongst themselves.\n+\t */\n+\tif (update_a->update->index || update_b->update->index)\n+\t\treturn update_a->update->index - update_b->update->index;\n+\n+\treturn strcmp(update_a->update->refname, update_b->update->refname);\n }\n \n static int write_transaction_table(struct reftable_writer *writer, void *cb_data)\n\n-- \n2.47.1\n\n"},{"id":"508857","messageId":"20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 3/7] refs/files: add count field to ref_lock","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:17Z","receivedAt":"2024-12-09T11:07:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"When refs are updated in the files-backend, a lock is obtained for the\ncorresponding file path. This is the case even for reflogs, i.e. a lock\nis obtained on the reference path instead of the reflog path. This\nworks, since generally, reflogs are updated alongside the ref.\n\nThe upcoming patches will add support for reflog updates in ref\ntransaction. This means, in a particular transaction we want to have ref\nupdates and reflog updates. For refs, in a given transaction there can\nonly be one update. But, we can theoretically have multiple reflog\nupdates in a given transaction.\n\nThe current flow does not support this, because currently refs & reflogs\nare treated as a single entity and capture the lock together. To\nseparate this, add a count field to ref_lock. With this, multiple\nupdates can hold onto a single ref_lock and the lock will only be\nreleased when all of them release the lock.\n\nThis patch only adds the `count` field to `ref_lock` and adds the logic\nto increment and decrement the lock. In a follow up commit, we'll\nseparate the reflog update logic from ref updates and utilize this\nfunctionality.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 40 insertions(+), 19 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -71,6 +71,8 @@ struct ref_lock {\n \tchar *ref_name;\n \tstruct lock_file lk;\n \tstruct object_id old_oid;\n+\t/* count keeps track of users of the lock */\n+\tunsigned int count;\n };\n \n struct files_ref_store {\n@@ -638,9 +640,12 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop,\n \n static void unlock_ref(struct ref_lock *lock)\n {\n-\trollback_lock_file(&lock->lk);\n-\tfree(lock->ref_name);\n-\tfree(lock);\n+\tlock->count--;\n+\tif (!lock->count) {\n+\t\trollback_lock_file(&lock->lk);\n+\t\tfree(lock->ref_name);\n+\t\tfree(lock);\n+\t}\n }\n \n /*\n@@ -696,6 +701,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t*lock_p = CALLOC_ARRAY(lock, 1);\n \n \tlock->ref_name = xstrdup(refname);\n+\tlock->count = 1;\n \tfiles_ref_path(refs, &ref_file, refname);\n \n retry:\n@@ -1169,6 +1175,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \n \tlock->ref_name = xstrdup(refname);\n+\tlock->count = 1;\n \n \tif (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {\n \t\tunable_to_lock_message(ref_file.buf, errno, err);\n@@ -2535,6 +2542,12 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid,\n \treturn -1;\n }\n \n+struct files_transaction_backend_data {\n+\tstruct ref_transaction *packed_transaction;\n+\tint packed_refs_locked;\n+\tstruct strmap ref_locks;\n+};\n+\n /*\n * Prepare for carrying out update:\n * - Lock the reference referred to by update.\n@@ -2557,11 +2570,14 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n {\n \tstruct strbuf referent = STRBUF_INIT;\n \tint mustexist = ref_update_expects_existing_old_ref(update);\n+\tstruct files_transaction_backend_data *backend_data;\n \tint ret = 0;\n \tstruct ref_lock *lock;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n \n+\tbackend_data = transaction->backend_data;\n+\n \tif ((update->flags & REF_HAVE_NEW) && ref_update_has_null_new_value(update))\n \t\tupdate->flags |= REF_DELETING;\n \n@@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\tgoto out;\n \t}\n \n-\tret = lock_raw_ref(refs, update->refname, mustexist,\n-\t\t\t affected_refnames,\n-\t\t\t &lock, &referent,\n-\t\t\t &update->type, err);\n-\tif (ret) {\n-\t\tchar *reason;\n+\tlock = strmap_get(&backend_data->ref_locks, update->refname);\n+\tif (lock) {\n+\t\tlock->count = lock->count + 1;\n+\t} else {\n+\t\tret = lock_raw_ref(refs, update->refname, mustexist,\n+\t\t\t\t affected_refnames,\n+\t\t\t\t &lock, &referent,\n+\t\t\t\t &update->type, err);\n+\t\tif (ret) {\n+\t\t\tchar *reason;\n+\n+\t\t\treason = strbuf_detach(err, NULL);\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n+\t\t\t\t ref_update_original_update_refname(update), reason);\n+\t\t\tfree(reason);\n+\t\t\tgoto out;\n+\t\t}\n \n-\t\treason = strbuf_detach(err, NULL);\n-\t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n-\t\t\t ref_update_original_update_refname(update), reason);\n-\t\tfree(reason);\n-\t\tgoto out;\n+\t\tstrmap_put(&backend_data->ref_locks, update->refname, lock);\n \t}\n \n \tupdate->backend_data = lock;\n@@ -2730,11 +2753,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \treturn ret;\n }\n \n-struct files_transaction_backend_data {\n-\tstruct ref_transaction *packed_transaction;\n-\tint packed_refs_locked;\n-};\n-\n /*\n * Unlock any references in `transaction` that are still locked, and\n * mark the transaction closed.\n@@ -2767,6 +2785,8 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n \t\tif (backend_data->packed_refs_locked)\n \t\t\tpacked_refs_unlock(refs->packed_ref_store);\n \n+\t\tstrmap_clear(&backend_data->ref_locks, 0);\n+\n \t\tfree(backend_data);\n \t}\n \n@@ -2796,6 +2816,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \n \tCALLOC_ARRAY(backend_data, 1);\n+\tstrmap_init(&backend_data->ref_locks);\n \ttransaction->backend_data = backend_data;\n \n \t/*\n\n-- \n2.47.1\n\n"},{"id":"508859","messageId":"20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 4/7] refs: extract out refname verification in transactions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:18Z","receivedAt":"2024-12-09T11:07:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,\nthe refname of the update is verified for:\n\n - Ensuring it is not a pseudoref.\n - Checking the refname format.\n\nThese checks are also be needed in a following commit where the function\nto add reflog updates to the transaction is introduced. Extract the code\nout into a new static function.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 43 ++++++++++++++++++++++++++++---------------\n 1 file changed, 28 insertions(+), 15 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(\n \treturn update;\n }\n \n+static int transaction_refname_verification(const char *refname,\n+\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t struct strbuf *err)\n+{\n+\tif (flags & REF_SKIP_REFNAME_VERIFICATION)\n+\t\treturn 0;\n+\n+\tif (is_pseudo_ref(refname)) {\n+\t\tstrbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n+\t\t\t refname);\n+\t\treturn -1;\n+\t} else if ((new_oid && !is_null_oid(new_oid)) ?\n+\t\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n+\t\t !refname_is_safe(refname)) {\n+\t\tstrbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n+\t\t\t refname);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const struct object_id *new_oid,\n@@ -1205,6 +1228,8 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t unsigned int flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n+\tint ret;\n+\n \tassert(err);\n \n \tif ((flags & REF_FORCE_CREATE_REFLOG) &&\n@@ -1213,21 +1238,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\tif (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&\n-\t ((new_oid && !is_null_oid(new_oid)) ?\n-\t\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n-\t\t\t !refname_is_safe(refname))) {\n-\t\tstrbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n-\t\t\t refname);\n-\t\treturn -1;\n-\t}\n-\n-\tif (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&\n-\t is_pseudo_ref(refname)) {\n-\t\tstrbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n-\t\t\t refname);\n-\t\treturn -1;\n-\t}\n+\tret = transaction_refname_verification(refname, new_oid, flags, err);\n+\tif (ret)\n+\t\treturn ret;\n \n \tif (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n \t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n\n-- \n2.47.1\n\n"},{"id":"508860","messageId":"20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:19Z","receivedAt":"2024-12-09T11:07:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a new function `ref_transaction_update_reflog`, for clients to\nadd a reflog update to a transaction. While the existing function\n`ref_transaction_update` also allows clients to add a reflog entry, this\nfunction does a few things more, It:\n - Enforces that only a reflog entry is added and does not update the\n ref itself.\n - Allows the users to also provide the committer information. This\n means clients can add reflog entries with custom committer\n information.\n\nA follow up commit will utilize this function to add reflog support to\n`git refs migrate`.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 89 +++++++++++++++++++++++++++++++++++++------------\n refs.h | 12 +++++++\n refs/files-backend.c | 48 +++++++++++++++-----------\n refs/refs-internal.h | 16 +++++----\n refs/reftable-backend.c | 6 ++--\n 5 files changed, 122 insertions(+), 49 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tfree(transaction);\n }\n \n-struct ref_update *ref_transaction_add_update(\n-\t\tstruct ref_transaction *transaction,\n-\t\tconst char *refname, unsigned int flags,\n-\t\tconst struct object_id *new_oid,\n-\t\tconst struct object_id *old_oid,\n-\t\tconst char *new_target, const char *old_target,\n-\t\tconst char *msg)\n+struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t const char *new_target,\n+\t\t\t\t\t const char *old_target,\n+\t\t\t\t\t const char *committer_info,\n+\t\t\t\t\t const char *msg)\n {\n \tstruct ref_update *update;\n \n@@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(\n \t\toidcpy(&update->new_oid, new_oid);\n \tif ((flags & REF_HAVE_OLD) && old_oid)\n \t\toidcpy(&update->old_oid, old_oid);\n-\tif (!(flags & REF_SKIP_CREATE_REFLOG))\n+\tif (!(flags & REF_SKIP_CREATE_REFLOG)) {\n+\t\tif (committer_info) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&sb, committer_info);\n+\t\t\tupdate->committer_info = strbuf_detach(&sb, NULL);\n+\t\t}\n+\n \t\tupdate->msg = normalize_reflog_message(msg);\n+\t}\n \n \treturn update;\n }\n@@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update(\n static int transaction_refname_verification(const char *refname,\n \t\t\t\t\t const struct object_id *new_oid,\n \t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t unsigned int reflog,\n \t\t\t\t\t struct strbuf *err)\n {\n \tif (flags & REF_SKIP_REFNAME_VERIFICATION)\n \t\treturn 0;\n \n \tif (is_pseudo_ref(refname)) {\n-\t\tstrbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n-\t\t\t refname);\n+\t\tif (reflog)\n+\t\t\tstrbuf_addf(err, _(\"refusing to update reflog for pseudoref '%s'\"),\n+\t\t\t\t refname);\n+\t\telse\n+\t\t\tstrbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n+\t\t\t\t refname);\n \t\treturn -1;\n \t} else if ((new_oid && !is_null_oid(new_oid)) ?\n \t\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n \t\t !refname_is_safe(refname)) {\n-\t\tstrbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n-\t\t\t refname);\n+\t\tif (reflog)\n+\t\t\tstrbuf_addf(err, _(\"refusing to update reflog with bad name '%s'\"),\n+\t\t\t\t refname);\n+\t\telse\n+\t\t\tstrbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n+\t\t\t\t refname);\n \t\treturn -1;\n \t}\n \n@@ -1238,7 +1256,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\tret = transaction_refname_verification(refname, new_oid, flags, err);\n+\tret = transaction_refname_verification(refname, new_oid, flags, 0, err);\n \tif (ret)\n \t\treturn ret;\n \n@@ -1255,18 +1273,47 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n \tflags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0);\n \n-\tref_transaction_add_update(transaction, refname, flags,\n-\t\t\t\t new_oid, old_oid, new_target,\n-\t\t\t\t old_target, msg);\n+\tref_transaction_add_update(transaction, refname, flags, new_oid,\n+\t\t\t\t old_oid, new_target, old_target, NULL, msg);\n+\treturn 0;\n+}\n+\n+int ref_transaction_update_reflog(struct ref_transaction *transaction,\n+\t\t\t\t const char *refname,\n+\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t const char *committer_info, unsigned int flags,\n+\t\t\t\t const char *msg, unsigned int index,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tstruct ref_update *update;\n+\tint ret;\n+\n+\tassert(err);\n+\n+\tret = transaction_refname_verification(refname, new_oid, flags, 1, err);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tflags |= REF_LOG_ONLY | REF_NO_DEREF;\n+\n+\tupdate = ref_transaction_add_update(transaction, refname, flags,\n+\t\t\t\t\t new_oid, old_oid, NULL, NULL,\n+\t\t\t\t\t committer_info, msg);\n+\t/*\n+\t * While we do set the old_oid value, we unset the flag to skip\n+\t * old_oid verification which only makes sense for refs.\n+\t */\n+\tupdate->flags &= ~REF_HAVE_OLD;\n+\tupdate->index = index;\n+\n \treturn 0;\n }\n \n int ref_transaction_create(struct ref_transaction *transaction,\n-\t\t\t const char *refname,\n-\t\t\t const struct object_id *new_oid,\n-\t\t\t const char *new_target,\n-\t\t\t unsigned int flags, const char *msg,\n-\t\t\t struct strbuf *err)\n+\t\t\t const char *refname, const struct object_id *new_oid,\n+\t\t\t const char *new_target, unsigned int flags,\n+\t\t\t const char *msg, struct strbuf *err)\n {\n \tif (new_oid && new_target)\n \t\tBUG(\"create called with both new_oid and new_target set\");\ndiff --git a/refs.h b/refs.h\nindex a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t unsigned int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n+/*\n+ * Similar to `ref_transaction_update`, but this function is only for adding\n+ * a reflog updates. Supports providing custom committer information.\n+ */\n+int ref_transaction_update_reflog(struct ref_transaction *transaction,\n+\t\t\t\t const char *refname,\n+\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t const char *committer_info, unsigned int flags,\n+\t\t\t\t const char *msg, unsigned int index,\n+\t\t\t\t struct strbuf *err);\n+\n /*\n * Add a reference creation to transaction. new_oid is the value that\n * the reference should have after the update; it must not be\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 9c929c1ac33bc62a75620e684a809d46b574f1c6..32975e0fd7a03ab8ddf99c0a68af99921d3f5090 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1268,10 +1268,10 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \ttransaction = ref_store_transaction_begin(&refs->base, 0, &err);\n \tif (!transaction)\n \t\tgoto cleanup;\n-\tref_transaction_add_update(\n-\t\t\ttransaction, r->name,\n-\t\t\tREF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING,\n-\t\t\tnull_oid(), &r->oid, NULL, NULL, NULL);\n+\tref_transaction_add_update(transaction, r->name,\n+\t\t\t\t REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD |\n+\t\t\t\t REF_IS_PRUNING, null_oid(), &r->oid, NULL,\n+\t\t\t\t NULL, NULL, NULL);\n \tif (ref_transaction_commit(transaction, &err))\n \t\tgoto cleanup;\n \n@@ -2418,7 +2418,7 @@ static int split_head_update(struct ref_update *update,\n \t\t\ttransaction, \"HEAD\",\n \t\t\tupdate->flags | REF_LOG_ONLY | REF_NO_DEREF,\n \t\t\t&update->new_oid, &update->old_oid,\n-\t\t\tNULL, NULL, update->msg);\n+\t\t\tNULL, NULL, update->committer_info, update->msg);\n \n \t/*\n \t * Add \"HEAD\". This insertion is O(N) in the transaction\n@@ -2482,7 +2482,8 @@ static int split_symref_update(struct ref_update *update,\n \t\t\ttransaction, referent, new_flags,\n \t\t\tupdate->new_target ? NULL : &update->new_oid,\n \t\t\tupdate->old_target ? NULL : &update->old_oid,\n-\t\t\tupdate->new_target, update->old_target, update->msg);\n+\t\t\tupdate->new_target, update->old_target, NULL,\n+\t\t\tupdate->msg);\n \n \tnew_update->parent_update = update;\n \n@@ -2911,11 +2912,11 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\tpacked_transaction;\n \t\t\t}\n \n-\t\t\tref_transaction_add_update(\n-\t\t\t\t\tpacked_transaction, update->refname,\n-\t\t\t\t\tREF_HAVE_NEW | REF_NO_DEREF,\n-\t\t\t\t\t&update->new_oid, NULL,\n-\t\t\t\t\tNULL, NULL, NULL);\n+\t\t\tref_transaction_add_update(packed_transaction,\n+\t\t\t\t\t\t update->refname,\n+\t\t\t\t\t\t REF_HAVE_NEW | REF_NO_DEREF,\n+\t\t\t\t\t\t &update->new_oid, NULL, NULL,\n+\t\t\t\t\t\t NULL, NULL, NULL);\n \t\t}\n \t}\n \n@@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n \t\t}\n \n \t\t/*\n-\t\t * packed-refs don't support symbolic refs and root refs, so we\n-\t\t * have to queue these references via the loose transaction.\n+\t\t * packed-refs don't support symbolic refs, root refs and reflogs,\n+\t\t * so we have to queue these references via the loose transaction.\n \t\t */\n-\t\tif (update->new_target || is_root_ref(update->refname)) {\n+\t\tif (update->new_target ||\n+\t\t is_root_ref(update->refname) ||\n+\t\t (update->flags & REF_LOG_ONLY)) {\n \t\t\tif (!loose_transaction) {\n \t\t\t\tloose_transaction = ref_store_transaction_begin(&refs->base, 0, err);\n \t\t\t\tif (!loose_transaction) {\n@@ -3092,15 +3095,22 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n \t\t\t\t}\n \t\t\t}\n \n-\t\t\tref_transaction_add_update(loose_transaction, update->refname,\n-\t\t\t\t\t\t update->flags & ~REF_HAVE_OLD,\n-\t\t\t\t\t\t update->new_target ? NULL : &update->new_oid, NULL,\n-\t\t\t\t\t\t update->new_target, NULL, NULL);\n+\t\t\tif (update->flags & REF_LOG_ONLY)\n+\t\t\t\tref_transaction_add_update(loose_transaction, update->refname,\n+\t\t\t\t\t\t\t update->flags, &update->new_oid,\n+\t\t\t\t\t\t\t &update->old_oid, NULL, NULL,\n+\t\t\t\t\t\t\t update->committer_info, update->msg);\n+\t\t\telse\n+\t\t\t\tref_transaction_add_update(loose_transaction, update->refname,\n+\t\t\t\t\t\t\t update->flags & ~REF_HAVE_OLD,\n+\t\t\t\t\t\t\t update->new_target ? NULL : &update->new_oid, NULL,\n+\t\t\t\t\t\t\t update->new_target, NULL, update->committer_info,\n+\t\t\t\t\t\t\t NULL);\n \t\t} else {\n \t\t\tref_transaction_add_update(packed_transaction, update->refname,\n \t\t\t\t\t\t update->flags & ~REF_HAVE_OLD,\n \t\t\t\t\t\t &update->new_oid, &update->old_oid,\n-\t\t\t\t\t\t NULL, NULL, NULL);\n+\t\t\t\t\t\t NULL, NULL, update->committer_info, NULL);\n \t\t}\n \t}\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex f5c733d099f0c6f1076a25f4f77d9d5eb345ec87..82c1387d1e6ab3658b31fe99c95f98645ff1ebf1 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -156,13 +156,15 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits,\n * respectively, are set in flags.\n */\n-struct ref_update *ref_transaction_add_update(\n-\t\tstruct ref_transaction *transaction,\n-\t\tconst char *refname, unsigned int flags,\n-\t\tconst struct object_id *new_oid,\n-\t\tconst struct object_id *old_oid,\n-\t\tconst char *new_target, const char *old_target,\n-\t\tconst char *msg);\n+struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t const char *new_target,\n+\t\t\t\t\t const char *old_target,\n+\t\t\t\t\t const char *committer_info,\n+\t\t\t\t\t const char *msg);\n \n /*\n * Transaction states.\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex c008f20be719fec3af6a8f81c821cb9c263764d7..b2e3ba877de9e59fea5a4d066eb13e60ef22a32b 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1078,7 +1078,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\tnew_update = ref_transaction_add_update(\n \t\t\t\t\ttransaction, \"HEAD\",\n \t\t\t\t\tu->flags | REF_LOG_ONLY | REF_NO_DEREF,\n-\t\t\t\t\t&u->new_oid, &u->old_oid, NULL, NULL, u->msg);\n+\t\t\t\t\t&u->new_oid, &u->old_oid, NULL, NULL, NULL,\n+\t\t\t\t\tu->msg);\n \t\t\tstring_list_insert(&affected_refnames, new_update->refname);\n \t\t}\n \n@@ -1161,7 +1162,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\ttransaction, referent.buf, new_flags,\n \t\t\t\t\tu->new_target ? NULL : &u->new_oid,\n \t\t\t\t\tu->old_target ? NULL : &u->old_oid,\n-\t\t\t\t\tu->new_target, u->old_target, u->msg);\n+\t\t\t\t\tu->new_target, u->old_target,\n+\t\t\t\t\tu->committer_info, u->msg);\n \n \t\t\t\tnew_update->parent_update = u;\n \n\n-- \n2.47.1\n\n"},{"id":"508861","messageId":"20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 6/7] refs: allow multiple reflog entries for the same refname","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:20Z","receivedAt":"2024-12-09T11:07:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reference transaction only allows a update for a given reference to\navoid conflicts. This, however, isn't an issue for reflogs. There are no\nconflicts to be resolved in reflogs and when migrating reflogs between\nbackends we'd have multiple reflog entries for the same refname.\n\nSo allow multiple reflog updates within a single transaction. Also the\nreflog creation logic isn't exposed to the end user. While this might\nchange in the future, currently, this reduces the scope of issues to\nthink about.\n\nThis is required to add reflog migration support to `git refs migrate`\nwhich currently doesn't support it.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 15 +++++++++++----\n refs/reftable-backend.c | 16 +++++++++++++---\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \n \tupdate->backend_data = lock;\n \n+\tif (update->flags & REF_LOG_ONLY)\n+\t\tgoto out;\n+\n \tif (update->type & REF_ISSYMREF) {\n \t\tif (update->flags & REF_NO_DEREF) {\n \t\t\t/*\n@@ -2830,13 +2833,16 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n-\t\tstruct string_list_item *item =\n-\t\t\tstring_list_append(&affected_refnames, update->refname);\n+\t\tstruct string_list_item *item;\n \n \t\tif ((update->flags & REF_IS_PRUNING) &&\n \t\t !(update->flags & REF_NO_DEREF))\n \t\t\tBUG(\"REF_IS_PRUNING set without REF_NO_DEREF\");\n \n+\t\tif (update->flags & REF_LOG_ONLY)\n+\t\t\tcontinue;\n+\n+\t\titem = string_list_append(&affected_refnames, update->refname);\n \t\t/*\n \t\t * We store a pointer to update in item->util, but at\n \t\t * the moment we never use the value of this field\n@@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n \n \t/* Fail if a refname appears more than once in the transaction: */\n \tfor (i = 0; i < transaction->nr; i++)\n-\t\tstring_list_append(&affected_refnames,\n-\t\t\t\t transaction->updates[i]->refname);\n+\t\tif (!(transaction->updates[i]->flags & REF_LOG_ONLY))\n+\t\t\tstring_list_append(&affected_refnames,\n+\t\t\t\t\t transaction->updates[i]->refname);\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -990,8 +990,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\tif (ret)\n \t\t\tgoto done;\n \n-\t\tstring_list_append(&affected_refnames,\n-\t\t\t\t transaction->updates[i]->refname);\n+\t\tif (!(transaction->updates[i]->flags & REF_LOG_ONLY))\n+\t\t\tstring_list_append(&affected_refnames,\n+\t\t\t\t\t transaction->updates[i]->refname);\n \t}\n \n \t/*\n@@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n \tconst char *committer_info;\n+\tstruct strintmap logs_ts;\n \tint ret = 0;\n \n \tcommitter_info = git_committer_info(0);\n@@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \n \tQSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n \n+\tstrintmap_init(&logs_ts, ts);\n+\n \treftable_writer_set_limits(writer, ts, ts);\n \n \tfor (i = 0; i < arg->updates_nr; i++) {\n@@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \n \t\t\tif (create_reflog) {\n \t\t\t\tstruct ident_split c;\n+\t\t\t\tuint64_t update_index;\n \n \t\t\t\tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \t\t\t\tlog = &logs[logs_nr++];\n@@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\t\t}\n \n \t\t\t\tfill_reftable_log_record(log, &c);\n-\t\t\t\tlog->update_index = ts;\n+\n+\t\t\t\tupdate_index = strintmap_get(&logs_ts, u->refname);\n+\t\t\t\tlog->update_index = update_index;\n+\t\t\t\tstrintmap_set(&logs_ts, u->refname, update_index+1);\n+\n \t\t\t\tlog->refname = xstrdup(u->refname);\n \t\t\t\tmemcpy(log->value.update.new_hash,\n \t\t\t\t u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1476,6 +1485,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n+\tstrintmap_clear(&logs_ts);\n \tfor (i = 0; i < logs_nr; i++)\n \t\treftable_log_record_release(&logs[i]);\n \tfree(logs);\n\n-- \n2.47.1\n\n"},{"id":"508862","messageId":"20241209-320-git-refs-migrate-reflogs-v1-7-d4bc37ee860f@gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"[PATCH 7/7] refs: add support for migrating reflogs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-09T11:07:21Z","receivedAt":"2024-12-09T11:07:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `git refs migrate` command was introduced in\n25a0023f28 (builtin/refs: new command to migrate ref storage formats,\n2024-06-06) to support migrating from one reference backend to another.\n\nOne limitation of the command was that it didn't support migrating\nrepositories which contained reflogs. A previous commit, added support\nfor adding reflog updates in ref transactions. Using the added\nfunctionality bake in reflog support for `git refs migrate`.\n\nTo ensure that the order of the reflogs is maintained during the\nmigration, we add the index for each reflog update as we iterate over\nthe reflogs from the old reference backend. This is to ensure that the\norder is maintained in the new backend.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/git-refs.txt | 2 --\n refs.c | 81 ++++++++++++++++++++++++++++++++--------------\n t/t1460-refs-migrate.sh | 73 ++++++++++++++++++++++++++++-------------\n 3 files changed, 107 insertions(+), 49 deletions(-)\n\ndiff --git a/Documentation/git-refs.txt b/Documentation/git-refs.txt\nindex ce31f93061db5e5d16aca516dd3d15f6527db870..9829984b0a4c4f54ec7f9b6c6c7072f62b1d198d 100644\n--- a/Documentation/git-refs.txt\n+++ b/Documentation/git-refs.txt\n@@ -57,8 +57,6 @@ KNOWN LIMITATIONS\n \n The ref format migration has several known limitations in its current form:\n \n-* It is not possible to migrate repositories that have reflogs.\n-\n * It is not possible to migrate repositories that have worktrees.\n \n * There is no way to block concurrent writes to the repository during an\ndiff --git a/refs.c b/refs.c\nindex 602a65873181a90751def525608a7fa7bea59562..4d10c7276391e8e85c66bd626bb0ecfec0941c6d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -30,6 +30,7 @@\n #include \"date.h\"\n #include \"commit.h\"\n #include \"wildmatch.h\"\n+#include \"ident.h\"\n \n /*\n * List of all available backends\n@@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,\n }\n \n struct migration_data {\n+\tunsigned int index;\n \tstruct ref_store *old_refs;\n \tstruct ref_transaction *transaction;\n \tstruct strbuf *errbuf;\n@@ -2722,6 +2724,53 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n \treturn ret;\n }\n \n+struct reflog_migration_data {\n+\tunsigned int *index;\n+\tconst char *refname;\n+\tstruct ref_store *old_refs;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf *errbuf;\n+};\n+\n+static int migrate_one_reflog_entry(struct object_id *old_oid,\n+\t\t\t\t struct object_id *new_oid,\n+\t\t\t\t const char *committer,\n+\t\t\t\t timestamp_t timestamp, int tz,\n+\t\t\t\t const char *msg, void *cb_data)\n+{\n+\tstruct reflog_migration_data *data = cb_data;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *date;\n+\tint ret;\n+\n+\tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n+\t/* committer contains name and email */\n+\tstrbuf_addstr(&sb, fmt_ident(\"\", committer, WANT_BLANK_IDENT, date, 0));\n+\n+\tret = ref_transaction_update_reflog(data->transaction, data->refname,\n+\t\t\t\t\t new_oid, old_oid, sb.buf,\n+\t\t\t\t\t REF_HAVE_NEW | REF_HAVE_OLD, msg,\n+\t\t\t\t\t (*data->index)++, data->errbuf);\n+\tstrbuf_release(&sb);\n+\n+\treturn ret;\n+}\n+\n+static int migrate_one_reflog(const char *refname, void *cb_data)\n+{\n+\tstruct migration_data *migration_data = cb_data;\n+\tstruct reflog_migration_data data;\n+\n+\tdata.refname = refname;\n+\tdata.old_refs = migration_data->old_refs;\n+\tdata.transaction = migration_data->transaction;\n+\tdata.errbuf = migration_data->errbuf;\n+\tdata.index = &migration_data->index;\n+\n+\treturn refs_for_each_reflog_ent(migration_data->old_refs, refname,\n+\t\t\t\t\tmigrate_one_reflog_entry, &data);\n+}\n+\n static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf)\n {\n \tstruct strbuf from_buf = STRBUF_INIT, to_buf = STRBUF_INIT;\n@@ -2788,13 +2837,6 @@ static int move_files(const char *from_path, const char *to_path, struct strbuf\n \treturn ret;\n }\n \n-static int count_reflogs(const char *reflog UNUSED, void *payload)\n-{\n-\tsize_t *reflog_count = payload;\n-\t(*reflog_count)++;\n-\treturn 0;\n-}\n-\n static int has_worktrees(void)\n {\n \tstruct worktree **worktrees = get_worktrees();\n@@ -2820,7 +2862,6 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \tstruct ref_transaction *transaction = NULL;\n \tstruct strbuf new_gitdir = STRBUF_INIT;\n \tstruct migration_data data;\n-\tsize_t reflog_count = 0;\n \tint did_migrate_refs = 0;\n \tint ret;\n \n@@ -2832,21 +2873,6 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \n \told_refs = get_main_ref_store(repo);\n \n-\t/*\n-\t * We do not have any interfaces that would allow us to write many\n-\t * reflog entries. Once we have them we can remove this restriction.\n-\t */\n-\tif (refs_for_each_reflog(old_refs, count_reflogs, &reflog_count) < 0) {\n-\t\tstrbuf_addstr(errbuf, \"cannot count reflogs\");\n-\t\tret = -1;\n-\t\tgoto done;\n-\t}\n-\tif (reflog_count) {\n-\t\tstrbuf_addstr(errbuf, \"migrating reflogs is not supported yet\");\n-\t\tret = -1;\n-\t\tgoto done;\n-\t}\n-\n \t/*\n \t * Worktrees complicate the migration because every worktree has a\n \t * separate ref storage. While it should be feasible to implement, this\n@@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \t * 1. Set up a new temporary directory and initialize it with the new\n \t * format. This is where all refs will be migrated into.\n \t *\n-\t * 2. Enumerate all refs and write them into the new ref storage.\n-\t * This operation is safe as we do not yet modify the main\n+\t * 2. Enumerate all refs and reflogs and write them into the new ref\n+\t * storage. This operation is safe as we do not yet modify the main\n \t * repository.\n \t *\n \t * 3. If we're in dry-run mode then we are done and can hand over the\n@@ -2924,6 +2950,11 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \tif (ret < 0)\n \t\tgoto done;\n \n+\tdata.index = 1;\n+\tret = refs_for_each_reflog(old_refs, migrate_one_reflog, &data);\n+\tif (ret < 0)\n+\t\tgoto done;\n+\n \tret = ref_transaction_commit(transaction, errbuf);\n \tif (ret < 0)\n \t\tgoto done;\ndiff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\nindex 1bfff3a7afd5acc470424dfe7ec3e97d45f5c481..f59bc4860f19c4af82dc6f2984bdb69d61fe3ec2 100755\n--- a/t/t1460-refs-migrate.sh\n+++ b/t/t1460-refs-migrate.sh\n@@ -7,23 +7,44 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+# Migrate the provided repository from one format to the other and\n+# verify that the references and logs are migrated over correctly.\n+# Usage: test_migration \n+# is the relative path to the repo to be migrated.\n+# is the ref format to be migrated to.\n+# (true or false) whether to skip reflog verification.\n test_migration () {\n-\tgit -C \"$1\" for-each-ref --include-root-refs \\\n+\trepo=$1 &&\n+\tformat=$2 &&\n+\tskip_reflog_verify=${3:-false} &&\n+\tgit -C \"$repo\" for-each-ref --include-root-refs \\\n \t\t--format='%(refname) %(objectname) %(symref)' >expect &&\n-\tgit -C \"$1\" refs migrate --ref-format=\"$2\" &&\n-\tgit -C \"$1\" for-each-ref --include-root-refs \\\n+\tif ! $skip_reflog_verify\n+\tthen\n+\t git -C \"$repo\" reflog --all >expect_logs &&\n+\t git -C \"$repo\" reflog list >expect_log_list\n+\tfi &&\n+\n+\tgit -C \"$repo\" refs migrate --ref-format=\"$2\" &&\n+\n+\tgit -C \"$repo\" for-each-ref --include-root-refs \\\n \t\t--format='%(refname) %(objectname) %(symref)' >actual &&\n \ttest_cmp expect actual &&\n+\tif ! $skip_reflog_verify\n+\tthen\n+\t\tgit -C \"$repo\" reflog --all >actual_logs &&\n+\t\tgit -C \"$repo\" reflog list >actual_log_list &&\n+\t\ttest_cmp expect_logs actual_logs &&\n+\t\ttest_cmp expect_log_list actual_log_list\n+\tfi &&\n \n-\tgit -C \"$1\" rev-parse --show-ref-format >actual &&\n-\techo \"$2\" >expect &&\n+\tgit -C \"$repo\" rev-parse --show-ref-format >actual &&\n+\techo \"$format\" >expect &&\n \ttest_cmp expect actual\n }\n \n test_expect_success 'setup' '\n-\trm -rf .git &&\n-\t# The migration does not yet support reflogs.\n-\tgit config --global core.logAllRefUpdates false\n+\trm -rf .git\n '\n \n test_expect_success \"superfluous arguments\" '\n@@ -78,19 +99,6 @@ do\n \t\t\ttest_cmp expect err\n \t\t'\n \n-\t\ttest_expect_success \"$from_format -> $to_format: migration with reflog fails\" '\n-\t\t\ttest_when_finished \"rm -rf repo\" &&\n-\t\t\tgit init --ref-format=$from_format repo &&\n-\t\t\ttest_config -C repo core.logAllRefUpdates true &&\n-\t\t\ttest_commit -C repo logged &&\n-\t\t\ttest_must_fail git -C repo refs migrate \\\n-\t\t\t\t--ref-format=$to_format 2>err &&\n-\t\t\tcat >expect <<-EOF &&\n-\t\t\terror: migrating reflogs is not supported yet\n-\t\t\tEOF\n-\t\t\ttest_cmp expect err\n-\t\t'\n-\n \t\ttest_expect_success \"$from_format -> $to_format: migration with worktree fails\" '\n \t\t\ttest_when_finished \"rm -rf repo\" &&\n \t\t\tgit init --ref-format=$from_format repo &&\n@@ -141,7 +149,7 @@ do\n \t\t\ttest_commit -C repo initial &&\n \t\t\ttest-tool -C repo ref-store main update-ref \"\" refs/heads/broken \\\n \t\t\t\t\"$(test_oid 001)\" \"$ZERO_OID\" REF_SKIP_CREATE_REFLOG,REF_SKIP_OID_VERIFICATION &&\n-\t\t\ttest_migration repo \"$to_format\" &&\n+\t\t\ttest_migration repo \"$to_format\" true &&\n \t\t\ttest_oid 001 >expect &&\n \t\t\tgit -C repo rev-parse refs/heads/broken >actual &&\n \t\t\ttest_cmp expect actual\n@@ -195,6 +203,27 @@ do\n \t\t\tgit -C repo rev-parse --show-ref-format >actual &&\n \t\t\ttest_cmp expect actual\n \t\t'\n+\n+\t\ttest_expect_success \"$from_format -> $to_format: reflogs of symrefs with target deleted\" '\n+\t\t\ttest_when_finished \"rm -rf repo\" &&\n+\t\t\tgit init --ref-format=$from_format repo &&\n+\t\t\ttest_commit -C repo initial &&\n+\t\t\tgit -C repo branch branch-1 HEAD &&\n+\t\t\tgit -C repo symbolic-ref refs/heads/symref refs/heads/branch-1 &&\n+\t\t\tcat >input <<-EOF &&\n+\t\t\tdelete refs/heads/branch-1\n+\t\t\tEOF\n+\t\t\tgit -C repo update-ref --stdin $to_format: reflogs order is retained\" '\n+\t\t\ttest_when_finished \"rm -rf repo\" &&\n+\t\t\tgit init --ref-format=$from_format repo &&\n+\t\t\ttest_commit --date \"100005000 +0700\" --no-tag -C repo initial &&\n+\t\t\ttest_commit --date \"100003000 +0700\" --no-tag -C repo second &&\n+\t\t\ttest_migration repo \"$to_format\"\n+\t\t'\n \tdone\n done\n \n\n-- \n2.47.1\n\n"},{"id":"508923","messageId":"xmqq34ivlora.fsf@gitster.g","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 0/7] refs: add reflog support to `git refs migrate`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-10T12:13:29Z","receivedAt":"2024-12-10T12:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The `git refs migrate` command was introduced in\n> 25a0023f28 (builtin/refs: new command to migrate ref storage formats,\n> 2024-06-06) to support migrating from one reference backend to another.\n\nThis topic pass the tests standalone for me locally, but seems to\nfail 1460.17 and 1460.31 when merged to 'seen'. I'll push out the\nintegration result tonight; it would be very much appreciated if you\ncan help find if there are semantic (or otherwise) mismerges that\nare causing this breakage.\n\nThanks.\n"},{"id":"508926","messageId":"CAP8UFD3FXtbuHH1G==rR00_O7MBjqQiw_qzGHPVLK4P13RRtjQ@mail.gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 1/7] refs: include committer info in `ref_update` struct","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-12-10T16:51:57Z","receivedAt":"2024-12-10T16:52:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak wrote:\n\n\n> If there is no `committer_info`\n> provided, the reference backends default to using\n> `git_committer_info(0)`. The field itself cannot be set to\n> `git_committer_info(0)` since the values are dynamic and must be\n> obtained right when the reflog is being committed.\n\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n> struct strbuf sb = STRBUF_INIT;\n> int ret = 0;\n>\n> + if (!committer)\n> + committer = git_committer_info(0);\n\nIt looks like this is where we obtain the value \"right when the reflog\nis being committed\".\n\n> +\n> strbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n> if (msg && *msg) {\n> strbuf_addch(&sb, '\\t');\n> @@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n> }\n>\n> static int files_log_ref_write(struct files_ref_store *refs,\n> - const char *refname, const struct object_id *old_oid,\n> - const struct object_id *new_oid, const char *msg,\n> + const char *refname,\n> + const struct object_id *old_oid,\n> + const struct object_id *new_oid,\n> + const char *committer_info, const char *msg,\n> int flags, struct strbuf *err)\n> {\n> int logfd, result;\n> @@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs,\n>\n> if (logfd < 0)\n> return 0;\n> - result = log_ref_write_fd(logfd, old_oid, new_oid,\n> - git_committer_info(0), msg);\n> + result = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg);\n\nHere we just pass the committer_info to the above function.\n\n> if (result) {\n> struct strbuf sb = STRBUF_INIT;\n> int save_errno = errno;\n> @@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n> files_assert_main_repository(refs, \"commit_ref_update\");\n>\n> clear_loose_ref_cache(refs);\n> - if (files_log_ref_write(refs, lock->ref_name,\n> - &lock->old_oid, oid,\n> + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL,\n> logmsg, flags, err)) {\n\nHere we don't have the info so we pass NULL.\n\n> char *old_msg = strbuf_detach(err, NULL);\n> strbuf_addf(err, \"cannot update the ref '%s': %s\",\n> @@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs,\n> if (head_ref && (head_flag & REF_ISSYMREF) &&\n> !strcmp(head_ref, lock->ref_name)) {\n> struct strbuf log_err = STRBUF_INIT;\n> - if (files_log_ref_write(refs, \"HEAD\",\n> - &lock->old_oid, oid,\n> + if (files_log_ref_write(refs, \"HEAD\", &lock->old_oid,\n> + oid, git_committer_info(0),\n\nHere we don't have the info either, so I think we should also pass\nNULL. It would then be computed \"right when the reflog is being\ncommitted\" in the above function. No?\n\n> logmsg, flags, &log_err)) {\n> error(\"%s\", log_err.buf);\n> strbuf_release(&log_err);\n\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n\nIt is not your fault but write_transaction_table() does the following\nright at the beginning of the function:\n\n committer_info = git_committer_info(0);\n if (split_ident_line(&committer_ident, committer_info,\nstrlen(committer_info)))\n BUG(\"failed splitting committer info\");\n\nbut then 'committer_ident' is only used in the hunk you are changing:\n\n> if (create_reflog) {\n> + struct ident_split c;\n> +\n> ALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n> log = &logs[logs_nr++];\n> memset(log, 0, sizeof(*log));\n>\n> - fill_reftable_log_record(log, &committer_ident);\n> + if (u->committer_info) {\n> + if (split_ident_line(&c, u->committer_info,\n> + strlen(u->committer_info)))\n> + BUG(\"failed splitting committer info\");\n> + } else {\n\nI would think it would be more efficient to only compute\n'committer_ident' here, right before we use it if needed. Or is there\nsomething I am missing?\n\n> + c = committer_ident;\n> + }\n> +\n> + fill_reftable_log_record(log, &c);\n> log->update_index = ts;\n> log->refname = xstrdup(u->refname);\n> memcpy(log->value.update.new_hash,\n"},{"id":"508927","messageId":"CAP8UFD0UAWib-GceEQi+ZzbosRcvSRbq-DuMLVVr9xuf8C03ow@mail.gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 3/7] refs/files: add count field to ref_lock","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-12-10T17:22:29Z","receivedAt":"2024-12-10T17:22:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>\n> When refs are updated in the files-backend, a lock is obtained for the\n> corresponding file path. This is the case even for reflogs, i.e. a lock\n> is obtained on the reference path instead of the reflog path. This\n> works, since generally, reflogs are updated alongside the ref.\n>\n> The upcoming patches will add support for reflog updates in ref\n> transaction. This means, in a particular transaction we want to have ref\n> updates and reflog updates. For refs, in a given transaction there can\n> only be one update. But, we can theoretically have multiple reflog\n> updates in a given transaction.\n\nNit: Giving an example might help understand where multiple reflog\nupdates can happen in a given transaction. Alternatively pointing to\nan existing doc that contains such an example or explanations might\nhelp too.\n\n> @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n> goto out;\n> }\n>\n> - ret = lock_raw_ref(refs, update->refname, mustexist,\n> - affected_refnames,\n> - &lock, &referent,\n> - &update->type, err);\n> - if (ret) {\n> - char *reason;\n> + lock = strmap_get(&backend_data->ref_locks, update->refname);\n> + if (lock) {\n> + lock->count = lock->count + 1;\n\nNit:\n lock->count++;\n\n> + } else {\n> + ret = lock_raw_ref(refs, update->refname, mustexist,\n> + affected_refnames,\n> + &lock, &referent,\n> + &update->type, err);\n> + if (ret) {\n> + char *reason;\n> +\n> + reason = strbuf_detach(err, NULL);\n> + strbuf_addf(err, \"cannot lock ref '%s': %s\",\n> + ref_update_original_update_refname(update), reason);\n> + free(reason);\n> + goto out;\n> + }\n>\n> - reason = strbuf_detach(err, NULL);\n> - strbuf_addf(err, \"cannot lock ref '%s': %s\",\n> - ref_update_original_update_refname(update), reason);\n> - free(reason);\n> - goto out;\n> + strmap_put(&backend_data->ref_locks, update->refname, lock);\n> }\n>\n> update->backend_data = lock;\n"},{"id":"508928","messageId":"CAOLa=ZRsAwM8oUnyYCaRKkkudN+QW+hRSKZEvPJLThSr14M7rA@mail.gmail.com","threadId":"62616","inReplyTo":"xmqq34ivlora.fsf@gitster.g","subject":"Re: [PATCH 0/7] refs: add reflog support to `git refs migrate`","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-10T17:42:29Z","receivedAt":"2024-12-10T17:42:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> The `git refs migrate` command was introduced in\n>> 25a0023f28 (builtin/refs: new command to migrate ref storage formats,\n>> 2024-06-06) to support migrating from one reference backend to another.\n>\n> This topic pass the tests standalone for me locally, but seems to\n> fail 1460.17 and 1460.31 when merged to 'seen'. I'll push out the\n> integration result tonight; it would be very much appreciated if you\n> can help find if there are semantic (or otherwise) mismerges that\n> are causing this breakage.\n>\n\nI see. I can reproduce it on 'seen' as you mentioned. Will debug and get\nback to you on this. Thanks for letting me know.\n"},{"id":"508929","messageId":"CAOLa=ZRwH8mGDF=afGnci6BXD8r+tA5gKgrQRb0BZFBWYDG7qA@mail.gmail.com","threadId":"62616","inReplyTo":"CAOLa=ZRsAwM8oUnyYCaRKkkudN+QW+hRSKZEvPJLThSr14M7rA@mail.gmail.com","subject":"Re: [PATCH 0/7] refs: add reflog support to `git refs migrate`","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-10T18:03:44Z","receivedAt":"2024-12-10T18:03:46Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"karthik nayak writes:\n\n> Junio C Hamano writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> The `git refs migrate` command was introduced in\n>>> 25a0023f28 (builtin/refs: new command to migrate ref storage formats,\n>>> 2024-06-06) to support migrating from one reference backend to another.\n>>\n>> This topic pass the tests standalone for me locally, but seems to\n>> fail 1460.17 and 1460.31 when merged to 'seen'. I'll push out the\n>> integration result tonight; it would be very much appreciated if you\n>> can help find if there are semantic (or otherwise) mismerges that\n>> are causing this breakage.\n>>\n>\n> I see. I can reproduce it on 'seen' as you mentioned. Will debug and get\n> back to you on this. Thanks for letting me know.\n\nSeems like this is due to 'kn/reftable-writer-log-write-verify', which I\nshould have totally seen coming. A quick fix like the one below fixes\nthe issue. I'll merge in 'kn/reftable-writer-log-write-verify' when I\nre-roll.\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1badf88df0..5c51a6a226 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1428,6 +1428,7 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n \tstruct reftable_log_record *logs = NULL;\n \tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tuint64_t max_update_index = ts;\n \tconst char *committer_info;\n \tstruct strintmap logs_ts;\n \tint ret = 0;\n@@ -1541,6 +1542,13 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n \t\t\t\tlog->update_index = update_index;\n \t\t\t\tstrintmap_set(&logs_ts, u->refname, update_index+1);\n\n+\t\t\t\t/*\n+\t\t\t\t * Note the max_update_index, so we can reset the limit\n+\t\t\t\t * before actually writing the logs.\n+\t\t\t\t */\n+\t\t\t\tif (update_index > max_update_index)\n+\t\t\t\t\tmax_update_index = update_index;\n+\n \t\t\t\tlog->refname = xstrdup(u->refname);\n \t\t\t\tmemcpy(log->value.update.new_hash,\n \t\t\t\t u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1604,6 +1612,8 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n \t * and log blocks.\n \t */\n \tif (logs) {\n+\t\treftable_writer_set_limits(writer, ts, max_update_index);\n+\n \t\tret = reftable_writer_add_logs(writer, logs, logs_nr);\n \t\tif (ret < 0)\n \t\t\tgoto done;\n"},{"id":"508957","messageId":"CAP8UFD3StHePPGe+44c83qBYoZLZme+Gw-cP7Weg-neyJg8DOw@mail.gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 3/7] refs/files: add count field to ref_lock","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-12-11T09:05:37Z","receivedAt":"2024-12-11T09:05:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>\n> When refs are updated in the files-backend, a lock is obtained for the\n> corresponding file path. This is the case even for reflogs, i.e. a lock\n> is obtained on the reference path instead of the reflog path. This\n> works, since generally, reflogs are updated alongside the ref.\n>\n> The upcoming patches will add support for reflog updates in ref\n> transaction. This means, in a particular transaction we want to have ref\n> updates and reflog updates. For refs, in a given transaction there can\n> only be one update.\n\nMaybe something like: \"For a given ref in a given transaction there\ncan be at most one update.\"\n\n> But, we can theoretically have multiple reflog\n> updates in a given transaction.\n\nAnd: \"But we can theoretically have multiple reflog updates for a\ngiven ref in a given transaction.\"\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -71,6 +71,8 @@ struct ref_lock {\n> char *ref_name;\n> struct lock_file lk;\n> struct object_id old_oid;\n> + /* count keeps track of users of the lock */\n> + unsigned int count;\n\nNit: maybe the following is a bit better:\n\n unsigned int count; /* track users of the lock (ref update +\nreflog updates) */\n\n> };\n"},{"id":"508958","messageId":"CAP8UFD3fZ21TXgtMcppXMHf35qgA-UH=0X9z-xJ456qXyV5=dA@mail.gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 4/7] refs: extract out refname verification in transactions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-12-11T09:26:31Z","receivedAt":"2024-12-11T09:26:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>\n> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,\n> the refname of the update is verified for:\n>\n> - Ensuring it is not a pseudoref.\n> - Checking the refname format.\n>\n> These checks are also be needed in a following commit where the function\n\ns/are also be needed/will also be needed/\n\n> to add reflog updates to the transaction is introduced. Extract the code\n> out into a new static function.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> refs.c | 43 ++++++++++++++++++++++++++++---------------\n> 1 file changed, 28 insertions(+), 15 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(\n> return update;\n> }\n>\n> +static int transaction_refname_verification(const char *refname,\n> + const struct object_id *new_oid,\n> + unsigned int flags,\n> + struct strbuf *err)\n\nWe have a number of functions named 'xxx_valid()' or 'xxx_ok()' while\nI couldn't find any 'yyy_verification()' function, so it might be\nbetter to name it 'transaction_refname_valid()' or maybe\n'transaction_refname_ok()'.\n\nAlso I think it should probably return a bool so 1 if the refname is\nvalid and 0 otherwise, unless we have plans in the future to follow\ndifferent code paths depending on the different ways it is not valid.\n\n> + ret = transaction_refname_verification(refname, new_oid, flags, err);\n> + if (ret)\n> + return ret;\n\nThen the above could be just:\n\n if (!transaction_refname_valid(refname, new_oid, flags, err))\n return -1;\n"},{"id":"508960","messageId":"CAP8UFD1vTEH=7HBX1sudwkv7now9VUkFR3Y347EVNGfToZt8fA@mail.gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-12-11T10:10:01Z","receivedAt":"2024-12-11T10:10:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>\n> Introduce a new function `ref_transaction_update_reflog`, for clients to\n> add a reflog update to a transaction. While the existing function\n> `ref_transaction_update` also allows clients to add a reflog entry, this\n> function does a few things more, It:\n> - Enforces that only a reflog entry is added and does not update the\n> ref itself.\n> - Allows the users to also provide the committer information. This\n> means clients can add reflog entries with custom committer\n> information.\n\n\n\n> A follow up commit will utilize this function to add reflog support to\n> `git refs migrate`.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> refs.c | 89 +++++++++++++++++++++++++++++++++++++------------\n> refs.h | 12 +++++++\n> refs/files-backend.c | 48 +++++++++++++++-----------\n> refs/refs-internal.h | 16 +++++----\n> refs/reftable-backend.c | 6 ++--\n> 5 files changed, 122 insertions(+), 49 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n> free(transaction);\n> }\n>\n> -struct ref_update *ref_transaction_add_update(\n> - struct ref_transaction *transaction,\n> - const char *refname, unsigned int flags,\n> - const struct object_id *new_oid,\n> - const struct object_id *old_oid,\n> - const char *new_target, const char *old_target,\n> - const char *msg)\n> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,\n> + const char *refname,\n> + unsigned int flags,\n> + const struct object_id *new_oid,\n> + const struct object_id *old_oid,\n> + const char *new_target,\n> + const char *old_target,\n> + const char *committer_info,\n\nThis change (adding a 'const char *committer_info' argument to\nref_transaction_add_update()) is not described in the commit message\nand it requires a number of changes to the callers of this function,\nso I think it might want to be in its own preparatory commit before\nthis one.\n\n> + const char *msg)\n> {\n> struct ref_update *update;\n>\n> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(\n> oidcpy(&update->new_oid, new_oid);\n> if ((flags & REF_HAVE_OLD) && old_oid)\n> oidcpy(&update->old_oid, old_oid);\n> - if (!(flags & REF_SKIP_CREATE_REFLOG))\n> + if (!(flags & REF_SKIP_CREATE_REFLOG)) {\n> + if (committer_info) {\n> + struct strbuf sb = STRBUF_INIT;\n> + strbuf_addstr(&sb, committer_info);\n> + update->committer_info = strbuf_detach(&sb, NULL);\n\nMaybe:\n update->committer_info = xstrdup(committer_info);\n\n> + }\n> +\n> update->msg = normalize_reflog_message(msg);\n> + }\n>\n> return update;\n> }\n> @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update(\n> static int transaction_refname_verification(const char *refname,\n> const struct object_id *new_oid,\n> unsigned int flags,\n> + unsigned int reflog,\n> struct strbuf *err)\n> {\n> if (flags & REF_SKIP_REFNAME_VERIFICATION)\n> return 0;\n>\n> if (is_pseudo_ref(refname)) {\n> - strbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n> - refname);\n> + if (reflog)\n> + strbuf_addf(err, _(\"refusing to update reflog for pseudoref '%s'\"),\n> + refname);\n> + else\n> + strbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n> + refname);\n\nMaybe:\n\n const char *what = reflog ? \"reflog for pseudoref\" : \"pseudoref\";\n strbuf_addf(err, _(\"refusing to update %s '%s'\"), what, refname);\n\n> return -1;\n> } else if ((new_oid && !is_null_oid(new_oid)) ?\n> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n> !refname_is_safe(refname)) {\n> - strbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n> - refname);\n> + if (reflog)\n> + strbuf_addf(err, _(\"refusing to update reflog with bad name '%s'\"),\n> + refname);\n> + else\n> + strbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n> + refname);\n\nMaybe:\n\n const char *what = reflog ? \"reflog with bad name\" :\n\"ref with bad name\";\n strbuf_addf(err, _(\"refusing to update %s '%s'\"), what, refname);\n\n> return -1;\n> }\n\n[...]\n\n> int ref_transaction_create(struct ref_transaction *transaction,\n> - const char *refname,\n> - const struct object_id *new_oid,\n> - const char *new_target,\n> - unsigned int flags, const char *msg,\n> - struct strbuf *err)\n> + const char *refname, const struct object_id *new_oid,\n> + const char *new_target, unsigned int flags,\n> + const char *msg, struct strbuf *err)\n\nThis looks like a wrapping or indenting only change. If you really\nwant to do it, it should probably be in its own preparatory commit.\n\n> index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction,\n> unsigned int flags, const char *msg,\n> struct strbuf *err);\n>\n> +/*\n> + * Similar to `ref_transaction_update`, but this function is only for adding\n> + * a reflog updates.\n\n\"a reflog update\" or \"reflog updates\".\n"},{"id":"508961","messageId":"CAOLa=ZRL0P4V11Eq3RYN2gPurCwgDUez4muH3+2yZoBvNNm0Xg@mail.gmail.com","threadId":"62616","inReplyTo":"CAP8UFD3FXtbuHH1G==rR00_O7MBjqQiw_qzGHPVLK4P13RRtjQ@mail.gmail.com","subject":"Re: [PATCH 1/7] refs: include committer info in `ref_update` struct","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-11T10:13:35Z","receivedAt":"2024-12-11T10:13:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak wrote:\n>\n>\n>> If there is no `committer_info`\n>> provided, the reference backends default to using\n>> `git_committer_info(0)`. The field itself cannot be set to\n>> `git_committer_info(0)` since the values are dynamic and must be\n>> obtained right when the reflog is being committed.\n>\n>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n>> struct strbuf sb = STRBUF_INIT;\n>> int ret = 0;\n>>\n>> + if (!committer)\n>> + committer = git_committer_info(0);\n>\n> It looks like this is where we obtain the value \"right when the reflog\n> is being committed\".\n>\n>> +\n>> strbuf_addf(&sb, \"%s %s %s\", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);\n>> if (msg && *msg) {\n>> strbuf_addch(&sb, '\\t');\n>> @@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n>> }\n>>\n>> static int files_log_ref_write(struct files_ref_store *refs,\n>> - const char *refname, const struct object_id *old_oid,\n>> - const struct object_id *new_oid, const char *msg,\n>> + const char *refname,\n>> + const struct object_id *old_oid,\n>> + const struct object_id *new_oid,\n>> + const char *committer_info, const char *msg,\n>> int flags, struct strbuf *err)\n>> {\n>> int logfd, result;\n>> @@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs,\n>>\n>> if (logfd < 0)\n>> return 0;\n>> - result = log_ref_write_fd(logfd, old_oid, new_oid,\n>> - git_committer_info(0), msg);\n>> + result = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg);\n>\n> Here we just pass the committer_info to the above function.\n>\n>> if (result) {\n>> struct strbuf sb = STRBUF_INIT;\n>> int save_errno = errno;\n>> @@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n>> files_assert_main_repository(refs, \"commit_ref_update\");\n>>\n>> clear_loose_ref_cache(refs);\n>> - if (files_log_ref_write(refs, lock->ref_name,\n>> - &lock->old_oid, oid,\n>> + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL,\n>> logmsg, flags, err)) {\n>\n> Here we don't have the info so we pass NULL.\n>\n>> char *old_msg = strbuf_detach(err, NULL);\n>> strbuf_addf(err, \"cannot update the ref '%s': %s\",\n>> @@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs,\n>> if (head_ref && (head_flag & REF_ISSYMREF) &&\n>> !strcmp(head_ref, lock->ref_name)) {\n>> struct strbuf log_err = STRBUF_INIT;\n>> - if (files_log_ref_write(refs, \"HEAD\",\n>> - &lock->old_oid, oid,\n>> + if (files_log_ref_write(refs, \"HEAD\", &lock->old_oid,\n>> + oid, git_committer_info(0),\n>\n> Here we don't have the info either, so I think we should also pass\n> NULL. It would then be computed \"right when the reflog is being\n> committed\" in the above function. No?\n>\n\nIndeed, passing NULL should be sufficient here, good catch.\n\n>> logmsg, flags, &log_err)) {\n>> error(\"%s\", log_err.buf);\n>> strbuf_release(&log_err);\n>\n>\n>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n>> index 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644\n>> --- a/refs/reftable-backend.c\n>> +++ b/refs/reftable-backend.c\n>> @@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>\n> It is not your fault but write_transaction_table() does the following\n> right at the beginning of the function:\n>\n> committer_info = git_committer_info(0);\n> if (split_ident_line(&committer_ident, committer_info,\n> strlen(committer_info)))\n> BUG(\"failed splitting committer info\");\n>\n> but then 'committer_ident' is only used in the hunk you are changing:\n>\n>> if (create_reflog) {\n>> + struct ident_split c;\n>> +\n>> ALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n>> log = &logs[logs_nr++];\n>> memset(log, 0, sizeof(*log));\n>>\n>> - fill_reftable_log_record(log, &committer_ident);\n>> + if (u->committer_info) {\n>> + if (split_ident_line(&c, u->committer_info,\n>> + strlen(u->committer_info)))\n>> + BUG(\"failed splitting committer info\");\n>> + } else {\n>\n> I would think it would be more efficient to only compute\n> 'committer_ident' here, right before we use it if needed. Or is there\n> something I am missing?\n>\n\nIt would if there wasn't a loop. Since we loop over multiple updates,\ncomputing committer_ident for each would end up being expensive. So it\nis done before the loop starts.\n\n>> + c = committer_ident;\n>> + }\n>> +\n>> + fill_reftable_log_record(log, &c);\n>> log->update_index = ts;\n>> log->refname = xstrdup(u->refname);\n>> memcpy(log->value.update.new_hash,\n"},{"id":"508962","messageId":"CAOLa=ZRuuS_TW=BPeC36eWfgcYegT5hVKd+pVLG6TDMMjJ7tsg@mail.gmail.com","threadId":"62616","inReplyTo":"CAP8UFD0UAWib-GceEQi+ZzbosRcvSRbq-DuMLVVr9xuf8C03ow@mail.gmail.com","subject":"Re: [PATCH 3/7] refs/files: add count field to ref_lock","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-11T10:18:51Z","receivedAt":"2024-12-11T10:18:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>>\n>> When refs are updated in the files-backend, a lock is obtained for the\n>> corresponding file path. This is the case even for reflogs, i.e. a lock\n>> is obtained on the reference path instead of the reflog path. This\n>> works, since generally, reflogs are updated alongside the ref.\n>>\n>> The upcoming patches will add support for reflog updates in ref\n>> transaction. This means, in a particular transaction we want to have ref\n>> updates and reflog updates. For refs, in a given transaction there can\n>> only be one update. But, we can theoretically have multiple reflog\n>> updates in a given transaction.\n>\n> Nit: Giving an example might help understand where multiple reflog\n> updates can happen in a given transaction. Alternatively pointing to\n> an existing doc that contains such an example or explanations might\n> help too.\n>\n\nThe use-case is added in the series. I've added a note about how this is\nneeded in reflog migration.\n\n>> @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n>> goto out;\n>> }\n>>\n>> - ret = lock_raw_ref(refs, update->refname, mustexist,\n>> - affected_refnames,\n>> - &lock, &referent,\n>> - &update->type, err);\n>> - if (ret) {\n>> - char *reason;\n>> + lock = strmap_get(&backend_data->ref_locks, update->refname);\n>> + if (lock) {\n>> + lock->count = lock->count + 1;\n>\n> Nit:\n> lock->count++;\n>\n\nWill fix, thanks.\n\n[snip]\n"},{"id":"508963","messageId":"CAOLa=ZRmGtxRrWnYPW6i8VX1MjQxoY8srKESgyaoutQxR-aZ3A@mail.gmail.com","threadId":"62616","inReplyTo":"CAP8UFD3StHePPGe+44c83qBYoZLZme+Gw-cP7Weg-neyJg8DOw@mail.gmail.com","subject":"Re: [PATCH 3/7] refs/files: add count field to ref_lock","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-11T10:26:38Z","receivedAt":"2024-12-11T10:26:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>>\n>> When refs are updated in the files-backend, a lock is obtained for the\n>> corresponding file path. This is the case even for reflogs, i.e. a lock\n>> is obtained on the reference path instead of the reflog path. This\n>> works, since generally, reflogs are updated alongside the ref.\n>>\n>> The upcoming patches will add support for reflog updates in ref\n>> transaction. This means, in a particular transaction we want to have ref\n>> updates and reflog updates. For refs, in a given transaction there can\n>> only be one update.\n>\n> Maybe something like: \"For a given ref in a given transaction there\n> can be at most one update.\"\n>\n\nSure.\n\n>> But, we can theoretically have multiple reflog\n>> updates in a given transaction.\n>\n> And: \"But we can theoretically have multiple reflog updates for a\n> given ref in a given transaction.\"\n>\n\nWill add.\n\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -71,6 +71,8 @@ struct ref_lock {\n>> char *ref_name;\n>> struct lock_file lk;\n>> struct object_id old_oid;\n>> + /* count keeps track of users of the lock */\n>> + unsigned int count;\n>\n> Nit: maybe the following is a bit better:\n>\n> unsigned int count; /* track users of the lock (ref update +\n> reflog updates) */\n\nThis is better, will amend this in too!\n"},{"id":"508964","messageId":"CAOLa=ZQtT9KXXro5kCbJUN7fFAAHirZXo06nStqn_u6urr149Q@mail.gmail.com","threadId":"62616","inReplyTo":"CAP8UFD3fZ21TXgtMcppXMHf35qgA-UH=0X9z-xJ456qXyV5=dA@mail.gmail.com","subject":"Re: [PATCH 4/7] refs: extract out refname verification in transactions","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-11T10:31:46Z","receivedAt":"2024-12-11T10:31:48Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>>\n>> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,\n>> the refname of the update is verified for:\n>>\n>> - Ensuring it is not a pseudoref.\n>> - Checking the refname format.\n>>\n>> These checks are also be needed in a following commit where the function\n>\n> s/are also be needed/will also be needed/\n>\n\nWill amend.\n\n>> to add reflog updates to the transaction is introduced. Extract the code\n>> out into a new static function.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> refs.c | 43 ++++++++++++++++++++++++++++---------------\n>> 1 file changed, 28 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(\n>> return update;\n>> }\n>>\n>> +static int transaction_refname_verification(const char *refname,\n>> + const struct object_id *new_oid,\n>> + unsigned int flags,\n>> + struct strbuf *err)\n>\n> We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while\n> I couldn't find any 'yyy_verification()' function, so it might be\n> better to name it 'transaction_refname_valid()' or maybe\n> 'transaction_refname_ok()'.\n>\n\nI think you're right, it helps to be consistent here. Will change to\n`transaction_refname_valid()`.\n\n> Also I think it should probably return a bool so 1 if the refname is\n> valid and 0 otherwise, unless we have plans in the future to follow\n> different code paths depending on the different ways it is not valid.\n>\n\nThat is a good idea.\n\n>> + ret = transaction_refname_verification(refname, new_oid, flags, err);\n>> + if (ret)\n>> + return ret;\n>\n> Then the above could be just:\n>\n> if (!transaction_refname_valid(refname, new_oid, flags, err))\n> return -1;\n\nYup, also will remove the need for the `ret` variable.\n"},{"id":"508965","messageId":"CAP8UFD2zogHD_g5FOVNFvBswPJF749wd4f2s9Pzgef-pv=U0Gg@mail.gmail.com","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-12-11T10:44:18Z","receivedAt":"2024-12-11T10:44:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>\n> The reference transaction only allows a update for a given reference to\n\ns/a update/an update/\n\nor: s/a update/a single update/\n\n> avoid conflicts. This, however, isn't an issue for reflogs. There are no\n> conflicts to be resolved in reflogs and when migrating reflogs between\n> backends we'd have multiple reflog entries for the same refname.\n\n\n> @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n> struct ident_split committer_ident = {0};\n> size_t logs_nr = 0, logs_alloc = 0, i;\n> const char *committer_info;\n> + struct strintmap logs_ts;\n\nHere a comment might help explain what logs_ts is used for.\n\n> int ret = 0;\n>\n> committer_info = git_committer_info(0);\n> @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>\n> QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n>\n> + strintmap_init(&logs_ts, ts);\n\nI am not sure I understand what logs_ts is used for and why its\ndefault value is set to ts.\n\nAlso ts is an uint64_t while the second argument to strintmap_init()\nis an int. I wonder if it could be an issue especially on 32 bits\nplatforms.\n\n> reftable_writer_set_limits(writer, ts, ts);\n>\n> for (i = 0; i < arg->updates_nr; i++) {\n> @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>\n> if (create_reflog) {\n> struct ident_split c;\n> + uint64_t update_index;\n>\n> ALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n> log = &logs[logs_nr++];\n> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n> }\n>\n> fill_reftable_log_record(log, &c);\n> - log->update_index = ts;\n> +\n> + update_index = strintmap_get(&logs_ts, u->refname);\n> + log->update_index = update_index;\n> + strintmap_set(&logs_ts, u->refname, update_index+1);\n\ns/update_index+1/update_index + 1/\n\nAlso is the 'update_index' var really needed or could we just do:\n\n log->update_index =\nstrintmap_get(&logs_ts, u->refname);\n strintmap_set(&logs_ts, u->refname,\nlog->update_index + 1);\n\n?\n\n> log->refname = xstrdup(u->refname);\n> memcpy(log->value.update.new_hash,\n> u->new_oid.hash, GIT_MAX_RAWSZ);\n"},{"id":"508985","messageId":"Z1mhCWXowokIaFR5@pks.im","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-11T14:26:17Z","receivedAt":"2024-12-11T14:26:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote:\n> diff --git a/refs.c b/refs.c\n> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n> \tfree(transaction);\n> }\n> \n> -struct ref_update *ref_transaction_add_update(\n> -\t\tstruct ref_transaction *transaction,\n> -\t\tconst char *refname, unsigned int flags,\n> -\t\tconst struct object_id *new_oid,\n> -\t\tconst struct object_id *old_oid,\n> -\t\tconst char *new_target, const char *old_target,\n> -\t\tconst char *msg)\n> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,\n> +\t\t\t\t\t const char *refname,\n> +\t\t\t\t\t unsigned int flags,\n> +\t\t\t\t\t const struct object_id *new_oid,\n> +\t\t\t\t\t const struct object_id *old_oid,\n> +\t\t\t\t\t const char *new_target,\n> +\t\t\t\t\t const char *old_target,\n> +\t\t\t\t\t const char *committer_info,\n> +\t\t\t\t\t const char *msg)\n> {\n> \tstruct ref_update *update;\n> \n\nI'd personally avoid reindenting this block. It's somewhat-common\npractice to not align all arguments with the opening brace when the line\nwould become too long. The reindents also distract a bit from the actual\nchanges done in other places further down.\n\n> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(\n> \t\toidcpy(&update->new_oid, new_oid);\n> \tif ((flags & REF_HAVE_OLD) && old_oid)\n> \t\toidcpy(&update->old_oid, old_oid);\n> -\tif (!(flags & REF_SKIP_CREATE_REFLOG))\n> +\tif (!(flags & REF_SKIP_CREATE_REFLOG)) {\n> +\t\tif (committer_info) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_addstr(&sb, committer_info);\n> +\t\t\tupdate->committer_info = strbuf_detach(&sb, NULL);\n\nCan't we simplify this via `xstrdup()`?\n\n> @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n> \t\t}\n> \n> \t\t/*\n> -\t\t * packed-refs don't support symbolic refs and root refs, so we\n> -\t\t * have to queue these references via the loose transaction.\n> +\t\t * packed-refs don't support symbolic refs, root refs and reflogs,\n> +\t\t * so we have to queue these references via the loose transaction.\n> \t\t */\n> -\t\tif (update->new_target || is_root_ref(update->refname)) {\n> +\t\tif (update->new_target ||\n> +\t\t is_root_ref(update->refname) ||\n> +\t\t (update->flags & REF_LOG_ONLY)) {\n> \t\t\tif (!loose_transaction) {\n> \t\t\t\tloose_transaction = ref_store_transaction_begin(&refs->base, 0, err);\n> \t\t\t\tif (!loose_transaction) {\n\nMakes sense. While we already had REF_LOG_ONLY beforehand, it was only\nused in very specific cases and thus the support implemented by the\nbackends is lacking. And given that the packed-ref backend does not\nsupport reflogs we have to queue these up via the loose backend.\n\nPatrick\n"},{"id":"508986","messageId":"Z1mhD2M4KgzA17k0@pks.im","threadId":"62616","inReplyTo":"CAP8UFD3fZ21TXgtMcppXMHf35qgA-UH=0X9z-xJ456qXyV5=dA@mail.gmail.com","subject":"Re: [PATCH 4/7] refs: extract out refname verification in transactions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-11T14:26:23Z","receivedAt":"2024-12-11T14:26:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 11, 2024 at 10:26:31AM +0100, Christian Couder wrote:\n> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n> > diff --git a/refs.c b/refs.c\n> > index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update(\n> > return update;\n> > }\n> >\n> > +static int transaction_refname_verification(const char *refname,\n> > + const struct object_id *new_oid,\n> > + unsigned int flags,\n> > + struct strbuf *err)\n> \n> We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while\n> I couldn't find any 'yyy_verification()' function, so it might be\n> better to name it 'transaction_refname_valid()' or maybe\n> 'transaction_refname_ok()'.\n> \n> Also I think it should probably return a bool so 1 if the refname is\n> valid and 0 otherwise, unless we have plans in the future to follow\n> different code paths depending on the different ways it is not valid.\n> \n> > + ret = transaction_refname_verification(refname, new_oid, flags, err);\n> > + if (ret)\n> > + return ret;\n> \n> Then the above could be just:\n> \n> if (!transaction_refname_valid(refname, new_oid, flags, err))\n> return -1;\n\nThe only question is whether we want to discern between an invalid\nrefname and an error. But reading through the code it doesn't seem like\nwe want to do that as we always return either `-1` on invalid names or\n`0` otherwise.\n\nSo agreed, this is a good suggestion.\n\nPatrick\n"},{"id":"508987","messageId":"Z1mhEsu8DBsCsYuA@pks.im","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-11T14:26:26Z","receivedAt":"2024-12-11T14:26:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 09, 2024 at 12:07:20PM +0100, Karthik Nayak wrote:\n> The reference transaction only allows a update for a given reference to\n> avoid conflicts. This, however, isn't an issue for reflogs. There are no\n> conflicts to be resolved in reflogs and when migrating reflogs between\n> backends we'd have multiple reflog entries for the same refname.\n> \n> So allow multiple reflog updates within a single transaction. Also the\n> reflog creation logic isn't exposed to the end user. While this might\n> change in the future, currently, this reduces the scope of issues to\n> think about.\n> \n> This is required to add reflog migration support to `git refs migrate`\n> which currently doesn't support it.\n\nNit: the second half of this sentence starting with \"which currently...\"\nfeels rather pointless, as it's implicit in the first half. I'd just\ndrop it.\n\n> Signed-off-by: Karthik Nayak \n> ---\n> refs/files-backend.c | 15 +++++++++++----\n> refs/reftable-backend.c | 16 +++++++++++++---\n> 2 files changed, 24 insertions(+), 7 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n> \n> \tupdate->backend_data = lock;\n> \n> +\tif (update->flags & REF_LOG_ONLY)\n> +\t\tgoto out;\n> +\n> \tif (update->type & REF_ISSYMREF) {\n> \t\tif (update->flags & REF_NO_DEREF) {\n> \t\t\t/*\n\nHm. Does this mean that we don't lock at all for REF_LOG_ONLY updates?\nReflogs themselves have no lockfile, so isn't it mandatory that we lock\nthe corresponding ref like we used to do? Otherwise I cannot see how we\navoid two concurrent writers to the same reflog.\n\n> @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n> \n> \t/* Fail if a refname appears more than once in the transaction: */\n> \tfor (i = 0; i < transaction->nr; i++)\n> -\t\tstring_list_append(&affected_refnames,\n> -\t\t\t\t transaction->updates[i]->refname);\n> +\t\tif (!(transaction->updates[i]->flags & REF_LOG_ONLY))\n> +\t\t\tstring_list_append(&affected_refnames,\n> +\t\t\t\t\t transaction->updates[i]->refname);\n> \tstring_list_sort(&affected_refnames);\n> \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n> \t\tret = TRANSACTION_GENERIC_ERROR;\n\nThis on the other hand is sensible -- having multiple REF_LOG_ONLY\ntransactions queued is fine.\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n> \t\t\t\t}\n> \n> \t\t\t\tfill_reftable_log_record(log, &c);\n> -\t\t\t\tlog->update_index = ts;\n> +\n> +\t\t\t\tupdate_index = strintmap_get(&logs_ts, u->refname);\n> +\t\t\t\tlog->update_index = update_index;\n> +\t\t\t\tstrintmap_set(&logs_ts, u->refname, update_index+1);\n\nSo we're now tracking update indices via another map in order to ensure\nthat the update index will be increased if we have multiple reflog\nentries for the same refname. Can we avoid that overhead by instead just\nhaving a global update index counter that increases for every single\nreflog entry, regardless of whether we have multiple ones queued up for\nthe same reference?\n\nI guess the result would be kind of weird as a single transaction with\nmultiple ref updates would now always contain N different update\nindices. Maybe there's an alternative that allows us to reduce the cost,\nlike only doing this for REF_LOG_ONLY updates?\n\nI'm mostly being careful because this here is the hot loop of writing\nrefs, so I don't want to regress performance.\n\nPatrick\n"},{"id":"508988","messageId":"Z1mhFqmznVxObmAn@pks.im","threadId":"62616","inReplyTo":"20241209-320-git-refs-migrate-reflogs-v1-7-d4bc37ee860f@gmail.com","subject":"Re: [PATCH 7/7] refs: add support for migrating reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-11T14:26:30Z","receivedAt":"2024-12-11T14:26:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 09, 2024 at 12:07:21PM +0100, Karthik Nayak wrote:\n> @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,\n> }\n> \n> struct migration_data {\n> +\tunsigned int index;\n> \tstruct ref_store *old_refs;\n> \tstruct ref_transaction *transaction;\n> \tstruct strbuf *errbuf;\n\nCalling this `reflog_index` might be a bit easier for context.\n\n> @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n> \t * 1. Set up a new temporary directory and initialize it with the new\n> \t * format. This is where all refs will be migrated into.\n> \t *\n> -\t * 2. Enumerate all refs and write them into the new ref storage.\n> -\t * This operation is safe as we do not yet modify the main\n> +\t * 2. Enumerate all refs and reflogs and write them into the new ref\n> +\t * storage. This operation is safe as we do not yet modify the main\n\nI'd rather move this into a third step, as it is separate from the ref\nenumeration.\n\nPatrick\n"},{"id":"508995","messageId":"CAOLa=ZQFAMdW78QbPYSoCtqetwT1S6z8dmLS_XG6Ex0nUsp9Cg@mail.gmail.com","threadId":"62616","inReplyTo":"CAP8UFD1vTEH=7HBX1sudwkv7now9VUkFR3Y347EVNGfToZt8fA@mail.gmail.com","subject":"Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-11T18:06:33Z","receivedAt":"2024-12-11T18:06:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n[snip]\n\n>> diff --git a/refs.c b/refs.c\n>> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n>> free(transaction);\n>> }\n>>\n>> -struct ref_update *ref_transaction_add_update(\n>> - struct ref_transaction *transaction,\n>> - const char *refname, unsigned int flags,\n>> - const struct object_id *new_oid,\n>> - const struct object_id *old_oid,\n>> - const char *new_target, const char *old_target,\n>> - const char *msg)\n>> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,\n>> + const char *refname,\n>> + unsigned int flags,\n>> + const struct object_id *new_oid,\n>> + const struct object_id *old_oid,\n>> + const char *new_target,\n>> + const char *old_target,\n>> + const char *committer_info,\n>\n> This change (adding a 'const char *committer_info' argument to\n> ref_transaction_add_update()) is not described in the commit message\n> and it requires a number of changes to the callers of this function,\n> so I think it might want to be in its own preparatory commit before\n> this one.\n>\n\nI think this is a great suggestion, it would reduce the congnitive load\nof the commit and make it easier to review. Will do.\n\n>> + const char *msg)\n>> {\n>> struct ref_update *update;\n>>\n>> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(\n>> oidcpy(&update->new_oid, new_oid);\n>> if ((flags & REF_HAVE_OLD) && old_oid)\n>> oidcpy(&update->old_oid, old_oid);\n>> - if (!(flags & REF_SKIP_CREATE_REFLOG))\n>> + if (!(flags & REF_SKIP_CREATE_REFLOG)) {\n>> + if (committer_info) {\n>> + struct strbuf sb = STRBUF_INIT;\n>> + strbuf_addstr(&sb, committer_info);\n>> + update->committer_info = strbuf_detach(&sb, NULL);\n>\n> Maybe:\n> update->committer_info = xstrdup(committer_info);\n>\n\nIndeed, I thought there was a better way. This is what I needed to have done.\n\n>> + }\n>> +\n>> update->msg = normalize_reflog_message(msg);\n>> + }\n>>\n>> return update;\n>> }\n>> @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update(\n>> static int transaction_refname_verification(const char *refname,\n>> const struct object_id *new_oid,\n>> unsigned int flags,\n>> + unsigned int reflog,\n>> struct strbuf *err)\n>> {\n>> if (flags & REF_SKIP_REFNAME_VERIFICATION)\n>> return 0;\n>>\n>> if (is_pseudo_ref(refname)) {\n>> - strbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n>> - refname);\n>> + if (reflog)\n>> + strbuf_addf(err, _(\"refusing to update reflog for pseudoref '%s'\"),\n>> + refname);\n>> + else\n>> + strbuf_addf(err, _(\"refusing to update pseudoref '%s'\"),\n>> + refname);\n>\n> Maybe:\n>\n> const char *what = reflog ? \"reflog for pseudoref\" : \"pseudoref\";\n> strbuf_addf(err, _(\"refusing to update %s '%s'\"), what, refname);\n>\n\nMuch nicer, will add.\n\n>> return -1;\n>> } else if ((new_oid && !is_null_oid(new_oid)) ?\n>> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n>> !refname_is_safe(refname)) {\n>> - strbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n>> - refname);\n>> + if (reflog)\n>> + strbuf_addf(err, _(\"refusing to update reflog with bad name '%s'\"),\n>> + refname);\n>> + else\n>> + strbuf_addf(err, _(\"refusing to update ref with bad name '%s'\"),\n>> + refname);\n>\n> Maybe:\n>\n> const char *what = reflog ? \"reflog with bad name\" :\n> \"ref with bad name\";\n> strbuf_addf(err, _(\"refusing to update %s '%s'\"), what, refname);\n>\n\nSimilar, will do.\n\n>> return -1;\n>> }\n>\n> [...]\n>\n>> int ref_transaction_create(struct ref_transaction *transaction,\n>> - const char *refname,\n>> - const struct object_id *new_oid,\n>> - const char *new_target,\n>> - unsigned int flags, const char *msg,\n>> - struct strbuf *err)\n>> + const char *refname, const struct object_id *new_oid,\n>> + const char *new_target, unsigned int flags,\n>> + const char *msg, struct strbuf *err)\n>\n> This looks like a wrapping or indenting only change. If you really\n> want to do it, it should probably be in its own preparatory commit.\n>\n\nI think it was the auto linter, will remove.\n\n>> index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction,\n>> unsigned int flags, const char *msg,\n>> struct strbuf *err);\n>>\n>> +/*\n>> + * Similar to `ref_transaction_update`, but this function is only for adding\n>> + * a reflog updates.\n>\n> \"a reflog update\" or \"reflog updates\".\n\nMakes sense. Thanks.\n"},{"id":"508996","messageId":"CAOLa=ZTLK=PL7YmkxzH5cZiHA21r1pM1qqbvu=fXi_rpRmHCYQ@mail.gmail.com","threadId":"62616","inReplyTo":"Z1mhCWXowokIaFR5@pks.im","subject":"Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-11T18:09:07Z","receivedAt":"2024-12-11T18:09:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote:\n>> diff --git a/refs.c b/refs.c\n>> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n>> \tfree(transaction);\n>> }\n>>\n>> -struct ref_update *ref_transaction_add_update(\n>> -\t\tstruct ref_transaction *transaction,\n>> -\t\tconst char *refname, unsigned int flags,\n>> -\t\tconst struct object_id *new_oid,\n>> -\t\tconst struct object_id *old_oid,\n>> -\t\tconst char *new_target, const char *old_target,\n>> -\t\tconst char *msg)\n>> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,\n>> +\t\t\t\t\t const char *refname,\n>> +\t\t\t\t\t unsigned int flags,\n>> +\t\t\t\t\t const struct object_id *new_oid,\n>> +\t\t\t\t\t const struct object_id *old_oid,\n>> +\t\t\t\t\t const char *new_target,\n>> +\t\t\t\t\t const char *old_target,\n>> +\t\t\t\t\t const char *committer_info,\n>> +\t\t\t\t\t const char *msg)\n>> {\n>> \tstruct ref_update *update;\n>>\n>\n> I'd personally avoid reindenting this block. It's somewhat-common\n> practice to not align all arguments with the opening brace when the line\n> would become too long. The reindents also distract a bit from the actual\n> changes done in other places further down.\n>\n\nMakes sense, I'll undo that.\n\n>> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update(\n>> \t\toidcpy(&update->new_oid, new_oid);\n>> \tif ((flags & REF_HAVE_OLD) && old_oid)\n>> \t\toidcpy(&update->old_oid, old_oid);\n>> -\tif (!(flags & REF_SKIP_CREATE_REFLOG))\n>> +\tif (!(flags & REF_SKIP_CREATE_REFLOG)) {\n>> +\t\tif (committer_info) {\n>> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n>> +\t\t\tstrbuf_addstr(&sb, committer_info);\n>> +\t\t\tupdate->committer_info = strbuf_detach(&sb, NULL);\n>\n> Can't we simplify this via `xstrdup()`?\n>\n\nYup, Christian suggested the same too, will fix it up.\n\n>> @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n>> \t\t}\n>>\n>> \t\t/*\n>> -\t\t * packed-refs don't support symbolic refs and root refs, so we\n>> -\t\t * have to queue these references via the loose transaction.\n>> +\t\t * packed-refs don't support symbolic refs, root refs and reflogs,\n>> +\t\t * so we have to queue these references via the loose transaction.\n>> \t\t */\n>> -\t\tif (update->new_target || is_root_ref(update->refname)) {\n>> +\t\tif (update->new_target ||\n>> +\t\t is_root_ref(update->refname) ||\n>> +\t\t (update->flags & REF_LOG_ONLY)) {\n>> \t\t\tif (!loose_transaction) {\n>> \t\t\t\tloose_transaction = ref_store_transaction_begin(&refs->base, 0, err);\n>> \t\t\t\tif (!loose_transaction) {\n>\n> Makes sense. While we already had REF_LOG_ONLY beforehand, it was only\n> used in very specific cases and thus the support implemented by the\n> backends is lacking. And given that the packed-ref backend does not\n> support reflogs we have to queue these up via the loose backend.\n>\n> Patrick\n"},{"id":"509031","messageId":"CAOLa=ZROACAy2_jDSUxZ9rAZ0o7eKugtuqZLh_aWkHH6NRjdcQ@mail.gmail.com","threadId":"62616","inReplyTo":"Z1mhFqmznVxObmAn@pks.im","subject":"Re: [PATCH 7/7] refs: add support for migrating reflogs","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-12T14:04:06Z","receivedAt":"2024-12-12T14:04:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Dec 09, 2024 at 12:07:21PM +0100, Karthik Nayak wrote:\n>> @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,\n>> }\n>>\n>> struct migration_data {\n>> +\tunsigned int index;\n>> \tstruct ref_store *old_refs;\n>> \tstruct ref_transaction *transaction;\n>> \tstruct strbuf *errbuf;\n>\n> Calling this `reflog_index` might be a bit easier for context.\n>\n\nMakes sense, will amend.\n\n>> @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>> \t * 1. Set up a new temporary directory and initialize it with the new\n>> \t * format. This is where all refs will be migrated into.\n>> \t *\n>> -\t * 2. Enumerate all refs and write them into the new ref storage.\n>> -\t * This operation is safe as we do not yet modify the main\n>> +\t * 2. Enumerate all refs and reflogs and write them into the new ref\n>> +\t * storage. This operation is safe as we do not yet modify the main\n>\n> I'd rather move this into a third step, as it is separate from the ref\n> enumeration.\n>\n> Patrick\n>\n\nI added it together since it is a single transaction, but I'm okay with\nmaking this change, will amend in. Thanks\n"},{"id":"509033","messageId":"CAOLa=ZQqQTEquJ0e5rG168-CVADR8K-uYma7Z8yiDCptyPgBQg@mail.gmail.com","threadId":"62616","inReplyTo":"Z1mhEsu8DBsCsYuA@pks.im","subject":"Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-12T14:47:54Z","receivedAt":"2024-12-12T14:47:55Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Dec 09, 2024 at 12:07:20PM +0100, Karthik Nayak wrote:\n>> The reference transaction only allows a update for a given reference to\n>> avoid conflicts. This, however, isn't an issue for reflogs. There are no\n>> conflicts to be resolved in reflogs and when migrating reflogs between\n>> backends we'd have multiple reflog entries for the same refname.\n>>\n>> So allow multiple reflog updates within a single transaction. Also the\n>> reflog creation logic isn't exposed to the end user. While this might\n>> change in the future, currently, this reduces the scope of issues to\n>> think about.\n>>\n>> This is required to add reflog migration support to `git refs migrate`\n>> which currently doesn't support it.\n>\n> Nit: the second half of this sentence starting with \"which currently...\"\n> feels rather pointless, as it's implicit in the first half. I'd just\n> drop it.\n>\n\nWill do.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> refs/files-backend.c | 15 +++++++++++----\n>> refs/reftable-backend.c | 16 +++++++++++++---\n>> 2 files changed, 24 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n>>\n>> \tupdate->backend_data = lock;\n>>\n>> +\tif (update->flags & REF_LOG_ONLY)\n>> +\t\tgoto out;\n>> +\n>> \tif (update->type & REF_ISSYMREF) {\n>> \t\tif (update->flags & REF_NO_DEREF) {\n>> \t\t\t/*\n>\n> Hm. Does this mean that we don't lock at all for REF_LOG_ONLY updates?\n> Reflogs themselves have no lockfile, so isn't it mandatory that we lock\n> the corresponding ref like we used to do? Otherwise I cannot see how we\n> avoid two concurrent writers to the same reflog.\n>\n\nNo it doesn't, this is after the lock is obtained. We simply exit early\nsince for reflog only updates, there is nothing further to do.\n\n>> @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n>>\n>> \t/* Fail if a refname appears more than once in the transaction: */\n>> \tfor (i = 0; i < transaction->nr; i++)\n>> -\t\tstring_list_append(&affected_refnames,\n>> -\t\t\t\t transaction->updates[i]->refname);\n>> +\t\tif (!(transaction->updates[i]->flags & REF_LOG_ONLY))\n>> +\t\t\tstring_list_append(&affected_refnames,\n>> +\t\t\t\t\t transaction->updates[i]->refname);\n>> \tstring_list_sort(&affected_refnames);\n>> \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n>> \t\tret = TRANSACTION_GENERIC_ERROR;\n>\n> This on the other hand is sensible -- having multiple REF_LOG_ONLY\n> transactions queued is fine.\n>\n>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n>> index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644\n>> --- a/refs/reftable-backend.c\n>> +++ b/refs/reftable-backend.c\n>> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>> \t\t\t\t}\n>>\n>> \t\t\t\tfill_reftable_log_record(log, &c);\n>> -\t\t\t\tlog->update_index = ts;\n>> +\n>> +\t\t\t\tupdate_index = strintmap_get(&logs_ts, u->refname);\n>> +\t\t\t\tlog->update_index = update_index;\n>> +\t\t\t\tstrintmap_set(&logs_ts, u->refname, update_index+1);\n>\n> So we're now tracking update indices via another map in order to ensure\n> that the update index will be increased if we have multiple reflog\n> entries for the same refname. Can we avoid that overhead by instead just\n> having a global update index counter that increases for every single\n> reflog entry, regardless of whether we have multiple ones queued up for\n> the same reference?\n>\n> I guess the result would be kind of weird as a single transaction with\n> multiple ref updates would now always contain N different update\n> indices. Maybe there's an alternative that allows us to reduce the cost,\n> like only doing this for REF_LOG_ONLY updates?\n>\n> I'm mostly being careful because this here is the hot loop of writing\n> refs, so I don't want to regress performance.\n\nThanks for bringing this up. I was thinking hard about this for a while.\nI also did some local benchmarking, for 10000 atomic writes, I couldn't\nfind a note-worthy regression.\n\nBut I really like the point you made about how this could probably use a\ncounter. I think we can use `u->index`. The index field was added to\nensure that the logs stay in order when we sort inside\n`write_transaction_table()`. But we can use the same here. We can simply\ndo\n\n log->update_index = ts + u->index;\n\nThis would increment the update_index as needed and also only kick in\nwhen the `index` itself is set. Which would be only for reflog migration\nbetween backends.\n\nI think this would work well.\n\nKarthik\n"},{"id":"509034","messageId":"CAOLa=ZTFJ+U3RmDQs8hwTCnfNfTarpxE1otcYAAV-FSOogAq=Q@mail.gmail.com","threadId":"62616","inReplyTo":"CAP8UFD2zogHD_g5FOVNFvBswPJF749wd4f2s9Pzgef-pv=U0Gg@mail.gmail.com","subject":"Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-12T14:52:04Z","receivedAt":"2024-12-12T14:52:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote:\n>>\n>> The reference transaction only allows a update for a given reference to\n>\n> s/a update/an update/\n>\n> or: s/a update/a single update/\n\n'a single update' sounds the best here, will add.\n\n>> avoid conflicts. This, however, isn't an issue for reflogs. There are no\n>> conflicts to be resolved in reflogs and when migrating reflogs between\n>> backends we'd have multiple reflog entries for the same refname.\n>\n>\n>> @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>> struct ident_split committer_ident = {0};\n>> size_t logs_nr = 0, logs_alloc = 0, i;\n>> const char *committer_info;\n>> + struct strintmap logs_ts;\n>\n> Here a comment might help explain what logs_ts is used for.\n>\n\nI think with Patricks comment, this whole code will be removed for\nsomething simpler.\n\n>> int ret = 0;\n>>\n>> committer_info = git_committer_info(0);\n>> @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>>\n>> QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n>>\n>> + strintmap_init(&logs_ts, ts);\n>\n> I am not sure I understand what logs_ts is used for and why its\n> default value is set to ts.\n>\n\nThe reason I added this was because in the reftable backend, the writer\nsorts logs before writing. So if the multiple reflogs contained the same\nupdate_index, their order might be changed. But for migrating reflogs,\nwe need to ensure we maintain the order. Using a map here, allowed us to\nincrement the update_index for reflogs for a given refname.\n\n> Also ts is an uint64_t while the second argument to strintmap_init()\n> is an int. I wonder if it could be an issue especially on 32 bits\n> platforms.\n>\n\nThis is fair point, I decided to scrap this ultimately and simply append\n`u->index` to the update_index. Which would provide the same desired\neffect.\n\n>> reftable_writer_set_limits(writer, ts, ts);\n>>\n>> for (i = 0; i < arg->updates_nr; i++) {\n>> @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>>\n>> if (create_reflog) {\n>> struct ident_split c;\n>> + uint64_t update_index;\n>>\n>> ALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n>> log = &logs[logs_nr++];\n>> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n>> }\n>>\n>> fill_reftable_log_record(log, &c);\n>> - log->update_index = ts;\n>> +\n>> + update_index = strintmap_get(&logs_ts, u->refname);\n>> + log->update_index = update_index;\n>> + strintmap_set(&logs_ts, u->refname, update_index+1);\n>\n> s/update_index+1/update_index + 1/\n>\n> Also is the 'update_index' var really needed or could we just do:\n>\n> log->update_index =\n> strintmap_get(&logs_ts, u->refname);\n> strintmap_set(&logs_ts, u->refname,\n> log->update_index + 1);\n>\n> ?\n>\n\nThe temp variable can be removed here indeed. But I'll remove all of\nthis in the next version. Thanks\n\n>> log->refname = xstrdup(u->refname);\n>> memcpy(log->value.update.new_hash,\n>> u->new_oid.hash, GIT_MAX_RAWSZ);\n"}]}