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

[Junio C Hamano <gitster@xxxxxxxxx>]

Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes:

>  /* 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

My initial knee-jerk reaction to the name "CREATE_EXISTS" was that
the "CREATE" part is redundant, as the only case that the fact that
something exists is a valid reason for an error is when we attempted
to create it.  EEXIST in errno(3) is not ECREATE_EXIST for the same
reason.

But let's let it pass; contrasting with the fact that what ought to
be named similarly to EISDIR is called NAME_CONFLICT, CREATE_EXISTS
is fine.

> 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;
>  }

OK.  So if a caller does not care which exact kind of error it got,
they will keep working just the way they used to work, and a caller
that cares can now check.  A nice API enhancement.

> @@ -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;
> +				}
>  			}

Likewise; this code is just propagating what check_old_oid() did
to its callers.

> @@ -2636,9 +2644,13 @@ 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 {
> +			int checkret;
> +			checkret = check_old_oid(update, &lock->old_oid, err);
> +			if  (checkret) {
> +				ret = checkret;
> +				goto out;
> +			}

Ditto.

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 32330b6bc6..c6b25ebac4 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;
>  		}

So it remains a little mystery how the ultimate caller uses this new
bit of information.

Which is OK---the next step seems to have a new condition on the
error status.