From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sun, 13 Oct 2024 14:03:45 GMT
Subject: Re: [PATCH v7 4/6] refs: add TRANSACTION_CREATE_EXISTS error
Message-ID: <cf7c4831-a766-430c-aac6-5cd5c2ceabed@gmail.com>
In-Reply-To: <20241012230428.3259229-4-bence@ferdinandy.com>

Hi Bence

On 13/10/2024 00:03, Bence Ferdinandy wrote:
> Currently there is only one special error for transaction, for when
> there is a naming conflict, all other errors are dumped under a generic
> error. Add a new special error case for when the caller requests the
> reference to be updated only when it does not yet exist and the
> reference actually does exist.

This looks like useful improvement. Are the changes to 
reftable-backend.c correct - it looks like where it previously returned 
TRANSACTION_GENERIC_ERR it now returns TRANSACTION_NAME_CONFLICT which I 
think is used to indicate a file/directory conflict (e.g. trying to 
create refs/heads/topic/one when refs/heads/topic exists)

Best Wishes

Phillip

> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
> ---
> 
> Notes:
>      v4: new patch
>      v5: no change
>      v6: no change
>      
>      v7: - change commit prefix to be more in line with project standards
>          - changed error checking to Karthik's suggestion
> 
>   refs.h                  |  4 +++-
>   refs/files-backend.c    | 24 ++++++++++++++++--------
>   refs/reftable-backend.c |  6 ++++--
>   3 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/refs.h b/refs.h
> index b09a3a4384..c83b1ec76e 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -759,8 +759,10 @@ int ref_transaction_verify(struct ref_transaction *transaction,
>   
>   /* Naming conflict (for example, the ref names A and A/B conflict). */
>   #define TRANSACTION_NAME_CONFLICT -1
> +/* When only creation was requested, but the ref already exists. */
> +#define TRANSACTION_CREATE_EXISTS -2
>   /* All other errors. */
> -#define TRANSACTION_GENERIC_ERROR -2
> +#define TRANSACTION_GENERIC_ERROR -3
>   
>   /*
>    * Perform the preparatory stages of committing `transaction`. Acquire
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 0824c0b8a9..e743ec44b5 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2502,14 +2502,18 @@ static int split_symref_update(struct ref_update *update,
>   static int check_old_oid(struct ref_update *update, struct object_id *oid,
>   			 struct strbuf *err)
>   {
> +	int ret = TRANSACTION_GENERIC_ERROR;
> +
>   	if (!(update->flags & REF_HAVE_OLD) ||
>   		   oideq(oid, &update->old_oid))
>   		return 0;
>   
> -	if (is_null_oid(&update->old_oid))
> +	if (is_null_oid(&update->old_oid)) {
>   		strbuf_addf(err, "cannot lock ref '%s': "
>   			    "reference already exists",
>   			    ref_update_original_update_refname(update));
> +		ret = TRANSACTION_CREATE_EXISTS;
> +	}
>   	else if (is_null_oid(oid))
>   		strbuf_addf(err, "cannot lock ref '%s': "
>   			    "reference is missing but expected %s",
> @@ -2522,7 +2526,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid,
>   			    oid_to_hex(oid),
>   			    oid_to_hex(&update->old_oid));
>   
> -	return -1;
> +	return ret;
>   }
>   
>   /*
> @@ -2602,9 +2606,11 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>   					ret = TRANSACTION_GENERIC_ERROR;
>   					goto out;
>   				}
> -			} else if  (check_old_oid(update, &lock->old_oid, err)) {
> -				ret = TRANSACTION_GENERIC_ERROR;
> -				goto out;
> +			} else {
> +				ret = check_old_oid(update, &lock->old_oid, err);
> +				if  (ret) {
> +					goto out;
> +				}
>   			}
>   		} else {
>   			/*
> @@ -2635,9 +2641,11 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>   				    update->old_target);
>   			ret = TRANSACTION_GENERIC_ERROR;
>   			goto out;
> -		} else if  (check_old_oid(update, &lock->old_oid, err)) {
> -			ret = TRANSACTION_GENERIC_ERROR;
> -			goto out;
> +		} else {
> +			ret = check_old_oid(update, &lock->old_oid, err);
> +			if  (ret) {
> +				goto out;
> +			}
>   		}
>   
>   		/*
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 3c96fbf66f..ebf8e57fbc 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1206,10 +1206,13 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>   				goto done;
>   			}
>   		} else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
> -			if (is_null_oid(&u->old_oid))
> +			ret = TRANSACTION_NAME_CONFLICT;
> +			if (is_null_oid(&u->old_oid)) {
>   				strbuf_addf(err, _("cannot lock ref '%s': "
>   						   "reference already exists"),
>   					    ref_update_original_update_refname(u));
> +				ret = TRANSACTION_CREATE_EXISTS;
> +			}
>   			else if (is_null_oid(&current_oid))
>   				strbuf_addf(err, _("cannot lock ref '%s': "
>   						   "reference is missing but expected %s"),
> @@ -1221,7 +1224,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>   					    ref_update_original_update_refname(u),
>   					    oid_to_hex(&current_oid),
>   					    oid_to_hex(&u->old_oid));
> -			ret = -1;
>   			goto done;
>   		}
>