[PATCH v3 4/8] update-ref: support symrefs in the verify command

[Karthik Nayak <karthik.188@gmail.com>]

From: Karthik Nayak <karthik.188@gmail.com>

In the previous commits, we added the required base for adding symref
support to the transaction commands provided by 'git-update-ref(1)'.
Using them, extend the 'verify' command to support symrefs.

The 'verify' command allows users to verify if a provided symbolic
reference `<ref>` contains the provided `<old-oid>` without changing the
`<ref>`. Now we alternatively allow users to provide a
`ref:<old-target>` instead to verify if a symref targets the provided
target. Since we're checking for symbolic refs, this will only work with
the 'no-deref' mode. This is because any dereferenced symbolic ref will
point to an object and not a ref.

Add and use `null_new_value`, a helper function which is used to check
if there is a new_value in a reference update. The new value could
either be a symref target `new_target` or a OID `new_oid`. We also add
tests to test the command in both the regular stdin mode and also with
the '-z' flag.

We also disable the reference-transaction hook for symref-updates which
will be tackled in its own commit.

Add required tests for symref support in 'verify' while also adding
reflog checks for the pre-existing 'verify' tests.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt | 13 ++++--
 builtin/update-ref.c             | 14 ++++--
 refs.c                           | 30 ++++++++++--
 refs.h                           |  1 +
 refs/files-backend.c             | 43 +++++++++++++++++
 refs/refs-internal.h             |  7 +++
 refs/reftable-backend.c          | 21 ++++++++-
 t/t1400-update-ref.sh            | 79 +++++++++++++++++++++++++++++++-
 8 files changed, 194 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 374a2ebd2b..9f8c059944 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -64,7 +64,7 @@ performs all modifications together.  Specify commands of the form:
 	update SP <ref> SP <new-oid> [SP <old-oid>] LF
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
-	verify SP <ref> [SP <old-oid>] LF
+	verify SP <ref> [SP (<old-oid> | ref:<old-target>)] LF
 	option SP <opt> LF
 	start LF
 	prepare LF
@@ -85,7 +85,7 @@ quoting:
 	update SP <ref> NUL <new-oid> NUL [<old-oid>] NUL
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
-	verify SP <ref> NUL [<old-oid>] NUL
+	verify SP <ref> NUL [(<old-oid> | ref:<old-target>)] NUL
 	option SP <opt> NUL
 	start NUL
 	prepare NUL
@@ -95,6 +95,12 @@ quoting:
 In this format, use 40 "0" to specify a zero value, and use the empty
 string to specify a missing value.
 
+For commands which support it, substituting the <old-oid> value with
+ref:<old-target> will ensure that the <ref> targets the specified
+old-target before the update.  Similarly, substituting the <new-oid>
+with ref:<new-target> will ensure that the <ref> is a symbolic ref
+targeting the new-target after the update.
+
 In either format, values can be specified in any form that Git
 recognizes as an object name.  Commands in any other format or a
 repeated <ref> produce an error.  Command meanings are:
@@ -115,7 +121,8 @@ delete::
 
 verify::
 	Verify <ref> against <old-oid> but do not change it.  If
-	<old-oid> is zero or missing, the ref must not exist.
+	<old-oid> is zero or missing, the ref must not exist. For
+	verifying symbolic refs, provide ref:<old-target>.
 
 option::
 	Modify the behavior of the next command naming a <ref>.
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 98ec356394..246167e835 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -308,6 +308,7 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
+	struct strbuf old_target = STRBUF_INIT;
 	char *refname;
 	struct object_id old_oid;
 
@@ -315,20 +316,27 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
 	if (!refname)
 		die("verify: missing <ref>");
 
-	if (parse_next_arg(&next, end, &old_oid, NULL,
-			   "verify", refname, PARSE_SHA1_OLD))
+	if (parse_next_arg(&next, end, &old_oid, &old_target,
+			   "verify", refname,
+			   PARSE_SHA1_OLD | PARSE_REFNAME_TARGETS))
 		oidclr(&old_oid);
 
