# [PATCH 0/7] refs: add reflog support to `git refs migrate` 30 messages from 2024-12-09 to 2024-12-12. Participants: Karthik Nayak, Junio C Hamano, Christian Couder, karthik nayak, Patrick Steinhardt. Thread: https://gitlist.dev/t/62616 ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 0/7] refs: add reflog support to `git refs migrate` Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f%40gmail.com ``` The `git refs migrate` command was introduced in 25a0023f28 (builtin/refs: new command to migrate ref storage formats, 2024-06-06) to support migrating from one reference backend to another. One limitation of the feature was that it didn't support migrating repositories which contained reflogs. This isn't a requirement on the server side as repositories are stored as bare repositories (which do not contain any reflogs). Clients however generally use reflogs and until now couldn't use the `git refs migrate` command to migrate their repositories to the new reftable format. One of the issues for adding reflog support is that the ref transactions don't support reflogs additions: 1. While there is REF_LOG_ONLY flag, there is no function to utilize the flag and add reflogs. 2. reference backends generally sort the updates by the refname. This wouldn't work for reflogs which need to ensure that they maintain the order of creation. 3. In the files backend, reflog entries are added by obtaining locks on the refs themselves. This means each update in the transaction, will obtain a ref_lock. This paradigm fails to accompany the fact that there could be multiple reflog updates for a refname in a single transaction. 4. The backends check for duplicate entries, which doesn't make sense in the context of adding multiple reflogs for a given refname. We overcome these issue we make the following changes: - Update the ref_update structure to also include the committer information. Using this, we can add a new function which only adds reflog updates to the transaction. - Add an index field to the ref_update structure, this will help order updates in pre-defined order, this fixes #2. - While the ideal fix for #3 would be to actually introduce reflog locks, this wouldn't be possible without breaking backward compatibility. So we add a count field to the existing ref_lock. With this, multiple reflog updates can share a single ref_lock. Overall, this series is a bit more involved, and I would appreciate it if it receives a bit more scrutiny. Signed-off-by: Karthik Nayak --- Karthik Nayak (7): refs: include committer info in `ref_update` struct refs: add `index` field to `struct ref_udpate` refs/files: add count field to ref_lock refs: extract out refname verification in transactions refs: introduce the `ref_transaction_update_reflog` function refs: allow multiple reflog entries for the same refname refs: add support for migrating reflogs Documentation/git-refs.txt | 2 - refs.c | 204 ++++++++++++++++++++++++++++++++------------- refs.h | 12 +++ refs/files-backend.c | 144 ++++++++++++++++++++------------ refs/refs-internal.h | 24 ++++-- refs/reftable-backend.c | 47 +++++++++-- t/t1460-refs-migrate.sh | 73 +++++++++++----- 7 files changed, 360 insertions(+), 146 deletions(-) --- --- base-commit: e66fd72e972df760a53c3d6da023c17adfc426d6 change-id: 20241111-320-git-refs-migrate-reflogs-a53e3a6cffc9 Thanks - Karthik ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 1/7] refs: include committer info in `ref_update` struct Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` The reference backends obtain the committer information from `git_committer_info(0)` when adding a reflog. The upcoming patches introduce support for migrating reflogs between the reference backends. This requires an interface to creating reflogs, including custom committer information. Add a new field `committer_info` to the `ref_update` struct, which is then used by the reference backends. If there is no `committer_info` provided, the reference backends default to using `git_committer_info(0)`. The field itself cannot be set to `git_committer_info(0)` since the values are dynamic and must be obtained right when the reflog is being committed. Signed-off-by: Karthik Nayak --- refs.c | 1 + refs/files-backend.c | 22 +++++++++++++--------- refs/refs-internal.h | 1 + refs/reftable-backend.c | 12 +++++++++++- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index 762f3e324d59c60cd4f05c2f257e54de8deb00e5..f003e51c6bf5229bfbce8ce61ffad7cdba0572e0 100644 --- a/refs.c +++ b/refs.c @@ -1151,6 +1151,7 @@ void ref_transaction_free(struct ref_transaction *transaction) for (i = 0; i < transaction->nr; i++) { free(transaction->updates[i]->msg); + free(transaction->updates[i]->committer_info); free((char *)transaction->updates[i]->new_target); free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); diff --git a/refs/files-backend.c b/refs/files-backend.c index 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, struct strbuf sb = STRBUF_INIT; int ret = 0; + if (!committer) + committer = git_committer_info(0); + strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); if (msg && *msg) { strbuf_addch(&sb, '\t'); @@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, } static int files_log_ref_write(struct files_ref_store *refs, - const char *refname, const struct object_id *old_oid, - const struct object_id *new_oid, const char *msg, + const char *refname, + const struct object_id *old_oid, + const struct object_id *new_oid, + const char *committer_info, const char *msg, int flags, struct strbuf *err) { int logfd, result; @@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs, if (logfd < 0) return 0; - result = log_ref_write_fd(logfd, old_oid, new_oid, - git_committer_info(0), msg); + result = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg); if (result) { struct strbuf sb = STRBUF_INIT; int save_errno = errno; @@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs, files_assert_main_repository(refs, "commit_ref_update"); clear_loose_ref_cache(refs); - if (files_log_ref_write(refs, lock->ref_name, - &lock->old_oid, oid, + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL, logmsg, flags, err)) { char *old_msg = strbuf_detach(err, NULL); strbuf_addf(err, "cannot update the ref '%s': %s", @@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs, if (head_ref && (head_flag & REF_ISSYMREF) && !strcmp(head_ref, lock->ref_name)) { struct strbuf log_err = STRBUF_INIT; - if (files_log_ref_write(refs, "HEAD", - &lock->old_oid, oid, + if (files_log_ref_write(refs, "HEAD", &lock->old_oid, + oid, git_committer_info(0), logmsg, flags, &log_err)) { error("%s", log_err.buf); strbuf_release(&log_err); @@ -2969,7 +2972,8 @@ static int parse_and_write_reflog(struct files_ref_store *refs, } if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, - &update->new_oid, update->msg, update->flags, err)) { + &update->new_oid, update->committer_info, + update->msg, update->flags, err)) { char *old_msg = strbuf_detach(err, NULL); strbuf_addf(err, "cannot update the ref '%s': %s", diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 58aa56d1b27c85d606ed7c8c0d908e4b87d1066b..0fd95cdacd99e4a728c22f5286f6b3f0f360c110 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -113,6 +113,7 @@ struct ref_update { void *backend_data; unsigned int type; char *msg; + char *committer_info; /* * If this ref_update was split off of a symref update via diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data } if (create_reflog) { + struct ident_split c; + ALLOC_GROW(logs, logs_nr + 1, logs_alloc); log = &logs[logs_nr++]; memset(log, 0, sizeof(*log)); - fill_reftable_log_record(log, &committer_ident); + if (u->committer_info) { + if (split_ident_line(&c, u->committer_info, + strlen(u->committer_info))) + BUG("failed splitting committer info"); + } else { + c = committer_ident; + } + + fill_reftable_log_record(log, &c); log->update_index = ts; log->refname = xstrdup(u->refname); memcpy(log->value.update.new_hash, -- 2.47.1 ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 2/7] refs: add `index` field to `struct ref_udpate` Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-2-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-2-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` The reftable backend, sorts its updates by refname before applying them, this ensures that the references are stored sorted. When migrating reflogs from one backend to another, the order of the reflogs must be maintained. Add a new `index` field to the `ref_update` struct to facilitate this. This field is used in the reftable backend's sort comparison function `transaction_update_cmp`, to ensure that indexed fields maintain their order. Signed-off-by: Karthik Nayak --- refs/refs-internal.h | 7 +++++++ refs/reftable-backend.c | 13 +++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 0fd95cdacd99e4a728c22f5286f6b3f0f360c110..f5c733d099f0c6f1076a25f4f77d9d5eb345ec87 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -115,6 +115,13 @@ struct ref_update { char *msg; char *committer_info; + /* + * The index overrides the default sort algorithm. This is needed + * when migrating reflogs and we want to ensure we carry over the + * same order. + */ + unsigned int index; + /* * If this ref_update was split off of a symref update via * split_symref_update(), then this member points at that diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index e882602487c66261d586a94101bb1b4e9a2ed60e..c008f20be719fec3af6a8f81c821cb9c263764d7 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1279,8 +1279,17 @@ static int reftable_be_transaction_abort(struct ref_store *ref_store UNUSED, static int transaction_update_cmp(const void *a, const void *b) { - return strcmp(((struct reftable_transaction_update *)a)->update->refname, - ((struct reftable_transaction_update *)b)->update->refname); + struct reftable_transaction_update *update_a = (struct reftable_transaction_update *)a; + struct reftable_transaction_update *update_b = (struct reftable_transaction_update *)b; + + /* + * If there is an index set, it should take preference (default is 0). + * This ensures that updates with indexes are sorted amongst themselves. + */ + if (update_a->update->index || update_b->update->index) + return update_a->update->index - update_b->update->index; + + return strcmp(update_a->update->refname, update_b->update->refname); } static int write_transaction_table(struct reftable_writer *writer, void *cb_data) -- 2.47.1 ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 3/7] refs/files: add count field to ref_lock Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` When refs are updated in the files-backend, a lock is obtained for the corresponding file path. This is the case even for reflogs, i.e. a lock is obtained on the reference path instead of the reflog path. This works, since generally, reflogs are updated alongside the ref. The upcoming patches will add support for reflog updates in ref transaction. This means, in a particular transaction we want to have ref updates and reflog updates. For refs, in a given transaction there can only be one update. But, we can theoretically have multiple reflog updates in a given transaction. The current flow does not support this, because currently refs & reflogs are treated as a single entity and capture the lock together. To separate this, add a count field to ref_lock. With this, multiple updates can hold onto a single ref_lock and the lock will only be released when all of them release the lock. This patch only adds the `count` field to `ref_lock` and adds the logic to increment and decrement the lock. In a follow up commit, we'll separate the reflog update logic from ref updates and utilize this functionality. Signed-off-by: Karthik Nayak --- refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -71,6 +71,8 @@ struct ref_lock { char *ref_name; struct lock_file lk; struct object_id old_oid; + /* count keeps track of users of the lock */ + unsigned int count; }; struct files_ref_store { @@ -638,9 +640,12 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop, static void unlock_ref(struct ref_lock *lock) { - rollback_lock_file(&lock->lk); - free(lock->ref_name); - free(lock); + lock->count--; + if (!lock->count) { + rollback_lock_file(&lock->lk); + free(lock->ref_name); + free(lock); + } } /* @@ -696,6 +701,7 @@ static int lock_raw_ref(struct files_ref_store *refs, *lock_p = CALLOC_ARRAY(lock, 1); lock->ref_name = xstrdup(refname); + lock->count = 1; files_ref_path(refs, &ref_file, refname); retry: @@ -1169,6 +1175,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, goto error_return; lock->ref_name = xstrdup(refname); + lock->count = 1; if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) { unable_to_lock_message(ref_file.buf, errno, err); @@ -2535,6 +2542,12 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, return -1; } +struct files_transaction_backend_data { + struct ref_transaction *packed_transaction; + int packed_refs_locked; + struct strmap ref_locks; +}; + /* * Prepare for carrying out update: * - Lock the reference referred to by update. @@ -2557,11 +2570,14 @@ static int lock_ref_for_update(struct files_ref_store *refs, { struct strbuf referent = STRBUF_INIT; int mustexist = ref_update_expects_existing_old_ref(update); + struct files_transaction_backend_data *backend_data; int ret = 0; struct ref_lock *lock; files_assert_main_repository(refs, "lock_ref_for_update"); + backend_data = transaction->backend_data; + if ((update->flags & REF_HAVE_NEW) && ref_update_has_null_new_value(update)) update->flags |= REF_DELETING; @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs, goto out; } - ret = lock_raw_ref(refs, update->refname, mustexist, - affected_refnames, - &lock, &referent, - &update->type, err); - if (ret) { - char *reason; + lock = strmap_get(&backend_data->ref_locks, update->refname); + if (lock) { + lock->count = lock->count + 1; + } else { + ret = lock_raw_ref(refs, update->refname, mustexist, + affected_refnames, + &lock, &referent, + &update->type, err); + if (ret) { + char *reason; + + reason = strbuf_detach(err, NULL); + strbuf_addf(err, "cannot lock ref '%s': %s", + ref_update_original_update_refname(update), reason); + free(reason); + goto out; + } - reason = strbuf_detach(err, NULL); - strbuf_addf(err, "cannot lock ref '%s': %s", - ref_update_original_update_refname(update), reason); - free(reason); - goto out; + strmap_put(&backend_data->ref_locks, update->refname, lock); } update->backend_data = lock; @@ -2730,11 +2753,6 @@ static int lock_ref_for_update(struct files_ref_store *refs, return ret; } -struct files_transaction_backend_data { - struct ref_transaction *packed_transaction; - int packed_refs_locked; -}; - /* * Unlock any references in `transaction` that are still locked, and * mark the transaction closed. @@ -2767,6 +2785,8 @@ static void files_transaction_cleanup(struct files_ref_store *refs, if (backend_data->packed_refs_locked) packed_refs_unlock(refs->packed_ref_store); + strmap_clear(&backend_data->ref_locks, 0); + free(backend_data); } @@ -2796,6 +2816,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, goto cleanup; CALLOC_ARRAY(backend_data, 1); + strmap_init(&backend_data->ref_locks); transaction->backend_data = backend_data; /* -- 2.47.1 ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 4/7] refs: extract out refname verification in transactions Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update, the refname of the update is verified for: - Ensuring it is not a pseudoref. - Checking the refname format. These checks are also be needed in a following commit where the function to add reflog updates to the transaction is introduced. Extract the code out into a new static function. Signed-off-by: Karthik Nayak --- refs.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/refs.c b/refs.c index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 --- a/refs.c +++ b/refs.c @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( return update; } +static int transaction_refname_verification(const char *refname, + const struct object_id *new_oid, + unsigned int flags, + struct strbuf *err) +{ + if (flags & REF_SKIP_REFNAME_VERIFICATION) + return 0; + + if (is_pseudo_ref(refname)) { + strbuf_addf(err, _("refusing to update pseudoref '%s'"), + refname); + return -1; + } else if ((new_oid && !is_null_oid(new_oid)) ? + check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : + !refname_is_safe(refname)) { + strbuf_addf(err, _("refusing to update ref with bad name '%s'"), + refname); + return -1; + } + + return 0; +} + int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, @@ -1205,6 +1228,8 @@ int ref_transaction_update(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err) { + int ret; + assert(err); if ((flags & REF_FORCE_CREATE_REFLOG) && @@ -1213,21 +1238,9 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } - if (!(flags & REF_SKIP_REFNAME_VERIFICATION) && - ((new_oid && !is_null_oid(new_oid)) ? - check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : - !refname_is_safe(refname))) { - strbuf_addf(err, _("refusing to update ref with bad name '%s'"), - refname); - return -1; - } - - if (!(flags & REF_SKIP_REFNAME_VERIFICATION) && - is_pseudo_ref(refname)) { - strbuf_addf(err, _("refusing to update pseudoref '%s'"), - refname); - return -1; - } + ret = transaction_refname_verification(refname, new_oid, flags, err); + if (ret) + return ret; if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS) BUG("illegal flags 0x%x passed to ref_transaction_update()", flags); -- 2.47.1 ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` Introduce a new function `ref_transaction_update_reflog`, for clients to add a reflog update to a transaction. While the existing function `ref_transaction_update` also allows clients to add a reflog entry, this function does a few things more, It: - Enforces that only a reflog entry is added and does not update the ref itself. - Allows the users to also provide the committer information. This means clients can add reflog entries with custom committer information. A follow up commit will utilize this function to add reflog support to `git refs migrate`. Signed-off-by: Karthik Nayak --- refs.c | 89 +++++++++++++++++++++++++++++++++++++------------ refs.h | 12 +++++++ refs/files-backend.c | 48 +++++++++++++++----------- refs/refs-internal.h | 16 +++++---- refs/reftable-backend.c | 6 ++-- 5 files changed, 122 insertions(+), 49 deletions(-) diff --git a/refs.c b/refs.c index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644 --- a/refs.c +++ b/refs.c @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } -struct ref_update *ref_transaction_add_update( - struct ref_transaction *transaction, - const char *refname, unsigned int flags, - const struct object_id *new_oid, - const struct object_id *old_oid, - const char *new_target, const char *old_target, - const char *msg) +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, + const char *refname, + unsigned int flags, + const struct object_id *new_oid, + const struct object_id *old_oid, + const char *new_target, + const char *old_target, + const char *committer_info, + const char *msg) { struct ref_update *update; @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update( oidcpy(&update->new_oid, new_oid); if ((flags & REF_HAVE_OLD) && old_oid) oidcpy(&update->old_oid, old_oid); - if (!(flags & REF_SKIP_CREATE_REFLOG)) + if (!(flags & REF_SKIP_CREATE_REFLOG)) { + if (committer_info) { + struct strbuf sb = STRBUF_INIT; + strbuf_addstr(&sb, committer_info); + update->committer_info = strbuf_detach(&sb, NULL); + } + update->msg = normalize_reflog_message(msg); + } return update; } @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update( static int transaction_refname_verification(const char *refname, const struct object_id *new_oid, unsigned int flags, + unsigned int reflog, struct strbuf *err) { if (flags & REF_SKIP_REFNAME_VERIFICATION) return 0; if (is_pseudo_ref(refname)) { - strbuf_addf(err, _("refusing to update pseudoref '%s'"), - refname); + if (reflog) + strbuf_addf(err, _("refusing to update reflog for pseudoref '%s'"), + refname); + else + strbuf_addf(err, _("refusing to update pseudoref '%s'"), + refname); return -1; } else if ((new_oid && !is_null_oid(new_oid)) ? check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : !refname_is_safe(refname)) { - strbuf_addf(err, _("refusing to update ref with bad name '%s'"), - refname); + if (reflog) + strbuf_addf(err, _("refusing to update reflog with bad name '%s'"), + refname); + else + strbuf_addf(err, _("refusing to update ref with bad name '%s'"), + refname); return -1; } @@ -1238,7 +1256,7 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } - ret = transaction_refname_verification(refname, new_oid, flags, err); + ret = transaction_refname_verification(refname, new_oid, flags, 0, err); if (ret) return ret; @@ -1255,18 +1273,47 @@ int ref_transaction_update(struct ref_transaction *transaction, flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); flags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0); - ref_transaction_add_update(transaction, refname, flags, - new_oid, old_oid, new_target, - old_target, msg); + ref_transaction_add_update(transaction, refname, flags, new_oid, + old_oid, new_target, old_target, NULL, msg); + return 0; +} + +int ref_transaction_update_reflog(struct ref_transaction *transaction, + const char *refname, + const struct object_id *new_oid, + const struct object_id *old_oid, + const char *committer_info, unsigned int flags, + const char *msg, unsigned int index, + struct strbuf *err) +{ + struct ref_update *update; + int ret; + + assert(err); + + ret = transaction_refname_verification(refname, new_oid, flags, 1, err); + if (ret) + return ret; + + flags |= REF_LOG_ONLY | REF_NO_DEREF; + + update = ref_transaction_add_update(transaction, refname, flags, + new_oid, old_oid, NULL, NULL, + committer_info, msg); + /* + * While we do set the old_oid value, we unset the flag to skip + * old_oid verification which only makes sense for refs. + */ + update->flags &= ~REF_HAVE_OLD; + update->index = index; + return 0; } int ref_transaction_create(struct ref_transaction *transaction, - const char *refname, - const struct object_id *new_oid, - const char *new_target, - unsigned int flags, const char *msg, - struct strbuf *err) + const char *refname, const struct object_id *new_oid, + const char *new_target, unsigned int flags, + const char *msg, struct strbuf *err) { if (new_oid && new_target) BUG("create called with both new_oid and new_target set"); diff --git a/refs.h b/refs.h index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644 --- a/refs.h +++ b/refs.h @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err); +/* + * Similar to `ref_transaction_update`, but this function is only for adding + * a reflog updates. Supports providing custom committer information. + */ +int ref_transaction_update_reflog(struct ref_transaction *transaction, + const char *refname, + const struct object_id *new_oid, + const struct object_id *old_oid, + const char *committer_info, unsigned int flags, + const char *msg, unsigned int index, + struct strbuf *err); + /* * Add a reference creation to transaction. new_oid is the value that * the reference should have after the update; it must not be diff --git a/refs/files-backend.c b/refs/files-backend.c index 9c929c1ac33bc62a75620e684a809d46b574f1c6..32975e0fd7a03ab8ddf99c0a68af99921d3f5090 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1268,10 +1268,10 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r) transaction = ref_store_transaction_begin(&refs->base, 0, &err); if (!transaction) goto cleanup; - ref_transaction_add_update( - transaction, r->name, - REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING, - null_oid(), &r->oid, NULL, NULL, NULL); + ref_transaction_add_update(transaction, r->name, + REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | + REF_IS_PRUNING, null_oid(), &r->oid, NULL, + NULL, NULL, NULL); if (ref_transaction_commit(transaction, &err)) goto cleanup; @@ -2418,7 +2418,7 @@ static int split_head_update(struct ref_update *update, transaction, "HEAD", update->flags | REF_LOG_ONLY | REF_NO_DEREF, &update->new_oid, &update->old_oid, - NULL, NULL, update->msg); + NULL, NULL, update->committer_info, update->msg); /* * Add "HEAD". This insertion is O(N) in the transaction @@ -2482,7 +2482,8 @@ static int split_symref_update(struct ref_update *update, transaction, referent, new_flags, update->new_target ? NULL : &update->new_oid, update->old_target ? NULL : &update->old_oid, - update->new_target, update->old_target, update->msg); + update->new_target, update->old_target, NULL, + update->msg); new_update->parent_update = update; @@ -2911,11 +2912,11 @@ static int files_transaction_prepare(struct ref_store *ref_store, packed_transaction; } - ref_transaction_add_update( - packed_transaction, update->refname, - REF_HAVE_NEW | REF_NO_DEREF, - &update->new_oid, NULL, - NULL, NULL, NULL); + ref_transaction_add_update(packed_transaction, + update->refname, + REF_HAVE_NEW | REF_NO_DEREF, + &update->new_oid, NULL, NULL, + NULL, NULL, NULL); } } @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } /* - * packed-refs don't support symbolic refs and root refs, so we - * have to queue these references via the loose transaction. + * packed-refs don't support symbolic refs, root refs and reflogs, + * so we have to queue these references via the loose transaction. */ - if (update->new_target || is_root_ref(update->refname)) { + if (update->new_target || + is_root_ref(update->refname) || + (update->flags & REF_LOG_ONLY)) { if (!loose_transaction) { loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); if (!loose_transaction) { @@ -3092,15 +3095,22 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } } - ref_transaction_add_update(loose_transaction, update->refname, - update->flags & ~REF_HAVE_OLD, - update->new_target ? NULL : &update->new_oid, NULL, - update->new_target, NULL, NULL); + if (update->flags & REF_LOG_ONLY) + ref_transaction_add_update(loose_transaction, update->refname, + update->flags, &update->new_oid, + &update->old_oid, NULL, NULL, + update->committer_info, update->msg); + else + ref_transaction_add_update(loose_transaction, update->refname, + update->flags & ~REF_HAVE_OLD, + update->new_target ? NULL : &update->new_oid, NULL, + update->new_target, NULL, update->committer_info, + NULL); } else { ref_transaction_add_update(packed_transaction, update->refname, update->flags & ~REF_HAVE_OLD, &update->new_oid, &update->old_oid, - NULL, NULL, NULL); + NULL, NULL, update->committer_info, NULL); } } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index f5c733d099f0c6f1076a25f4f77d9d5eb345ec87..82c1387d1e6ab3658b31fe99c95f98645ff1ebf1 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -156,13 +156,15 @@ int ref_update_reject_duplicates(struct string_list *refnames, * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits, * respectively, are set in flags. */ -struct ref_update *ref_transaction_add_update( - struct ref_transaction *transaction, - const char *refname, unsigned int flags, - const struct object_id *new_oid, - const struct object_id *old_oid, - const char *new_target, const char *old_target, - const char *msg); +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, + const char *refname, + unsigned int flags, + const struct object_id *new_oid, + const struct object_id *old_oid, + const char *new_target, + const char *old_target, + const char *committer_info, + const char *msg); /* * Transaction states. diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index c008f20be719fec3af6a8f81c821cb9c263764d7..b2e3ba877de9e59fea5a4d066eb13e60ef22a32b 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1078,7 +1078,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, new_update = ref_transaction_add_update( transaction, "HEAD", u->flags | REF_LOG_ONLY | REF_NO_DEREF, - &u->new_oid, &u->old_oid, NULL, NULL, u->msg); + &u->new_oid, &u->old_oid, NULL, NULL, NULL, + u->msg); string_list_insert(&affected_refnames, new_update->refname); } @@ -1161,7 +1162,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction, referent.buf, new_flags, u->new_target ? NULL : &u->new_oid, u->old_target ? NULL : &u->old_oid, - u->new_target, u->old_target, u->msg); + u->new_target, u->old_target, + u->committer_info, u->msg); new_update->parent_update = u; -- 2.47.1 ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 6/7] refs: allow multiple reflog entries for the same refname Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` The reference transaction only allows a update for a given reference to avoid conflicts. This, however, isn't an issue for reflogs. There are no conflicts to be resolved in reflogs and when migrating reflogs between backends we'd have multiple reflog entries for the same refname. So allow multiple reflog updates within a single transaction. Also the reflog creation logic isn't exposed to the end user. While this might change in the future, currently, this reduces the scope of issues to think about. This is required to add reflog migration support to `git refs migrate` which currently doesn't support it. Signed-off-by: Karthik Nayak --- refs/files-backend.c | 15 +++++++++++---- refs/reftable-backend.c | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, update->backend_data = lock; + if (update->flags & REF_LOG_ONLY) + goto out; + if (update->type & REF_ISSYMREF) { if (update->flags & REF_NO_DEREF) { /* @@ -2830,13 +2833,16 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - struct string_list_item *item = - string_list_append(&affected_refnames, update->refname); + struct string_list_item *item; if ((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF)) BUG("REF_IS_PRUNING set without REF_NO_DEREF"); + if (update->flags & REF_LOG_ONLY) + continue; + + item = string_list_append(&affected_refnames, update->refname); /* * We store a pointer to update in item->util, but at * the moment we never use the value of this field @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, /* Fail if a refname appears more than once in the transaction: */ for (i = 0; i < transaction->nr; i++) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); + if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) + string_list_append(&affected_refnames, + transaction->updates[i]->refname); string_list_sort(&affected_refnames); if (ref_update_reject_duplicates(&affected_refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -990,8 +990,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, if (ret) goto done; - string_list_append(&affected_refnames, - transaction->updates[i]->refname); + if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) + string_list_append(&affected_refnames, + transaction->updates[i]->refname); } /* @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data struct ident_split committer_ident = {0}; size_t logs_nr = 0, logs_alloc = 0, i; const char *committer_info; + struct strintmap logs_ts; int ret = 0; committer_info = git_committer_info(0); @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data QSORT(arg->updates, arg->updates_nr, transaction_update_cmp); + strintmap_init(&logs_ts, ts); + reftable_writer_set_limits(writer, ts, ts); for (i = 0; i < arg->updates_nr; i++) { @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data if (create_reflog) { struct ident_split c; + uint64_t update_index; ALLOC_GROW(logs, logs_nr + 1, logs_alloc); log = &logs[logs_nr++]; @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data } fill_reftable_log_record(log, &c); - log->update_index = ts; + + update_index = strintmap_get(&logs_ts, u->refname); + log->update_index = update_index; + strintmap_set(&logs_ts, u->refname, update_index+1); + log->refname = xstrdup(u->refname); memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); @@ -1476,6 +1485,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data done: assert(ret != REFTABLE_API_ERROR); + strintmap_clear(&logs_ts); for (i = 0; i < logs_nr; i++) reftable_log_record_release(&logs[i]); free(logs); -- 2.47.1 ``` ## Karthik Nayak, 2024-12-09 11:07 Subject: [PATCH 7/7] refs: add support for migrating reflogs Message-ID: <20241209-320-git-refs-migrate-reflogs-v1-7-d4bc37ee860f@gmail.com> URL: https://gitlist.dev/e/20241209-320-git-refs-migrate-reflogs-v1-7-d4bc37ee860f%40gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` The `git refs migrate` command was introduced in 25a0023f28 (builtin/refs: new command to migrate ref storage formats, 2024-06-06) to support migrating from one reference backend to another. One limitation of the command was that it didn't support migrating repositories which contained reflogs. A previous commit, added support for adding reflog updates in ref transactions. Using the added functionality bake in reflog support for `git refs migrate`. To ensure that the order of the reflogs is maintained during the migration, we add the index for each reflog update as we iterate over the reflogs from the old reference backend. This is to ensure that the order is maintained in the new backend. Signed-off-by: Karthik Nayak --- Documentation/git-refs.txt | 2 -- refs.c | 81 ++++++++++++++++++++++++++++++++-------------- t/t1460-refs-migrate.sh | 73 ++++++++++++++++++++++++++++------------- 3 files changed, 107 insertions(+), 49 deletions(-) diff --git a/Documentation/git-refs.txt b/Documentation/git-refs.txt index ce31f93061db5e5d16aca516dd3d15f6527db870..9829984b0a4c4f54ec7f9b6c6c7072f62b1d198d 100644 --- a/Documentation/git-refs.txt +++ b/Documentation/git-refs.txt @@ -57,8 +57,6 @@ KNOWN LIMITATIONS The ref format migration has several known limitations in its current form: -* It is not possible to migrate repositories that have reflogs. - * It is not possible to migrate repositories that have worktrees. * There is no way to block concurrent writes to the repository during an diff --git a/refs.c b/refs.c index 602a65873181a90751def525608a7fa7bea59562..4d10c7276391e8e85c66bd626bb0ecfec0941c6d 100644 --- a/refs.c +++ b/refs.c @@ -30,6 +30,7 @@ #include "date.h" #include "commit.h" #include "wildmatch.h" +#include "ident.h" /* * List of all available backends @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, } struct migration_data { + unsigned int index; struct ref_store *old_refs; struct ref_transaction *transaction; struct strbuf *errbuf; @@ -2722,6 +2724,53 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con return ret; } +struct reflog_migration_data { + unsigned int *index; + const char *refname; + struct ref_store *old_refs; + struct ref_transaction *transaction; + struct strbuf *errbuf; +}; + +static int migrate_one_reflog_entry(struct object_id *old_oid, + struct object_id *new_oid, + const char *committer, + timestamp_t timestamp, int tz, + const char *msg, void *cb_data) +{ + struct reflog_migration_data *data = cb_data; + struct strbuf sb = STRBUF_INIT; + const char *date; + int ret; + + date = show_date(timestamp, tz, DATE_MODE(NORMAL)); + /* committer contains name and email */ + strbuf_addstr(&sb, fmt_ident("", committer, WANT_BLANK_IDENT, date, 0)); + + ret = ref_transaction_update_reflog(data->transaction, data->refname, + new_oid, old_oid, sb.buf, + REF_HAVE_NEW | REF_HAVE_OLD, msg, + (*data->index)++, data->errbuf); + strbuf_release(&sb); + + return ret; +} + +static int migrate_one_reflog(const char *refname, void *cb_data) +{ + struct migration_data *migration_data = cb_data; + struct reflog_migration_data data; + + data.refname = refname; + data.old_refs = migration_data->old_refs; + data.transaction = migration_data->transaction; + data.errbuf = migration_data->errbuf; + data.index = &migration_data->index; + + return refs_for_each_reflog_ent(migration_data->old_refs, refname, + migrate_one_reflog_entry, &data); +} + static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf) { struct strbuf from_buf = STRBUF_INIT, to_buf = STRBUF_INIT; @@ -2788,13 +2837,6 @@ static int move_files(const char *from_path, const char *to_path, struct strbuf return ret; } -static int count_reflogs(const char *reflog UNUSED, void *payload) -{ - size_t *reflog_count = payload; - (*reflog_count)++; - return 0; -} - static int has_worktrees(void) { struct worktree **worktrees = get_worktrees(); @@ -2820,7 +2862,6 @@ int repo_migrate_ref_storage_format(struct repository *repo, struct ref_transaction *transaction = NULL; struct strbuf new_gitdir = STRBUF_INIT; struct migration_data data; - size_t reflog_count = 0; int did_migrate_refs = 0; int ret; @@ -2832,21 +2873,6 @@ int repo_migrate_ref_storage_format(struct repository *repo, old_refs = get_main_ref_store(repo); - /* - * We do not have any interfaces that would allow us to write many - * reflog entries. Once we have them we can remove this restriction. - */ - if (refs_for_each_reflog(old_refs, count_reflogs, &reflog_count) < 0) { - strbuf_addstr(errbuf, "cannot count reflogs"); - ret = -1; - goto done; - } - if (reflog_count) { - strbuf_addstr(errbuf, "migrating reflogs is not supported yet"); - ret = -1; - goto done; - } - /* * Worktrees complicate the migration because every worktree has a * separate ref storage. While it should be feasible to implement, this @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo, * 1. Set up a new temporary directory and initialize it with the new * format. This is where all refs will be migrated into. * - * 2. Enumerate all refs and write them into the new ref storage. - * This operation is safe as we do not yet modify the main + * 2. Enumerate all refs and reflogs and write them into the new ref + * storage. This operation is safe as we do not yet modify the main * repository. * * 3. If we're in dry-run mode then we are done and can hand over the @@ -2924,6 +2950,11 @@ int repo_migrate_ref_storage_format(struct repository *repo, if (ret < 0) goto done; + data.index = 1; + ret = refs_for_each_reflog(old_refs, migrate_one_reflog, &data); + if (ret < 0) + goto done; + ret = ref_transaction_commit(transaction, errbuf); if (ret < 0) goto done; diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh index 1bfff3a7afd5acc470424dfe7ec3e97d45f5c481..f59bc4860f19c4af82dc6f2984bdb69d61fe3ec2 100755 --- a/t/t1460-refs-migrate.sh +++ b/t/t1460-refs-migrate.sh @@ -7,23 +7,44 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +# Migrate the provided repository from one format to the other and +# verify that the references and logs are migrated over correctly. +# Usage: test_migration +# is the relative path to the repo to be migrated. +# is the ref format to be migrated to. +# (true or false) whether to skip reflog verification. test_migration () { - git -C "$1" for-each-ref --include-root-refs \ + repo=$1 && + format=$2 && + skip_reflog_verify=${3:-false} && + git -C "$repo" for-each-ref --include-root-refs \ --format='%(refname) %(objectname) %(symref)' >expect && - git -C "$1" refs migrate --ref-format="$2" && - git -C "$1" for-each-ref --include-root-refs \ + if ! $skip_reflog_verify + then + git -C "$repo" reflog --all >expect_logs && + git -C "$repo" reflog list >expect_log_list + fi && + + git -C "$repo" refs migrate --ref-format="$2" && + + git -C "$repo" for-each-ref --include-root-refs \ --format='%(refname) %(objectname) %(symref)' >actual && test_cmp expect actual && + if ! $skip_reflog_verify + then + git -C "$repo" reflog --all >actual_logs && + git -C "$repo" reflog list >actual_log_list && + test_cmp expect_logs actual_logs && + test_cmp expect_log_list actual_log_list + fi && - git -C "$1" rev-parse --show-ref-format >actual && - echo "$2" >expect && + git -C "$repo" rev-parse --show-ref-format >actual && + echo "$format" >expect && test_cmp expect actual } test_expect_success 'setup' ' - rm -rf .git && - # The migration does not yet support reflogs. - git config --global core.logAllRefUpdates false + rm -rf .git ' test_expect_success "superfluous arguments" ' @@ -78,19 +99,6 @@ do test_cmp expect err ' - test_expect_success "$from_format -> $to_format: migration with reflog fails" ' - test_when_finished "rm -rf repo" && - git init --ref-format=$from_format repo && - test_config -C repo core.logAllRefUpdates true && - test_commit -C repo logged && - test_must_fail git -C repo refs migrate \ - --ref-format=$to_format 2>err && - cat >expect <<-EOF && - error: migrating reflogs is not supported yet - EOF - test_cmp expect err - ' - test_expect_success "$from_format -> $to_format: migration with worktree fails" ' test_when_finished "rm -rf repo" && git init --ref-format=$from_format repo && @@ -141,7 +149,7 @@ do test_commit -C repo initial && test-tool -C repo ref-store main update-ref "" refs/heads/broken \ "$(test_oid 001)" "$ZERO_OID" REF_SKIP_CREATE_REFLOG,REF_SKIP_OID_VERIFICATION && - test_migration repo "$to_format" && + test_migration repo "$to_format" true && test_oid 001 >expect && git -C repo rev-parse refs/heads/broken >actual && test_cmp expect actual @@ -195,6 +203,27 @@ do git -C repo rev-parse --show-ref-format >actual && test_cmp expect actual ' + + test_expect_success "$from_format -> $to_format: reflogs of symrefs with target deleted" ' + test_when_finished "rm -rf repo" && + git init --ref-format=$from_format repo && + test_commit -C repo initial && + git -C repo branch branch-1 HEAD && + git -C repo symbolic-ref refs/heads/symref refs/heads/branch-1 && + cat >input <<-EOF && + delete refs/heads/branch-1 + EOF + git -C repo update-ref --stdin $to_format: reflogs order is retained" ' + test_when_finished "rm -rf repo" && + git init --ref-format=$from_format repo && + test_commit --date "100005000 +0700" --no-tag -C repo initial && + test_commit --date "100003000 +0700" --no-tag -C repo second && + test_migration repo "$to_format" + ' done done -- 2.47.1 ``` ## Junio C Hamano, 2024-12-10 12:13 Subject: Re: [PATCH 0/7] refs: add reflog support to `git refs migrate` Message-ID: URL: https://gitlist.dev/e/xmqq34ivlora.fsf%40gitster.g In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-0-d4bc37ee860f@gmail.com> ``` Karthik Nayak writes: > The `git refs migrate` command was introduced in > 25a0023f28 (builtin/refs: new command to migrate ref storage formats, > 2024-06-06) to support migrating from one reference backend to another. This topic pass the tests standalone for me locally, but seems to fail 1460.17 and 1460.31 when merged to 'seen'. I'll push out the integration result tonight; it would be very much appreciated if you can help find if there are semantic (or otherwise) mismerges that are causing this breakage. Thanks. ``` ## Christian Couder, 2024-12-10 16:51 Subject: Re: [PATCH 1/7] refs: include committer info in `ref_update` struct Message-ID: URL: https://gitlist.dev/e/CAP8UFD3FXtbuHH1G%3D%3DrR00_O7MBjqQiw_qzGHPVLK4P13RRtjQ%40mail.gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-1-d4bc37ee860f@gmail.com> ``` On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak wrote: > If there is no `committer_info` > provided, the reference backends default to using > `git_committer_info(0)`. The field itself cannot be set to > `git_committer_info(0)` since the values are dynamic and must be > obtained right when the reflog is being committed. > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, > struct strbuf sb = STRBUF_INIT; > int ret = 0; > > + if (!committer) > + committer = git_committer_info(0); It looks like this is where we obtain the value "right when the reflog is being committed". > + > strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); > if (msg && *msg) { > strbuf_addch(&sb, '\t'); > @@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, > } > > static int files_log_ref_write(struct files_ref_store *refs, > - const char *refname, const struct object_id *old_oid, > - const struct object_id *new_oid, const char *msg, > + const char *refname, > + const struct object_id *old_oid, > + const struct object_id *new_oid, > + const char *committer_info, const char *msg, > int flags, struct strbuf *err) > { > int logfd, result; > @@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs, > > if (logfd < 0) > return 0; > - result = log_ref_write_fd(logfd, old_oid, new_oid, > - git_committer_info(0), msg); > + result = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg); Here we just pass the committer_info to the above function. > if (result) { > struct strbuf sb = STRBUF_INIT; > int save_errno = errno; > @@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs, > files_assert_main_repository(refs, "commit_ref_update"); > > clear_loose_ref_cache(refs); > - if (files_log_ref_write(refs, lock->ref_name, > - &lock->old_oid, oid, > + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL, > logmsg, flags, err)) { Here we don't have the info so we pass NULL. > char *old_msg = strbuf_detach(err, NULL); > strbuf_addf(err, "cannot update the ref '%s': %s", > @@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs, > if (head_ref && (head_flag & REF_ISSYMREF) && > !strcmp(head_ref, lock->ref_name)) { > struct strbuf log_err = STRBUF_INIT; > - if (files_log_ref_write(refs, "HEAD", > - &lock->old_oid, oid, > + if (files_log_ref_write(refs, "HEAD", &lock->old_oid, > + oid, git_committer_info(0), Here we don't have the info either, so I think we should also pass NULL. It would then be computed "right when the reflog is being committed" in the above function. No? > logmsg, flags, &log_err)) { > error("%s", log_err.buf); > strbuf_release(&log_err); > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data It is not your fault but write_transaction_table() does the following right at the beginning of the function: committer_info = git_committer_info(0); if (split_ident_line(&committer_ident, committer_info, strlen(committer_info))) BUG("failed splitting committer info"); but then 'committer_ident' is only used in the hunk you are changing: > if (create_reflog) { > + struct ident_split c; > + > ALLOC_GROW(logs, logs_nr + 1, logs_alloc); > log = &logs[logs_nr++]; > memset(log, 0, sizeof(*log)); > > - fill_reftable_log_record(log, &committer_ident); > + if (u->committer_info) { > + if (split_ident_line(&c, u->committer_info, > + strlen(u->committer_info))) > + BUG("failed splitting committer info"); > + } else { I would think it would be more efficient to only compute 'committer_ident' here, right before we use it if needed. Or is there something I am missing? > + c = committer_ident; > + } > + > + fill_reftable_log_record(log, &c); > log->update_index = ts; > log->refname = xstrdup(u->refname); > memcpy(log->value.update.new_hash, ``` ## Christian Couder, 2024-12-10 17:22 Subject: Re: [PATCH 3/7] refs/files: add count field to ref_lock Message-ID: URL: https://gitlist.dev/e/CAP8UFD0UAWib-GceEQi%2BZzbosRcvSRbq-DuMLVVr9xuf8C03ow%40mail.gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com> ``` On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: > > When refs are updated in the files-backend, a lock is obtained for the > corresponding file path. This is the case even for reflogs, i.e. a lock > is obtained on the reference path instead of the reflog path. This > works, since generally, reflogs are updated alongside the ref. > > The upcoming patches will add support for reflog updates in ref > transaction. This means, in a particular transaction we want to have ref > updates and reflog updates. For refs, in a given transaction there can > only be one update. But, we can theoretically have multiple reflog > updates in a given transaction. Nit: Giving an example might help understand where multiple reflog updates can happen in a given transaction. Alternatively pointing to an existing doc that contains such an example or explanations might help too. > @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs, > goto out; > } > > - ret = lock_raw_ref(refs, update->refname, mustexist, > - affected_refnames, > - &lock, &referent, > - &update->type, err); > - if (ret) { > - char *reason; > + lock = strmap_get(&backend_data->ref_locks, update->refname); > + if (lock) { > + lock->count = lock->count + 1; Nit: lock->count++; > + } else { > + ret = lock_raw_ref(refs, update->refname, mustexist, > + affected_refnames, > + &lock, &referent, > + &update->type, err); > + if (ret) { > + char *reason; > + > + reason = strbuf_detach(err, NULL); > + strbuf_addf(err, "cannot lock ref '%s': %s", > + ref_update_original_update_refname(update), reason); > + free(reason); > + goto out; > + } > > - reason = strbuf_detach(err, NULL); > - strbuf_addf(err, "cannot lock ref '%s': %s", > - ref_update_original_update_refname(update), reason); > - free(reason); > - goto out; > + strmap_put(&backend_data->ref_locks, update->refname, lock); > } > > update->backend_data = lock; ``` ## karthik nayak, 2024-12-10 17:42 Subject: Re: [PATCH 0/7] refs: add reflog support to `git refs migrate` Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRsAwM8oUnyYCaRKkkudN%2BQW%2BhRSKZEvPJLThSr14M7rA%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> The `git refs migrate` command was introduced in >> 25a0023f28 (builtin/refs: new command to migrate ref storage formats, >> 2024-06-06) to support migrating from one reference backend to another. > > This topic pass the tests standalone for me locally, but seems to > fail 1460.17 and 1460.31 when merged to 'seen'. I'll push out the > integration result tonight; it would be very much appreciated if you > can help find if there are semantic (or otherwise) mismerges that > are causing this breakage. > I see. I can reproduce it on 'seen' as you mentioned. Will debug and get back to you on this. Thanks for letting me know. ``` ## karthik nayak, 2024-12-10 18:03 Subject: Re: [PATCH 0/7] refs: add reflog support to `git refs migrate` Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRwH8mGDF%3DafGnci6BXD8r%2BtA5gKgrQRb0BZFBWYDG7qA%40mail.gmail.com In-Reply-To: ``` karthik nayak writes: > Junio C Hamano writes: > >> Karthik Nayak writes: >> >>> The `git refs migrate` command was introduced in >>> 25a0023f28 (builtin/refs: new command to migrate ref storage formats, >>> 2024-06-06) to support migrating from one reference backend to another. >> >> This topic pass the tests standalone for me locally, but seems to >> fail 1460.17 and 1460.31 when merged to 'seen'. I'll push out the >> integration result tonight; it would be very much appreciated if you >> can help find if there are semantic (or otherwise) mismerges that >> are causing this breakage. >> > > I see. I can reproduce it on 'seen' as you mentioned. Will debug and get > back to you on this. Thanks for letting me know. Seems like this is due to 'kn/reftable-writer-log-write-verify', which I should have totally seen coming. A quick fix like the one below fixes the issue. I'll merge in 'kn/reftable-writer-log-write-verify' when I re-roll. diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 1badf88df0..5c51a6a226 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1428,6 +1428,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data struct reftable_log_record *logs = NULL; struct ident_split committer_ident = {0}; size_t logs_nr = 0, logs_alloc = 0, i; + uint64_t max_update_index = ts; const char *committer_info; struct strintmap logs_ts; int ret = 0; @@ -1541,6 +1542,13 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data log->update_index = update_index; strintmap_set(&logs_ts, u->refname, update_index+1); + /* + * Note the max_update_index, so we can reset the limit + * before actually writing the logs. + */ + if (update_index > max_update_index) + max_update_index = update_index; + log->refname = xstrdup(u->refname); memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); @@ -1604,6 +1612,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data * and log blocks. */ if (logs) { + reftable_writer_set_limits(writer, ts, max_update_index); + ret = reftable_writer_add_logs(writer, logs, logs_nr); if (ret < 0) goto done; ``` ## Christian Couder, 2024-12-11 09:05 Subject: Re: [PATCH 3/7] refs/files: add count field to ref_lock Message-ID: URL: https://gitlist.dev/e/CAP8UFD3StHePPGe%2B44c83qBYoZLZme%2BGw-cP7Weg-neyJg8DOw%40mail.gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com> ``` On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: > > When refs are updated in the files-backend, a lock is obtained for the > corresponding file path. This is the case even for reflogs, i.e. a lock > is obtained on the reference path instead of the reflog path. This > works, since generally, reflogs are updated alongside the ref. > > The upcoming patches will add support for reflog updates in ref > transaction. This means, in a particular transaction we want to have ref > updates and reflog updates. For refs, in a given transaction there can > only be one update. Maybe something like: "For a given ref in a given transaction there can be at most one update." > But, we can theoretically have multiple reflog > updates in a given transaction. And: "But we can theoretically have multiple reflog updates for a given ref in a given transaction." > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -71,6 +71,8 @@ struct ref_lock { > char *ref_name; > struct lock_file lk; > struct object_id old_oid; > + /* count keeps track of users of the lock */ > + unsigned int count; Nit: maybe the following is a bit better: unsigned int count; /* track users of the lock (ref update + reflog updates) */ > }; ``` ## Christian Couder, 2024-12-11 09:26 Subject: Re: [PATCH 4/7] refs: extract out refname verification in transactions Message-ID: URL: https://gitlist.dev/e/CAP8UFD3fZ21TXgtMcppXMHf35qgA-UH%3D0X9z-xJ456qXyV5%3DdA%40mail.gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f@gmail.com> ``` On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: > > Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update, > the refname of the update is verified for: > > - Ensuring it is not a pseudoref. > - Checking the refname format. > > These checks are also be needed in a following commit where the function s/are also be needed/will also be needed/ > to add reflog updates to the transaction is introduced. Extract the code > out into a new static function. > > Signed-off-by: Karthik Nayak > --- > refs.c | 43 ++++++++++++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/refs.c b/refs.c > index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 > --- a/refs.c > +++ b/refs.c > @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( > return update; > } > > +static int transaction_refname_verification(const char *refname, > + const struct object_id *new_oid, > + unsigned int flags, > + struct strbuf *err) We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while I couldn't find any 'yyy_verification()' function, so it might be better to name it 'transaction_refname_valid()' or maybe 'transaction_refname_ok()'. Also I think it should probably return a bool so 1 if the refname is valid and 0 otherwise, unless we have plans in the future to follow different code paths depending on the different ways it is not valid. > + ret = transaction_refname_verification(refname, new_oid, flags, err); > + if (ret) > + return ret; Then the above could be just: if (!transaction_refname_valid(refname, new_oid, flags, err)) return -1; ``` ## Christian Couder, 2024-12-11 10:10 Subject: Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function Message-ID: URL: https://gitlist.dev/e/CAP8UFD1vTEH%3D7HBX1sudwkv7now9VUkFR3Y347EVNGfToZt8fA%40mail.gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com> ``` On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: > > Introduce a new function `ref_transaction_update_reflog`, for clients to > add a reflog update to a transaction. While the existing function > `ref_transaction_update` also allows clients to add a reflog entry, this > function does a few things more, It: > - Enforces that only a reflog entry is added and does not update the > ref itself. > - Allows the users to also provide the committer information. This > means clients can add reflog entries with custom committer > information. > A follow up commit will utilize this function to add reflog support to > `git refs migrate`. > > Signed-off-by: Karthik Nayak > --- > refs.c | 89 +++++++++++++++++++++++++++++++++++++------------ > refs.h | 12 +++++++ > refs/files-backend.c | 48 +++++++++++++++----------- > refs/refs-internal.h | 16 +++++---- > refs/reftable-backend.c | 6 ++-- > 5 files changed, 122 insertions(+), 49 deletions(-) > > diff --git a/refs.c b/refs.c > index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644 > --- a/refs.c > +++ b/refs.c > @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction) > free(transaction); > } > > -struct ref_update *ref_transaction_add_update( > - struct ref_transaction *transaction, > - const char *refname, unsigned int flags, > - const struct object_id *new_oid, > - const struct object_id *old_oid, > - const char *new_target, const char *old_target, > - const char *msg) > +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, > + const char *refname, > + unsigned int flags, > + const struct object_id *new_oid, > + const struct object_id *old_oid, > + const char *new_target, > + const char *old_target, > + const char *committer_info, This change (adding a 'const char *committer_info' argument to ref_transaction_add_update()) is not described in the commit message and it requires a number of changes to the callers of this function, so I think it might want to be in its own preparatory commit before this one. > + const char *msg) > { > struct ref_update *update; > > @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update( > oidcpy(&update->new_oid, new_oid); > if ((flags & REF_HAVE_OLD) && old_oid) > oidcpy(&update->old_oid, old_oid); > - if (!(flags & REF_SKIP_CREATE_REFLOG)) > + if (!(flags & REF_SKIP_CREATE_REFLOG)) { > + if (committer_info) { > + struct strbuf sb = STRBUF_INIT; > + strbuf_addstr(&sb, committer_info); > + update->committer_info = strbuf_detach(&sb, NULL); Maybe: update->committer_info = xstrdup(committer_info); > + } > + > update->msg = normalize_reflog_message(msg); > + } > > return update; > } > @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update( > static int transaction_refname_verification(const char *refname, > const struct object_id *new_oid, > unsigned int flags, > + unsigned int reflog, > struct strbuf *err) > { > if (flags & REF_SKIP_REFNAME_VERIFICATION) > return 0; > > if (is_pseudo_ref(refname)) { > - strbuf_addf(err, _("refusing to update pseudoref '%s'"), > - refname); > + if (reflog) > + strbuf_addf(err, _("refusing to update reflog for pseudoref '%s'"), > + refname); > + else > + strbuf_addf(err, _("refusing to update pseudoref '%s'"), > + refname); Maybe: const char *what = reflog ? "reflog for pseudoref" : "pseudoref"; strbuf_addf(err, _("refusing to update %s '%s'"), what, refname); > return -1; > } else if ((new_oid && !is_null_oid(new_oid)) ? > check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : > !refname_is_safe(refname)) { > - strbuf_addf(err, _("refusing to update ref with bad name '%s'"), > - refname); > + if (reflog) > + strbuf_addf(err, _("refusing to update reflog with bad name '%s'"), > + refname); > + else > + strbuf_addf(err, _("refusing to update ref with bad name '%s'"), > + refname); Maybe: const char *what = reflog ? "reflog with bad name" : "ref with bad name"; strbuf_addf(err, _("refusing to update %s '%s'"), what, refname); > return -1; > } [...] > int ref_transaction_create(struct ref_transaction *transaction, > - const char *refname, > - const struct object_id *new_oid, > - const char *new_target, > - unsigned int flags, const char *msg, > - struct strbuf *err) > + const char *refname, const struct object_id *new_oid, > + const char *new_target, unsigned int flags, > + const char *msg, struct strbuf *err) This looks like a wrapping or indenting only change. If you really want to do it, it should probably be in its own preparatory commit. > index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644 > --- a/refs.h > +++ b/refs.h > @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction, > unsigned int flags, const char *msg, > struct strbuf *err); > > +/* > + * Similar to `ref_transaction_update`, but this function is only for adding > + * a reflog updates. "a reflog update" or "reflog updates". ``` ## karthik nayak, 2024-12-11 10:13 Subject: Re: [PATCH 1/7] refs: include committer info in `ref_update` struct Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRL0P4V11Eq3RYN2gPurCwgDUez4muH3%2B2yZoBvNNm0Xg%40mail.gmail.com In-Reply-To: ``` Christian Couder writes: > On Mon, Dec 9, 2024 at 12:10 PM Karthik Nayak wrote: > > >> If there is no `committer_info` >> provided, the reference backends default to using >> `git_committer_info(0)`. The field itself cannot be set to >> `git_committer_info(0)` since the values are dynamic and must be >> obtained right when the reflog is being committed. > > >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 64f51f0da905a9a8a1ac4109c6b0a9a85a355db7..13f8539e6caa923cd4834775fcb0cd7f90d82014 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -1858,6 +1858,9 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, >> struct strbuf sb = STRBUF_INIT; >> int ret = 0; >> >> + if (!committer) >> + committer = git_committer_info(0); > > It looks like this is where we obtain the value "right when the reflog > is being committed". > >> + >> strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer); >> if (msg && *msg) { >> strbuf_addch(&sb, '\t'); >> @@ -1871,8 +1874,10 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, >> } >> >> static int files_log_ref_write(struct files_ref_store *refs, >> - const char *refname, const struct object_id *old_oid, >> - const struct object_id *new_oid, const char *msg, >> + const char *refname, >> + const struct object_id *old_oid, >> + const struct object_id *new_oid, >> + const char *committer_info, const char *msg, >> int flags, struct strbuf *err) >> { >> int logfd, result; >> @@ -1889,8 +1894,7 @@ static int files_log_ref_write(struct files_ref_store *refs, >> >> if (logfd < 0) >> return 0; >> - result = log_ref_write_fd(logfd, old_oid, new_oid, >> - git_committer_info(0), msg); >> + result = log_ref_write_fd(logfd, old_oid, new_oid, committer_info, msg); > > Here we just pass the committer_info to the above function. > >> if (result) { >> struct strbuf sb = STRBUF_INIT; >> int save_errno = errno; >> @@ -1974,8 +1978,7 @@ static int commit_ref_update(struct files_ref_store *refs, >> files_assert_main_repository(refs, "commit_ref_update"); >> >> clear_loose_ref_cache(refs); >> - if (files_log_ref_write(refs, lock->ref_name, >> - &lock->old_oid, oid, >> + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, oid, NULL, >> logmsg, flags, err)) { > > Here we don't have the info so we pass NULL. > >> char *old_msg = strbuf_detach(err, NULL); >> strbuf_addf(err, "cannot update the ref '%s': %s", >> @@ -2007,8 +2010,8 @@ static int commit_ref_update(struct files_ref_store *refs, >> if (head_ref && (head_flag & REF_ISSYMREF) && >> !strcmp(head_ref, lock->ref_name)) { >> struct strbuf log_err = STRBUF_INIT; >> - if (files_log_ref_write(refs, "HEAD", >> - &lock->old_oid, oid, >> + if (files_log_ref_write(refs, "HEAD", &lock->old_oid, >> + oid, git_committer_info(0), > > Here we don't have the info either, so I think we should also pass > NULL. It would then be computed "right when the reflog is being > committed" in the above function. No? > Indeed, passing NULL should be sufficient here, good catch. >> logmsg, flags, &log_err)) { >> error("%s", log_err.buf); >> strbuf_release(&log_err); > > >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index 647ef9b05b1dc9a376ed054330b487f7595c5caa..e882602487c66261d586a94101bb1b4e9a2ed60e 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -1379,11 +1379,21 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > > It is not your fault but write_transaction_table() does the following > right at the beginning of the function: > > committer_info = git_committer_info(0); > if (split_ident_line(&committer_ident, committer_info, > strlen(committer_info))) > BUG("failed splitting committer info"); > > but then 'committer_ident' is only used in the hunk you are changing: > >> if (create_reflog) { >> + struct ident_split c; >> + >> ALLOC_GROW(logs, logs_nr + 1, logs_alloc); >> log = &logs[logs_nr++]; >> memset(log, 0, sizeof(*log)); >> >> - fill_reftable_log_record(log, &committer_ident); >> + if (u->committer_info) { >> + if (split_ident_line(&c, u->committer_info, >> + strlen(u->committer_info))) >> + BUG("failed splitting committer info"); >> + } else { > > I would think it would be more efficient to only compute > 'committer_ident' here, right before we use it if needed. Or is there > something I am missing? > It would if there wasn't a loop. Since we loop over multiple updates, computing committer_ident for each would end up being expensive. So it is done before the loop starts. >> + c = committer_ident; >> + } >> + >> + fill_reftable_log_record(log, &c); >> log->update_index = ts; >> log->refname = xstrdup(u->refname); >> memcpy(log->value.update.new_hash, ``` ## karthik nayak, 2024-12-11 10:18 Subject: Re: [PATCH 3/7] refs/files: add count field to ref_lock Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRuuS_TW%3DBPeC36eWfgcYegT5hVKd%2BpVLG6TDMMjJ7tsg%40mail.gmail.com In-Reply-To: ``` Christian Couder writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: >> >> When refs are updated in the files-backend, a lock is obtained for the >> corresponding file path. This is the case even for reflogs, i.e. a lock >> is obtained on the reference path instead of the reflog path. This >> works, since generally, reflogs are updated alongside the ref. >> >> The upcoming patches will add support for reflog updates in ref >> transaction. This means, in a particular transaction we want to have ref >> updates and reflog updates. For refs, in a given transaction there can >> only be one update. But, we can theoretically have multiple reflog >> updates in a given transaction. > > Nit: Giving an example might help understand where multiple reflog > updates can happen in a given transaction. Alternatively pointing to > an existing doc that contains such an example or explanations might > help too. > The use-case is added in the series. I've added a note about how this is needed in reflog migration. >> @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs, >> goto out; >> } >> >> - ret = lock_raw_ref(refs, update->refname, mustexist, >> - affected_refnames, >> - &lock, &referent, >> - &update->type, err); >> - if (ret) { >> - char *reason; >> + lock = strmap_get(&backend_data->ref_locks, update->refname); >> + if (lock) { >> + lock->count = lock->count + 1; > > Nit: > lock->count++; > Will fix, thanks. [snip] ``` ## karthik nayak, 2024-12-11 10:26 Subject: Re: [PATCH 3/7] refs/files: add count field to ref_lock Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRmGtxRrWnYPW6i8VX1MjQxoY8srKESgyaoutQxR-aZ3A%40mail.gmail.com In-Reply-To: ``` Christian Couder writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: >> >> When refs are updated in the files-backend, a lock is obtained for the >> corresponding file path. This is the case even for reflogs, i.e. a lock >> is obtained on the reference path instead of the reflog path. This >> works, since generally, reflogs are updated alongside the ref. >> >> The upcoming patches will add support for reflog updates in ref >> transaction. This means, in a particular transaction we want to have ref >> updates and reflog updates. For refs, in a given transaction there can >> only be one update. > > Maybe something like: "For a given ref in a given transaction there > can be at most one update." > Sure. >> But, we can theoretically have multiple reflog >> updates in a given transaction. > > And: "But we can theoretically have multiple reflog updates for a > given ref in a given transaction." > Will add. >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 13f8539e6caa923cd4834775fcb0cd7f90d82014..9c929c1ac33bc62a75620e684a809d46b574f1c6 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -71,6 +71,8 @@ struct ref_lock { >> char *ref_name; >> struct lock_file lk; >> struct object_id old_oid; >> + /* count keeps track of users of the lock */ >> + unsigned int count; > > Nit: maybe the following is a bit better: > > unsigned int count; /* track users of the lock (ref update + > reflog updates) */ This is better, will amend this in too! ``` ## karthik nayak, 2024-12-11 10:31 Subject: Re: [PATCH 4/7] refs: extract out refname verification in transactions Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQtT9KXXro5kCbJUN7fFAAHirZXo06nStqn_u6urr149Q%40mail.gmail.com In-Reply-To: ``` Christian Couder writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: >> >> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update, >> the refname of the update is verified for: >> >> - Ensuring it is not a pseudoref. >> - Checking the refname format. >> >> These checks are also be needed in a following commit where the function > > s/are also be needed/will also be needed/ > Will amend. >> to add reflog updates to the transaction is introduced. Extract the code >> out into a new static function. >> >> Signed-off-by: Karthik Nayak >> --- >> refs.c | 43 ++++++++++++++++++++++++++++--------------- >> 1 file changed, 28 insertions(+), 15 deletions(-) >> >> diff --git a/refs.c b/refs.c >> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( >> return update; >> } >> >> +static int transaction_refname_verification(const char *refname, >> + const struct object_id *new_oid, >> + unsigned int flags, >> + struct strbuf *err) > > We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while > I couldn't find any 'yyy_verification()' function, so it might be > better to name it 'transaction_refname_valid()' or maybe > 'transaction_refname_ok()'. > I think you're right, it helps to be consistent here. Will change to `transaction_refname_valid()`. > Also I think it should probably return a bool so 1 if the refname is > valid and 0 otherwise, unless we have plans in the future to follow > different code paths depending on the different ways it is not valid. > That is a good idea. >> + ret = transaction_refname_verification(refname, new_oid, flags, err); >> + if (ret) >> + return ret; > > Then the above could be just: > > if (!transaction_refname_valid(refname, new_oid, flags, err)) > return -1; Yup, also will remove the need for the `ret` variable. ``` ## Christian Couder, 2024-12-11 10:44 Subject: Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname Message-ID: URL: https://gitlist.dev/e/CAP8UFD2zogHD_g5FOVNFvBswPJF749wd4f2s9Pzgef-pv%3DU0Gg%40mail.gmail.com In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com> ``` On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: > > The reference transaction only allows a update for a given reference to s/a update/an update/ or: s/a update/a single update/ > avoid conflicts. This, however, isn't an issue for reflogs. There are no > conflicts to be resolved in reflogs and when migrating reflogs between > backends we'd have multiple reflog entries for the same refname. > @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > struct ident_split committer_ident = {0}; > size_t logs_nr = 0, logs_alloc = 0, i; > const char *committer_info; > + struct strintmap logs_ts; Here a comment might help explain what logs_ts is used for. > int ret = 0; > > committer_info = git_committer_info(0); > @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > > QSORT(arg->updates, arg->updates_nr, transaction_update_cmp); > > + strintmap_init(&logs_ts, ts); I am not sure I understand what logs_ts is used for and why its default value is set to ts. Also ts is an uint64_t while the second argument to strintmap_init() is an int. I wonder if it could be an issue especially on 32 bits platforms. > reftable_writer_set_limits(writer, ts, ts); > > for (i = 0; i < arg->updates_nr; i++) { > @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > > if (create_reflog) { > struct ident_split c; > + uint64_t update_index; > > ALLOC_GROW(logs, logs_nr + 1, logs_alloc); > log = &logs[logs_nr++]; > @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > } > > fill_reftable_log_record(log, &c); > - log->update_index = ts; > + > + update_index = strintmap_get(&logs_ts, u->refname); > + log->update_index = update_index; > + strintmap_set(&logs_ts, u->refname, update_index+1); s/update_index+1/update_index + 1/ Also is the 'update_index' var really needed or could we just do: log->update_index = strintmap_get(&logs_ts, u->refname); strintmap_set(&logs_ts, u->refname, log->update_index + 1); ? > log->refname = xstrdup(u->refname); > memcpy(log->value.update.new_hash, > u->new_oid.hash, GIT_MAX_RAWSZ); ``` ## Patrick Steinhardt, 2024-12-11 14:26 Subject: Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function Message-ID: URL: https://gitlist.dev/e/Z1mhCWXowokIaFR5%40pks.im In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com> ``` On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote: > diff --git a/refs.c b/refs.c > index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644 > --- a/refs.c > +++ b/refs.c > @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction) > free(transaction); > } > > -struct ref_update *ref_transaction_add_update( > - struct ref_transaction *transaction, > - const char *refname, unsigned int flags, > - const struct object_id *new_oid, > - const struct object_id *old_oid, > - const char *new_target, const char *old_target, > - const char *msg) > +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, > + const char *refname, > + unsigned int flags, > + const struct object_id *new_oid, > + const struct object_id *old_oid, > + const char *new_target, > + const char *old_target, > + const char *committer_info, > + const char *msg) > { > struct ref_update *update; > I'd personally avoid reindenting this block. It's somewhat-common practice to not align all arguments with the opening brace when the line would become too long. The reindents also distract a bit from the actual changes done in other places further down. > @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update( > oidcpy(&update->new_oid, new_oid); > if ((flags & REF_HAVE_OLD) && old_oid) > oidcpy(&update->old_oid, old_oid); > - if (!(flags & REF_SKIP_CREATE_REFLOG)) > + if (!(flags & REF_SKIP_CREATE_REFLOG)) { > + if (committer_info) { > + struct strbuf sb = STRBUF_INIT; > + strbuf_addstr(&sb, committer_info); > + update->committer_info = strbuf_detach(&sb, NULL); Can't we simplify this via `xstrdup()`? > @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, > } > > /* > - * packed-refs don't support symbolic refs and root refs, so we > - * have to queue these references via the loose transaction. > + * packed-refs don't support symbolic refs, root refs and reflogs, > + * so we have to queue these references via the loose transaction. > */ > - if (update->new_target || is_root_ref(update->refname)) { > + if (update->new_target || > + is_root_ref(update->refname) || > + (update->flags & REF_LOG_ONLY)) { > if (!loose_transaction) { > loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); > if (!loose_transaction) { Makes sense. While we already had REF_LOG_ONLY beforehand, it was only used in very specific cases and thus the support implemented by the backends is lacking. And given that the packed-ref backend does not support reflogs we have to queue these up via the loose backend. Patrick ``` ## Patrick Steinhardt, 2024-12-11 14:26 Subject: Re: [PATCH 4/7] refs: extract out refname verification in transactions Message-ID: URL: https://gitlist.dev/e/Z1mhD2M4KgzA17k0%40pks.im In-Reply-To: ``` On Wed, Dec 11, 2024 at 10:26:31AM +0100, Christian Couder wrote: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: > > diff --git a/refs.c b/refs.c > > index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( > > return update; > > } > > > > +static int transaction_refname_verification(const char *refname, > > + const struct object_id *new_oid, > > + unsigned int flags, > > + struct strbuf *err) > > We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while > I couldn't find any 'yyy_verification()' function, so it might be > better to name it 'transaction_refname_valid()' or maybe > 'transaction_refname_ok()'. > > Also I think it should probably return a bool so 1 if the refname is > valid and 0 otherwise, unless we have plans in the future to follow > different code paths depending on the different ways it is not valid. > > > + ret = transaction_refname_verification(refname, new_oid, flags, err); > > + if (ret) > > + return ret; > > Then the above could be just: > > if (!transaction_refname_valid(refname, new_oid, flags, err)) > return -1; The only question is whether we want to discern between an invalid refname and an error. But reading through the code it doesn't seem like we want to do that as we always return either `-1` on invalid names or `0` otherwise. So agreed, this is a good suggestion. Patrick ``` ## Patrick Steinhardt, 2024-12-11 14:26 Subject: Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname Message-ID: URL: https://gitlist.dev/e/Z1mhEsu8DBsCsYuA%40pks.im In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com> ``` On Mon, Dec 09, 2024 at 12:07:20PM +0100, Karthik Nayak wrote: > The reference transaction only allows a update for a given reference to > avoid conflicts. This, however, isn't an issue for reflogs. There are no > conflicts to be resolved in reflogs and when migrating reflogs between > backends we'd have multiple reflog entries for the same refname. > > So allow multiple reflog updates within a single transaction. Also the > reflog creation logic isn't exposed to the end user. While this might > change in the future, currently, this reduces the scope of issues to > think about. > > This is required to add reflog migration support to `git refs migrate` > which currently doesn't support it. Nit: the second half of this sentence starting with "which currently..." feels rather pointless, as it's implicit in the first half. I'd just drop it. > Signed-off-by: Karthik Nayak > --- > refs/files-backend.c | 15 +++++++++++---- > refs/reftable-backend.c | 16 +++++++++++++--- > 2 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, > > update->backend_data = lock; > > + if (update->flags & REF_LOG_ONLY) > + goto out; > + > if (update->type & REF_ISSYMREF) { > if (update->flags & REF_NO_DEREF) { > /* Hm. Does this mean that we don't lock at all for REF_LOG_ONLY updates? Reflogs themselves have no lockfile, so isn't it mandatory that we lock the corresponding ref like we used to do? Otherwise I cannot see how we avoid two concurrent writers to the same reflog. > @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, > > /* Fail if a refname appears more than once in the transaction: */ > for (i = 0; i < transaction->nr; i++) > - string_list_append(&affected_refnames, > - transaction->updates[i]->refname); > + if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) > + string_list_append(&affected_refnames, > + transaction->updates[i]->refname); > string_list_sort(&affected_refnames); > if (ref_update_reject_duplicates(&affected_refnames, err)) { > ret = TRANSACTION_GENERIC_ERROR; This on the other hand is sensible -- having multiple REF_LOG_ONLY transactions queued is fine. > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > } > > fill_reftable_log_record(log, &c); > - log->update_index = ts; > + > + update_index = strintmap_get(&logs_ts, u->refname); > + log->update_index = update_index; > + strintmap_set(&logs_ts, u->refname, update_index+1); So we're now tracking update indices via another map in order to ensure that the update index will be increased if we have multiple reflog entries for the same refname. Can we avoid that overhead by instead just having a global update index counter that increases for every single reflog entry, regardless of whether we have multiple ones queued up for the same reference? I guess the result would be kind of weird as a single transaction with multiple ref updates would now always contain N different update indices. Maybe there's an alternative that allows us to reduce the cost, like only doing this for REF_LOG_ONLY updates? I'm mostly being careful because this here is the hot loop of writing refs, so I don't want to regress performance. Patrick ``` ## Patrick Steinhardt, 2024-12-11 14:26 Subject: Re: [PATCH 7/7] refs: add support for migrating reflogs Message-ID: URL: https://gitlist.dev/e/Z1mhFqmznVxObmAn%40pks.im In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-7-d4bc37ee860f@gmail.com> ``` On Mon, Dec 09, 2024 at 12:07:21PM +0100, Karthik Nayak wrote: > @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, > } > > struct migration_data { > + unsigned int index; > struct ref_store *old_refs; > struct ref_transaction *transaction; > struct strbuf *errbuf; Calling this `reflog_index` might be a bit easier for context. > @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo, > * 1. Set up a new temporary directory and initialize it with the new > * format. This is where all refs will be migrated into. > * > - * 2. Enumerate all refs and write them into the new ref storage. > - * This operation is safe as we do not yet modify the main > + * 2. Enumerate all refs and reflogs and write them into the new ref > + * storage. This operation is safe as we do not yet modify the main I'd rather move this into a third step, as it is separate from the ref enumeration. Patrick ``` ## karthik nayak, 2024-12-11 18:06 Subject: Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQFAMdW78QbPYSoCtqetwT1S6z8dmLS_XG6Ex0nUsp9Cg%40mail.gmail.com In-Reply-To: ``` Christian Couder writes: [snip] >> diff --git a/refs.c b/refs.c >> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction) >> free(transaction); >> } >> >> -struct ref_update *ref_transaction_add_update( >> - struct ref_transaction *transaction, >> - const char *refname, unsigned int flags, >> - const struct object_id *new_oid, >> - const struct object_id *old_oid, >> - const char *new_target, const char *old_target, >> - const char *msg) >> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, >> + const char *refname, >> + unsigned int flags, >> + const struct object_id *new_oid, >> + const struct object_id *old_oid, >> + const char *new_target, >> + const char *old_target, >> + const char *committer_info, > > This change (adding a 'const char *committer_info' argument to > ref_transaction_add_update()) is not described in the commit message > and it requires a number of changes to the callers of this function, > so I think it might want to be in its own preparatory commit before > this one. > I think this is a great suggestion, it would reduce the congnitive load of the commit and make it easier to review. Will do. >> + const char *msg) >> { >> struct ref_update *update; >> >> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update( >> oidcpy(&update->new_oid, new_oid); >> if ((flags & REF_HAVE_OLD) && old_oid) >> oidcpy(&update->old_oid, old_oid); >> - if (!(flags & REF_SKIP_CREATE_REFLOG)) >> + if (!(flags & REF_SKIP_CREATE_REFLOG)) { >> + if (committer_info) { >> + struct strbuf sb = STRBUF_INIT; >> + strbuf_addstr(&sb, committer_info); >> + update->committer_info = strbuf_detach(&sb, NULL); > > Maybe: > update->committer_info = xstrdup(committer_info); > Indeed, I thought there was a better way. This is what I needed to have done. >> + } >> + >> update->msg = normalize_reflog_message(msg); >> + } >> >> return update; >> } >> @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update( >> static int transaction_refname_verification(const char *refname, >> const struct object_id *new_oid, >> unsigned int flags, >> + unsigned int reflog, >> struct strbuf *err) >> { >> if (flags & REF_SKIP_REFNAME_VERIFICATION) >> return 0; >> >> if (is_pseudo_ref(refname)) { >> - strbuf_addf(err, _("refusing to update pseudoref '%s'"), >> - refname); >> + if (reflog) >> + strbuf_addf(err, _("refusing to update reflog for pseudoref '%s'"), >> + refname); >> + else >> + strbuf_addf(err, _("refusing to update pseudoref '%s'"), >> + refname); > > Maybe: > > const char *what = reflog ? "reflog for pseudoref" : "pseudoref"; > strbuf_addf(err, _("refusing to update %s '%s'"), what, refname); > Much nicer, will add. >> return -1; >> } else if ((new_oid && !is_null_oid(new_oid)) ? >> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : >> !refname_is_safe(refname)) { >> - strbuf_addf(err, _("refusing to update ref with bad name '%s'"), >> - refname); >> + if (reflog) >> + strbuf_addf(err, _("refusing to update reflog with bad name '%s'"), >> + refname); >> + else >> + strbuf_addf(err, _("refusing to update ref with bad name '%s'"), >> + refname); > > Maybe: > > const char *what = reflog ? "reflog with bad name" : > "ref with bad name"; > strbuf_addf(err, _("refusing to update %s '%s'"), what, refname); > Similar, will do. >> return -1; >> } > > [...] > >> int ref_transaction_create(struct ref_transaction *transaction, >> - const char *refname, >> - const struct object_id *new_oid, >> - const char *new_target, >> - unsigned int flags, const char *msg, >> - struct strbuf *err) >> + const char *refname, const struct object_id *new_oid, >> + const char *new_target, unsigned int flags, >> + const char *msg, struct strbuf *err) > > This looks like a wrapping or indenting only change. If you really > want to do it, it should probably be in its own preparatory commit. > I think it was the auto linter, will remove. >> index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction, >> unsigned int flags, const char *msg, >> struct strbuf *err); >> >> +/* >> + * Similar to `ref_transaction_update`, but this function is only for adding >> + * a reflog updates. > > "a reflog update" or "reflog updates". Makes sense. Thanks. ``` ## karthik nayak, 2024-12-11 18:09 Subject: Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZTLK%3DPL7YmkxzH5cZiHA21r1pM1qqbvu%3DfXi_rpRmHCYQ%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote: >> diff --git a/refs.c b/refs.c >> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction) >> free(transaction); >> } >> >> -struct ref_update *ref_transaction_add_update( >> - struct ref_transaction *transaction, >> - const char *refname, unsigned int flags, >> - const struct object_id *new_oid, >> - const struct object_id *old_oid, >> - const char *new_target, const char *old_target, >> - const char *msg) >> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, >> + const char *refname, >> + unsigned int flags, >> + const struct object_id *new_oid, >> + const struct object_id *old_oid, >> + const char *new_target, >> + const char *old_target, >> + const char *committer_info, >> + const char *msg) >> { >> struct ref_update *update; >> > > I'd personally avoid reindenting this block. It's somewhat-common > practice to not align all arguments with the opening brace when the line > would become too long. The reindents also distract a bit from the actual > changes done in other places further down. > Makes sense, I'll undo that. >> @@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update( >> oidcpy(&update->new_oid, new_oid); >> if ((flags & REF_HAVE_OLD) && old_oid) >> oidcpy(&update->old_oid, old_oid); >> - if (!(flags & REF_SKIP_CREATE_REFLOG)) >> + if (!(flags & REF_SKIP_CREATE_REFLOG)) { >> + if (committer_info) { >> + struct strbuf sb = STRBUF_INIT; >> + strbuf_addstr(&sb, committer_info); >> + update->committer_info = strbuf_detach(&sb, NULL); > > Can't we simplify this via `xstrdup()`? > Yup, Christian suggested the same too, will fix it up. >> @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, >> } >> >> /* >> - * packed-refs don't support symbolic refs and root refs, so we >> - * have to queue these references via the loose transaction. >> + * packed-refs don't support symbolic refs, root refs and reflogs, >> + * so we have to queue these references via the loose transaction. >> */ >> - if (update->new_target || is_root_ref(update->refname)) { >> + if (update->new_target || >> + is_root_ref(update->refname) || >> + (update->flags & REF_LOG_ONLY)) { >> if (!loose_transaction) { >> loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); >> if (!loose_transaction) { > > Makes sense. While we already had REF_LOG_ONLY beforehand, it was only > used in very specific cases and thus the support implemented by the > backends is lacking. And given that the packed-ref backend does not > support reflogs we have to queue these up via the loose backend. > > Patrick ``` ## karthik nayak, 2024-12-12 14:04 Subject: Re: [PATCH 7/7] refs: add support for migrating reflogs Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZROACAy2_jDSUxZ9rAZ0o7eKugtuqZLh_aWkHH6NRjdcQ%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Mon, Dec 09, 2024 at 12:07:21PM +0100, Karthik Nayak wrote: >> @@ -2687,6 +2688,7 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, >> } >> >> struct migration_data { >> + unsigned int index; >> struct ref_store *old_refs; >> struct ref_transaction *transaction; >> struct strbuf *errbuf; > > Calling this `reflog_index` might be a bit easier for context. > Makes sense, will amend. >> @@ -2868,8 +2894,8 @@ int repo_migrate_ref_storage_format(struct repository *repo, >> * 1. Set up a new temporary directory and initialize it with the new >> * format. This is where all refs will be migrated into. >> * >> - * 2. Enumerate all refs and write them into the new ref storage. >> - * This operation is safe as we do not yet modify the main >> + * 2. Enumerate all refs and reflogs and write them into the new ref >> + * storage. This operation is safe as we do not yet modify the main > > I'd rather move this into a third step, as it is separate from the ref > enumeration. > > Patrick > I added it together since it is a single transaction, but I'm okay with making this change, will amend in. Thanks ``` ## karthik nayak, 2024-12-12 14:47 Subject: Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQqQTEquJ0e5rG168-CVADR8K-uYma7Z8yiDCptyPgBQg%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Mon, Dec 09, 2024 at 12:07:20PM +0100, Karthik Nayak wrote: >> The reference transaction only allows a update for a given reference to >> avoid conflicts. This, however, isn't an issue for reflogs. There are no >> conflicts to be resolved in reflogs and when migrating reflogs between >> backends we'd have multiple reflog entries for the same refname. >> >> So allow multiple reflog updates within a single transaction. Also the >> reflog creation logic isn't exposed to the end user. While this might >> change in the future, currently, this reduces the scope of issues to >> think about. >> >> This is required to add reflog migration support to `git refs migrate` >> which currently doesn't support it. > > Nit: the second half of this sentence starting with "which currently..." > feels rather pointless, as it's implicit in the first half. I'd just > drop it. > Will do. >> Signed-off-by: Karthik Nayak >> --- >> refs/files-backend.c | 15 +++++++++++---- >> refs/reftable-backend.c | 16 +++++++++++++--- >> 2 files changed, 24 insertions(+), 7 deletions(-) >> >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 32975e0fd7a03ab8ddf99c0a68af99921d3f5090..10fba1e97b967fbc04c62a0a6d7d9648ce1c51fb 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -2612,6 +2612,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, >> >> update->backend_data = lock; >> >> + if (update->flags & REF_LOG_ONLY) >> + goto out; >> + >> if (update->type & REF_ISSYMREF) { >> if (update->flags & REF_NO_DEREF) { >> /* > > Hm. Does this mean that we don't lock at all for REF_LOG_ONLY updates? > Reflogs themselves have no lockfile, so isn't it mandatory that we lock > the corresponding ref like we used to do? Otherwise I cannot see how we > avoid two concurrent writers to the same reflog. > No it doesn't, this is after the lock is obtained. We simply exit early since for reflog only updates, there is nothing further to do. >> @@ -3036,8 +3042,9 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, >> >> /* Fail if a refname appears more than once in the transaction: */ >> for (i = 0; i < transaction->nr; i++) >> - string_list_append(&affected_refnames, >> - transaction->updates[i]->refname); >> + if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) >> + string_list_append(&affected_refnames, >> + transaction->updates[i]->refname); >> string_list_sort(&affected_refnames); >> if (ref_update_reject_duplicates(&affected_refnames, err)) { >> ret = TRANSACTION_GENERIC_ERROR; > > This on the other hand is sensible -- having multiple REF_LOG_ONLY > transactions queued is fine. > >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..d9d2e28122a00ddd7f835c35a5851e390761885b 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data >> } >> >> fill_reftable_log_record(log, &c); >> - log->update_index = ts; >> + >> + update_index = strintmap_get(&logs_ts, u->refname); >> + log->update_index = update_index; >> + strintmap_set(&logs_ts, u->refname, update_index+1); > > So we're now tracking update indices via another map in order to ensure > that the update index will be increased if we have multiple reflog > entries for the same refname. Can we avoid that overhead by instead just > having a global update index counter that increases for every single > reflog entry, regardless of whether we have multiple ones queued up for > the same reference? > > I guess the result would be kind of weird as a single transaction with > multiple ref updates would now always contain N different update > indices. Maybe there's an alternative that allows us to reduce the cost, > like only doing this for REF_LOG_ONLY updates? > > I'm mostly being careful because this here is the hot loop of writing > refs, so I don't want to regress performance. Thanks for bringing this up. I was thinking hard about this for a while. I also did some local benchmarking, for 10000 atomic writes, I couldn't find a note-worthy regression. But I really like the point you made about how this could probably use a counter. I think we can use `u->index`. The index field was added to ensure that the logs stay in order when we sort inside `write_transaction_table()`. But we can use the same here. We can simply do log->update_index = ts + u->index; This would increment the update_index as needed and also only kick in when the `index` itself is set. Which would be only for reflog migration between backends. I think this would work well. Karthik ``` ## karthik nayak, 2024-12-12 14:52 Subject: Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZTFJ%2BU3RmDQs8hwTCnfNfTarpxE1otcYAAV-FSOogAq%3DQ%40mail.gmail.com In-Reply-To: ``` Christian Couder writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak wrote: >> >> The reference transaction only allows a update for a given reference to > > s/a update/an update/ > > or: s/a update/a single update/ 'a single update' sounds the best here, will add. >> avoid conflicts. This, however, isn't an issue for reflogs. There are no >> conflicts to be resolved in reflogs and when migrating reflogs between >> backends we'd have multiple reflog entries for the same refname. > > >> @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data >> struct ident_split committer_ident = {0}; >> size_t logs_nr = 0, logs_alloc = 0, i; >> const char *committer_info; >> + struct strintmap logs_ts; > > Here a comment might help explain what logs_ts is used for. > I think with Patricks comment, this whole code will be removed for something simpler. >> int ret = 0; >> >> committer_info = git_committer_info(0); >> @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data >> >> QSORT(arg->updates, arg->updates_nr, transaction_update_cmp); >> >> + strintmap_init(&logs_ts, ts); > > I am not sure I understand what logs_ts is used for and why its > default value is set to ts. > The reason I added this was because in the reftable backend, the writer sorts logs before writing. So if the multiple reflogs contained the same update_index, their order might be changed. But for migrating reflogs, we need to ensure we maintain the order. Using a map here, allowed us to increment the update_index for reflogs for a given refname. > Also ts is an uint64_t while the second argument to strintmap_init() > is an int. I wonder if it could be an issue especially on 32 bits > platforms. > This is fair point, I decided to scrap this ultimately and simply append `u->index` to the update_index. Which would provide the same desired effect. >> reftable_writer_set_limits(writer, ts, ts); >> >> for (i = 0; i < arg->updates_nr; i++) { >> @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data >> >> if (create_reflog) { >> struct ident_split c; >> + uint64_t update_index; >> >> ALLOC_GROW(logs, logs_nr + 1, logs_alloc); >> log = &logs[logs_nr++]; >> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data >> } >> >> fill_reftable_log_record(log, &c); >> - log->update_index = ts; >> + >> + update_index = strintmap_get(&logs_ts, u->refname); >> + log->update_index = update_index; >> + strintmap_set(&logs_ts, u->refname, update_index+1); > > s/update_index+1/update_index + 1/ > > Also is the 'update_index' var really needed or could we just do: > > log->update_index = > strintmap_get(&logs_ts, u->refname); > strintmap_set(&logs_ts, u->refname, > log->update_index + 1); > > ? > The temp variable can be removed here indeed. But I'll remove all of this in the next version. Thanks >> log->refname = xstrdup(u->refname); >> memcpy(log->value.update.new_hash, >> u->new_oid.hash, GIT_MAX_RAWSZ); ```