| Message ID | 20240507125859.132116-1-knayak@gitlab.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | refs: add support for transactional symref updates | expand |
Hi Karthik On 07/05/2024 13:58, Karthik Nayak wrote: > Changes since v6: > * Made the check for old/new oid & target more stricter by removing > the exception for null oid's. > * Extracted `ref_update_check_old_target` and `original_update_refname` > to refs.c > * ^This allowed us to generalize the error messages for non-matching > old_target values between files and reftable backend. > * Better line wrapping in code. > * Fixed some grammar in commit messages. Thanks for re-rolling - these changes address all of my comments on v6 and the range-diff below looks good. Best Wishes Phillip > Range diff against v6: > > 1: a354190905 ! 1: defc1b3521 refs: accept symref values in `ref_transaction_update()` > @@ refs.c: struct ref_update *ref_transaction_add_update( > if (transaction->state != REF_TRANSACTION_OPEN) > BUG("update called for transaction that is not open"); > > -+ if (old_oid && !is_null_oid(old_oid) && old_target) > ++ if (old_oid && old_target) > + BUG("only one of old_oid and old_target should be non NULL"); > -+ if (new_oid && !is_null_oid(new_oid) && new_target) > ++ if (new_oid && new_target) > + BUG("only one of new_oid and new_target should be non NULL"); > + > FLEX_ALLOC_STR(update, refname, refname); > 2: 0d9c5b9804 = 2: 54bb78a27c files-backend: extract out `create_symref_lock()` > 3: e0219ffd31 = 3: c16b7c5da0 refs: support symrefs in 'reference-transaction' hook > -: ---------- > 4: 2d268f12cc refs: move `original_update_refname` to 'refs.c' > 4: b22c59c722 ! 5: 7db3a2245f refs: add support for transactional symref updates > @@ Commit message > updates. While this is exposed to users via 'git-update-ref' and its > '--stdin' mode, it is also used internally within various commands. > > - However, we never supported transactional updates of symrefs. Let's add > - support for symrefs in both the 'files' and the 'reftable' backend. > + However, we do not support transactional updates of symrefs. This commit > + adds support for symrefs in both the 'files' and the 'reftable' backend. > > Here, we add and use `ref_update_has_null_new_value()`, a helper > function which is used to check if there is a new_value in a reference > update. The new value could either be a symref target `new_target` or a > OID `new_oid`. > > - With this, now transactional updates (verify, create, delete, update) > - can be used for: > + We also add another common function `ref_update_check_old_target` which > + will be used to check if the update's old_target corresponds to a > + reference's current target. > + > + Now transactional updates (verify, create, delete, update) can be used > + for: > - regular refs > - symbolic refs > - conversion of regular to symbolic refs and vice versa > @@ refs.c: int ref_transaction_update(struct ref_transaction *transaction, > > ref_transaction_add_update(transaction, refname, flags, > new_oid, old_oid, new_target, > -@@ refs.c: int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg > - { > - return refs_copy_existing_ref(get_main_ref_store(the_repository), oldref, newref, logmsg); > +@@ refs.c: const char *ref_update_original_update_refname(struct ref_update *update) > + return update->refname; > } > -+ > + > +int ref_update_has_null_new_value(struct ref_update *update) > +{ > + return !update->new_target && is_null_oid(&update->new_oid); > +} > - > - ## refs/files-backend.c ## > -@@ refs/files-backend.c: static const char *original_update_refname(struct ref_update *update) > - return update->refname; > - } > - > -+/* > -+ * Check whether the old_target values stored in update are consistent > -+ * with current_target, which is the symbolic reference's current value. > -+ * If everything is OK, return 0; otherwise, write an error message to > -+ * err and return -1. > -+ */ > -+static int check_old_target(struct ref_update *update, > -+ const char *current_target, > -+ struct strbuf *err) > ++ > ++int ref_update_check_old_target(const char *referent, struct ref_update *update, > ++ struct strbuf *err) > +{ > + if (!update->old_target) > + BUG("called without old_target set"); > + > -+ if (!strcmp(update->old_target, current_target)) > ++ if (!strcmp(referent, update->old_target)) > + return 0; > + > -+ if (!strcmp(current_target, "")) > -+ strbuf_addf(err, "cannot lock ref '%s': " > ++ if (!strcmp(referent, "")) > ++ strbuf_addf(err, "verifying symref target: '%s': " > + "reference is missing but expected %s", > -+ original_update_refname(update), > ++ ref_update_original_update_refname(update), > + update->old_target); > + else > -+ strbuf_addf(err, "cannot lock ref '%s': " > ++ strbuf_addf(err, "verifying symref target: '%s': " > + "is at %s but expected %s", > -+ original_update_refname(update), > -+ current_target, update->old_target); > -+ > ++ ref_update_original_update_refname(update), > ++ referent, update->old_target); > + return -1; > +} > -+ > - /* > - * Check whether the REF_HAVE_OLD and old_oid values stored in update > - * are consistent with oid, which is the reference's current value. If > + > + ## refs/files-backend.c ## > +@@ refs/files-backend.c: static int split_symref_update(struct ref_update *update, > + > + new_update = ref_transaction_add_update( > + transaction, referent, new_flags, > +- &update->new_oid, &update->old_oid, > +- NULL, NULL, update->msg); > ++ update->new_target ? NULL : &update->new_oid, > ++ update->old_target ? NULL : &update->old_oid, > ++ update->new_target, update->old_target, update->msg); > + > + new_update->parent_update = update; > + > @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs, > > files_assert_main_repository(refs, "lock_ref_for_update"); > @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref > + } > + > + if (update->old_target) { > -+ if (check_old_target(update, referent.buf, err)) { > ++ if (ref_update_check_old_target(referent.buf, update, err)) { > + ret = TRANSACTION_GENERIC_ERROR; > + goto out; > + } > @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref > + * be set for symrefs, but we're strict about its usage. > + */ > + if (update->old_target) { > -+ if (check_old_target(update, referent.buf, err)) { > ++ if (ref_update_check_old_target(referent.buf, update, err)) { > + ret = TRANSACTION_GENERIC_ERROR; > + goto out; > + } > @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref > - !(update->flags & REF_DELETING) && > - !(update->flags & REF_LOG_ONLY)) { > + if (update->new_target && !(update->flags & REF_LOG_ONLY)) { > -+ if (create_symref_lock(refs, lock, update->refname, update->new_target, err)) { > ++ if (create_symref_lock(refs, lock, update->refname, > ++ update->new_target, err)) { > + ret = TRANSACTION_GENERIC_ERROR; > + goto out; > + } > @@ refs/files-backend.c: static int files_transaction_finish(struct ref_store *ref_ > if (commit_ref(lock)) { > > ## refs/refs-internal.h ## > -@@ refs/refs-internal.h: void base_ref_store_init(struct ref_store *refs, struct repository *repo, > +@@ refs/refs-internal.h: struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor > */ > - struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store); > + const char *ref_update_original_update_refname(struct ref_update *update); > > +/* > + * Helper function to check if the new value is null, this > @@ refs/refs-internal.h: void base_ref_store_init(struct ref_store *refs, struct re > + * ref or a symbolic ref. > + */ > +int ref_update_has_null_new_value(struct ref_update *update); > ++ > ++/* > ++ * Check whether the old_target values stored in update are consistent > ++ * with the referent, which is the symbolic reference's current value. > ++ * If everything is OK, return 0; otherwise, write an error message to > ++ * err and return -1. > ++ */ > ++int ref_update_check_old_target(const char *referent, struct ref_update *update, > ++ struct strbuf *err); > + > #endif /* REFS_REFS_INTERNAL_H */ > > @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s > */ > - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { > + if (u->old_target) { > -+ if (strcmp(referent.buf, u->old_target)) { > -+ if (!strcmp(referent.buf, "")) > -+ strbuf_addf(err, "verifying symref target: '%s': " > -+ "reference is missing but expected %s", > -+ original_update_refname(u), > -+ u->old_target); > -+ else > -+ strbuf_addf(err, "verifying symref target: '%s': " > -+ "is at %s but expected %s", > -+ original_update_refname(u), > -+ referent.buf, u->old_target); > ++ if (ref_update_check_old_target(referent.buf, u, err)) { > + ret = -1; > + goto done; > + } > + } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { > if (is_null_oid(&u->old_oid)) > strbuf_addf(err, _("cannot lock ref '%s': " > - "reference already exists"), > + "reference already exists"), > @@ refs/reftable-backend.c: static int write_transaction_table(struct reftable_writer *writer, void *cb_data > * - `core.logAllRefUpdates` tells us to create the reflog for > * the given ref. > */ > - if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) { > -+ if ((u->flags & REF_HAVE_NEW) && !(u->type & REF_ISSYMREF) && ref_update_has_null_new_value(u)) { > ++ if ((u->flags & REF_HAVE_NEW) && > ++ !(u->type & REF_ISSYMREF) && > ++ ref_update_has_null_new_value(u)) { > struct reftable_log_record log = {0}; > struct reftable_iterator it = {0}; > > @@ refs/reftable-backend.c: static int write_transaction_table(struct reftable_writ > + fill_reftable_log_record(log); > + log->update_index = ts; > + log->refname = xstrdup(u->refname); > -+ memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); > -+ memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ); > ++ memcpy(log->value.update.new_hash, > ++ u->new_oid.hash, GIT_MAX_RAWSZ); > ++ memcpy(log->value.update.old_hash, > ++ tx_update->current_oid.hash, GIT_MAX_RAWSZ); > + log->value.update.message = > + xstrndup(u->msg, arg->refs->write_options.block_size / 2); > + } > 5: 636bf5ce98 = 6: 354ebbe17f refs: use transaction in `refs_create_symref()` > 6: 07fb23374f = 7: c8a23b3454 refs: rename `refs_create_symref()` to `refs_update_symref()` > 7: 5c05813bcc = 8: fa1b8f445b refs: remove `create_symref` and associated dead code > > > Karthik Nayak (8): > refs: accept symref values in `ref_transaction_update()` > files-backend: extract out `create_symref_lock()` > refs: support symrefs in 'reference-transaction' hook > refs: move `original_update_refname` to 'refs.c' > refs: add support for transactional symref updates > refs: use transaction in `refs_create_symref()` > refs: rename `refs_create_symref()` to `refs_update_symref()` > refs: remove `create_symref` and associated dead code > > Documentation/githooks.txt | 14 ++- > branch.c | 2 +- > builtin/branch.c | 2 +- > builtin/fast-import.c | 5 +- > builtin/fetch.c | 2 +- > builtin/receive-pack.c | 1 + > builtin/replace.c | 2 +- > builtin/tag.c | 1 + > builtin/update-ref.c | 1 + > builtin/worktree.c | 2 +- > refs.c | 119 ++++++++++++++---- > refs.h | 20 ++- > refs/debug.c | 13 -- > refs/files-backend.c | 208 +++++++++++++++++-------------- > refs/packed-backend.c | 1 - > refs/refs-internal.h | 40 +++++- > refs/reftable-backend.c | 183 +++++++++------------------ > sequencer.c | 9 +- > t/helper/test-ref-store.c | 2 +- > t/t0610-reftable-basics.sh | 2 +- > t/t1416-ref-transaction-hooks.sh | 23 ++++ > walker.c | 2 +- > 22 files changed, 374 insertions(+), 280 deletions(-) >
Karthik Nayak <karthik.188@gmail.com> writes: > Changes since v7: > * I had rebased v7 on next. I've rebased v8 on master. That's the only difference > between the two versions. I've applied them to the same base as used to queue the previous round, which I think is "436d4e5b14 The seventeenth batch". It went without conflicts, and tests fine in isolation. I'll see if it plays well with other topics in 'seen' later in the day but not now. Thanks. > Junio, this might cause conflicts when merging, I think you resolved them for > v6 and hope its the same now. Let me know if I can help otherwise somehow. The easiest for both of us would be to do this: (1) Build on whatever base you want, and format-patch the series. If you are doing "rebase -i" in-place to update from the previous round, this will reuse the previous base so (2) and (3) may become trivial. (2) Find the base of where the last round was queued, something like $ mine='kn/ref-transaction-symref' $ git checkout "origin/seen^{/^Merge branch '$mine'}...master" (3) Apply your format-patch result. There are three cases (3)-1. Things apply cleanly and tests fine. Go to (4). (3)-2. Things apply cleanly but does not build or test fails, or things do not apply cleanly. In the latter case, you have textual or semantic conflicts coming from the difference between the old base and the base you used to build in (1). Identify what caused the breakages (e.g., a topic or two may have merged since the base used by (2) until the base used by (1)). Check out the latest 'origin/master' (which may be newer than the base used by (2)), "merge --no-ff" the topics you newly depend on in there, and use the result of the merge(s) as the base, rebuild the series and test again. Run format-patch from the last such merges to the tip of your topic. If you did $ git checkout origin/master $ git merge --no-ff --into kn/ref-transaction-symref fo/obar $ git merge --no-ff --into kn/ref-transaction-symref ba/zqux ... rebuild the topic ... Then you'd just format your topic above these "preparing the ground" merges, e.g. $ git format-patch "HEAD^{/^Merge branch 'ba/zqux'}"..HEAD Do not forget to write in the cover letter you did this, including the topics you have in your base on top of 'master'. Then go to (4). (4) Make a trial merge of your topic into 'next' and 'seen', e.g. $ git checkout --detach 'origin/seen' && git revert -m 1 <the merge of the previous iteration into seen> && git merge kn/ref-transaction-symref The "revert" is needed if the previous iteration of your topic is already in 'seen' (like in this case). You could choose to rebuild master..origin/seen from scratch while excluding your previous iteration, which may emulate what happens on my end more closely. This trial merge may conflict. It is primarily to see what conflicts _other_ topics may have with your topic. In other words, you do not have to depend on to make your topic work on 'master'. It may become the job of the other topic owners to resolve conflicts if your topic goes to 'next' before theirs. Make a note on what conflict you saw in the cover letter. You do not necessarily have to resolve them, but it would be a good opportunity to learn what others are doing in an related area. $ git checkout --detach 'origin/next' && git merge kn/ref-transaction-symref This is to see what conflicts your topic has with other topics that are already cooking. This should not conflict if (3)-2 prepared a base on top of updated master plus dependent topics taken from 'next'. Unless the context is severe (one way to tell is try the same trial merge with your old iteration, which may conflict in a similar way), expect that it will be handled on my end (if it gets unmanageable, I'll ask to rebase when I receive your patches). Something like the above should be added to the SubmittingPatches document (or its successor to cover more advanced topics, perhaps). Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> Changes since v7: >> * I had rebased v7 on next. I've rebased v8 on master. That's the only difference >> between the two versions. > > I've applied them to the same base as used to queue the previous > round, which I think is "436d4e5b14 The seventeenth batch". It went > without conflicts, and tests fine in isolation. I'll see if it plays > well with other topics in 'seen' later in the day but not now. > > Thanks. > >> Junio, this might cause conflicts when merging, I think you resolved them for >> v6 and hope its the same now. Let me know if I can help otherwise somehow. > > The easiest for both of us would be to do this: > > (1) Build on whatever base you want, and format-patch the series. > If you are doing "rebase -i" in-place to update from the > previous round, this will reuse the previous base so (2) and > (3) may become trivial. > > (2) Find the base of where the last round was queued, something like > > $ mine='kn/ref-transaction-symref' > $ git checkout "origin/seen^{/^Merge branch '$mine'}...master" > I find the '...' always so confusing, I would say suggesting to use 'git-merge-base' would be much nicer here. > (3) Apply your format-patch result. There are three cases > > (3)-1. Things apply cleanly and tests fine. Go to (4). > > (3)-2. Things apply cleanly but does not build or test fails, > or things do not apply cleanly. > > In the latter case, you have textual or semantic conflicts > coming from the difference between the old base and the base > you used to build in (1). Identify what caused the breakages > (e.g., a topic or two may have merged since the base used by > (2) until the base used by (1)). > > Check out the latest 'origin/master' (which may be newer than > the base used by (2)), "merge --no-ff" the topics you newly For my own understanding, even if we use '--ff' the end result should be the same, but using '--no-ff' would ensure that the changes and conflicts are isolated to the merge commit, right? > depend on in there, and use the result of the merge(s) as the > base, rebuild the series and test again. Run format-patch from > the last such merges to the tip of your topic. If you did > > $ git checkout origin/master > $ git merge --no-ff --into kn/ref-transaction-symref fo/obar > $ git merge --no-ff --into kn/ref-transaction-symref ba/zqux > ... rebuild the topic ... > I guess you mean '--into-name' here? I would skip mentioning this since it doesn't have any real effect and is perhaps confusing. > Then you'd just format your topic above these "preparing the > ground" merges, e.g. > > $ git format-patch "HEAD^{/^Merge branch 'ba/zqux'}"..HEAD > > Do not forget to write in the cover letter you did this, > including the topics you have in your base on top of 'master'. > Then go to (4). > > (4) Make a trial merge of your topic into 'next' and 'seen', e.g. > > $ git checkout --detach 'origin/seen' && > git revert -m 1 <the merge of the previous iteration into seen> && > git merge kn/ref-transaction-symref > > The "revert" is needed if the previous iteration of your topic > is already in 'seen' (like in this case). You could choose to > rebuild master..origin/seen from scratch while excluding your > previous iteration, which may emulate what happens on my end > more closely. > > This trial merge may conflict. It is primarily to see what > conflicts _other_ topics may have with your topic. In other > words, you do not have to depend on to make your topic work on > 'master'. It may become the job of the other topic owners to > resolve conflicts if your topic goes to 'next' before theirs. > > Make a note on what conflict you saw in the cover letter. You > do not necessarily have to resolve them, but it would be a good > opportunity to learn what others are doing in an related area. > > $ git checkout --detach 'origin/next' && > git merge kn/ref-transaction-symref > > This is to see what conflicts your topic has with other topics > that are already cooking. This should not conflict if (3)-2 > prepared a base on top of updated master plus dependent topics > taken from 'next'. Unless the context is severe (one way to > tell is try the same trial merge with your old iteration, which > may conflict in a similar way), expect that it will be handled > on my end (if it gets unmanageable, I'll ask to rebase when I > receive your patches). > > Something like the above should be added to the SubmittingPatches > document (or its successor to cover more advanced topics, perhaps). > > Thanks. The rest of this looks good, I'll cleanup add the appropriate syntax, merge in your patches [1] and send something soon! [1]: https://lore.kernel.org/all/20240510165526.1412338-1-gitster@pobox.com/#t
Karthik Nayak <karthik.188@gmail.com> writes: > I find the '...' always so confusing, I would say suggesting to use > 'git-merge-base' would be much nicer here. They are equivalent, except that "..." in git checkout --detach A...B is internal as opposed to git checkout --detach $(merge-base A B) that uses one extra process. I thought people loath spawning processes? >> Check out the latest 'origin/master' (which may be newer than >> the base used by (2)), "merge --no-ff" the topics you newly > > For my own understanding, even if we use '--ff' the end result should be > the same, but using '--no-ff' would ensure that the changes and > conflicts are isolated to the merge commit, right? It will make it easy for you to look at git log --first-parent --oneline master.. by hiding the base commits you depend on behind a single merge commit. >> $ git merge --no-ff --into kn/ref-transaction-symref ba/zqux >> ... rebuild the topic ... > > I guess you mean '--into-name' here? I would skip mentioning this since > it doesn't have any real effect and is perhaps confusing. Again this is to help that "one liner first-parent chain" output.
From: Karthik Nayak <karthik.188@gmail.com> The patch series takes over from the existing patch series, wherein we introduced symref-* commands to git-update-ref. Since there was a ton of discussions on the UX of the patch series and its application, I thought it would be best to shorten the series and split it into multiple smaller series. This series adds transactional support for symrefs in the reference db. Then we switch refs_create_symref() to start using transactions for symref updates. This allows us to deprecate the create_symref code in the ref_storage_be interface and remove all associated code which is no longer used. The split was primarily done so we can merge the non-user facing parts of the previous series. While pertaining the user facing components into another set of patches wherein deeper discussion on the UX can be held without worrying about the internal implementation. Also by using this new functionality in a pre-existing command, we can leverage the existing tests to catch any inconsistencies. One of which was how 'git-symbolic-ref' doesn't add reflog for dangling symrefs, which I've modified my patch to do the same. We also modify the reference transaction hook to support symrefs. For any symref update the reference transaction hook will output the value with a 'ref:' prefix. Previous versions: V1: https://lore.kernel.org/git/20240330224623.579457-1-knayak@gitlab.com V2: https://lore.kernel.org/git/20240412095908.1134387-1-knayak@gitlab.com V3: https://lore.kernel.org/git/20240423212818.574123-1-knayak@gitlab.com V4: https://lore.kernel.org/r/20240426152449.228860-1-knayak@gitlab.com V5: https://lore.kernel.org/r/20240501202229.2695774-1-knayak@gitlab.com V6: https://lore.kernel.org/r/20240503124115.252413-1-knayak@gitlab.com V7: https://lore.kernel.org/r/20240507060035.28602-1-knayak@gitlab.com Thanks for all the reviews! Changes since v7: * I had rebased v7 on next. I've rebased v8 on master. That's the only difference between the two versions. Junio, this might cause conflicts when merging, I think you resolved them for v6 and hope its the same now. Let me know if I can help otherwise somehow. Changes since v6: * Made the check for old/new oid & target more stricter by removing the exception for null oid's. * Extracted `ref_update_check_old_target` and `original_update_refname` to refs.c * ^This allowed us to generalize the error messages for non-matching old_target values between files and reftable backend. * Better line wrapping in code. * Fixed some grammar in commit messages. Range diff against v6: 1: a354190905 ! 1: defc1b3521 refs: accept symref values in `ref_transaction_update()` @@ refs.c: struct ref_update *ref_transaction_add_update( if (transaction->state != REF_TRANSACTION_OPEN) BUG("update called for transaction that is not open"); -+ if (old_oid && !is_null_oid(old_oid) && old_target) ++ if (old_oid && old_target) + BUG("only one of old_oid and old_target should be non NULL"); -+ if (new_oid && !is_null_oid(new_oid) && new_target) ++ if (new_oid && new_target) + BUG("only one of new_oid and new_target should be non NULL"); + FLEX_ALLOC_STR(update, refname, refname); 2: 0d9c5b9804 = 2: 54bb78a27c files-backend: extract out `create_symref_lock()` 3: e0219ffd31 = 3: c16b7c5da0 refs: support symrefs in 'reference-transaction' hook -: ---------- > 4: 2d268f12cc refs: move `original_update_refname` to 'refs.c' 4: b22c59c722 ! 5: 7db3a2245f refs: add support for transactional symref updates @@ Commit message updates. While this is exposed to users via 'git-update-ref' and its '--stdin' mode, it is also used internally within various commands. - However, we never supported transactional updates of symrefs. Let's add - support for symrefs in both the 'files' and the 'reftable' backend. + However, we do not support transactional updates of symrefs. This commit + adds support for symrefs in both the 'files' and the 'reftable' backend. Here, we add and use `ref_update_has_null_new_value()`, a helper function which is used to check if there is a new_value in a reference update. The new value could either be a symref target `new_target` or a OID `new_oid`. - With this, now transactional updates (verify, create, delete, update) - can be used for: + We also add another common function `ref_update_check_old_target` which + will be used to check if the update's old_target corresponds to a + reference's current target. + + Now transactional updates (verify, create, delete, update) can be used + for: - regular refs - symbolic refs - conversion of regular to symbolic refs and vice versa @@ refs.c: int ref_transaction_update(struct ref_transaction *transaction, ref_transaction_add_update(transaction, refname, flags, new_oid, old_oid, new_target, -@@ refs.c: int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg - { - return refs_copy_existing_ref(get_main_ref_store(the_repository), oldref, newref, logmsg); +@@ refs.c: const char *ref_update_original_update_refname(struct ref_update *update) + return update->refname; } -+ + +int ref_update_has_null_new_value(struct ref_update *update) +{ + return !update->new_target && is_null_oid(&update->new_oid); +} - - ## refs/files-backend.c ## -@@ refs/files-backend.c: static const char *original_update_refname(struct ref_update *update) - return update->refname; - } - -+/* -+ * Check whether the old_target values stored in update are consistent -+ * with current_target, which is the symbolic reference's current value. -+ * If everything is OK, return 0; otherwise, write an error message to -+ * err and return -1. -+ */ -+static int check_old_target(struct ref_update *update, -+ const char *current_target, -+ struct strbuf *err) ++ ++int ref_update_check_old_target(const char *referent, struct ref_update *update, ++ struct strbuf *err) +{ + if (!update->old_target) + BUG("called without old_target set"); + -+ if (!strcmp(update->old_target, current_target)) ++ if (!strcmp(referent, update->old_target)) + return 0; + -+ if (!strcmp(current_target, "")) -+ strbuf_addf(err, "cannot lock ref '%s': " ++ if (!strcmp(referent, "")) ++ strbuf_addf(err, "verifying symref target: '%s': " + "reference is missing but expected %s", -+ original_update_refname(update), ++ ref_update_original_update_refname(update), + update->old_target); + else -+ strbuf_addf(err, "cannot lock ref '%s': " ++ strbuf_addf(err, "verifying symref target: '%s': " + "is at %s but expected %s", -+ original_update_refname(update), -+ current_target, update->old_target); -+ ++ ref_update_original_update_refname(update), ++ referent, update->old_target); + return -1; +} -+ - /* - * Check whether the REF_HAVE_OLD and old_oid values stored in update - * are consistent with oid, which is the reference's current value. If + + ## refs/files-backend.c ## +@@ refs/files-backend.c: static int split_symref_update(struct ref_update *update, + + new_update = ref_transaction_add_update( + transaction, referent, new_flags, +- &update->new_oid, &update->old_oid, +- NULL, NULL, update->msg); ++ update->new_target ? NULL : &update->new_oid, ++ update->old_target ? NULL : &update->old_oid, ++ update->new_target, update->old_target, update->msg); + + new_update->parent_update = update; + @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs, files_assert_main_repository(refs, "lock_ref_for_update"); @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref + } + + if (update->old_target) { -+ if (check_old_target(update, referent.buf, err)) { ++ if (ref_update_check_old_target(referent.buf, update, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref + * be set for symrefs, but we're strict about its usage. + */ + if (update->old_target) { -+ if (check_old_target(update, referent.buf, err)) { ++ if (ref_update_check_old_target(referent.buf, update, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref - !(update->flags & REF_DELETING) && - !(update->flags & REF_LOG_ONLY)) { + if (update->new_target && !(update->flags & REF_LOG_ONLY)) { -+ if (create_symref_lock(refs, lock, update->refname, update->new_target, err)) { ++ if (create_symref_lock(refs, lock, update->refname, ++ update->new_target, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } @@ refs/files-backend.c: static int files_transaction_finish(struct ref_store *ref_ if (commit_ref(lock)) { ## refs/refs-internal.h ## -@@ refs/refs-internal.h: void base_ref_store_init(struct ref_store *refs, struct repository *repo, +@@ refs/refs-internal.h: struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor */ - struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store); + const char *ref_update_original_update_refname(struct ref_update *update); +/* + * Helper function to check if the new value is null, this @@ refs/refs-internal.h: void base_ref_store_init(struct ref_store *refs, struct re + * ref or a symbolic ref. + */ +int ref_update_has_null_new_value(struct ref_update *update); ++ ++/* ++ * Check whether the old_target values stored in update are consistent ++ * with the referent, which is the symbolic reference's current value. ++ * If everything is OK, return 0; otherwise, write an error message to ++ * err and return -1. ++ */ ++int ref_update_check_old_target(const char *referent, struct ref_update *update, ++ struct strbuf *err); + #endif /* REFS_REFS_INTERNAL_H */ @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s */ - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { + if (u->old_target) { -+ if (strcmp(referent.buf, u->old_target)) { -+ if (!strcmp(referent.buf, "")) -+ strbuf_addf(err, "verifying symref target: '%s': " -+ "reference is missing but expected %s", -+ original_update_refname(u), -+ u->old_target); -+ else -+ strbuf_addf(err, "verifying symref target: '%s': " -+ "is at %s but expected %s", -+ original_update_refname(u), -+ referent.buf, u->old_target); ++ if (ref_update_check_old_target(referent.buf, u, err)) { + ret = -1; + goto done; + } + } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { if (is_null_oid(&u->old_oid)) strbuf_addf(err, _("cannot lock ref '%s': " - "reference already exists"), + "reference already exists"), @@ refs/reftable-backend.c: static int write_transaction_table(struct reftable_writer *writer, void *cb_data * - `core.logAllRefUpdates` tells us to create the reflog for * the given ref. */ - if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) { -+ if ((u->flags & REF_HAVE_NEW) && !(u->type & REF_ISSYMREF) && ref_update_has_null_new_value(u)) { ++ if ((u->flags & REF_HAVE_NEW) && ++ !(u->type & REF_ISSYMREF) && ++ ref_update_has_null_new_value(u)) { struct reftable_log_record log = {0}; struct reftable_iterator it = {0}; @@ refs/reftable-backend.c: static int write_transaction_table(struct reftable_writ + fill_reftable_log_record(log); + log->update_index = ts; + log->refname = xstrdup(u->refname); -+ memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); -+ memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ); ++ memcpy(log->value.update.new_hash, ++ u->new_oid.hash, GIT_MAX_RAWSZ); ++ memcpy(log->value.update.old_hash, ++ tx_update->current_oid.hash, GIT_MAX_RAWSZ); + log->value.update.message = + xstrndup(u->msg, arg->refs->write_options.block_size / 2); + } 5: 636bf5ce98 = 6: 354ebbe17f refs: use transaction in `refs_create_symref()` 6: 07fb23374f = 7: c8a23b3454 refs: rename `refs_create_symref()` to `refs_update_symref()` 7: 5c05813bcc = 8: fa1b8f445b refs: remove `create_symref` and associated dead code Karthik Nayak (8): refs: accept symref values in `ref_transaction_update()` files-backend: extract out `create_symref_lock()` refs: support symrefs in 'reference-transaction' hook refs: move `original_update_refname` to 'refs.c' refs: add support for transactional symref updates refs: use transaction in `refs_create_symref()` refs: rename `refs_create_symref()` to `refs_update_symref()` refs: remove `create_symref` and associated dead code Documentation/githooks.txt | 14 ++- branch.c | 2 +- builtin/branch.c | 2 +- builtin/fast-import.c | 5 +- builtin/fetch.c | 2 +- builtin/receive-pack.c | 1 + builtin/replace.c | 2 +- builtin/tag.c | 1 + builtin/update-ref.c | 1 + builtin/worktree.c | 2 +- refs.c | 119 ++++++++++++++---- refs.h | 20 ++- refs/debug.c | 13 -- refs/files-backend.c | 208 +++++++++++++++++-------------- refs/packed-backend.c | 1 - refs/refs-internal.h | 40 +++++- refs/reftable-backend.c | 183 +++++++++------------------ sequencer.c | 9 +- t/helper/test-ref-store.c | 2 +- t/t0610-reftable-basics.sh | 2 +- t/t1416-ref-transaction-hooks.sh | 23 ++++ walker.c | 2 +- 22 files changed, 374 insertions(+), 280 deletions(-)