Patrick Steinhardt [off-list ref] writes:

On Fri, Jun 07, 2024 at 03:32:59PM +0200, Karthik Nayak wrote:

quoted

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