[v5,0/7] refs: add support for transactional symref updates

Message

karthik nayak May 1, 2024, 8:22 p.m. UTC

From: Karthik Nayak <[e-mail]>

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/[e-mail]/
V2: https://lore.kernel.org/git/[e-mail]/
V3: https://lore.kernel.org/git/[e-mail]/
V4: https://lore.kernel.org/r/[e-mail]

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 <[e-mail]>
     
      ## 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 <[e-mail]>
     
    @@ 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 <[e-mail]>
     
      ## 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 <[e-mail]>
     
    @@ 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 code


Karthik 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(-)

Comments

Junio C Hamano May 2, 2024, 12:20 a.m. UTC | #1

Karthik Nayak <[e-mail]> writes:

> From: Karthik Nayak <[e-mail]>
>
> 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.

This split probably makes sense in the context of the evolution of
this series.  If this were without any prior discussion, a change to
the internal mechanism, without showing how the end-user facing half
would use it fully, would have been hard to evaluate, but now that
we know where the new mechanism wants to take us, we can fairly
evaluate it alone without the end-user facing part.

I've read only the earlier half of the series but so far the pieces
make sense to me.

karthik nayak May 2, 2024, 5:53 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> Karthik Nayak <[e-mail]> writes:
>
>> From: Karthik Nayak <[e-mail]>
>>
>> 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.
>
> This split probably makes sense in the context of the evolution of
> this series.  If this were without any prior discussion, a change to
> the internal mechanism, without showing how the end-user facing half
> would use it fully, would have been hard to evaluate, but now that
> we know where the new mechanism wants to take us, we can fairly
> evaluate it alone without the end-user facing part.
>
> I've read only the earlier half of the series but so far the pieces
> make sense to me.

Yes I agree, I think it's also nice to see how a bunch of code can be
removed to use this generic functionality.

I'll wait for a day/two for other reviews. Thanks for your review.