Re: [PATCH v6 4/6] transaction: add TRANSACTION_CREATE_EXISTS error

From: karthik nayak <karthik.188@gmail.com>
Date: Oct 10, 2024, 21:12 UTC
Message-ID: <CAOLa=ZQA6vc246LRxW8n+a5qNDGxsSLUAhT-x-9tdAKDb2LBCQ@mail.gmail.com>
In-Reply-To: <20241010133022.1733542-4-bence@ferdinandy.com>

Bence Ferdinandy <bence@ferdinandy.com> writes:

> transaction: add TRANSACTION_CREATE_EXISTS error

Nit: but it would be nice to have s/transaction/refs here and similarly
in other patches. Phrasing 'Documentation/SubmittingPatches':

  It is also conventional in most cases to prefix the first line with
  "area: " where the area is a filename or identifier for the general
  area of the code being modified, e.g.

    * doc: clarify distinction between sign-off and pgp-signing
    * githooks.txt: improve the intro section

So in this sense, it would be the refs area, no?

Show 82 quoted lines

> 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.
>
> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
> ---
>
> Notes:
>     v4: new patch
>     v5: no change
>     v6: no change
>
>  refs.h                  |  4 +++-
>  refs/files-backend.c    | 28 ++++++++++++++++++++--------
>  refs/reftable-backend.c |  6 ++++--
>  3 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/refs.h b/refs.h
> index f38616db84..166affbc89 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 8415f2d020..272ad81315 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;
>  }
>
>  /*
> @@ -2603,9 +2607,13 @@ 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 {
> +				int checkret;
> +				checkret = check_old_oid(update, &lock->old_oid, err);
> +				if  (checkret) {
> +					ret = checkret;
> +					goto out;
> +				}

Can't we simply do:

  ret = check_old_oid(update, &lock->old_oid, err);
  if (ret) {
     goto out
  }

if ret is '0', it shouldn't matter no?

[snip]