Re: [PATCH v2 5/7] refs: introduce enum-based transaction error types

[Patrick Steinhardt <ps@pks.im>]

On Tue, Feb 25, 2025 at 10:29:08AM +0100, Karthik Nayak wrote:
> diff --git a/refs.h b/refs.h
> index b14ba1f9ff..8e9ead174c 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -16,6 +16,31 @@ struct worktree;
>  enum ref_storage_format ref_storage_format_by_name(const char *name);
>  const char *ref_storage_format_to_name(enum ref_storage_format ref_storage_format);
>  
> +/*
> + * enum transaction_error represents the following return codes:
> + * TRANSACTION_OK: success code.
> + * TRANSACTION_GENERIC_ERROR error_code: default error code.
> + * TRANSACTION_NAME_CONFLICT error_code: ref name conflict like A vs A/B.
> + * TRANSACTION_CREATE_EXISTS error_code: ref to be created already exists.
> + * TRANSACTION_NONEXISTENT_REF error_code: ref expected but doesn't exist.
> + * TRANSACTION_INCORRECT_OLD_VALUE error_code: provided old_oid or old_target of
> + * reference doesn't match actual.
> + * TRANSACTION_INVALID_NEW_VALUE error_code: provided new_oid or new_target is
> + * invalid.
> + * TRANSACTION_EXPECTED_SYMREF error_code: expected ref to be symref, but is a
> + * regular ref.
> + */
> +enum transaction_error {
> +	TRANSACTION_OK = 0,
> +	TRANSACTION_GENERIC_ERROR = -1,
> +	TRANSACTION_NAME_CONFLICT = -2,
> +	TRANSACTION_CREATE_EXISTS = -3,
> +	TRANSACTION_NONEXISTENT_REF = -4,
> +	TRANSACTION_INCORRECT_OLD_VALUE = -5,
> +	TRANSACTION_INVALID_NEW_VALUE = -6,
> +	TRANSACTION_EXPECTED_SYMREF = -7,
> +};

Nit: how about we name this `ref_transaction_error` and adapt the the
enum values accordingly? We may eventually also introduce similar errors
for the object database, so it may make sense to have the errors be
specific. Doing both the enum and changing the name might be a bit hard
to review, so we could also rename in a preparatory commit. Or we just
punt on it for now and do it once it becomes necessary, that would also
be fine with me.

I also wonder whether we really want to introduce `TRANSACTION_OK`. It's
always a bit of a mouthful, and in many cases one ends up with a mixture
of `ret < 0`, `ret != TRANSACTION_OK` and `ret != 0`, which may lead to
confusion. Continuing to use `0` for the successful case should be fine.

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 3247871574..75e1ebf67d 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1672,7 +1676,8 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
>  		data->own_lock = 1;
>  	}
>  
> -	if (write_with_updates(refs, &transaction->refnames, err))
> +	ret = write_with_updates(refs, &transaction->refnames, err);
> +	if (ret)
>  		goto failure;
>  
>  	transaction->state = REF_TRANSACTION_PREPARED;

Do we also want to change the local variable declaration of `int ret` to
use the new type?

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 2c1e2995de..e1fd9c2de2 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1255,11 +1255,12 @@ static int prepare_single_update(struct reftable_ref_store *refs,
>  					   "but is a regular ref"),
>  				    ref_update_original_update_refname(u),
>  				    u->old_target);
> -			return -1;
> +			return TRANSACTION_EXPECTED_SYMREF;
>  		}
>  
> -		if (ref_update_check_old_target(referent->buf, u, err)) {
> -			return -1;
> +		ret = ref_update_check_old_target(referent->buf, u, err);
> +		if (ret) {
> +			return ret;
>  		}
>  	} else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
>  		if (is_null_oid(&u->old_oid)) {

Nit: superfluous braces that we could remove while at it.

Patrick