| Message ID | 20241209-320-git-refs-migrate-reflogs-v1-3-d4bc37ee860f@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add reflog support to `git refs migrate` | expand |
On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: > > When refs are updated in the files-backend, a lock is obtained for the > corresponding file path. This is the case even for reflogs, i.e. a lock > is obtained on the reference path instead of the reflog path. This > works, since generally, reflogs are updated alongside the ref. > > The upcoming patches will add support for reflog updates in ref > transaction. This means, in a particular transaction we want to have ref > updates and reflog updates. For refs, in a given transaction there can > only be one update. But, we can theoretically have multiple reflog > updates in a given transaction. Nit: Giving an example might help understand where multiple reflog updates can happen in a given transaction. Alternatively pointing to an existing doc that contains such an example or explanations might help too. > @@ -2572,18 +2588,25 @@ static int lock_ref_for_update(struct files_ref_store *refs, > goto out; > } > > - ret = lock_raw_ref(refs, update->refname, mustexist, > - affected_refnames, > - &lock, &referent, > - &update->type, err); > - if (ret) { > - char *reason; > + lock = strmap_get(&backend_data->ref_locks, update->refname); > + if (lock) { > + lock->count = lock->count + 1; Nit: lock->count++; > + } else { > + ret = lock_raw_ref(refs, update->refname, mustexist, > + affected_refnames, > + &lock, &referent, > + &update->type, err); > + if (ret) { > + char *reason; > + > + reason = strbuf_detach(err, NULL); > + strbuf_addf(err, "cannot lock ref '%s': %s", > + ref_update_original_update_refname(update), reason); > + free(reason); > + goto out; > + } > > - reason = strbuf_detach(err, NULL); > - strbuf_addf(err, "cannot lock ref '%s': %s", > - ref_update_original_update_refname(update), reason); > - free(reason); > - goto out; > + strmap_put(&backend_data->ref_locks, update->refname, lock); > } > > update->backend_data = lock;
On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: > > When refs are updated in the files-backend, a lock is obtained for the > corresponding file path. This is the case even for reflogs, i.e. a lock > is obtained on the reference path instead of the reflog path. This > works, since generally, reflogs are updated alongside the ref. > > The upcoming patches will add support for reflog updates in ref > transaction. This means, in a particular transaction we want to have ref > updates and reflog updates. For refs, in a given transaction there can > only be one update. 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 <christian.couder@gmail.com> writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: >> >> When refs are updated in the files-backend, a lock is obtained for the >> corresponding file path. This is the case even for reflogs, i.e. a lock >> is obtained on the reference path instead of the reflog path. This >> works, since generally, reflogs are updated alongside the ref. >> >> The upcoming patches will add support for reflog updates in ref >> transaction. This means, in a particular transaction we want to have ref >> updates and reflog updates. For refs, in a given transaction there can >> only be one update. But, we can theoretically have multiple reflog >> updates in a given transaction. > > Nit: Giving an example might help understand where multiple reflog > updates can happen in a given transaction. Alternatively pointing to > an existing doc that contains such an example or explanations might > help too. > 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]
Christian Couder <christian.couder@gmail.com> writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: >> >> When refs are updated in the files-backend, a lock is obtained for the >> corresponding file path. This is the case even for reflogs, i.e. a lock >> is obtained on the reference path instead of the reflog path. This >> works, since generally, reflogs are updated alongside the ref. >> >> The upcoming patches will add support for reflog updates in ref >> transaction. This means, in a particular transaction we want to have ref >> updates and reflog updates. For refs, in a given transaction there can >> only be one update. > > Maybe something like: "For a given ref in a given transaction there > can be at most one update." > 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!
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; /*
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 <karthik.188@gmail.com> --- refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 19 deletions(-)