Re: [PATCH v8 1/6] refs: atomically record overwritten ref in update_symref

From: karthik nayak <karthik.188@gmail.com>
Date: Oct 15, 2024, 07:41 UTC
Message-ID: <CAOLa=ZTMzz-9XE8AY8vk7Gpbm+U0T-5jWNJzKLyd621_URiDEA@mail.gmail.com>
In-Reply-To: <20241014225431.1394565-2-bence@ferdinandy.com>

Bence Ferdinandy <bence@ferdinandy.com> writes:

[snip]

Show 29 quoted lines

> diff --git a/refs.c b/refs.c
> index 5f729ed412..b964ac44d0 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 *referent)
>  {
>  	struct ref_transaction *transaction;
>  	struct strbuf err = STRBUF_INIT;
> @@ -2122,17 +2123,23 @@ int refs_update_symref(struct ref_store *refs, const char *ref,
>
>  	transaction = ref_store_transaction_begin(refs, &err);
>  	if (!transaction ||
> -	    ref_transaction_update(transaction, ref, NULL, NULL,
> +		ref_transaction_update(transaction, ref, NULL, NULL,
>  				   target, NULL, REF_NO_DEREF,
>  				   logmsg, &err) ||
> -	    ref_transaction_commit(transaction, &err)) {
> +		ref_transaction_prepare(transaction, &err)) {
>  		ret = error("%s", err.buf);
> +		goto cleanup;
>  	}
> +	if (referent)
> +		refs_read_symbolic_ref(refs, ref, referent);

Shouldn't we also check the return value here?

Show 10 quoted lines

> +
> +	if (ref_transaction_commit(transaction, &err))
> +		ret = error("%s", err.buf);
>
> +cleanup:
>  	strbuf_release(&err);
>  	if (transaction)
>  		ref_transaction_free(transaction);
> -
>

Why remove this whiteline?

>  	return ret;
>  }
>

[snip]