+	if (old_target.len && !(update_flags & REF_NO_DEREF))
+		die("verify %s: cannot operate on symrefs in deref mode", refname);
+
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	if (ref_transaction_verify(transaction, refname, &old_oid,
+	if (ref_transaction_verify(transaction, refname,
+				   old_target.len ? NULL : &old_oid,
+				   old_target.len ? old_target.buf : NULL,
 				   update_flags, &err))
 		die("%s", err.buf);
 
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
+	strbuf_release(&old_target);
 }
 
 static void report_ok(const char *command)
diff --git a/refs.c b/refs.c
index 060a31616d..0e1013b5ab 100644
--- a/refs.c
+++ b/refs.c
@@ -1217,6 +1217,8 @@ void ref_transaction_free(struct ref_transaction *transaction)
 
 	for (i = 0; i < transaction->nr; i++) {
 		free(transaction->updates[i]->msg);
+		free((void *)transaction->updates[i]->old_target);
+		free((void *)transaction->updates[i]->new_target);
 		free(transaction->updates[i]);
 	}
 	free(transaction->updates);
@@ -1247,9 +1249,13 @@ struct ref_update *ref_transaction_add_update(
 
 	update->flags = flags;
 
-	if (flags & REF_HAVE_NEW)
+	if (new_target)
+		update->new_target = xstrdup(new_target);
+	if (old_target)
+		update->old_target = xstrdup(old_target);
+	if (new_oid && flags & REF_HAVE_NEW)
 		oidcpy(&update->new_oid, new_oid);
-	if (flags & REF_HAVE_OLD)
+	if (old_oid && flags & REF_HAVE_OLD)
 		oidcpy(&update->old_oid, old_oid);
 	update->msg = normalize_reflog_message(msg);
 	return update;
@@ -1286,6 +1292,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
 
 	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
+	flags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0);
 
 	ref_transaction_add_update(transaction, refname, flags,
 				   new_oid, old_oid, new_target,
@@ -1325,14 +1332,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 int ref_transaction_verify(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
+			   const char *old_target,
 			   unsigned int flags,
 			   struct strbuf *err)
 {
-	if (!old_oid)
-		BUG("verify called with old_oid set to NULL");
+	if (!old_target && !old_oid)
+		BUG("verify called with old_oid and old_target set to NULL");
+	if (old_target && !(flags & REF_NO_DEREF))
+		BUG("verify cannot operate on symrefs with deref mode");
 	return ref_transaction_update(transaction, refname,
 				      NULL, old_oid,
-				      NULL, NULL,
+				      NULL, old_target,
 				      flags, NULL, err);
 }
 
@@ -2349,6 +2359,12 @@ static int run_transaction_hook(struct ref_transaction *transaction,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 
+		/*
+		 * Skip reference transaction for symbolic refs.
+		 */
+		if (update->new_target || update->old_target)
+			continue;
+
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "%s %s %s\n",
 			    oid_to_hex(&update->old_oid),
@@ -2802,3 +2818,7 @@ int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg
 {
 	return refs_copy_existing_ref(get_main_ref_store(the_repository), oldref, newref, logmsg);
 }
+
+int ref_update_is_null_new_value(struct ref_update *update) {
+	return !update->new_target && is_null_oid(&update->new_oid);
+}
diff --git a/refs.h b/refs.h
index c792e13a64..27b9aeaf54 100644
--- a/refs.h
+++ b/refs.h
@@ -780,6 +780,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 int ref_transaction_verify(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
+			   const char *old_target,
 			   unsigned int flags,
 			   struct strbuf *err);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2420dac2aa..53197fa3af 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2425,6 +2425,37 @@ static const char *original_update_refname(struct ref_update *update)
 	return update->refname;
 }
 
+/*
+ * Check whether the REF_HAVE_OLD and old_target values stored in
+ * update are consistent with ref, which is the symbolic reference's
+ * current value. If everything is OK, return 0; otherwise, write an
+ * error message to err and return -1.
+ */
+static int check_old_target(struct ref_update *update, char *ref,
+			    struct strbuf *err)
+{
+	if (!(update->flags & REF_HAVE_OLD) ||
+	    !strcmp(update->old_target, ref))
+		return 0;
+
+	if (!strcmp(update->old_target, ""))
+		strbuf_addf(err, "cannot lock ref '%s': "
+			    "reference already exists",
+			    original_update_refname(update));
+	else if (!strcmp(ref, ""))
+		strbuf_addf(err, "cannot lock ref '%s': "
+			    "reference is missing but expected %s",
+			    original_update_refname(update),
+			    update->old_target);
+	else
+		strbuf_addf(err, "cannot lock ref '%s': "
+			    "is at %s but expected %s",
+			    original_update_refname(update),
+			    ref, update->old_target);
+
+	return -1;
+}
+
 /*
  * Check whether the REF_HAVE_OLD and old_oid values stored in update
  * are consistent with oid, which is the reference's current value. If
@@ -2528,6 +2559,18 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 					ret = TRANSACTION_GENERIC_ERROR;
 					goto out;
 				}
+			}
+
+			/*
+			 * For symref verification, we need to check the reference value
+			 * rather than the oid. If we're dealing with regular refs or we're
+			 * verifying a dereferenced symref, we then check the oid.
+			 */
+			if (update->old_target) {
+				if (check_old_target(update, referent.buf, err)) {
+					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/refs-internal.h b/refs/refs-internal.h
index 3040d4797c..23e65f65e8 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -748,4 +748,11 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
  */
 struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
 
+/*
+ * Helper function to check if the new value is null, this
+ * takes into consideration that the update could be a regular
+ * ref or a symbolic ref.
+ */
+int ref_update_is_null_new_value(struct ref_update *update);
+
 #endif /* REFS_REFS_INTERNAL_H */
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 6104471199..a2474245aa 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -938,7 +938,26 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 		 * individual refs. But the error messages match what the files
 		 * backend returns, which keeps our tests happy.
 		 */
-		if (u->flags & REF_HAVE_OLD && !oideq(&current_oid, &u->old_oid)) {
+		if ((u->flags & REF_HAVE_OLD) && u->old_target) {
+			if (strcmp(referent.buf, u->old_target)) {
+				if (!strcmp(u->old_target, ""))
+					strbuf_addf(err, "verifying symref target: '%s': "
+						    "provided target is empty",
+						    original_update_refname(u));
+				else if (!strcmp(referent.buf, ""))
+					strbuf_addf(err, "verifying symref target: '%s': "
+						    "reference is missing but expected %s",
+						    original_update_refname(u),
+						    u->old_target);
+				else
+					strbuf_addf(err, "verifying symref target: '%s': "
+						    "is at %s but expected %s",
+						    original_update_refname(u),
+						    referent.buf, u->old_target);
+				ret = -1;
+				goto done;
+			}
+		} else if (u->flags & REF_HAVE_OLD && !oideq(&current_oid, &u->old_oid)) {
 			if (is_null_oid(&u->old_oid))
 				strbuf_addf(err, _("cannot lock ref '%s': "
 					    "reference already exists"),
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ec3443cc87..1f2b63755a 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -890,17 +890,23 @@ test_expect_success 'stdin update/create/verify combination works' '
 '
 
 test_expect_success 'stdin verify succeeds for correct value' '
+	test-tool ref-store main for-each-reflog-ent $m >before &&
 	git rev-parse $m >expect &&
 	echo "verify $m $m" >stdin &&
 	git update-ref --stdin <stdin &&
 	git rev-parse $m >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test-tool ref-store main for-each-reflog-ent $m >after &&
+	test_cmp before after
 '
 
 test_expect_success 'stdin verify succeeds for missing reference' '
+	test-tool ref-store main for-each-reflog-ent $m >before &&
 	echo "verify refs/heads/missing $Z" >stdin &&
 	git update-ref --stdin <stdin &&
-	test_must_fail git rev-parse --verify -q refs/heads/missing
+	test_must_fail git rev-parse --verify -q refs/heads/missing &&
+	test-tool ref-store main for-each-reflog-ent $m >after &&
+	test_cmp before after
 '
 
 test_expect_success 'stdin verify treats no value as missing' '
@@ -1641,4 +1647,73 @@ test_expect_success PIPE 'transaction flushes status updates' '
 	test_cmp expected actual
 '
 
+create_stdin_buf () {
+	if test "$1" = "-z"
+	then
+		shift
+		printf "$F" "$@" >stdin
+	else
+		echo "$@" >stdin
+	fi
+}
+
+for type in "" "-z"
+do
+
+	test_expect_success "stdin ${type} verify symref fails without --no-deref" '
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "verify refs/heads/symref" "ref:$a" &&
+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+		grep "fatal: verify refs/heads/symref: cannot operate on symrefs in deref mode" err
+	'
+
+	test_expect_success "stdin ${type} verify symref fails with too many arguments" '
+		create_stdin_buf ${type} "verify refs/heads/symref" "ref:$a" "ref:$a" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err  &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: ref:$a" err
+		else
+			grep "fatal: verify refs/heads/symref: extra input:  ref:$a" err
+		fi
+	'
+
+	test_expect_success "stdin ${type} verify symref succeeds for correct value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+		create_stdin_buf ${type} "verify refs/heads/symref" "ref:$a" &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+		test_cmp before after
+	'
+
+	test_expect_success "stdin ${type} verify symref succeeds for missing reference" '
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+		create_stdin_buf ${type} "verify refs/heads/missing" "$Z" &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		test_must_fail git rev-parse --verify -q refs/heads/missing &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+		test_cmp before after
+	'
+
+	test_expect_success "stdin ${type} verify symref fails for wrong value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		create_stdin_buf ${type} "verify refs/heads/symref" "$b" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} verify symref fails for mistaken null value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		create_stdin_buf ${type} "verify refs/heads/symref" "$Z" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+done
+
 test_done
-- 
2.43.GIT