Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes:
> 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);
> @@ -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).
> 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?
> 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[] = {
