Re: [PATCH v4 1/5] refs_update_symref: atomically record overwritten ref
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 1, 2024, 19:10 UTC
- Message-ID
- <xmqq7carwser.fsf@gitster.g>
- In-Reply-To
- <20240930222025.2349008-2-bence@ferdinandy.com>
Bence Ferdinandy <bence@ferdinandy.com> writes:
Show 15 quoted lines
> diff --git a/refs.c b/refs.c
> index 5f729ed412..301db0dcdc 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2114,7 +2114,8 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct
> }
>
> int refs_update_symref(struct ref_store *refs, const char *ref,
> - const char *target, const char *logmsg)
> + const char *target, const char *logmsg,
> + struct strbuf *before_target)
> {
> struct ref_transaction *transaction;
> struct strbuf err = STRBUF_INIT;
> @@ -2130,6 +2131,10 @@ int refs_update_symref(struct ref_store *refs, const char *ref,Let's extend the precontext of this hunk a bit. The function begins like this:
transaction = ref_store_transaction_begin(refs, &err);
if (!transaction ||
ref_transaction_update(transaction, ref, NULL, NULL,
target, NULL, REF_NO_DEREF,
logmsg, &err) ||
ref_transaction_commit(transaction, &err)) {
ret = error("%s", err.buf);> } > strbuf_release(&err);
We begin a transaction, update ref to point to target in the transaction, and commit the transaction. An error at any stage of this three-step process will bypass the rest and we give an error message.
> + > + if (before_target && transaction->updates[0]->before_target) > + strbuf_addstr(before_target, transaction->updates[0]->before_target);
What if ref_store_transaction_begin() failed?
If we want to say "we append the before_target recorded in the transaction to the caller-supplied strbuf only when we manage to do the update, and we leave before_target intact otherwise" We'd at least need
if (transaction && before_target &&
transaction->updates[0]->before_target)wouldn't it? Like the code that frees it (below), this new call should be prepared to see !transaction.
> if (transaction) > ref_transaction_free(transaction);
Show 5 quoted lines
> @@ -2948,4 +2953,3 @@ int ref_update_expects_existing_old_ref(struct ref_update *update) > return (update->flags & REF_HAVE_OLD) && > (!is_null_oid(&update->old_oid) || update->old_target); > } > -
Good (even though it is unrelated to the topic of this series).
Show 23 quoted lines
> diff --git a/refs.h b/refs.h
> index 108dfc93b3..f38616db84 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -571,7 +571,8 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref,
> const char *newref, const char *logmsg);
>
> int refs_update_symref(struct ref_store *refs, const char *refname,
> - const char *target, const char *logmsg);
> + const char *target, const char *logmsg,
> + struct strbuf *before_target);
>
> enum action_on_err {
> UPDATE_REFS_MSG_ON_ERR,
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 0824c0b8a9..8415f2d020 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2577,6 +2577,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> }
>
> update->backend_data = lock;
> + update->before_target = xstrdup_or_null(referent.buf);OK, so this comes from the backends, as they are the only thing that knows what the current value is (the caller can only indirectly infer if it has old_target, in which case the backend checks if the attempt is stale).
Do we need a corresponding change for the other, reftable, backend?
Show 13 quoted lines
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> index 65346dee55..a911302bea 100644
> --- a/t/helper/test-ref-store.c
> +++ b/t/helper/test-ref-store.c
> @@ -120,7 +120,7 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv)
> const char *target = notnull(*argv++, "target");
> const char *logmsg = *argv++;
>
> - return refs_update_symref(refs, refname, target, logmsg);
> + return refs_update_symref(refs, refname, target, logmsg, NULL);
> }
>
> static struct flag_definition transaction_flags[] = {