Re: [PATCH v4 1/5] refs_update_symref: atomically record overwritten ref

[Junio C Hamano <gitster@pobox.com>]

Bence Ferdinandy <bence@ferdinandy.com> 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[] = {