diff mbox series

[v5,2/7] refs: specify error for regular refs with `old_target`

Message ID 20240607133304.2333280-3-knayak@gitlab.com (mailing list archive)
State Accepted
Commit aa6e99f1226fe46f1649f48d020011e19556e8e1
Headers show
Series update-ref: add symref support for --stdin | expand

Commit Message

karthik nayak June 7, 2024, 1:32 p.m. UTC 
From: Karthik Nayak <karthik.188@gmail.com>

When a reference update tries to update a symref, but the ref in
question is actually a regular ref, we raise an error. However the error
raised in this situation is:

  verifying symref target: '<ref>': reference is missing but expected <old-target>

which is very generic and doesn't indicate the mismatch of types. Let's
make this error more specific:

  cannot lock ref '<ref>': expected symref with target '<old-target>': but is a regular ref

so that users have a clearer understanding.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c    | 14 ++++++++------
 refs/reftable-backend.c | 10 ++++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

Comments

Patrick Steinhardt June 10, 2024, 6:54 a.m. UTC | #1 
On Fri, Jun 07, 2024 at 03:32:59PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 194e74eb4d..fc57c9d220 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2491,14 +2491,16 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  
>  		/*
>  		 * Even if the ref is a regular ref, if `old_target` is set, we
> -		 * check the referent value. Ideally `old_target` should only
> -		 * be set for symrefs, but we're strict about its usage.
> +		 * fail with an error.
>  		 */
>  		if (update->old_target) {
> -			if (ref_update_check_old_target(referent.buf, update, err)) {
> -				ret = TRANSACTION_GENERIC_ERROR;
> -				goto out;
> -			}
> +			strbuf_addf(err, _("cannot lock ref '%s': "
> +					   "expected symref with target '%s': "
> +					   "but is a regular ref"),
> +				    ref_update_original_update_refname(update),
> +				    update->old_target);
> +			ret = TRANSACTION_GENERIC_ERROR;
> +			goto out;

Shouldn't the second colon be a comma? "expected symref, but is a
regular ref" reads way more natural to me than "expected symref: but is
a regular ref".

Other than that this series looks good to me now, thanks!

Patrick
karthik nayak June 10, 2024, 8:26 a.m. UTC | #2 
Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Jun 07, 2024 at 03:32:59PM +0200, Karthik Nayak wrote:
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 194e74eb4d..fc57c9d220 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2491,14 +2491,16 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>
>>  		/*
>>  		 * Even if the ref is a regular ref, if `old_target` is set, we
>> -		 * check the referent value. Ideally `old_target` should only
>> -		 * be set for symrefs, but we're strict about its usage.
>> +		 * fail with an error.
>>  		 */
>>  		if (update->old_target) {
>> -			if (ref_update_check_old_target(referent.buf, update, err)) {
>> -				ret = TRANSACTION_GENERIC_ERROR;
>> -				goto out;
>> -			}
>> +			strbuf_addf(err, _("cannot lock ref '%s': "
>> +					   "expected symref with target '%s': "
>> +					   "but is a regular ref"),
>> +				    ref_update_original_update_refname(update),
>> +				    update->old_target);
>> +			ret = TRANSACTION_GENERIC_ERROR;
>> +			goto out;
>
> Shouldn't the second colon be a comma? "expected symref, but is a
> regular ref" reads way more natural to me than "expected symref: but is
> a regular ref".
>

It makes sense the way you put it, but we also print the 'target', so it
is something like "cannot lock ref 'refs/heads/branch1': expected symref with
target 'refs/heads/master': but is a regular ref". I felt here the colon
divides the error into three segments
1. States that we couldn't lock the file
2. States that we were expecting a symref with a particular target
3. States that the ref in question is actually a regular ref

Having said that, I'm not too bullish on this and happy to change it :)

> Other than that this series looks good to me now, thanks!
>
> Patrick
>

Thanks for all the reviews. Since this is the only change, I'm inclined
to leave it as is.

Karthik
diff mbox series

Patch

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 194e74eb4d..fc57c9d220 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2491,14 +2491,16 @@  static int lock_ref_for_update(struct files_ref_store *refs,
 
 		/*
 		 * Even if the ref is a regular ref, if `old_target` is set, we
-		 * check the referent value. Ideally `old_target` should only
-		 * be set for symrefs, but we're strict about its usage.
+		 * fail with an error.
 		 */
 		if (update->old_target) {
-			if (ref_update_check_old_target(referent.buf, update, err)) {
-				ret = TRANSACTION_GENERIC_ERROR;
-				goto out;
-			}
+			strbuf_addf(err, _("cannot lock ref '%s': "
+					   "expected symref with target '%s': "
+					   "but is a regular ref"),
+				    ref_update_original_update_refname(update),
+				    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;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index b838cf8f00..c66ab9ecd8 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -928,6 +928,16 @@  static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 		 * backend returns, which keeps our tests happy.
 		 */
 		if (u->old_target) {
+			if (!(u->type & REF_ISSYMREF)) {
+				strbuf_addf(err, _("cannot lock ref '%s': "
+					   "expected symref with target '%s': "
+					   "but is a regular ref"),
+					    ref_update_original_update_refname(u),
+					    u->old_target);
+				ret = -1;
+				goto done;
+			}
+
 			if (ref_update_check_old_target(referent.buf, u, err)) {
 				ret = -1;
 				goto done;