[PATCH v5 0/7] refs: add support for transactional symref updates
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- May 1, 2024, 20:22 UTC
- Message-ID
- <20240501202229.2695774-1-knayak@gitlab.com>
- In-Reply-To
- <20240426152449.228860-1-knayak@gitlab.com>
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
Changes over v4 are: - Dropped the patches for adding support in git-update-ref. - Added changes to use transaction in 'refs_create_symref()' and deprecate 'create_symref'. - Cleaned up the commit messages, documentation and comments. - Added better handling, like if 'old_target' is set, the value of the reference is checked, irrelevant of its type.
Range diff:
1: 4a56e3ede4 ! 1: a354190905 refs: accept symref values in `ref_transaction[_add]_update`
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- refs: accept symref values in `ref_transaction[_add]_update`
+ refs: accept symref values in `ref_transaction_update()`
- The `ref_transaction[_add]_update` functions obtain ref information and
- flags to create a `ref_update` and add it to the transaction at hand.
+ The function `ref_transaction_update()` obtains ref information and
+ flags to create a `ref_update` and add them to the transaction at hand.
To extend symref support in transactions, we need to also accept the
- old and new ref targets and process it. In this commit, let's add the
- required parameters to the function and modify all call sites.
+ old and new ref targets and process it. This commit adds the required
+ parameters to the function and modifies all call sites.
The two parameters added are `new_target` and `old_target`. The
`new_target` is used to denote what the reference should point to when
- the transaction is applied.
+ the transaction is applied. Some functions allow this parameter to be
+ NULL, meaning that the reference is not changed.
The `old_target` denotes the value the reference must have before the
update. Some functions allow this parameter to be NULL, meaning that the
old value of the reference is not checked.
- The handling logic of these parameters will be added in consequent
- commits as we add symref commands to the '--stdin' mode of
- 'git-update-ref'.
+ We also update the internal function `ref_transaction_add_update()`
+ similarly to take the two new parameters.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
@@ refs.c: struct ref_update *ref_transaction_add_update(
BUG("update called for transaction that is not open");
+ if (old_oid && !is_null_oid(old_oid) && old_target)
-+ BUG("Only one of old_oid and old_target should be non NULL");
++ BUG("only one of old_oid and old_target should be non NULL");
+ if (new_oid && !is_null_oid(new_oid) && new_target)
-+ BUG("Only one of new_oid and new_target should be non NULL");
++ BUG("only one of new_oid and new_target should be non NULL");
+
FLEX_ALLOC_STR(update, refname, refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
@@ refs.h: struct ref_transaction *ref_transaction_begin(struct strbuf *err);
* transaction.
*
+ * new_target -- the target reference that the reference will be
-+ * update to point to. This takes precedence over new_oid when
-+ * set. If the reference is a regular reference, it will be
-+ * converted to a symbolic reference.
++ * updated to point to. If the reference is a regular reference,
++ * it will be converted to a symbolic reference. Cannot be set
++ * together with `new_oid`. A copy of this value is made in the
++ * transaction.
+ *
+ * old_target -- the reference that the reference must be pointing to.
-+ * Will only be taken into account when the reference is a symbolic
-+ * reference.
++ * Canont be set together with `old_oid`. A copy of this value is
++ * made in the transaction.
+ *
* flags -- flags affecting the update, passed to
* update_ref_lock(). Possible flags: REF_NO_DEREF,
@@ refs/refs-internal.h: struct ref_update {
+ /*
+ * If set, point the reference to this value. This can also be
+ * used to convert regular references to become symbolic refs.
++ * Cannot be set together with `new_oid`.
+ */
+ const char *new_target;
+
+ /*
-+ * If set and the reference is a symbolic ref, check that the
-+ * reference previously pointed to this value.
++ * If set, check that the reference previously pointed to this
++ * value. Cannot be set together with `old_oid`.
+ */
+ const char *old_target;
+
2: 496bf14f28 ! 2: 7dff21dbef files-backend: extract out `create_symref_lock`
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- files-backend: extract out `create_symref_lock`
+ files-backend: extract out `create_symref_lock()`
- The function `create_symref_locked` creates a symref by creating a
+ The function `create_symref_locked()` creates a symref by creating a
'<symref>.lock' file and then committing the symref lock, which creates
the final symref.
- Split this into two individual functions `create_and_commit_symref` and
- `create_symref_locked`. This way we can create the symref lock and
- commit it at different times. This will be used to provide symref
- support in `git-update-ref(1)`.
+ Extract the early half of `create_symref_locked()` into a new helper
+ function `create_symref_lock()`. Because the name of the new function is
+ too similar to the original, rename the original to
+ `create_and_commit_symref()` to avoid confusion.
+
+ The new function `create_symref_locked()` can be used to create the
+ symref lock in a separate step from that of committing it. This allows
+ to add transactional support for symrefs, where the lock would be
+ created in the preparation step and the lock would be committed in the
+ finish step.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
@@ refs/files-backend.c: static void update_symref_reflog(struct files_ref_store *r
+ return error("unable to fdopen %s: %s",
+ get_lock_file_path(&lock->lk), strerror(errno));
+
-+ /* no error check; commit_ref will check ferror */
-+ fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
++ if (fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target) < 0)
++ return error("unable to fprintf %s: %s",
++ get_lock_file_path(&lock->lk), strerror(errno));
+ return 0;
+}
+
@@ refs/files-backend.c: static void update_symref_reflog(struct files_ref_store *r
- if (commit_ref(lock) < 0)
- return error("unable to write symref for %s: %s", refname,
- strerror(errno));
- return 0;
+- return 0;
++ return ret;
}
+ static int files_create_symref(struct ref_store *ref_store,
@@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store,
return -1;
}
3: 6337859cbb < -: ---------- update-ref: add support for 'symref-verify' command
4: e611cb5a8c < -: ---------- update-ref: add support for 'symref-delete' command
5: 37f8be2f3f < -: ---------- update-ref: add support for 'symref-create' command
6: b385f4d0d7 < -: ---------- update-ref: add support for 'symref-update' command
7: ef335e47d1 < -: ---------- ref: support symrefs in 'reference-transaction' hook
-: ---------- > 3: 901a586683 refs: support symrefs in 'reference-transaction' hook
-: ---------- > 4: 6c97f6a660 refs: add support for transactional symref updates
-: ---------- > 5: 5b55406430 refs: use transaction in `refs_create_symref()`
-: ---------- > 6: 9e25816e68 refs: rename `refs_create_symref()` to `refs_update_symref()`
-: ---------- > 7: 3836e25932 refs: remove `create_symref` and associated dead codeKarthik Nayak (7): refs: accept symref values in `ref_transaction_update()` files-backend: extract out `create_symref_lock()` refs: support symrefs in 'reference-transaction' hook 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 | 87 +++++++++++---- refs.h | 20 +++- refs/debug.c | 13 --- refs/files-backend.c | 185 +++++++++++++++++++------------ refs/packed-backend.c | 1 - refs/refs-internal.h | 26 ++++- refs/reftable-backend.c | 159 +++++++++----------------- 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, 321 insertions(+), 240 deletions(-)
-- 2.43.GIT