[PATCH 0/6] update-ref: add symref support for --stdin

[PATCH 0/6] update-ref: add symref support for --stdin

[Karthik Nayak]

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

The 'update-ref' command is used to update refs using transactions. The
command allows users to also utilize a '--stdin' mode to provide a
batch of sub-commands which can be processed in a transaction.
      
Currently, the sub-commands involve {verify, delete, create, update}
and they allow users to work with regular refs in the repository. To
work with symrefs, users only have the option of using
'git-symbolic-ref', which doesn't provide transaction support to the
users eventhough it uses the same behind the hood. 

Recently, we modified the reference backend to add symref support,
following which, 'git-symbolic-ref' also uses the transaction backend.
But, it doesn't expose this to the user. To allow users to work with
symrefs via transaction, this series adds support for new sub-commands
{symrer-verify, symref-delete, symref-create, symref-update} to the
'--stdin' mode of update-ref. These complement the existing
sub-commands.

The patches 1, 5 fix small issues in the reference backends. The other
patches 2, 3, 4 & 6, each add one of the new sub-commands.

The series is based off master, with 'kn/ref-transaction-symref' merged
in. 

Karthik Nayak (6):
  refs: create and use `ref_update_ref_must_exist()`
  update-ref: add support for 'symref-verify' command
  update-ref: add support for 'symref-delete' command
  update-ref: add support for 'symref-create' command
  reftable: pick either 'oid' or 'target' for new updates
  update-ref: add support for 'symref-update' command

 Documentation/git-update-ref.txt |  25 ++
 builtin/clone.c                  |   2 +-
 builtin/fetch.c                  |   2 +-
 builtin/receive-pack.c           |   3 +-
 builtin/update-ref.c             | 230 +++++++++++++++++-
 refs.c                           |  34 ++-
 refs.h                           |   6 +-
 refs/files-backend.c             |   3 +-
 refs/refs-internal.h             |   6 +
 refs/reftable-backend.c          |   7 +-
 t/t0600-reffiles-backend.sh      |  32 +++
 t/t1400-update-ref.sh            | 404 ++++++++++++++++++++++++++++++-
 t/t1416-ref-transaction-hooks.sh |  54 +++++
 13 files changed, 775 insertions(+), 33 deletions(-)

-- 
2.43.GIT

[PATCH 1/6] refs: create and use `ref_update_ref_must_exist()`

[Karthik Nayak]

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

The files and reftable backend, need to check if a ref must exist, so
that the required validation can be done. A ref must exist only when the
`old_oid` value of the update has been explicitly set and it is not the
`null_oid` value.

Since we also support symrefs now, we need to ensure that even when
`old_target` is set a ref must exist. While this was missed when we
added symref support in transactions, there are no active users of this
path. As we introduce the 'symref-verify' command in the upcoming
commits, it is important to fix this.

So let's export this to a function called `ref_update_ref_must_exist()`
and expose it internally via 'refs-internal.h'.

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

diff --git a/refs.c b/refs.c
index fa5471d219..59858fafdb 100644
--- a/refs.c
+++ b/refs.c
@@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
 			    referent, update->old_target);
 	return -1;
 }
+
+int ref_update_ref_must_exist(struct ref_update *update)
+{
+	return (update->flags & REF_HAVE_OLD) &&
+		(!is_null_oid(&update->old_oid) || update->old_target);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3957bfa579..2df204f891 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 			       struct strbuf *err)
 {
 	struct strbuf referent = STRBUF_INIT;
-	int mustexist = (update->flags & REF_HAVE_OLD) &&
-		!is_null_oid(&update->old_oid);
+	int mustexist = ref_update_ref_must_exist(update);
 	int ret = 0;
 	struct ref_lock *lock;
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 53a6c5d842..5da3029e6c 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
 int ref_update_check_old_target(const char *referent, struct ref_update *update,
 				struct strbuf *err);
 
+/*
+ * Check if the ref must exist, this means that the old_oid or
+ * old_target is non NULL.
+ */
+int ref_update_ref_must_exist(struct ref_update *update);
+
 #endif /* REFS_REFS_INTERNAL_H */
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 98cebbcf39..975061d103 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -825,7 +825,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 					      &current_oid, &referent, &u->type);
 		if (ret < 0)
 			goto done;
-		if (ret > 0 && (!(u->flags & REF_HAVE_OLD) || is_null_oid(&u->old_oid))) {
+		if (ret > 0 && !ref_update_ref_must_exist(u)) {
 			/*
 			 * The reference does not exist, and we either have no
 			 * old object ID or expect the reference to not exist.
-- 
2.43.GIT

[PATCH 2/6] update-ref: add support for 'symref-verify' command

[Karthik Nayak]

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

The 'symref-verify' command allows users to verify if a provided <ref>
contains the provided <old-target> without changing the <ref>. If
<old-target> is not provided, the command will verify that the <ref>
doesn't exist.

The command allows users to verify symbolic refs within a transaction,
and this means users can perform a set of changes in a transaction only
when the verification holds good.

Since we're checking for symbolic refs, this command will only work with
the 'no-deref' mode. This is because any dereferenced symbolic ref will
point to an object and not a ref and the regular 'verify' command can be
used in such situations.

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 |  7 +++
 builtin/update-ref.c             | 80 +++++++++++++++++++++++----
 refs.c                           |  9 ++-
 refs.h                           |  1 +
 t/t1400-update-ref.sh            | 94 +++++++++++++++++++++++++++++++-
 t/t1416-ref-transaction-hooks.sh | 30 ++++++++++
 6 files changed, 206 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 374a2ebd2b..9fe78b3501 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-verify SP <ref> [SP <old-target>] LF
 	option SP <opt> LF
 	start LF
 	prepare LF
@@ -86,6 +87,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-verify SP <ref> [NUL <old-target>] NUL
 	option SP <opt> NUL
 	start NUL
 	prepare NUL
@@ -117,6 +119,11 @@ verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-verify::
+	Verify symbolic <ref> against <old-target> but do not change it.
+	If <old-target> is missing, the ref must not exist.  Can only be
+	used in `no-deref` mode.
+
 option::
 	Modify the behavior of the next command naming a <ref>.
 	The only valid option is `no-deref` to avoid dereferencing
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 21fdbf6ac8..6dce1cd663 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -76,6 +76,29 @@ static char *parse_refname(const char **next)
 	return strbuf_detach(&ref, NULL);
 }
 
+/*
+ * Wrapper around parse_refname which skips the next delimiter.
+ */
+static char *parse_next_refname(const char **next)
+{
+	if (line_termination) {
+		/* Without -z, consume SP and use next argument */
+		if (!**next || **next == line_termination)
+			return NULL;
+		if (**next != ' ')
+			die("expected SP but got: %s", *next);
+	} else {
+		/* With -z, read the next NUL-terminated line */
+		if (**next)
+			return NULL;
+	}
+	/* Skip the delimiter */
+	(*next)++;
+
+	return parse_refname(next);
+}
+
+
 /*
  * The value being parsed is <old-oid> (as opposed to <new-oid>; the
  * difference affects which error messages are generated):
@@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
 		die("verify %s: extra input: %s", refname, next);
 
 	if (ref_transaction_verify(transaction, refname, &old_oid,
-				   update_flags, &err))
+				   NULL, update_flags, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	strbuf_release(&err);
+}
+
+static void parse_cmd_symref_verify(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	struct strbuf err = STRBUF_INIT;
+	struct object_id old_oid;
+	char *refname, *old_target;
+
+	if (!(update_flags & REF_NO_DEREF))
+		die("symref-verify: cannot operate with deref mode");
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-verify: missing <ref>");
+
+	/*
+	 * old_ref is optional, if not provided, we need to ensure that the
+	 * ref doesn't exist.
+	 */
+	old_target = parse_next_refname(&next);
+	if (!old_target)
+		oidcpy(&old_oid, null_oid());
+
+	if (*next != line_termination)
+		die("symref-verify %s: extra input: %s", refname, next);
+
+	if (ref_transaction_verify(transaction, refname,
+				   old_target ? NULL : &old_oid,
+				   old_target, update_flags, &err))
 		die("%s", err.buf);
 
 	update_flags = default_flags;
 	free(refname);
+	free(old_target);
 	strbuf_release(&err);
 }
 
@@ -380,15 +439,16 @@ static const struct parse_cmd {
 	unsigned args;
 	enum update_refs_state state;
 } command[] = {
-	{ "update",  parse_cmd_update,  3, UPDATE_REFS_OPEN },
-	{ "create",  parse_cmd_create,  2, UPDATE_REFS_OPEN },
-	{ "delete",  parse_cmd_delete,  2, UPDATE_REFS_OPEN },
-	{ "verify",  parse_cmd_verify,  2, UPDATE_REFS_OPEN },
-	{ "option",  parse_cmd_option,  1, UPDATE_REFS_OPEN },
-	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
-	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
-	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
-	{ "commit",  parse_cmd_commit,  0, UPDATE_REFS_CLOSED },
+	{ "update",        parse_cmd_update,        3, UPDATE_REFS_OPEN },
+	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
+	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
+	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
+	{ "option",        parse_cmd_option,        1, UPDATE_REFS_OPEN },
+	{ "start",         parse_cmd_start,         0, UPDATE_REFS_STARTED },
+	{ "prepare",       parse_cmd_prepare,       0, UPDATE_REFS_PREPARED },
+	{ "abort",         parse_cmd_abort,         0, UPDATE_REFS_CLOSED },
+	{ "commit",        parse_cmd_commit,        0, UPDATE_REFS_CLOSED },
 };
 
 static void update_refs_stdin(void)
diff --git a/refs.c b/refs.c
index 59858fafdb..ee4c6ed99c 100644
--- a/refs.c
+++ b/refs.c
@@ -1331,14 +1331,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);
 }
 
diff --git a/refs.h b/refs.h
index 71cc1c58e0..48cec1ba72 100644
--- a/refs.h
+++ b/refs.h
@@ -781,6 +781,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/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ec3443cc87..8784c59a53 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,88 @@ 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} symref-verify fails without --no-deref" '
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" &&
+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+		grep "fatal: symref-verify: cannot operate with deref mode" err
+	'
+
+	test_expect_success "stdin ${type} symref-verify fails with too many arguments" '
+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" "$a" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err  &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-verify refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin ${type} symref-verify 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} "symref-verify refs/heads/symref" "$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} symref-verify no value is treated as zero value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
+	'
+
+	test_expect_success "stdin ${type} symref-verify succeeds for dangling reference" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
+		test_must_fail git symbolic-ref refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+		create_stdin_buf ${type} "symref-verify refs/heads/symref2" "refs/heads/nonexistent" &&
+		git update-ref --stdin ${type} --no-deref <stdin
+	'
+
+	test_expect_success "stdin ${type} symref-verify fails for missing reference" '
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+		create_stdin_buf ${type} "symref-verify refs/heads/missing" "refs/heads/unknown" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		grep "fatal: cannot lock ref ${SQ}refs/heads/missing${SQ}: unable to resolve reference ${SQ}refs/heads/missing${SQ}" err &&
+		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} symref-verify fails for wrong value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		create_stdin_buf ${type} "symref-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} symref-verify fails for mistaken null value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		create_stdin_buf ${type} "symref-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
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 067fd57290..fd58b902f4 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
 	test_cmp expect actual
 '
 
+test_expect_success 'hook gets all queued symref updates' '
+	test_when_finished "rm actual" &&
+
+	git update-ref refs/heads/branch $POST_OID &&
+	git symbolic-ref refs/heads/symref refs/heads/main &&
+
+	test_hook reference-transaction <<-\EOF &&
+	echo "$*" >>actual
+	while read -r line
+	do
+		printf "%s\n" "$line"
+	done >>actual
+	EOF
+
+	cat >expect <<-EOF &&
+	prepared
+	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	committed
+	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	EOF
+
+	git update-ref --no-deref --stdin <<-EOF &&
+	start
+	symref-verify refs/heads/symref refs/heads/main
+	prepare
+	commit
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.43.GIT

[PATCH 3/6] update-ref: add support for 'symref-delete' command

[Karthik Nayak]

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

Add a new command 'symref-delete' to allow deletions of symbolic refs in
a transaction via the '--stdin' mode of the 'git-update-ref' command.
The 'symref-delete' command can, when given an <old-target>, delete the
provided <ref> only when it points to <old-target>.

This command is only compatible with the 'no-deref' mode because we
optionally want to check the 'old_target' of the ref being deleted.
De-referencing a symbolic ref would provide a regular ref and we already
have the 'delete' command for regular refs.

While users can also use 'git symbolic-ref -d' to delete symbolic refs,
the 'symref-delete' command in 'git-update-ref' allows users to do so
within a transaction, which promises atomicity of the operation and can
be batched with other commands.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |  5 +++
 builtin/fetch.c                  |  2 +-
 builtin/receive-pack.c           |  3 +-
 builtin/update-ref.c             | 33 ++++++++++++++++++-
 refs.c                           | 12 ++++---
 refs.h                           |  4 ++-
 t/t1400-update-ref.sh            | 56 +++++++++++++++++++++++++++++++-
 t/t1416-ref-transaction-hooks.sh | 19 ++++++++++-
 8 files changed, 124 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9fe78b3501..16e02f6979 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-delete SP <ref> [SP <old-target>] LF
 	symref-verify SP <ref> [SP <old-target>] LF
 	option SP <opt> LF
 	start LF
@@ -87,6 +88,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-delete SP <ref> [NUL <old-target>] NUL
 	symref-verify SP <ref> [NUL <old-target>] NUL
 	option SP <opt> NUL
 	start NUL
@@ -119,6 +121,9 @@ verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-delete::
+	Delete <ref> after verifying it exists with <old-target>, if given.
+
 symref-verify::
 	Verify symbolic <ref> against <old-target> but do not change it.
 	If <old-target> is missing, the ref must not exist.  Can only be
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 66840b7c5b..d02592efca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1383,7 +1383,7 @@ static int prune_refs(struct display_state *display_state,
 		if (transaction) {
 			for (ref = stale_refs; ref; ref = ref->next) {
 				result = ref_transaction_delete(transaction, ref->name, NULL, 0,
-								"fetch: prune", &err);
+								NULL, "fetch: prune", &err);
 				if (result)
 					goto cleanup;
 			}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b150ef39a8..9a4667d57d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1576,7 +1576,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		if (ref_transaction_delete(transaction,
 					   namespaced_name,
 					   old_oid,
-					   0, "push", &err)) {
+					   0, NULL,
+					   "push", &err)) {
 			rp_error("%s", err.buf);
 			ret = "failed to delete";
 		} else {
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6dce1cd663..0ef9c38d8d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -293,7 +293,7 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
 
 	if (ref_transaction_delete(transaction, refname,
 				   have_old ? &old_oid : NULL,
-				   update_flags, msg, &err))
+				   update_flags, NULL, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = default_flags;
@@ -301,6 +301,36 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
 	strbuf_release(&err);
 }
 
+
+static void parse_cmd_symref_delete(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *refname, *old_target;
+
+	if (!(update_flags & REF_NO_DEREF))
+		die("symref-delete: cannot operate with deref mode");
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-delete: missing <ref>");
+
+	old_target = parse_next_refname(&next);
+
+	if (*next != line_termination)
+		die("symref-delete %s: extra input: %s", refname, next);
+
+	if (ref_transaction_delete(transaction, refname, NULL,
+				   update_flags, old_target, msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(old_target);
+	strbuf_release(&err);
+}
+
+
 static void parse_cmd_verify(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -443,6 +473,7 @@ static const struct parse_cmd {
 	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
 	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
 	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
 	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
 	{ "option",        parse_cmd_option,        1, UPDATE_REFS_OPEN },
 	{ "start",         parse_cmd_start,         0, UPDATE_REFS_STARTED },
diff --git a/refs.c b/refs.c
index ee4c6ed99c..c2c9889466 100644
--- a/refs.c
+++ b/refs.c
@@ -979,7 +979,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
 	transaction = ref_store_transaction_begin(refs, &err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_oid,
-				   flags, msg, &err) ||
+				   flags, NULL, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
@@ -1317,14 +1317,18 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int flags,
+			   const char *old_target,
+			   const char *msg,
 			   struct strbuf *err)
 {
 	if (old_oid && is_null_oid(old_oid))
 		BUG("delete called with old_oid set to zeros");
+	if (old_target && !(flags & REF_NO_DEREF))
+		BUG("delete cannot operate on symrefs with deref mode");
 	return ref_transaction_update(transaction, refname,
 				      null_oid(), old_oid,
-				      NULL, NULL, flags,
+				      NULL, old_target, flags,
 				      msg, err);
 }
 
@@ -2765,7 +2769,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,
 
 	for_each_string_list_item(item, refnames) {
 		ret = ref_transaction_delete(transaction, item->string,
-					     NULL, flags, msg, &err);
+					     NULL, flags, NULL, msg, &err);
 		if (ret) {
 			warning(_("could not delete reference %s: %s"),
 				item->string, err.buf);
diff --git a/refs.h b/refs.h
index 48cec1ba72..053c382f0b 100644
--- a/refs.h
+++ b/refs.h
@@ -767,7 +767,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int flags,
+			   const char *old_target,
+			   const char *msg,
 			   struct strbuf *err);
 
 /*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 8784c59a53..fbc6b234cb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1689,7 +1689,7 @@ do
 		test_cmp before after
 	'
 
-	test_expect_success "stdin ${type} symref-verify no value is treated as zero value" '
+	test_expect_success "stdin ${type} symref-verify fails with no value" '
 		git symbolic-ref refs/heads/symref >expect &&
 		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
 		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
@@ -1729,6 +1729,60 @@ do
 		test_cmp expect actual
 	'
 
+	test_expect_success "stdin ${type} symref-delete fails without --no-deref" '
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+		grep "fatal: symref-delete: cannot operate with deref mode" err
+	'
+
+	test_expect_success "stdin ${type} symref-delete fails with no ref" '
+		create_stdin_buf ${type} "symref-delete " &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		grep "fatal: symref-delete: missing <ref>" err
+	'
+
+	test_expect_success "stdin ${type} symref-delete fails with too many arguments" '
+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" "$a" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-delete refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin ${type} symref-delete fails with wrong old value" '
+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-delete works with right old value" '
+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		test_must_fail git rev-parse --verify -q refs/heads/symref
+	'
+
+	test_expect_success "stdin ${type} symref-delete works with empty old value" '
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "" &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		test_must_fail git rev-parse --verify -q $b
+	'
+
+	test_expect_success "stdin ${type} symref-delete succeeds for dangling reference" '
+		test_must_fail git symbolic-ref refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+		create_stdin_buf ${type} "symref-delete refs/heads/symref2" "refs/heads/nonexistent" &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		test_must_fail git symbolic-ref -d refs/heads/symref2
+	'
+
 done
 
 test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index fd58b902f4..ccde1b944b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -162,6 +162,7 @@ test_expect_success 'hook gets all queued symref updates' '
 
 	git update-ref refs/heads/branch $POST_OID &&
 	git symbolic-ref refs/heads/symref refs/heads/main &&
+	git symbolic-ref refs/heads/symrefd refs/heads/main &&
 
 	test_hook reference-transaction <<-\EOF &&
 	echo "$*" >>actual
@@ -171,16 +172,32 @@ test_expect_success 'hook gets all queued symref updates' '
 	done >>actual
 	EOF
 
-	cat >expect <<-EOF &&
+	# In the files backend, "delete" also triggers an additional transaction
+	# update on the packed-refs backend, which constitutes additional reflog
+	# entries.
+	if test_have_prereq REFFILES
+	then
+		cat >expect <<-EOF
+		aborted
+		$ZERO_OID $ZERO_OID refs/heads/symrefd
+		EOF
+	else
+		>expect
+	fi &&
+
+	cat >>expect <<-EOF &&
 	prepared
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	committed
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	EOF
 
 	git update-ref --no-deref --stdin <<-EOF &&
 	start
 	symref-verify refs/heads/symref refs/heads/main
+	symref-delete refs/heads/symrefd refs/heads/main
 	prepare
 	commit
 	EOF
-- 
2.43.GIT

[PATCH 4/6] update-ref: add support for 'symref-create' command

[Karthik Nayak]

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

Add 'symref-create' command to the '--stdin' mode 'git-update-ref' to
allow creation of symbolic refs in a transaction. The 'symref-create'
command takes in a <new-target>, which the created <ref> will point to.

Also, support the 'core.prefersymlinkrefs' config, wherein if the config
is set and the filesystem supports symlinks, we create the symbolic ref
as a symlink. We fallback to creating a regular symref if creating the
symlink is unsuccessful.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |  6 +++
 builtin/clone.c                  |  2 +-
 builtin/update-ref.c             | 32 +++++++++++++++-
 refs.c                           |  7 ++--
 refs.h                           |  1 +
 t/t0600-reffiles-backend.sh      | 32 ++++++++++++++++
 t/t1400-update-ref.sh            | 65 ++++++++++++++++++++++++++++++++
 t/t1416-ref-transaction-hooks.sh |  3 ++
 8 files changed, 143 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 16e02f6979..364ef78af1 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-create SP <ref> SP <new-target> LF
 	symref-delete SP <ref> [SP <old-target>] LF
 	symref-verify SP <ref> [SP <old-target>] LF
 	option SP <opt> LF
@@ -88,6 +89,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-create SP <ref> NUL <new-target> NUL
 	symref-delete SP <ref> [NUL <old-target>] NUL
 	symref-verify SP <ref> [NUL <old-target>] NUL
 	option SP <opt> NUL
@@ -121,6 +123,10 @@ verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-create:
+	Create symbolic ref <ref> with <new-target> after verifying
+	it does not exist.
+
 symref-delete::
 	Delete <ref> after verifying it exists with <old-target>, if given.
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 93fdfc945a..ac1e131d8b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -546,7 +546,7 @@ static void write_remote_refs(const struct ref *local_refs)
 		if (!r->peer_ref)
 			continue;
 		if (ref_transaction_create(t, r->peer_ref->name, &r->old_oid,
-					   0, NULL, &err))
+					   NULL, 0, NULL, &err))
 			die("%s", err.buf);
 	}
 
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0ef9c38d8d..16d184603b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,7 +257,7 @@ static void parse_cmd_create(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	if (ref_transaction_create(transaction, refname, &new_oid,
+	if (ref_transaction_create(transaction, refname, &new_oid, NULL,
 				   update_flags | create_reflog_flag,
 				   msg, &err))
 		die("%s", err.buf);
@@ -267,6 +267,35 @@ static void parse_cmd_create(struct ref_transaction *transaction,
 	strbuf_release(&err);
 }
 
+
+static void parse_cmd_symref_create(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *refname, *new_target;
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-create: missing <ref>");
+
+	new_target = parse_next_refname(&next);
+	if (!new_target)
+		die("symref-create %s: missing <new-target>", refname);
+
+	if (*next != line_termination)
+		die("symref-create %s: extra input: %s", refname, next);
+
+	if (ref_transaction_create(transaction, refname, NULL, new_target,
+				   update_flags | create_reflog_flag,
+				   msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(new_target);
+	strbuf_release(&err);
+}
+
 static void parse_cmd_delete(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -473,6 +502,7 @@ static const struct parse_cmd {
 	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
 	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
 	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
 	{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
 	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
 	{ "option",        parse_cmd_option,        1, UPDATE_REFS_OPEN },
diff --git a/refs.c b/refs.c
index c2c9889466..6b724343fe 100644
--- a/refs.c
+++ b/refs.c
@@ -1302,15 +1302,16 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
+			   const char *new_target,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err)
 {
-	if (!new_oid || is_null_oid(new_oid)) {
-		strbuf_addf(err, "'%s' has a null OID", refname);
+	if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
+		strbuf_addf(err, "'%s' has a null OID or no new target", refname);
 		return 1;
 	}
 	return ref_transaction_update(transaction, refname, new_oid,
-				      null_oid(), NULL, NULL, flags,
+				      null_oid(), new_target, NULL, flags,
 				      msg, err);
 }
 
diff --git a/refs.h b/refs.h
index 053c382f0b..055cc9173b 100644
--- a/refs.h
+++ b/refs.h
@@ -753,6 +753,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
+			   const char *new_target,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err);
 
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index a390cffc80..cc7e20431e 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	esac
 '
 
+test_expect_success SYMLINKS 'symref transaction supports symlinks' '
+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
+	git update-ref refs/heads/new @ &&
+	test_config core.prefersymlinkrefs true &&
+	cat >stdin <<-EOF &&
+	start
+	symref-create TESTSYMREFONE refs/heads/new
+	prepare
+	commit
+	EOF
+	git update-ref --no-deref --stdin <stdin &&
+	test_path_is_symlink .git/TESTSYMREFONE &&
+	test "$(test_readlink .git/TESTSYMREFONE)" = refs/heads/new
+'
+
+test_expect_success 'symref transaction supports false symlink config' '
+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
+	git update-ref refs/heads/new @ &&
+	test_config core.prefersymlinkrefs false &&
+	cat >stdin <<-EOF &&
+	start
+	symref-create TESTSYMREFONE refs/heads/new
+	prepare
+	commit
+	EOF
+	git update-ref --no-deref --stdin <stdin &&
+	test_path_is_file .git/TESTSYMREFONE &&
+	git symbolic-ref TESTSYMREFONE >actual &&
+	echo refs/heads/new >expect &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fbc6b234cb..7955988ecc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1783,6 +1783,71 @@ do
 		test_must_fail git symbolic-ref -d refs/heads/symref2
 	'
 
+	test_expect_success "stdin ${type} symref-create fails with too many arguments" '
+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-create refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin ${type} symref-create fails with no target" '
+		create_stdin_buf ${type} "symref-create refs/heads/symref" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
+	'
+
+	test_expect_success "stdin ${type} symref-create fails with empty target" '
+		create_stdin_buf ${type} "symref-create refs/heads/symref" "" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
+	'
+
+	test_expect_success "stdin ${type} symref-create works" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-create works with --no-deref" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" &&
+		git update-ref --stdin ${type} <stdin 2>err
+	'
+
+	test_expect_success "stdin ${type} create dangling symref ref works" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-create refs/heads/symref" "refs/heads/unkown" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo refs/heads/unkown >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-create does not create reflogs by default" '
+		test_when_finished "git symbolic-ref -d refs/symref" &&
+		create_stdin_buf ${type} "symref-create refs/symref" "$a" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual &&
+		test_must_fail git reflog exists refs/symref
+	'
+
+	test_expect_success "stdin ${type} symref-create reflogs with --create-reflog" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" >stdin &&
+		git update-ref --create-reflog --stdin ${type} --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual &&
+		git reflog exists refs/heads/symref
+	'
+
 done
 
 test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ccde1b944b..ff77dcca6b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -189,15 +189,18 @@ test_expect_success 'hook gets all queued symref updates' '
 	prepared
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
 	committed
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
 	EOF
 
 	git update-ref --no-deref --stdin <<-EOF &&
 	start
 	symref-verify refs/heads/symref refs/heads/main
 	symref-delete refs/heads/symrefd refs/heads/main
+	symref-create refs/heads/symrefc refs/heads/main
 	prepare
 	commit
 	EOF
-- 
2.43.GIT

[PATCH 5/6] reftable: pick either 'oid' or 'target' for new updates

[Karthik Nayak]

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

When creating a reference transaction update, we can provide the old/new
oid/target for the update. We have checks in place to ensure that for
each old/new, either oid or target is set and not both.

In the reftable backend, when dealing with updates without the
`REF_NO_DEREF` flag, we don't selectively propagate data as needed.
Since there are no active users of the path, this is not caught. As we
want to introduce the 'symref-update' command in the upcoming commit,
which would use this flow, correct it.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/reftable-backend.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 975061d103..c17e68100d 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -896,8 +896,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 				 */
 				new_update = ref_transaction_add_update(
 					transaction, referent.buf, new_flags,
-					&u->new_oid, &u->old_oid, u->new_target,
-					u->old_target, u->msg);
+					u->new_target ? NULL : &u->new_oid,
+					u->old_target ? NULL : &u->old_oid,
+					u->new_target, u->old_target, u->msg);
 
 				new_update->parent_update = u;
 
-- 
2.43.GIT

[PATCH 6/6] update-ref: add support for 'symref-update' command

[Karthik Nayak]

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

Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
allow updates of symbolic refs. The 'symref-update' command takes in a
<new-target>, which the <ref> will be updated to. If the <ref> doesn't
exist it will be created.

It also optionally takes either an `ref <old-target>` or `oid
<old-oid>`. If the <old-target> is provided, it checks to see if the
<ref> targets the <old-target> before the update. If <old-oid> is provided
it checks <ref> to ensure that it is a regular ref and <old-oid> is the
OID before the update. This by extension also means that this when a
zero <old-oid> is provided, it ensures that the ref didn't exist before.

The command allows users to perform symbolic ref updates within a
transaction. This provides atomicity and allows users to perform a set
of operations together.

This command will also support deref mode, to ensure that we can update
dereferenced regular refs to symrefs.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |   7 ++
 builtin/update-ref.c             |  85 ++++++++++++++
 t/t1400-update-ref.sh            | 191 +++++++++++++++++++++++++++++++
 t/t1416-ref-transaction-hooks.sh |   4 +
 4 files changed, 287 insertions(+)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 364ef78af1..afcf33cf60 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-update SP <ref> SP <new-target> [SP (ref SP <old-target> | oid SP <old-oid>)] LF
 	symref-create SP <ref> SP <new-target> LF
 	symref-delete SP <ref> [SP <old-target>] LF
 	symref-verify SP <ref> [SP <old-target>] LF
@@ -89,6 +90,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-update SP <ref> NUL <new-target> [NUL (ref NUL <old-target> | oid NUL <old-oid>)] NUL
 	symref-create SP <ref> NUL <new-target> NUL
 	symref-delete SP <ref> [NUL <old-target>] NUL
 	symref-verify SP <ref> [NUL <old-target>] NUL
@@ -119,6 +121,11 @@ delete::
 	Delete <ref> after verifying it exists with <old-oid>, if
 	given.  If given, <old-oid> may not be zero.
 
+symref-update::
+	Set <ref> to <new-target> after verifying <old-target> or <old-oid>,
+	if given. Specify a zero <old-oid> to ensure that the ref does not
+	exist before the update.
+
 verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 16d184603b..389136dc2f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -98,6 +98,41 @@ static char *parse_next_refname(const char **next)
 	return parse_refname(next);
 }
 
+/*
+ * Wrapper around parse_arg which skips the next delimiter.
+ */
+static char *parse_next_arg(const char **next)
+{
+	struct strbuf arg = STRBUF_INIT;
+
+	if (line_termination) {
+		/* Without -z, consume SP and use next argument */
+		if (!**next || **next == line_termination)
+			return NULL;
+		if (**next != ' ')
+			die("expected SP but got: %s", *next);
+	} else {
+		/* With -z, read the next NUL-terminated line */
+		if (**next)
+			return NULL;
+	}
+	/* Skip the delimiter */
+	(*next)++;
+
+	if (line_termination) {
+		/* Without -z, use the next argument */
+		*next = parse_arg(*next, &arg);
+	} else {
+		/* With -z, use everything up to the next NUL */
+		strbuf_addstr(&arg, *next);
+		*next += arg.len;
+	}
+
+	if (arg.len)
+		return strbuf_detach(&arg, NULL);
+	return NULL;
+}
+
 
 /*
  * The value being parsed is <old-oid> (as opposed to <new-oid>; the
@@ -237,6 +272,55 @@ static void parse_cmd_update(struct ref_transaction *transaction,
 	strbuf_release(&err);
 }
 
+static void parse_cmd_symref_update(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	char *refname, *new_target, *old_arg;
+	char *old_target = NULL;
+	struct strbuf err = STRBUF_INIT;
+	struct object_id old_oid;
+	int have_old = 0;
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-update: missing <ref>");
+
+	new_target = parse_next_refname(&next);
+	if (!new_target)
+		die("symref-update %s: missing <new-target>", refname);
+
+	old_arg = parse_next_arg(&next);
+	if (old_arg) {
+		old_target = parse_next_refname(&next);
+		if (!old_target)
+			die("symref-update %s: expected old value", refname);
+
+		if (!strcmp(old_arg, "oid") &&
+		    !repo_get_oid(the_repository, old_target, &old_oid)) {
+			old_target = NULL;
+			have_old = 1;
+		} else if (strcmp(old_arg, "ref"))
+			die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
+	}
+
+	if (*next != line_termination)
+		die("symref-update %s: extra input: %s", refname, next);
+
+	if (ref_transaction_update(transaction, refname, NULL,
+				   have_old ? &old_oid : NULL,
+				   new_target, old_target,
+				   update_flags |= create_reflog_flag,
+				   msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(old_arg);
+	free(old_target);
+	free(new_target);
+	strbuf_release(&err);
+}
+
 static void parse_cmd_create(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -502,6 +586,7 @@ static const struct parse_cmd {
 	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
 	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
 	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-update", parse_cmd_symref_update, 4, UPDATE_REFS_OPEN },
 	{ "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
 	{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
 	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7955988ecc..f7d00cc024 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1360,6 +1360,7 @@ test_expect_success 'fails with duplicate HEAD update' '
 '
 
 test_expect_success 'fails with duplicate ref update via symref' '
+	test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
 	git branch target2 $A &&
 	git symbolic-ref refs/heads/symref2 refs/heads/target2 &&
 	cat >stdin <<-EOF &&
@@ -1848,6 +1849,196 @@ do
 		git reflog exists refs/heads/symref
 	'
 
+	test_expect_success "stdin ${type} symref-update fails with too many arguments" '
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "ref" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-update refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin ${type} symref-update fails with wrong old value argument" '
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "foo" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		grep "fatal: symref-update refs/heads/symref: invalid arg ${SQ}foo${SQ} for old value" err
+	'
+
+	test_expect_success "stdin ${type} symref-update creates with zero old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update creates with no old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update creates dangling" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "refs/heads/nonexistent" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo refs/heads/nonexistent >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update fails with wrong old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" "ref" "$b" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected $b" err &&
+		test_must_fail git rev-parse --verify -q $c
+	'
+
+	test_expect_success "stdin ${type} symref-update updates dangling ref" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update updates dangling ref with old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "ref" "refs/heads/nonexistent" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update fails update dangling ref with wrong old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "ref" "refs/heads/wrongref" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
+		echo refs/heads/nonexistent >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update works with right old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" "ref" "$a" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $m >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update works with no old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" >stdin &&
+		git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $m >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update fails with empty old ref-target" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" "ref" "" >stdin &&
+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update creates (with deref)" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin ${type} <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/symref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+		grep "$Z $(git rev-parse $a)" actual
+	'
+
+	test_expect_success "stdin ${type} symref-update regular ref to symref with correct old-oid" '
+		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse $a)" >stdin &&
+		git update-ref --stdin ${type} <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+	'
+
+	test_expect_success "stdin ${type} symref-update regular ref to symref fails with wrong old-oid" '
+		test_when_finished "git update-ref -d refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+		echo $(git rev-parse $a) >expect &&
+		git rev-parse refs/heads/regularref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update existing symref with zero old-oid" '
+		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref refs/heads/target2 &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+		grep "fatal: cannot lock ref ${SQ}refs/heads/symref${SQ}: reference already exists" err &&
+		echo refs/heads/target2 >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin ${type} symref-update regular ref to symref (with deref)" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_when_finished "git update-ref -d --no-deref refs/heads/symref2" &&
+		git update-ref refs/heads/symref2 $a &&
+		git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 &&
+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref ${type} --stdin <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/symref2 >actual &&
+		test_cmp expect actual &&
+		echo refs/heads/symref2 >expect &&
+		git symbolic-ref --no-recurse refs/heads/symref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+	'
+
+	test_expect_success "stdin ${type} symref-update regular ref to symref" '
+		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		create_stdin_buf ${type} "symref-update refs/heads/regularref" "$a" >stdin &&
+		git update-ref ${type} --stdin <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+	'
+
 done
 
 test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ff77dcca6b..5a812ca3c0 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -163,6 +163,7 @@ test_expect_success 'hook gets all queued symref updates' '
 	git update-ref refs/heads/branch $POST_OID &&
 	git symbolic-ref refs/heads/symref refs/heads/main &&
 	git symbolic-ref refs/heads/symrefd refs/heads/main &&
+	git symbolic-ref refs/heads/symrefu refs/heads/main &&
 
 	test_hook reference-transaction <<-\EOF &&
 	echo "$*" >>actual
@@ -190,10 +191,12 @@ test_expect_success 'hook gets all queued symref updates' '
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+	ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
 	committed
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+	ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
 	EOF
 
 	git update-ref --no-deref --stdin <<-EOF &&
@@ -201,6 +204,7 @@ test_expect_success 'hook gets all queued symref updates' '
 	symref-verify refs/heads/symref refs/heads/main
 	symref-delete refs/heads/symrefd refs/heads/main
 	symref-create refs/heads/symrefc refs/heads/main
+	symref-update refs/heads/symrefu refs/heads/branch ref refs/heads/main
 	prepare
 	commit
 	EOF
-- 
2.43.GIT

Re: [PATCH 1/6] refs: create and use `ref_update_ref_must_exist()`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3107 bytes --]

On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> The files and reftable backend, need to check if a ref must exist, so
> that the required validation can be done. A ref must exist only when the
> `old_oid` value of the update has been explicitly set and it is not the
> `null_oid` value.
> 
> Since we also support symrefs now, we need to ensure that even when
> `old_target` is set a ref must exist. While this was missed when we
> added symref support in transactions, there are no active users of this
> path. As we introduce the 'symref-verify' command in the upcoming
> commits, it is important to fix this.
> 
> So let's export this to a function called `ref_update_ref_must_exist()`
> and expose it internally via 'refs-internal.h'.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c                  | 6 ++++++
>  refs/files-backend.c    | 3 +--
>  refs/refs-internal.h    | 6 ++++++
>  refs/reftable-backend.c | 2 +-
>  4 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index fa5471d219..59858fafdb 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
>  			    referent, update->old_target);
>  	return -1;
>  }
> +
> +int ref_update_ref_must_exist(struct ref_update *update)
> +{
> +	return (update->flags & REF_HAVE_OLD) &&
> +		(!is_null_oid(&update->old_oid) || update->old_target);
> +}
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3957bfa579..2df204f891 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  			       struct strbuf *err)
>  {
>  	struct strbuf referent = STRBUF_INIT;
> -	int mustexist = (update->flags & REF_HAVE_OLD) &&
> -		!is_null_oid(&update->old_oid);
> +	int mustexist = ref_update_ref_must_exist(update);

Okay. So we didn't notice this was broken because even though we started
writing symrefs via transactions now, none of the calles ever assert
that the old ref exists?

>  	int ret = 0;
>  	struct ref_lock *lock;
>  
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 53a6c5d842..5da3029e6c 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
>  int ref_update_check_old_target(const char *referent, struct ref_update *update,
>  				struct strbuf *err);
>  
> +/*
> + * Check if the ref must exist, this means that the old_oid or
> + * old_target is non NULL.
> + */
> +int ref_update_ref_must_exist(struct ref_update *update);

Seeing `ref_update_ref_must_exist()` as a standalone function wouldn't
quite tell me what it really does. It sounds a bit like this would
already assert the ref exists at the time of calling it.

We could call this `ref_upate_expects_existing_old_ref()`, which might
clarify the intent a bit.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] update-ref: add support for 'symref-verify' command

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3906 bytes --]

On Tue, May 14, 2024 at 02:44:07PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> The 'symref-verify' command allows users to verify if a provided <ref>
> contains the provided <old-target> without changing the <ref>. If
> <old-target> is not provided, the command will verify that the <ref>
> doesn't exist.
> 
> The command allows users to verify symbolic refs within a transaction,
> and this means users can perform a set of changes in a transaction only
> when the verification holds good.
> 
> Since we're checking for symbolic refs, this command will only work with
> the 'no-deref' mode. This is because any dereferenced symbolic ref will
> point to an object and not a ref and the regular 'verify' command can be
> used in such situations.
> 
> Add required tests for symref support in 'verify' while also adding
> reflog checks for the pre-existing 'verify' tests.

I'm a bit surprised that you add reflog-related tests, and you don't
really explain why you do it. Do we change any behaviour relating to
reflogs here? If there is a particular reason that is independent of the
new "symref-verify" command, then I'd expect this to be part of a
separate commit.

[snip]
> diff --git a/refs.c b/refs.c
> index 59858fafdb..ee4c6ed99c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1331,14 +1331,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");

Should we also BUG on `old_target && old_oid`?

> @@ -1641,4 +1647,88 @@ 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
> +}

I think this would be easier to use if you didn't handle the redirect to
"stdin" over here, but at the calling site. Otherwise, the caller needs
to be aware of the inner workings.

> +for type in "" "-z"
> +do
> +
> +	test_expect_success "stdin ${type} symref-verify fails without --no-deref" '

We typically avoid curly braces unless required.

[snip]
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index 067fd57290..fd58b902f4 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'hook gets all queued symref updates' '
> +	test_when_finished "rm actual" &&
> +
> +	git update-ref refs/heads/branch $POST_OID &&
> +	git symbolic-ref refs/heads/symref refs/heads/main &&
> +
> +	test_hook reference-transaction <<-\EOF &&
> +	echo "$*" >>actual
> +	while read -r line
> +	do
> +		printf "%s\n" "$line"
> +	done >>actual
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	prepared
> +	ref:refs/heads/main $ZERO_OID refs/heads/symref
> +	committed
> +	ref:refs/heads/main $ZERO_OID refs/heads/symref
> +	EOF
> +
> +	git update-ref --no-deref --stdin <<-EOF &&
> +	start
> +	symref-verify refs/heads/symref refs/heads/main
> +	prepare
> +	commit
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_done

So the reference-transaction hook executes even for "symref-verify"?
This feels quite unexpected to me. Do we do the same for "verify"?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] update-ref: add support for 'symref-delete' command

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On Tue, May 14, 2024 at 02:44:08PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
[snip]
> @@ -1317,14 +1317,18 @@ int ref_transaction_create(struct ref_transaction *transaction,
>  int ref_transaction_delete(struct ref_transaction *transaction,
>  			   const char *refname,
>  			   const struct object_id *old_oid,
> -			   unsigned int flags, const char *msg,
> +			   unsigned int flags,
> +			   const char *old_target,
> +			   const char *msg,
>  			   struct strbuf *err)
>  {
>  	if (old_oid && is_null_oid(old_oid))
>  		BUG("delete called with old_oid set to zeros");
> +	if (old_target && !(flags & REF_NO_DEREF))
> +		BUG("delete cannot operate on symrefs with deref mode");
>  	return ref_transaction_update(transaction, refname,
>  				      null_oid(), old_oid,
> -				      NULL, NULL, flags,
> +				      NULL, old_target, flags,
>  				      msg, err);
>  }

Same comment here, should we BUG on `old_oid && old_target`?

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 8784c59a53..fbc6b234cb 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1689,7 +1689,7 @@ do
>  		test_cmp before after
>  	'
>  
> -	test_expect_success "stdin ${type} symref-verify no value is treated as zero value" '
> +	test_expect_success "stdin ${type} symref-verify fails with no value" '
>  		git symbolic-ref refs/heads/symref >expect &&
>  		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
>  		test_must_fail git update-ref --stdin ${type} --no-deref <stdin

This change belongs into the preceding commit.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] update-ref: add support for 'symref-create' command

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

On Tue, May 14, 2024 at 02:44:09PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> diff --git a/refs.c b/refs.c
> index c2c9889466..6b724343fe 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1302,15 +1302,16 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  int ref_transaction_create(struct ref_transaction *transaction,
>  			   const char *refname,
>  			   const struct object_id *new_oid,
> +			   const char *new_target,
>  			   unsigned int flags, const char *msg,
>  			   struct strbuf *err)
>  {
> -	if (!new_oid || is_null_oid(new_oid)) {
> -		strbuf_addf(err, "'%s' has a null OID", refname);
> +	if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
> +		strbuf_addf(err, "'%s' has a null OID or no new target", refname);
>  		return 1;
>  	}

Shouldn't this be "'%s' has neither an OID nor a target"?

Also, we again miss `new_oid && new_target`.

> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index a390cffc80..cc7e20431e 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
>  	esac
>  '
>  
> +test_expect_success SYMLINKS 'symref transaction supports symlinks' '
> +	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
> +	git update-ref refs/heads/new @ &&
> +	test_config core.prefersymlinkrefs true &&
> +	cat >stdin <<-EOF &&
> +	start
> +	symref-create TESTSYMREFONE refs/heads/new

Let's future proof this and create the ref with a name that matches our
root ref restrictions, like "TEST_SYMREF_HEAD". We do plan to enforce
those soonish, so these tests would break.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] update-ref: add support for 'symref-update' command

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4489 bytes --]

On Tue, May 14, 2024 at 02:44:11PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
> allow updates of symbolic refs. The 'symref-update' command takes in a
> <new-target>, which the <ref> will be updated to. If the <ref> doesn't
> exist it will be created.
> 
> It also optionally takes either an `ref <old-target>` or `oid
> <old-oid>`. If the <old-target> is provided, it checks to see if the
> <ref> targets the <old-target> before the update. If <old-oid> is provided
> it checks <ref> to ensure that it is a regular ref and <old-oid> is the
> OID before the update. This by extension also means that this when a
> zero <old-oid> is provided, it ensures that the ref didn't exist before.

It's somewhat unfortunate that the syntax diverges from the "update"
command. "update" also has essentially the same issue now, that we
cannot verify that its old value is a symref, right? Can we fix that in
a backwards compatible way?

It would also be great to explain why exactly the syntax needs to
diverge.

> The command allows users to perform symbolic ref updates within a
> transaction. This provides atomicity and allows users to perform a set
> of operations together.
> 
> This command will also support deref mode, to ensure that we can update
> dereferenced regular refs to symrefs.

Will it support deref mode or does it support it with this patch?

> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 16d184603b..389136dc2f 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -98,6 +98,41 @@ static char *parse_next_refname(const char **next)
>  	return parse_refname(next);
>  }
>  
> +/*
> + * Wrapper around parse_arg which skips the next delimiter.
> + */
> +static char *parse_next_arg(const char **next)
> +{
> +	struct strbuf arg = STRBUF_INIT;
> +
> +	if (line_termination) {
> +		/* Without -z, consume SP and use next argument */
> +		if (!**next || **next == line_termination)
> +			return NULL;
> +		if (**next != ' ')
> +			die("expected SP but got: %s", *next);
> +	} else {
> +		/* With -z, read the next NUL-terminated line */
> +		if (**next)
> +			return NULL;
> +	}
> +	/* Skip the delimiter */
> +	(*next)++;
> +
> +	if (line_termination) {
> +		/* Without -z, use the next argument */
> +		*next = parse_arg(*next, &arg);
> +	} else {
> +		/* With -z, use everything up to the next NUL */
> +		strbuf_addstr(&arg, *next);
> +		*next += arg.len;
> +	}
> +
> +	if (arg.len)
> +		return strbuf_detach(&arg, NULL);
> +	return NULL;
> +}
> +
>  

There's an extra newline here.

>  /*
>   * The value being parsed is <old-oid> (as opposed to <new-oid>; the
> @@ -237,6 +272,55 @@ static void parse_cmd_update(struct ref_transaction *transaction,
>  	strbuf_release(&err);
>  }
>  
> +static void parse_cmd_symref_update(struct ref_transaction *transaction,
> +				    const char *next, const char *end)
> +{
> +	char *refname, *new_target, *old_arg;
> +	char *old_target = NULL;
> +	struct strbuf err = STRBUF_INIT;
> +	struct object_id old_oid;
> +	int have_old = 0;
> +
> +	refname = parse_refname(&next);
> +	if (!refname)
> +		die("symref-update: missing <ref>");
> +
> +	new_target = parse_next_refname(&next);
> +	if (!new_target)
> +		die("symref-update %s: missing <new-target>", refname);
> +
> +	old_arg = parse_next_arg(&next);
> +	if (old_arg) {
> +		old_target = parse_next_refname(&next);
> +		if (!old_target)
> +			die("symref-update %s: expected old value", refname);
> +
> +		if (!strcmp(old_arg, "oid") &&
> +		    !repo_get_oid(the_repository, old_target, &old_oid)) {
> +			old_target = NULL;
> +			have_old = 1;

This one feels weird to me. Shouldn't we return an error in case we are
unable to parse the old OID?

> +		} else if (strcmp(old_arg, "ref"))
> +			die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);

When one of the branches has curly braces, then all branches should.

> +	}
> +
> +	if (*next != line_termination)
> +		die("symref-update %s: extra input: %s", refname, next);
> +
> +	if (ref_transaction_update(transaction, refname, NULL,
> +				   have_old ? &old_oid : NULL,
> +				   new_target, old_target,
> +				   update_flags |= create_reflog_flag,

This should be `update_flags | create_reflog_flag`, shouldn't it?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] refs: create and use `ref_update_ref_must_exist()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3349 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The files and reftable backend, need to check if a ref must exist, so
>> that the required validation can be done. A ref must exist only when the
>> `old_oid` value of the update has been explicitly set and it is not the
>> `null_oid` value.
>>
>> Since we also support symrefs now, we need to ensure that even when
>> `old_target` is set a ref must exist. While this was missed when we
>> added symref support in transactions, there are no active users of this
>> path. As we introduce the 'symref-verify' command in the upcoming
>> commits, it is important to fix this.
>>
>> So let's export this to a function called `ref_update_ref_must_exist()`
>> and expose it internally via 'refs-internal.h'.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs.c                  | 6 ++++++
>>  refs/files-backend.c    | 3 +--
>>  refs/refs-internal.h    | 6 ++++++
>>  refs/reftable-backend.c | 2 +-
>>  4 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index fa5471d219..59858fafdb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
>>  			    referent, update->old_target);
>>  	return -1;
>>  }
>> +
>> +int ref_update_ref_must_exist(struct ref_update *update)
>> +{
>> +	return (update->flags & REF_HAVE_OLD) &&
>> +		(!is_null_oid(&update->old_oid) || update->old_target);
>> +}
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 3957bfa579..2df204f891 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>  			       struct strbuf *err)
>>  {
>>  	struct strbuf referent = STRBUF_INIT;
>> -	int mustexist = (update->flags & REF_HAVE_OLD) &&
>> -		!is_null_oid(&update->old_oid);
>> +	int mustexist = ref_update_ref_must_exist(update);
>
> Okay. So we didn't notice this was broken because even though we started
> writing symrefs via transactions now, none of the calles ever assert
> that the old ref exists?
>

Yup, that's correct. The `git-symbolic-ref(1)` command doesn't ever
check for the old value and it is the only user of transactional symrefs
at this point.

>>  	int ret = 0;
>>  	struct ref_lock *lock;
>>
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index 53a6c5d842..5da3029e6c 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
>>  int ref_update_check_old_target(const char *referent, struct ref_update *update,
>>  				struct strbuf *err);
>>
>> +/*
>> + * Check if the ref must exist, this means that the old_oid or
>> + * old_target is non NULL.
>> + */
>> +int ref_update_ref_must_exist(struct ref_update *update);
>
> Seeing `ref_update_ref_must_exist()` as a standalone function wouldn't
> quite tell me what it really does. It sounds a bit like this would
> already assert the ref exists at the time of calling it.
>
> We could call this `ref_upate_expects_existing_old_ref()`, which might
> clarify the intent a bit.
>
> Patrick

Yeah, that's better, will change. Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/6] update-ref: add support for 'symref-verify' command

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4794 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, May 14, 2024 at 02:44:07PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The 'symref-verify' command allows users to verify if a provided <ref>
>> contains the provided <old-target> without changing the <ref>. If
>> <old-target> is not provided, the command will verify that the <ref>
>> doesn't exist.
>>
>> The command allows users to verify symbolic refs within a transaction,
>> and this means users can perform a set of changes in a transaction only
>> when the verification holds good.
>>
>> Since we're checking for symbolic refs, this command will only work with
>> the 'no-deref' mode. This is because any dereferenced symbolic ref will
>> point to an object and not a ref and the regular 'verify' command can be
>> used in such situations.
>>
>> Add required tests for symref support in 'verify' while also adding
>> reflog checks for the pre-existing 'verify' tests.
>
> I'm a bit surprised that you add reflog-related tests, and you don't
> really explain why you do it. Do we change any behaviour relating to
> reflogs here? If there is a particular reason that is independent of the
> new "symref-verify" command, then I'd expect this to be part of a
> separate commit.
>

Ah! There is no divergence in behavior, rather this is behavior which is
never captured in tests. So I thought it makes to have tests around it.

> [snip]
>> diff --git a/refs.c b/refs.c
>> index 59858fafdb..ee4c6ed99c 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1331,14 +1331,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");
>
> Should we also BUG on `old_target && old_oid`?
>

I didn't do this, because `ref_transaction_add_update` downstream from
this already does that. But I guess no harm in adding it here too.

>> @@ -1641,4 +1647,88 @@ 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
>> +}
>
> I think this would be easier to use if you didn't handle the redirect to
> "stdin" over here, but at the calling site. Otherwise, the caller needs
> to be aware of the inner workings.
>

Not sure what you mean by easier here, but I think it would be nicer to
read, since the client would now determine the destination of the
formatting and this would align with what the test needs to do. Will
change!

>> +for type in "" "-z"
>> +do
>> +
>> +	test_expect_success "stdin ${type} symref-verify fails without --no-deref" '
>
> We typically avoid curly braces unless required.
>

Will change, thanks!

> [snip]
>> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
>> index 067fd57290..fd58b902f4 100755
>> --- a/t/t1416-ref-transaction-hooks.sh
>> +++ b/t/t1416-ref-transaction-hooks.sh
>> @@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success 'hook gets all queued symref updates' '
>> +	test_when_finished "rm actual" &&
>> +
>> +	git update-ref refs/heads/branch $POST_OID &&
>> +	git symbolic-ref refs/heads/symref refs/heads/main &&
>> +
>> +	test_hook reference-transaction <<-\EOF &&
>> +	echo "$*" >>actual
>> +	while read -r line
>> +	do
>> +		printf "%s\n" "$line"
>> +	done >>actual
>> +	EOF
>> +
>> +	cat >expect <<-EOF &&
>> +	prepared
>> +	ref:refs/heads/main $ZERO_OID refs/heads/symref
>> +	committed
>> +	ref:refs/heads/main $ZERO_OID refs/heads/symref
>> +	EOF
>> +
>> +	git update-ref --no-deref --stdin <<-EOF &&
>> +	start
>> +	symref-verify refs/heads/symref refs/heads/main
>> +	prepare
>> +	commit
>> +	EOF
>> +	test_cmp expect actual
>> +'
>> +
>>  test_done
>
> So the reference-transaction hook executes even for "symref-verify"?
> This feels quite unexpected to me. Do we do the same for "verify"?
>
> Patrick

Yes this is the same for verify as well. I was surprised to find this
too. It's just the way ref update code is written, all updates land
trigger the hook. This means verify, which is also a form of update,
with just the new value not set, also triggers the hook. I've kept the
same behavior with symref-verify.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 4/6] update-ref: add support for 'symref-create' command

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, May 14, 2024 at 02:44:09PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>> diff --git a/refs.c b/refs.c
>> index c2c9889466..6b724343fe 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1302,15 +1302,16 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>  int ref_transaction_create(struct ref_transaction *transaction,
>>  			   const char *refname,
>>  			   const struct object_id *new_oid,
>> +			   const char *new_target,
>>  			   unsigned int flags, const char *msg,
>>  			   struct strbuf *err)
>>  {
>> -	if (!new_oid || is_null_oid(new_oid)) {
>> -		strbuf_addf(err, "'%s' has a null OID", refname);
>> +	if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
>> +		strbuf_addf(err, "'%s' has a null OID or no new target", refname);
>>  		return 1;
>>  	}
>
> Shouldn't this be "'%s' has neither an OID nor a target"?
>

Well it's actually all three. If new_oid is not set, or if new_oid is
NULL or there is no new_target.

I think "'%s' has neither a valid OID nor a target" would be best.

> Also, we again miss `new_oid && new_target`.
>

Will fix for all the commands!

>> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
>> index a390cffc80..cc7e20431e 100755
>> --- a/t/t0600-reffiles-backend.sh
>> +++ b/t/t0600-reffiles-backend.sh
>> @@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
>>  	esac
>>  '
>>
>> +test_expect_success SYMLINKS 'symref transaction supports symlinks' '
>> +	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
>> +	git update-ref refs/heads/new @ &&
>> +	test_config core.prefersymlinkrefs true &&
>> +	cat >stdin <<-EOF &&
>> +	start
>> +	symref-create TESTSYMREFONE refs/heads/new
>
> Let's future proof this and create the ref with a name that matches our
> root ref restrictions, like "TEST_SYMREF_HEAD". We do plan to enforce
> those soonish, so these tests would break.
>
> Patrick

Makes sense, will fix. Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/6] update-ref: add support for 'symref-verify' command

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Fri, May 17, 2024 at 06:21:53PM +0200, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Tue, May 14, 2024 at 02:44:07PM +0200, Karthik Nayak wrote:
> > So the reference-transaction hook executes even for "symref-verify"?
> > This feels quite unexpected to me. Do we do the same for "verify"?
> 
> Yes this is the same for verify as well. I was surprised to find this
> too. It's just the way ref update code is written, all updates land
> trigger the hook. This means verify, which is also a form of update,
> with just the new value not set, also triggers the hook. I've kept the
> same behavior with symref-verify.

Weird. I wonder whether we should declare this a bug. In any case, that
can be handled in a separate patch series.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] update-ref: add support for 'symref-update' command

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5451 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, May 14, 2024 at 02:44:11PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
>> allow updates of symbolic refs. The 'symref-update' command takes in a
>> <new-target>, which the <ref> will be updated to. If the <ref> doesn't
>> exist it will be created.
>>
>> It also optionally takes either an `ref <old-target>` or `oid
>> <old-oid>`. If the <old-target> is provided, it checks to see if the
>> <ref> targets the <old-target> before the update. If <old-oid> is provided
>> it checks <ref> to ensure that it is a regular ref and <old-oid> is the
>> OID before the update. This by extension also means that this when a
>> zero <old-oid> is provided, it ensures that the ref didn't exist before.
>
> It's somewhat unfortunate that the syntax diverges from the "update"
> command. "update" also has essentially the same issue now, that we
> cannot verify that its old value is a symref, right? Can we fix that in
> a backwards compatible way?
>

I think Peff mentioned [1] of a way. So we convert the existing

    update SP <ref> SP <newvalue> [SP <oldvalue>] LF
    update SP <ref> NUL <newvalue> NUL [<oldvalue>] NUL // -z

to

    update SP <ref> SP <newvalue> [SP (<oldvalue> | ref <old_target>)] LF
    update SP <ref> NUL <newvalue> NUL [(<oldvalue> | ref NUL
<old_target>)] NUL // -z

this should work, I think. I will play around this and add it in. Please
let me know if you can think of a scenario where this breaks.

> It would also be great to explain why exactly the syntax needs to
> diverge.
>

Yeah makes sense, will add it to the commit message.

>> The command allows users to perform symbolic ref updates within a
>> transaction. This provides atomicity and allows users to perform a set
>> of operations together.
>>
>> This command will also support deref mode, to ensure that we can update
>> dereferenced regular refs to symrefs.
>
> Will it support deref mode or does it support it with this patch?
>

Will rephrase, in this patch itself though.

>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index 16d184603b..389136dc2f 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -98,6 +98,41 @@ static char *parse_next_refname(const char **next)
>>  	return parse_refname(next);
>>  }
>>
>> +/*
>> + * Wrapper around parse_arg which skips the next delimiter.
>> + */
>> +static char *parse_next_arg(const char **next)
>> +{
>> +	struct strbuf arg = STRBUF_INIT;
>> +
>> +	if (line_termination) {
>> +		/* Without -z, consume SP and use next argument */
>> +		if (!**next || **next == line_termination)
>> +			return NULL;
>> +		if (**next != ' ')
>> +			die("expected SP but got: %s", *next);
>> +	} else {
>> +		/* With -z, read the next NUL-terminated line */
>> +		if (**next)
>> +			return NULL;
>> +	}
>> +	/* Skip the delimiter */
>> +	(*next)++;
>> +
>> +	if (line_termination) {
>> +		/* Without -z, use the next argument */
>> +		*next = parse_arg(*next, &arg);
>> +	} else {
>> +		/* With -z, use everything up to the next NUL */
>> +		strbuf_addstr(&arg, *next);
>> +		*next += arg.len;
>> +	}
>> +
>> +	if (arg.len)
>> +		return strbuf_detach(&arg, NULL);
>> +	return NULL;
>> +}
>> +
>>
>
> There's an extra newline here.
>

Will remove.

>>  /*
>>   * The value being parsed is <old-oid> (as opposed to <new-oid>; the
>> @@ -237,6 +272,55 @@ static void parse_cmd_update(struct ref_transaction *transaction,
>>  	strbuf_release(&err);
>>  }
>>
>> +static void parse_cmd_symref_update(struct ref_transaction *transaction,
>> +				    const char *next, const char *end)
>> +{
>> +	char *refname, *new_target, *old_arg;
>> +	char *old_target = NULL;
>> +	struct strbuf err = STRBUF_INIT;
>> +	struct object_id old_oid;
>> +	int have_old = 0;
>> +
>> +	refname = parse_refname(&next);
>> +	if (!refname)
>> +		die("symref-update: missing <ref>");
>> +
>> +	new_target = parse_next_refname(&next);
>> +	if (!new_target)
>> +		die("symref-update %s: missing <new-target>", refname);
>> +
>> +	old_arg = parse_next_arg(&next);
>> +	if (old_arg) {
>> +		old_target = parse_next_refname(&next);
>> +		if (!old_target)
>> +			die("symref-update %s: expected old value", refname);
>> +
>> +		if (!strcmp(old_arg, "oid") &&
>> +		    !repo_get_oid(the_repository, old_target, &old_oid)) {
>> +			old_target = NULL;
>> +			have_old = 1;
>
> This one feels weird to me. Shouldn't we return an error in case we are
> unable to parse the old OID?
>

Yup, that was missed, will add a check. I also realized that at the top
we do `old_target = parse_next_refname()`, while it makes more sense to
do `old_target = parse_next_arg()`.

>> +		} else if (strcmp(old_arg, "ref"))
>> +			die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
>
> When one of the branches has curly braces, then all branches should.
>

Will change.

>> +	}
>> +
>> +	if (*next != line_termination)
>> +		die("symref-update %s: extra input: %s", refname, next);
>> +
>> +	if (ref_transaction_update(transaction, refname, NULL,
>> +				   have_old ? &old_oid : NULL,
>> +				   new_target, old_target,
>> +				   update_flags |= create_reflog_flag,
>
> This should be `update_flags | create_reflog_flag`, shouldn't it?
>

Definitely.

> Patrick

Thanks for the review.

[1]: https://lore.kernel.org/git/20240426204145.GC13703@coredump.intra.peff.net/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 6/6] update-ref: add support for 'symref-update' command

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

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

> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Tue, May 14, 2024 at 02:44:11PM +0200, Karthik Nayak wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
>>> allow updates of symbolic refs. The 'symref-update' command takes in a
>>> <new-target>, which the <ref> will be updated to. If the <ref> doesn't
>>> exist it will be created.
>>>
>>> It also optionally takes either an `ref <old-target>` or `oid
>>> <old-oid>`. If the <old-target> is provided, it checks to see if the
>>> <ref> targets the <old-target> before the update. If <old-oid> is provided
>>> it checks <ref> to ensure that it is a regular ref and <old-oid> is the
>>> OID before the update. This by extension also means that this when a
>>> zero <old-oid> is provided, it ensures that the ref didn't exist before.
>>
>> It's somewhat unfortunate that the syntax diverges from the "update"
>> command. "update" also has essentially the same issue now, that we
>> cannot verify that its old value is a symref, right? Can we fix that in
>> a backwards compatible way?
>>
>
> I think Peff mentioned [1] of a way. So we convert the existing
>
>     update SP <ref> SP <newvalue> [SP <oldvalue>] LF
>     update SP <ref> NUL <newvalue> NUL [<oldvalue>] NUL // -z
>
> to
>
>     update SP <ref> SP <newvalue> [SP (<oldvalue> | ref <old_target>)] LF
>     update SP <ref> NUL <newvalue> NUL [(<oldvalue> | ref NUL <old_target>)] NUL // -z
>
> this should work, I think. I will play around this and add it in. Please
> let me know if you can think of a scenario where this breaks.

Thinking about this more, I'll actually do this in a follow up series. I
want to cleanup the existing tests already to move them to the loop that
I introduced in this series. That would make it easier to test and
ensure that we can port the existing 'update' command to also support
`old_target`.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 0/6] update-ref: add symref support for --stdin

[Karthik Nayak]

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

The 'update-ref' command is used to update refs using transactions. The
command allows users to also utilize a '--stdin' mode to provide a
batch of sub-commands which can be processed in a transaction.

Currently, the sub-commands involve {verify, delete, create, update}
and they allow users to work with regular refs in the repository. To
work with symrefs, users only have the option of using
'git-symbolic-ref', which doesn't provide transaction support to the
users eventhough it uses the same behind the hood. 

Recently, we modified the reference backend to add symref support,
following which, 'git-symbolic-ref' also uses the transaction backend.
But, it doesn't expose this to the user. To allow users to work with
symrefs via transaction, this series adds support for new sub-commands
{symrer-verify, symref-delete, symref-create, symref-update} to the
'--stdin' mode of update-ref. These complement the existing
sub-commands.

The patches 1, 5 fix small issues in the reference backends. The other
patches 2, 3, 4 & 6, each add one of the new sub-commands.

The series is based off master, with 'kn/ref-transaction-symref' merged
in. There seem to be no conflicts with 'next' or 'seen'.

There was some discussion [1] also about adding `old_target` support to
the existing `update` command. I think its worthwhile to do this with
some tests cleanup, will follow that up as a separate series.  

Changes since v1:
* Rename the function `ref_update_ref_must_exist` to `ref_update_expects_existing_old_ref`
to better clarify its usage.
* Add checks in all the `ref_transaction_*()` functions to check if both
_target_ and _oid_ values are set.
* Clean up the tests
  - Change `create_stdin_buf` to `format_command` and allow clients to
  decide the destination.
  - Remove unecessary curly braces.
  - Rename TESTSYMREFONE to TEST_SYMREF_HEAD to stay compatible with our
  new definitions of refs and pseudorefs.
* Add more explanation in the commit message around the deviation of syntax
for the `symref-update` command.

[1]: https://lore.kernel.org/r/CAOLa=ZQW-cCV5BP_fCvuZimfkjwAzjEiqXYRPft1Wf9kAX=_bw@mail.gmail.com

Range diff vs v1:
 1:  1bc4cc3fc4 =  1:  1bc4cc3fc4 refs: accept symref values in `ref_transaction_update()`
 2:  57d0b1e2ea =  2:  57d0b1e2ea files-backend: extract out `create_symref_lock()`
 3:  a8ae923f85 =  3:  a8ae923f85 refs: support symrefs in 'reference-transaction' hook
 4:  e9965ba477 =  4:  e9965ba477 refs: move `original_update_refname` to 'refs.c'
 5:  644daf7785 =  5:  644daf7785 refs: add support for transactional symref updates
 6:  300b38e46f =  6:  300b38e46f refs: use transaction in `refs_create_symref()`
 7:  f151dfe3c9 =  7:  f151dfe3c9 refs: rename `refs_create_symref()` to `refs_update_symref()`
 8:  4865707bda =  8:  4865707bda refs: remove `create_symref` and associated dead code
 9:  4cb67dce7c !  9:  2bbdeff798 refs: create and use `ref_update_ref_must_exist()`
    @@ Metadata
     Author: Karthik Nayak <karthik.188@gmail.com>
     
      ## Commit message ##
    -    refs: create and use `ref_update_ref_must_exist()`
    +    refs: create and use `ref_update_expects_existing_old_ref()`
     
         The files and reftable backend, need to check if a ref must exist, so
         that the required validation can be done. A ref must exist only when the
    @@ Commit message
         path. As we introduce the 'symref-verify' command in the upcoming
         commits, it is important to fix this.
     
    -    So let's export this to a function called `ref_update_ref_must_exist()`
    -    and expose it internally via 'refs-internal.h'.
    +    So let's export this to a function called
    +    `ref_update_expects_existing_old_ref()` and expose it internally via
    +    'refs-internal.h'.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
    @@ refs.c: int ref_update_check_old_target(const char *referent, struct ref_update
      	return -1;
      }
     +
    -+int ref_update_ref_must_exist(struct ref_update *update)
    ++int ref_update_expects_existing_old_ref(struct ref_update *update)
     +{
     +	return (update->flags & REF_HAVE_OLD) &&
     +		(!is_null_oid(&update->old_oid) || update->old_target);
    @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref
      	struct strbuf referent = STRBUF_INIT;
     -	int mustexist = (update->flags & REF_HAVE_OLD) &&
     -		!is_null_oid(&update->old_oid);
    -+	int mustexist = ref_update_ref_must_exist(update);
    ++	int mustexist = ref_update_expects_existing_old_ref(update);
      	int ret = 0;
      	struct ref_lock *lock;
      
    @@ refs/refs-internal.h: int ref_update_has_null_new_value(struct ref_update *updat
     + * Check if the ref must exist, this means that the old_oid or
     + * old_target is non NULL.
     + */
    -+int ref_update_ref_must_exist(struct ref_update *update);
    ++int ref_update_expects_existing_old_ref(struct ref_update *update);
     +
      #endif /* REFS_REFS_INTERNAL_H */
     
    @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s
      		if (ret < 0)
      			goto done;
     -		if (ret > 0 && (!(u->flags & REF_HAVE_OLD) || is_null_oid(&u->old_oid))) {
    -+		if (ret > 0 && !ref_update_ref_must_exist(u)) {
    ++		if (ret > 0 && !ref_update_expects_existing_old_ref(u)) {
      			/*
      			 * The reference does not exist, and we either have no
      			 * old object ID or expect the reference to not exist.
10:  9fda13b468 ! 10:  f509066cab update-ref: add support for 'symref-verify' command
    @@ Commit message
         point to an object and not a ref and the regular 'verify' command can be
         used in such situations.
     
    -    Add required tests for symref support in 'verify' while also adding
    -    reflog checks for the pre-existing 'verify' tests.
    +    Add required tests for symref support in 'verify'. Since we're here,
    +    also add reflog checks for the pre-existing 'verify' tests, there is no
    +    divergence from behavior, but we never tested to ensure that reflog
    +    wasn't affected by the 'verify' command.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
    @@ refs.c: int ref_transaction_delete(struct ref_transaction *transaction,
     -		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_oid && old_target)
    ++		BUG("verify called with both old_oid and old_target set");
     +	if (old_target && !(flags & REF_NO_DEREF))
     +		BUG("verify cannot operate on symrefs with deref mode");
      	return ref_transaction_update(transaction, refname,
    @@ t/t1400-update-ref.sh: test_expect_success PIPE 'transaction flushes status upda
      	test_cmp expected actual
      '
      
    -+create_stdin_buf () {
    ++format_command () {
     +	if test "$1" = "-z"
     +	then
     +		shift
    -+		printf "$F" "$@" >stdin
    ++		printf "$F" "$@"
     +	else
    -+		echo "$@" >stdin
    ++		echo "$@"
     +	fi
     +}
     +
     +for type in "" "-z"
     +do
     +
    -+	test_expect_success "stdin ${type} symref-verify fails without --no-deref" '
    ++	test_expect_success "stdin $type symref-verify fails without --no-deref" '
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
     +		grep "fatal: symref-verify: cannot operate with deref mode" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify fails with too many arguments" '
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err  &&
    ++	test_expect_success "stdin $type symref-verify fails with too many arguments" '
    ++		format_command $type "symref-verify refs/heads/symref" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err  &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: test_expect_success PIPE 'transaction flushes status upda
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify succeeds for correct value" '
    ++	test_expect_success "stdin $type symref-verify 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} "symref-verify refs/heads/symref" "$a" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
    ++		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} symref-verify no value is treated as zero value" '
    ++	test_expect_success "stdin $type symref-verify fails with no value" '
     +		git symbolic-ref refs/heads/symref >expect &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
    ++		format_command $type "symref-verify refs/heads/symref" "" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify succeeds for dangling reference" '
    ++	test_expect_success "stdin $type symref-verify succeeds for dangling reference" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
     +		test_must_fail git symbolic-ref refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref2" "refs/heads/nonexistent" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin
    ++		format_command $type "symref-verify refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify fails for missing reference" '
    ++	test_expect_success "stdin $type symref-verify fails for missing reference" '
     +		test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/missing" "refs/heads/unknown" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++		format_command $type "symref-verify refs/heads/missing" "refs/heads/unknown" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: cannot lock ref ${SQ}refs/heads/missing${SQ}: unable to resolve reference ${SQ}refs/heads/missing${SQ}" err &&
     +		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} symref-verify fails for wrong value" '
    ++	test_expect_success "stdin $type symref-verify fails for wrong value" '
     +		git symbolic-ref refs/heads/symref >expect &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$b" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-verify refs/heads/symref" "$b" >stdin &&
    ++		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} symref-verify fails for mistaken null value" '
    ++	test_expect_success "stdin $type symref-verify fails for mistaken null value" '
     +		git symbolic-ref refs/heads/symref >expect &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$Z" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-verify refs/heads/symref" "$Z" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
11:  d07031827b ! 11:  a11f4c1e48 update-ref: add support for 'symref-delete' command
    @@ refs.c: int ref_transaction_create(struct ref_transaction *transaction,
      {
      	if (old_oid && is_null_oid(old_oid))
      		BUG("delete called with old_oid set to zeros");
    ++	if (old_oid && old_target)
    ++		BUG("delete called with both old_oid and old_target set");
     +	if (old_target && !(flags & REF_NO_DEREF))
     +		BUG("delete cannot operate on symrefs with deref mode");
      	return ref_transaction_update(transaction, refname,
    @@ refs.h: int ref_transaction_create(struct ref_transaction *transaction,
      /*
     
      ## t/t1400-update-ref.sh ##
    -@@ t/t1400-update-ref.sh: do
    - 		test_cmp before after
    - 	'
    - 
    --	test_expect_success "stdin ${type} symref-verify no value is treated as zero value" '
    -+	test_expect_success "stdin ${type} symref-verify fails with no value" '
    - 		git symbolic-ref refs/heads/symref >expect &&
    - 		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
    - 		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
     @@ t/t1400-update-ref.sh: do
      		test_cmp expect actual
      	'
      
    -+	test_expect_success "stdin ${type} symref-delete fails without --no-deref" '
    ++	test_expect_success "stdin $type symref-delete fails without --no-deref" '
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
     +		grep "fatal: symref-delete: cannot operate with deref mode" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete fails with no ref" '
    -+		create_stdin_buf ${type} "symref-delete " &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-delete fails with no ref" '
    ++		format_command $type "symref-delete " >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: symref-delete: missing <ref>" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete fails with too many arguments" '
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-delete fails with too many arguments" '
    ++		format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: do
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete fails with wrong old value" '
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-delete fails with wrong old value" '
    ++		format_command $type "symref-delete refs/heads/symref" "$m" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete works with right old value" '
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++	test_expect_success "stdin $type symref-delete works with right old value" '
    ++		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		test_must_fail git rev-parse --verify -q refs/heads/symref
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete works with empty old value" '
    -+		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++	test_expect_success "stdin $type symref-delete works with empty old value" '
    ++		git symbolic-ref refs/heads/symref $a >stdin &&
    ++		format_command $type "symref-delete refs/heads/symref" "" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		test_must_fail git rev-parse --verify -q $b
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete succeeds for dangling reference" '
    ++	test_expect_success "stdin $type symref-delete succeeds for dangling reference" '
     +		test_must_fail git symbolic-ref refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref2" "refs/heads/nonexistent" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		test_must_fail git symbolic-ref -d refs/heads/symref2
     +	'
     +
12:  1038e96a44 ! 12:  9b71c9e07b update-ref: add support for 'symref-create' command
    @@ refs.c: int ref_transaction_update(struct ref_transaction *transaction,
      {
     -	if (!new_oid || is_null_oid(new_oid)) {
     -		strbuf_addf(err, "'%s' has a null OID", refname);
    ++	if (new_oid && new_target)
    ++		BUG("create called with both new_oid and new_target set");
     +	if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
    -+		strbuf_addf(err, "'%s' has a null OID or no new target", refname);
    ++		strbuf_addf(err, "'%s' has neither a valid OID nor a target", refname);
      		return 1;
      	}
      	return ref_transaction_update(transaction, refname, new_oid,
    @@ t/t0600-reffiles-backend.sh: test_expect_success POSIXPERM 'git reflog expire ho
      '
      
     +test_expect_success SYMLINKS 'symref transaction supports symlinks' '
    -+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
    ++	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
     +	git update-ref refs/heads/new @ &&
     +	test_config core.prefersymlinkrefs true &&
     +	cat >stdin <<-EOF &&
     +	start
    -+	symref-create TESTSYMREFONE refs/heads/new
    ++	symref-create TEST_SYMREF_HEAD refs/heads/new
     +	prepare
     +	commit
     +	EOF
     +	git update-ref --no-deref --stdin <stdin &&
    -+	test_path_is_symlink .git/TESTSYMREFONE &&
    -+	test "$(test_readlink .git/TESTSYMREFONE)" = refs/heads/new
    ++	test_path_is_symlink .git/TEST_SYMREF_HEAD &&
    ++	test "$(test_readlink .git/TEST_SYMREF_HEAD)" = refs/heads/new
     +'
     +
     +test_expect_success 'symref transaction supports false symlink config' '
    -+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
    ++	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
     +	git update-ref refs/heads/new @ &&
     +	test_config core.prefersymlinkrefs false &&
     +	cat >stdin <<-EOF &&
     +	start
    -+	symref-create TESTSYMREFONE refs/heads/new
    ++	symref-create TEST_SYMREF_HEAD refs/heads/new
     +	prepare
     +	commit
     +	EOF
     +	git update-ref --no-deref --stdin <stdin &&
    -+	test_path_is_file .git/TESTSYMREFONE &&
    -+	git symbolic-ref TESTSYMREFONE >actual &&
    ++	test_path_is_file .git/TEST_SYMREF_HEAD &&
    ++	git symbolic-ref TEST_SYMREF_HEAD >actual &&
     +	echo refs/heads/new >expect &&
     +	test_cmp expect actual
     +'
    @@ t/t1400-update-ref.sh: do
      		test_must_fail git symbolic-ref -d refs/heads/symref2
      	'
      
    -+	test_expect_success "stdin ${type} symref-create fails with too many arguments" '
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" "$a" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-create fails with too many arguments" '
    ++		format_command $type "symref-create refs/heads/symref" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: do
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create fails with no target" '
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
    ++	test_expect_success "stdin $type symref-create fails with no target" '
    ++		format_command $type "symref-create refs/heads/symref" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create fails with empty target" '
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
    ++	test_expect_success "stdin $type symref-create fails with empty target" '
    ++		format_command $type "symref-create refs/heads/symref" "" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create works" '
    ++	test_expect_success "stdin $type symref-create works" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create works with --no-deref" '
    ++	test_expect_success "stdin $type symref-create works with --no-deref" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" &&
    -+		git update-ref --stdin ${type} <stdin 2>err
    ++		format_command $type "symref-create refs/heads/symref" "$a" &&
    ++		git update-ref --stdin $type <stdin 2>err
     +	'
     +
    -+	test_expect_success "stdin ${type} create dangling symref ref works" '
    ++	test_expect_success "stdin $type create dangling symref ref works" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "refs/heads/unkown" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/heads/symref" "refs/heads/unkown" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo refs/heads/unkown >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create does not create reflogs by default" '
    ++	test_expect_success "stdin $type symref-create does not create reflogs by default" '
     +		test_when_finished "git symbolic-ref -d refs/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/symref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual &&
     +		test_must_fail git reflog exists refs/symref
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create reflogs with --create-reflog" '
    ++	test_expect_success "stdin $type symref-create reflogs with --create-reflog" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" >stdin &&
    -+		git update-ref --create-reflog --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --create-reflog --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual &&
    @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'hook gets all queued symr
      	prepare
      	commit
      	EOF
    +
    + ## t/t5605-clone-local.sh ##
    +@@ t/t5605-clone-local.sh: test_expect_success REFFILES 'local clone from repo with corrupt refs fails grac
    + 	echo a >corrupt/.git/refs/heads/topic &&
    + 
    + 	test_must_fail git clone corrupt working 2>err &&
    +-	grep "has a null OID" err
    ++	grep "has neither a valid OID nor a target" err
    + '
    + 
    + test_done
13:  78dd51b65f = 13:  a9b1a31756 reftable: pick either 'oid' or 'target' for new updates
14:  562e061063 ! 14:  1bbbe86743 update-ref: add support for 'symref-update' command
    @@ Commit message
         OID before the update. This by extension also means that this when a
         zero <old-oid> is provided, it ensures that the ref didn't exist before.
     
    +    The divergence in syntax from the regular `update` command is because if
    +    we don't use a `(ref | oid)` prefix for the old_value, then there is
    +    ambiguity around if the value provided should be treated as an oid or a
    +    reference. This is more so the reason, because we allow anything
    +    committish to be provided as an oid.
    +
         The command allows users to perform symbolic ref updates within a
         transaction. This provides atomicity and allows users to perform a set
         of operations together.
     
    -    This command will also support deref mode, to ensure that we can update
    +    This command supports deref mode, to ensure that we can update
         dereferenced regular refs to symrefs.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
    @@ builtin/update-ref.c: static char *parse_next_refname(const char **next)
     +		return strbuf_detach(&arg, NULL);
     +	return NULL;
     +}
    -+
      
      /*
       * The value being parsed is <old-oid> (as opposed to <new-oid>; the
    @@ builtin/update-ref.c: static void parse_cmd_update(struct ref_transaction *trans
     +	char *old_target = NULL;
     +	struct strbuf err = STRBUF_INIT;
     +	struct object_id old_oid;
    -+	int have_old = 0;
    ++	int have_old_oid = 0;
     +
     +	refname = parse_refname(&next);
     +	if (!refname)
    @@ builtin/update-ref.c: static void parse_cmd_update(struct ref_transaction *trans
     +
     +	old_arg = parse_next_arg(&next);
     +	if (old_arg) {
    -+		old_target = parse_next_refname(&next);
    ++		old_target = parse_next_arg(&next);
     +		if (!old_target)
     +			die("symref-update %s: expected old value", refname);
     +
    -+		if (!strcmp(old_arg, "oid") &&
    -+		    !repo_get_oid(the_repository, old_target, &old_oid)) {
    ++		if (!strcmp(old_arg, "oid")) {
    ++			if (repo_get_oid(the_repository, old_target, &old_oid))
    ++				die("symref-update %s: invalid oid: %s", refname, old_target);
    ++
     +			old_target = NULL;
    -+			have_old = 1;
    -+		} else if (strcmp(old_arg, "ref"))
    ++			have_old_oid = 1;
    ++		} else if (!strcmp(old_arg, "ref")) {
    ++			if (check_refname_format(old_target, REFNAME_ALLOW_ONELEVEL))
    ++				die("symref-update %s: invalid ref: %s", refname, old_target);
    ++		} else {
     +			die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
    ++		}
     +	}
     +
     +	if (*next != line_termination)
     +		die("symref-update %s: extra input: %s", refname, next);
     +
     +	if (ref_transaction_update(transaction, refname, NULL,
    -+				   have_old ? &old_oid : NULL,
    ++				   have_old_oid ? &old_oid : NULL,
     +				   new_target, old_target,
    -+				   update_flags |= create_reflog_flag,
    ++				   update_flags | create_reflog_flag,
     +				   msg, &err))
     +		die("%s", err.buf);
     +
    @@ t/t1400-update-ref.sh: do
      		git reflog exists refs/heads/symref
      	'
      
    -+	test_expect_success "stdin ${type} symref-update fails with too many arguments" '
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "ref" "$a" "$a" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-update fails with too many arguments" '
    ++		format_command $type "symref-update refs/heads/symref" "$a" "ref" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: do
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails with wrong old value argument" '
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "foo" "$a" "$a" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-update fails with wrong old value argument" '
    ++		format_command $type "symref-update refs/heads/symref" "$a" "foo" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: symref-update refs/heads/symref: invalid arg ${SQ}foo${SQ} for old value" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates with zero old value" '
    ++	test_expect_success "stdin $type symref-update creates with zero old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates with no old value" '
    ++	test_expect_success "stdin $type symref-update creates with no old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates dangling" '
    ++	test_expect_success "stdin $type symref-update creates dangling" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "refs/heads/nonexistent" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo refs/heads/nonexistent >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails with wrong old value" '
    ++	test_expect_success "stdin $type symref-update fails with wrong old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" "ref" "$b" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++		format_command $type "symref-update refs/heads/symref" "$m" "ref" "$b" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected $b" err &&
     +		test_must_fail git rev-parse --verify -q $c
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update updates dangling ref" '
    ++	test_expect_success "stdin $type symref-update updates dangling ref" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update updates dangling ref with old value" '
    ++	test_expect_success "stdin $type symref-update updates dangling ref with old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "ref" "refs/heads/nonexistent" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails update dangling ref with wrong old value" '
    ++	test_expect_success "stdin $type symref-update fails update dangling ref with wrong old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "ref" "refs/heads/wrongref" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/wrongref" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
     +		echo refs/heads/nonexistent >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update works with right old value" '
    ++	test_expect_success "stdin $type symref-update works with right old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" "ref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$m" "ref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $m >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update works with no old value" '
    ++	test_expect_success "stdin $type symref-update works with no old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$m" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $m >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails with empty old ref-target" '
    ++	test_expect_success "stdin $type symref-update fails with empty old ref-target" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$m" "ref" "" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$m" "ref" "" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates (with deref)" '
    ++	test_expect_success "stdin $type symref-update creates (with deref)" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/symref >actual &&
     +		test_cmp expect actual &&
    @@ t/t1400-update-ref.sh: do
     +		grep "$Z $(git rev-parse $a)" actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to symref with correct old-oid" '
    ++	test_expect_success "stdin $type symref-update regular ref to symref with correct old-oid" '
     +		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
     +		git update-ref --no-deref refs/heads/regularref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse $a)" >stdin &&
    -+		git update-ref --stdin ${type} <stdin &&
    ++		format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse $a)" >stdin &&
    ++		git update-ref --stdin $type <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
     +		test_cmp expect actual &&
    @@ t/t1400-update-ref.sh: do
     +		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to symref fails with wrong old-oid" '
    ++	test_expect_success "stdin $type symref-update regular ref to symref fails with wrong old-oid" '
    ++		test_when_finished "git update-ref -d refs/heads/regularref" &&
    ++		git update-ref --no-deref refs/heads/regularref $a &&
    ++		format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
    ++		grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}: is at $(git rev-parse $a) but expected $(git rev-parse refs/heads/target2)" err &&
    ++		echo $(git rev-parse $a) >expect &&
    ++		git rev-parse refs/heads/regularref >actual &&
    ++		test_cmp expect actual
    ++	'
    ++
    ++	test_expect_success "stdin $type symref-update regular ref to symref fails with invalid old-oid" '
     +		test_when_finished "git update-ref -d refs/heads/regularref" &&
     +		git update-ref --no-deref refs/heads/regularref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-update refs/heads/regularref" "$a" "oid" "not-a-ref-oid" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
    ++		grep "fatal: symref-update refs/heads/regularref: invalid oid: not-a-ref-oid" err &&
     +		echo $(git rev-parse $a) >expect &&
     +		git rev-parse refs/heads/regularref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update existing symref with zero old-oid" '
    ++	test_expect_success "stdin $type symref-update existing symref with zero old-oid" '
     +		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref refs/heads/target2 &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
     +		grep "fatal: cannot lock ref ${SQ}refs/heads/symref${SQ}: reference already exists" err &&
     +		echo refs/heads/target2 >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to symref (with deref)" '
    ++	test_expect_success "stdin $type symref-update regular ref to symref (with deref)" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_when_finished "git update-ref -d --no-deref refs/heads/symref2" &&
     +		git update-ref refs/heads/symref2 $a &&
     +		git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref" "$a" >stdin &&
    -+		git update-ref ${type} --stdin <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref $type --stdin <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/symref2 >actual &&
     +		test_cmp expect actual &&
    @@ t/t1400-update-ref.sh: do
     +		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to symref" '
    ++	test_expect_success "stdin $type symref-update regular ref to symref" '
     +		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
     +		git update-ref --no-deref refs/heads/regularref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/regularref" "$a" >stdin &&
    -+		git update-ref ${type} --stdin <stdin &&
    ++		format_command $type "symref-update refs/heads/regularref" "$a" >stdin &&
    ++		git update-ref $type --stdin <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
     +		test_cmp expect actual &&

Karthik Nayak (6):
  refs: create and use `ref_update_expects_existing_old_ref()`
  update-ref: add support for 'symref-verify' command
  update-ref: add support for 'symref-delete' command
  update-ref: add support for 'symref-create' command
  reftable: pick either 'oid' or 'target' for new updates
  update-ref: add support for 'symref-update' command

 Documentation/git-update-ref.txt |  25 ++
 builtin/clone.c                  |   2 +-
 builtin/fetch.c                  |   2 +-
 builtin/receive-pack.c           |   3 +-
 builtin/update-ref.c             | 235 ++++++++++++++++-
 refs.c                           |  40 ++-
 refs.h                           |   6 +-
 refs/files-backend.c             |   3 +-
 refs/refs-internal.h             |   6 +
 refs/reftable-backend.c          |   7 +-
 t/t0600-reffiles-backend.sh      |  32 +++
 t/t1400-update-ref.sh            | 416 ++++++++++++++++++++++++++++++-
 t/t1416-ref-transaction-hooks.sh |  54 ++++
 t/t5605-clone-local.sh           |   2 +-
 14 files changed, 799 insertions(+), 34 deletions(-)

-- 
2.43.GIT

[PATCH v2 1/6] refs: create and use `ref_update_expects_existing_old_ref()`

[Karthik Nayak]

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

The files and reftable backend, need to check if a ref must exist, so
that the required validation can be done. A ref must exist only when the
`old_oid` value of the update has been explicitly set and it is not the
`null_oid` value.

Since we also support symrefs now, we need to ensure that even when
`old_target` is set a ref must exist. While this was missed when we
added symref support in transactions, there are no active users of this
path. As we introduce the 'symref-verify' command in the upcoming
commits, it is important to fix this.

So let's export this to a function called
`ref_update_expects_existing_old_ref()` and expose it internally via
'refs-internal.h'.

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

diff --git a/refs.c b/refs.c
index fa5471d219..00e87f8ee3 100644
--- a/refs.c
+++ b/refs.c
@@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
 			    referent, update->old_target);
 	return -1;
 }
+
+int ref_update_expects_existing_old_ref(struct ref_update *update)
+{
+	return (update->flags & REF_HAVE_OLD) &&
+		(!is_null_oid(&update->old_oid) || update->old_target);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3957bfa579..ef760869ca 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 			       struct strbuf *err)
 {
 	struct strbuf referent = STRBUF_INIT;
-	int mustexist = (update->flags & REF_HAVE_OLD) &&
-		!is_null_oid(&update->old_oid);
+	int mustexist = ref_update_expects_existing_old_ref(update);
 	int ret = 0;
 	struct ref_lock *lock;
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 53a6c5d842..ee298ec0d5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
 int ref_update_check_old_target(const char *referent, struct ref_update *update,
 				struct strbuf *err);
 
+/*
+ * Check if the ref must exist, this means that the old_oid or
+ * old_target is non NULL.
+ */
+int ref_update_expects_existing_old_ref(struct ref_update *update);
+
 #endif /* REFS_REFS_INTERNAL_H */
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 98cebbcf39..dc7aa7f274 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -825,7 +825,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 					      &current_oid, &referent, &u->type);
 		if (ret < 0)
 			goto done;
-		if (ret > 0 && (!(u->flags & REF_HAVE_OLD) || is_null_oid(&u->old_oid))) {
+		if (ret > 0 && !ref_update_expects_existing_old_ref(u)) {
 			/*
 			 * The reference does not exist, and we either have no
 			 * old object ID or expect the reference to not exist.
-- 
2.43.GIT

[PATCH v2 2/6] update-ref: add support for 'symref-verify' command

[Karthik Nayak]

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

The 'symref-verify' command allows users to verify if a provided <ref>
contains the provided <old-target> without changing the <ref>. If
<old-target> is not provided, the command will verify that the <ref>
doesn't exist.

The command allows users to verify symbolic refs within a transaction,
and this means users can perform a set of changes in a transaction only
when the verification holds good.

Since we're checking for symbolic refs, this command will only work with
the 'no-deref' mode. This is because any dereferenced symbolic ref will
point to an object and not a ref and the regular 'verify' command can be
used in such situations.

Add required tests for symref support in 'verify'. Since we're here,
also add reflog checks for the pre-existing 'verify' tests, there is no
divergence from behavior, but we never tested to ensure that reflog
wasn't affected by the 'verify' command.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |  7 +++
 builtin/update-ref.c             | 80 +++++++++++++++++++++++----
 refs.c                           | 11 +++-
 refs.h                           |  1 +
 t/t1400-update-ref.sh            | 94 +++++++++++++++++++++++++++++++-
 t/t1416-ref-transaction-hooks.sh | 30 ++++++++++
 6 files changed, 208 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 374a2ebd2b..9fe78b3501 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-verify SP <ref> [SP <old-target>] LF
 	option SP <opt> LF
 	start LF
 	prepare LF
@@ -86,6 +87,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-verify SP <ref> [NUL <old-target>] NUL
 	option SP <opt> NUL
 	start NUL
 	prepare NUL
@@ -117,6 +119,11 @@ verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-verify::
+	Verify symbolic <ref> against <old-target> but do not change it.
+	If <old-target> is missing, the ref must not exist.  Can only be
+	used in `no-deref` mode.
+
 option::
 	Modify the behavior of the next command naming a <ref>.
 	The only valid option is `no-deref` to avoid dereferencing
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 21fdbf6ac8..6dce1cd663 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -76,6 +76,29 @@ static char *parse_refname(const char **next)
 	return strbuf_detach(&ref, NULL);
 }
 
+/*
+ * Wrapper around parse_refname which skips the next delimiter.
+ */
+static char *parse_next_refname(const char **next)
+{
+	if (line_termination) {
+		/* Without -z, consume SP and use next argument */
+		if (!**next || **next == line_termination)
+			return NULL;
+		if (**next != ' ')
+			die("expected SP but got: %s", *next);
+	} else {
+		/* With -z, read the next NUL-terminated line */
+		if (**next)
+			return NULL;
+	}
+	/* Skip the delimiter */
+	(*next)++;
+
+	return parse_refname(next);
+}
+
+
 /*
  * The value being parsed is <old-oid> (as opposed to <new-oid>; the
  * difference affects which error messages are generated):
@@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
 		die("verify %s: extra input: %s", refname, next);
 
 	if (ref_transaction_verify(transaction, refname, &old_oid,
-				   update_flags, &err))
+				   NULL, update_flags, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	strbuf_release(&err);
+}
+
+static void parse_cmd_symref_verify(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	struct strbuf err = STRBUF_INIT;
+	struct object_id old_oid;
+	char *refname, *old_target;
+
+	if (!(update_flags & REF_NO_DEREF))
+		die("symref-verify: cannot operate with deref mode");
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-verify: missing <ref>");
+
+	/*
+	 * old_ref is optional, if not provided, we need to ensure that the
+	 * ref doesn't exist.
+	 */
+	old_target = parse_next_refname(&next);
+	if (!old_target)
+		oidcpy(&old_oid, null_oid());
+
+	if (*next != line_termination)
+		die("symref-verify %s: extra input: %s", refname, next);
+
+	if (ref_transaction_verify(transaction, refname,
+				   old_target ? NULL : &old_oid,
+				   old_target, update_flags, &err))
 		die("%s", err.buf);
 
 	update_flags = default_flags;
 	free(refname);
+	free(old_target);
 	strbuf_release(&err);
 }
 
@@ -380,15 +439,16 @@ static const struct parse_cmd {
 	unsigned args;
 	enum update_refs_state state;
 } command[] = {
-	{ "update",  parse_cmd_update,  3, UPDATE_REFS_OPEN },
-	{ "create",  parse_cmd_create,  2, UPDATE_REFS_OPEN },
-	{ "delete",  parse_cmd_delete,  2, UPDATE_REFS_OPEN },
-	{ "verify",  parse_cmd_verify,  2, UPDATE_REFS_OPEN },
-	{ "option",  parse_cmd_option,  1, UPDATE_REFS_OPEN },
-	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
-	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
-	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
-	{ "commit",  parse_cmd_commit,  0, UPDATE_REFS_CLOSED },
+	{ "update",        parse_cmd_update,        3, UPDATE_REFS_OPEN },
+	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
+	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
+	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
+	{ "option",        parse_cmd_option,        1, UPDATE_REFS_OPEN },
+	{ "start",         parse_cmd_start,         0, UPDATE_REFS_STARTED },
+	{ "prepare",       parse_cmd_prepare,       0, UPDATE_REFS_PREPARED },
+	{ "abort",         parse_cmd_abort,         0, UPDATE_REFS_CLOSED },
+	{ "commit",        parse_cmd_commit,        0, UPDATE_REFS_CLOSED },
 };
 
 static void update_refs_stdin(void)
diff --git a/refs.c b/refs.c
index 00e87f8ee3..865264d487 100644
--- a/refs.c
+++ b/refs.c
@@ -1331,14 +1331,19 @@ 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_oid && old_target)
+		BUG("verify called with both old_oid and old_target set");
+	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);
 }
 
diff --git a/refs.h b/refs.h
index 71cc1c58e0..48cec1ba72 100644
--- a/refs.h
+++ b/refs.h
@@ -781,6 +781,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/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ec3443cc87..07e111b063 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,88 @@ test_expect_success PIPE 'transaction flushes status updates' '
 	test_cmp expected actual
 '
 
+format_command () {
+	if test "$1" = "-z"
+	then
+		shift
+		printf "$F" "$@"
+	else
+		echo "$@"
+	fi
+}
+
+for type in "" "-z"
+do
+
+	test_expect_success "stdin $type symref-verify fails without --no-deref" '
+		git symbolic-ref refs/heads/symref $a &&
+		format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type <stdin 2>err &&
+		grep "fatal: symref-verify: cannot operate with deref mode" err
+	'
+
+	test_expect_success "stdin $type symref-verify fails with too many arguments" '
+		format_command $type "symref-verify refs/heads/symref" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err  &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-verify refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin $type symref-verify succeeds for correct value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+		format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
+		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 symref-verify fails with no value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		format_command $type "symref-verify refs/heads/symref" "" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin
+	'
+
+	test_expect_success "stdin $type symref-verify succeeds for dangling reference" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
+		test_must_fail git symbolic-ref refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+		format_command $type "symref-verify refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin
+	'
+
+	test_expect_success "stdin $type symref-verify fails for missing reference" '
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+		format_command $type "symref-verify refs/heads/missing" "refs/heads/unknown" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		grep "fatal: cannot lock ref ${SQ}refs/heads/missing${SQ}: unable to resolve reference ${SQ}refs/heads/missing${SQ}" err &&
+		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 symref-verify fails for wrong value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		format_command $type "symref-verify refs/heads/symref" "$b" >stdin &&
+		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 symref-verify fails for mistaken null value" '
+		git symbolic-ref refs/heads/symref >expect &&
+		format_command $type "symref-verify refs/heads/symref" "$Z" >stdin &&
+		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
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 067fd57290..fd58b902f4 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
 	test_cmp expect actual
 '
 
+test_expect_success 'hook gets all queued symref updates' '
+	test_when_finished "rm actual" &&
+
+	git update-ref refs/heads/branch $POST_OID &&
+	git symbolic-ref refs/heads/symref refs/heads/main &&
+
+	test_hook reference-transaction <<-\EOF &&
+	echo "$*" >>actual
+	while read -r line
+	do
+		printf "%s\n" "$line"
+	done >>actual
+	EOF
+
+	cat >expect <<-EOF &&
+	prepared
+	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	committed
+	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	EOF
+
+	git update-ref --no-deref --stdin <<-EOF &&
+	start
+	symref-verify refs/heads/symref refs/heads/main
+	prepare
+	commit
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.43.GIT

[PATCH v2 3/6] update-ref: add support for 'symref-delete' command

[Karthik Nayak]

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

Add a new command 'symref-delete' to allow deletions of symbolic refs in
a transaction via the '--stdin' mode of the 'git-update-ref' command.
The 'symref-delete' command can, when given an <old-target>, delete the
provided <ref> only when it points to <old-target>.

This command is only compatible with the 'no-deref' mode because we
optionally want to check the 'old_target' of the ref being deleted.
De-referencing a symbolic ref would provide a regular ref and we already
have the 'delete' command for regular refs.

While users can also use 'git symbolic-ref -d' to delete symbolic refs,
the 'symref-delete' command in 'git-update-ref' allows users to do so
within a transaction, which promises atomicity of the operation and can
be batched with other commands.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |  5 +++
 builtin/fetch.c                  |  2 +-
 builtin/receive-pack.c           |  3 +-
 builtin/update-ref.c             | 33 ++++++++++++++++++-
 refs.c                           | 14 ++++++---
 refs.h                           |  4 ++-
 t/t1400-update-ref.sh            | 54 ++++++++++++++++++++++++++++++++
 t/t1416-ref-transaction-hooks.sh | 19 ++++++++++-
 8 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9fe78b3501..16e02f6979 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-delete SP <ref> [SP <old-target>] LF
 	symref-verify SP <ref> [SP <old-target>] LF
 	option SP <opt> LF
 	start LF
@@ -87,6 +88,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-delete SP <ref> [NUL <old-target>] NUL
 	symref-verify SP <ref> [NUL <old-target>] NUL
 	option SP <opt> NUL
 	start NUL
@@ -119,6 +121,9 @@ verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-delete::
+	Delete <ref> after verifying it exists with <old-target>, if given.
+
 symref-verify::
 	Verify symbolic <ref> against <old-target> but do not change it.
 	If <old-target> is missing, the ref must not exist.  Can only be
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 66840b7c5b..d02592efca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1383,7 +1383,7 @@ static int prune_refs(struct display_state *display_state,
 		if (transaction) {
 			for (ref = stale_refs; ref; ref = ref->next) {
 				result = ref_transaction_delete(transaction, ref->name, NULL, 0,
-								"fetch: prune", &err);
+								NULL, "fetch: prune", &err);
 				if (result)
 					goto cleanup;
 			}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b150ef39a8..9a4667d57d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1576,7 +1576,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		if (ref_transaction_delete(transaction,
 					   namespaced_name,
 					   old_oid,
-					   0, "push", &err)) {
+					   0, NULL,
+					   "push", &err)) {
 			rp_error("%s", err.buf);
 			ret = "failed to delete";
 		} else {
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6dce1cd663..0ef9c38d8d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -293,7 +293,7 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
 
 	if (ref_transaction_delete(transaction, refname,
 				   have_old ? &old_oid : NULL,
-				   update_flags, msg, &err))
+				   update_flags, NULL, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = default_flags;
@@ -301,6 +301,36 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
 	strbuf_release(&err);
 }
 
+
+static void parse_cmd_symref_delete(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *refname, *old_target;
+
+	if (!(update_flags & REF_NO_DEREF))
+		die("symref-delete: cannot operate with deref mode");
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-delete: missing <ref>");
+
+	old_target = parse_next_refname(&next);
+
+	if (*next != line_termination)
+		die("symref-delete %s: extra input: %s", refname, next);
+
+	if (ref_transaction_delete(transaction, refname, NULL,
+				   update_flags, old_target, msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(old_target);
+	strbuf_release(&err);
+}
+
+
 static void parse_cmd_verify(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -443,6 +473,7 @@ static const struct parse_cmd {
 	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
 	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
 	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
 	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
 	{ "option",        parse_cmd_option,        1, UPDATE_REFS_OPEN },
 	{ "start",         parse_cmd_start,         0, UPDATE_REFS_STARTED },
diff --git a/refs.c b/refs.c
index 865264d487..3afebfcb63 100644
--- a/refs.c
+++ b/refs.c
@@ -979,7 +979,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
 	transaction = ref_store_transaction_begin(refs, &err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_oid,
-				   flags, msg, &err) ||
+				   flags, NULL, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
@@ -1317,14 +1317,20 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int flags,
+			   const char *old_target,
+			   const char *msg,
 			   struct strbuf *err)
 {
 	if (old_oid && is_null_oid(old_oid))
 		BUG("delete called with old_oid set to zeros");
+	if (old_oid && old_target)
+		BUG("delete called with both old_oid and old_target set");
+	if (old_target && !(flags & REF_NO_DEREF))
+		BUG("delete cannot operate on symrefs with deref mode");
 	return ref_transaction_update(transaction, refname,
 				      null_oid(), old_oid,
-				      NULL, NULL, flags,
+				      NULL, old_target, flags,
 				      msg, err);
 }
 
@@ -2767,7 +2773,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,
 
 	for_each_string_list_item(item, refnames) {
 		ret = ref_transaction_delete(transaction, item->string,
-					     NULL, flags, msg, &err);
+					     NULL, flags, NULL, msg, &err);
 		if (ret) {
 			warning(_("could not delete reference %s: %s"),
 				item->string, err.buf);
diff --git a/refs.h b/refs.h
index 48cec1ba72..053c382f0b 100644
--- a/refs.h
+++ b/refs.h
@@ -767,7 +767,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int flags,
+			   const char *old_target,
+			   const char *msg,
 			   struct strbuf *err);
 
 /*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 07e111b063..22d560f0bf 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1729,6 +1729,60 @@ do
 		test_cmp expect actual
 	'
 
+	test_expect_success "stdin $type symref-delete fails without --no-deref" '
+		git symbolic-ref refs/heads/symref $a &&
+		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type <stdin 2>err &&
+		grep "fatal: symref-delete: cannot operate with deref mode" err
+	'
+
+	test_expect_success "stdin $type symref-delete fails with no ref" '
+		format_command $type "symref-delete " >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		grep "fatal: symref-delete: missing <ref>" err
+	'
+
+	test_expect_success "stdin $type symref-delete fails with too many arguments" '
+		format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-delete refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin $type symref-delete fails with wrong old value" '
+		format_command $type "symref-delete refs/heads/symref" "$m" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-delete works with right old value" '
+		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		test_must_fail git rev-parse --verify -q refs/heads/symref
+	'
+
+	test_expect_success "stdin $type symref-delete works with empty old value" '
+		git symbolic-ref refs/heads/symref $a >stdin &&
+		format_command $type "symref-delete refs/heads/symref" "" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		test_must_fail git rev-parse --verify -q $b
+	'
+
+	test_expect_success "stdin $type symref-delete succeeds for dangling reference" '
+		test_must_fail git symbolic-ref refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+		format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		test_must_fail git symbolic-ref -d refs/heads/symref2
+	'
+
 done
 
 test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index fd58b902f4..ccde1b944b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -162,6 +162,7 @@ test_expect_success 'hook gets all queued symref updates' '
 
 	git update-ref refs/heads/branch $POST_OID &&
 	git symbolic-ref refs/heads/symref refs/heads/main &&
+	git symbolic-ref refs/heads/symrefd refs/heads/main &&
 
 	test_hook reference-transaction <<-\EOF &&
 	echo "$*" >>actual
@@ -171,16 +172,32 @@ test_expect_success 'hook gets all queued symref updates' '
 	done >>actual
 	EOF
 
-	cat >expect <<-EOF &&
+	# In the files backend, "delete" also triggers an additional transaction
+	# update on the packed-refs backend, which constitutes additional reflog
+	# entries.
+	if test_have_prereq REFFILES
+	then
+		cat >expect <<-EOF
+		aborted
+		$ZERO_OID $ZERO_OID refs/heads/symrefd
+		EOF
+	else
+		>expect
+	fi &&
+
+	cat >>expect <<-EOF &&
 	prepared
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	committed
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
+	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	EOF
 
 	git update-ref --no-deref --stdin <<-EOF &&
 	start
 	symref-verify refs/heads/symref refs/heads/main
+	symref-delete refs/heads/symrefd refs/heads/main
 	prepare
 	commit
 	EOF
-- 
2.43.GIT

[PATCH v2 4/6] update-ref: add support for 'symref-create' command

[Karthik Nayak]

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

Add 'symref-create' command to the '--stdin' mode 'git-update-ref' to
allow creation of symbolic refs in a transaction. The 'symref-create'
command takes in a <new-target>, which the created <ref> will point to.

Also, support the 'core.prefersymlinkrefs' config, wherein if the config
is set and the filesystem supports symlinks, we create the symbolic ref
as a symlink. We fallback to creating a regular symref if creating the
symlink is unsuccessful.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |  6 +++
 builtin/clone.c                  |  2 +-
 builtin/update-ref.c             | 32 +++++++++++++++-
 refs.c                           |  9 +++--
 refs.h                           |  1 +
 t/t0600-reffiles-backend.sh      | 32 ++++++++++++++++
 t/t1400-update-ref.sh            | 65 ++++++++++++++++++++++++++++++++
 t/t1416-ref-transaction-hooks.sh |  3 ++
 t/t5605-clone-local.sh           |  2 +-
 9 files changed, 146 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 16e02f6979..364ef78af1 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-create SP <ref> SP <new-target> LF
 	symref-delete SP <ref> [SP <old-target>] LF
 	symref-verify SP <ref> [SP <old-target>] LF
 	option SP <opt> LF
@@ -88,6 +89,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-create SP <ref> NUL <new-target> NUL
 	symref-delete SP <ref> [NUL <old-target>] NUL
 	symref-verify SP <ref> [NUL <old-target>] NUL
 	option SP <opt> NUL
@@ -121,6 +123,10 @@ verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-create:
+	Create symbolic ref <ref> with <new-target> after verifying
+	it does not exist.
+
 symref-delete::
 	Delete <ref> after verifying it exists with <old-target>, if given.
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 93fdfc945a..ac1e131d8b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -546,7 +546,7 @@ static void write_remote_refs(const struct ref *local_refs)
 		if (!r->peer_ref)
 			continue;
 		if (ref_transaction_create(t, r->peer_ref->name, &r->old_oid,
-					   0, NULL, &err))
+					   NULL, 0, NULL, &err))
 			die("%s", err.buf);
 	}
 
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0ef9c38d8d..16d184603b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,7 +257,7 @@ static void parse_cmd_create(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	if (ref_transaction_create(transaction, refname, &new_oid,
+	if (ref_transaction_create(transaction, refname, &new_oid, NULL,
 				   update_flags | create_reflog_flag,
 				   msg, &err))
 		die("%s", err.buf);
@@ -267,6 +267,35 @@ static void parse_cmd_create(struct ref_transaction *transaction,
 	strbuf_release(&err);
 }
 
+
+static void parse_cmd_symref_create(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *refname, *new_target;
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-create: missing <ref>");
+
+	new_target = parse_next_refname(&next);
+	if (!new_target)
+		die("symref-create %s: missing <new-target>", refname);
+
+	if (*next != line_termination)
+		die("symref-create %s: extra input: %s", refname, next);
+
+	if (ref_transaction_create(transaction, refname, NULL, new_target,
+				   update_flags | create_reflog_flag,
+				   msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(new_target);
+	strbuf_release(&err);
+}
+
 static void parse_cmd_delete(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -473,6 +502,7 @@ static const struct parse_cmd {
 	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
 	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
 	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
 	{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
 	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
 	{ "option",        parse_cmd_option,        1, UPDATE_REFS_OPEN },
diff --git a/refs.c b/refs.c
index 3afebfcb63..7172f31bf2 100644
--- a/refs.c
+++ b/refs.c
@@ -1302,15 +1302,18 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
+			   const char *new_target,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err)
 {
-	if (!new_oid || is_null_oid(new_oid)) {
-		strbuf_addf(err, "'%s' has a null OID", refname);
+	if (new_oid && new_target)
+		BUG("create called with both new_oid and new_target set");
+	if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
+		strbuf_addf(err, "'%s' has neither a valid OID nor a target", refname);
 		return 1;
 	}
 	return ref_transaction_update(transaction, refname, new_oid,
-				      null_oid(), NULL, NULL, flags,
+				      null_oid(), new_target, NULL, flags,
 				      msg, err);
 }
 
diff --git a/refs.h b/refs.h
index 053c382f0b..055cc9173b 100644
--- a/refs.h
+++ b/refs.h
@@ -753,6 +753,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
+			   const char *new_target,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err);
 
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index a390cffc80..7cfb5e8c6c 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	esac
 '
 
+test_expect_success SYMLINKS 'symref transaction supports symlinks' '
+	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
+	git update-ref refs/heads/new @ &&
+	test_config core.prefersymlinkrefs true &&
+	cat >stdin <<-EOF &&
+	start
+	symref-create TEST_SYMREF_HEAD refs/heads/new
+	prepare
+	commit
+	EOF
+	git update-ref --no-deref --stdin <stdin &&
+	test_path_is_symlink .git/TEST_SYMREF_HEAD &&
+	test "$(test_readlink .git/TEST_SYMREF_HEAD)" = refs/heads/new
+'
+
+test_expect_success 'symref transaction supports false symlink config' '
+	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
+	git update-ref refs/heads/new @ &&
+	test_config core.prefersymlinkrefs false &&
+	cat >stdin <<-EOF &&
+	start
+	symref-create TEST_SYMREF_HEAD refs/heads/new
+	prepare
+	commit
+	EOF
+	git update-ref --no-deref --stdin <stdin &&
+	test_path_is_file .git/TEST_SYMREF_HEAD &&
+	git symbolic-ref TEST_SYMREF_HEAD >actual &&
+	echo refs/heads/new >expect &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 22d560f0bf..78001f879a 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1783,6 +1783,71 @@ do
 		test_must_fail git symbolic-ref -d refs/heads/symref2
 	'
 
+	test_expect_success "stdin $type symref-create fails with too many arguments" '
+		format_command $type "symref-create refs/heads/symref" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-create refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin $type symref-create fails with no target" '
+		format_command $type "symref-create refs/heads/symref" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin
+	'
+
+	test_expect_success "stdin $type symref-create fails with empty target" '
+		format_command $type "symref-create refs/heads/symref" "" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin
+	'
+
+	test_expect_success "stdin $type symref-create works" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-create works with --no-deref" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-create refs/heads/symref" "$a" &&
+		git update-ref --stdin $type <stdin 2>err
+	'
+
+	test_expect_success "stdin $type create dangling symref ref works" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-create refs/heads/symref" "refs/heads/unkown" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo refs/heads/unkown >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-create does not create reflogs by default" '
+		test_when_finished "git symbolic-ref -d refs/symref" &&
+		format_command $type "symref-create refs/symref" "$a" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		git symbolic-ref refs/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual &&
+		test_must_fail git reflog exists refs/symref
+	'
+
+	test_expect_success "stdin $type symref-create reflogs with --create-reflog" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
+		git update-ref --create-reflog --stdin $type --no-deref <stdin &&
+		git symbolic-ref refs/heads/symref >expect &&
+		echo $a >actual &&
+		test_cmp expect actual &&
+		git reflog exists refs/heads/symref
+	'
+
 done
 
 test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ccde1b944b..ff77dcca6b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -189,15 +189,18 @@ test_expect_success 'hook gets all queued symref updates' '
 	prepared
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
 	committed
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
 	EOF
 
 	git update-ref --no-deref --stdin <<-EOF &&
 	start
 	symref-verify refs/heads/symref refs/heads/main
 	symref-delete refs/heads/symrefd refs/heads/main
+	symref-create refs/heads/symrefc refs/heads/main
 	prepare
 	commit
 	EOF
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index a3055869bc..339d8c786f 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -163,7 +163,7 @@ test_expect_success REFFILES 'local clone from repo with corrupt refs fails grac
 	echo a >corrupt/.git/refs/heads/topic &&
 
 	test_must_fail git clone corrupt working 2>err &&
-	grep "has a null OID" err
+	grep "has neither a valid OID nor a target" err
 '
 
 test_done
-- 
2.43.GIT

[PATCH v2 5/6] reftable: pick either 'oid' or 'target' for new updates

[Karthik Nayak]

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

When creating a reference transaction update, we can provide the old/new
oid/target for the update. We have checks in place to ensure that for
each old/new, either oid or target is set and not both.

In the reftable backend, when dealing with updates without the
`REF_NO_DEREF` flag, we don't selectively propagate data as needed.
Since there are no active users of the path, this is not caught. As we
want to introduce the 'symref-update' command in the upcoming commit,
which would use this flow, correct it.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/reftable-backend.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index dc7aa7f274..8582f2ff2f 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -896,8 +896,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 				 */
 				new_update = ref_transaction_add_update(
 					transaction, referent.buf, new_flags,
-					&u->new_oid, &u->old_oid, u->new_target,
-					u->old_target, u->msg);
+					u->new_target ? NULL : &u->new_oid,
+					u->old_target ? NULL : &u->old_oid,
+					u->new_target, u->old_target, u->msg);
 
 				new_update->parent_update = u;
 
-- 
2.43.GIT

[PATCH v2 6/6] update-ref: add support for 'symref-update' command

[Karthik Nayak]

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

Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
allow updates of symbolic refs. The 'symref-update' command takes in a
<new-target>, which the <ref> will be updated to. If the <ref> doesn't
exist it will be created.

It also optionally takes either an `ref <old-target>` or `oid
<old-oid>`. If the <old-target> is provided, it checks to see if the
<ref> targets the <old-target> before the update. If <old-oid> is provided
it checks <ref> to ensure that it is a regular ref and <old-oid> is the
OID before the update. This by extension also means that this when a
zero <old-oid> is provided, it ensures that the ref didn't exist before.

The divergence in syntax from the regular `update` command is because if
we don't use a `(ref | oid)` prefix for the old_value, then there is
ambiguity around if the value provided should be treated as an oid or a
reference. This is more so the reason, because we allow anything
committish to be provided as an oid.

The command allows users to perform symbolic ref updates within a
transaction. This provides atomicity and allows users to perform a set
of operations together.

This command supports deref mode, to ensure that we can update
dereferenced regular refs to symrefs.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |   7 ++
 builtin/update-ref.c             |  90 ++++++++++++++
 t/t1400-update-ref.sh            | 203 +++++++++++++++++++++++++++++++
 t/t1416-ref-transaction-hooks.sh |   4 +
 4 files changed, 304 insertions(+)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 364ef78af1..afcf33cf60 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-update SP <ref> SP <new-target> [SP (ref SP <old-target> | oid SP <old-oid>)] LF
 	symref-create SP <ref> SP <new-target> LF
 	symref-delete SP <ref> [SP <old-target>] LF
 	symref-verify SP <ref> [SP <old-target>] LF
@@ -89,6 +90,7 @@ quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-update SP <ref> NUL <new-target> [NUL (ref NUL <old-target> | oid NUL <old-oid>)] NUL
 	symref-create SP <ref> NUL <new-target> NUL
 	symref-delete SP <ref> [NUL <old-target>] NUL
 	symref-verify SP <ref> [NUL <old-target>] NUL
@@ -119,6 +121,11 @@ delete::
 	Delete <ref> after verifying it exists with <old-oid>, if
 	given.  If given, <old-oid> may not be zero.
 
+symref-update::
+	Set <ref> to <new-target> after verifying <old-target> or <old-oid>,
+	if given. Specify a zero <old-oid> to ensure that the ref does not
+	exist before the update.
+
 verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 16d184603b..bda37c161d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -98,6 +98,40 @@ static char *parse_next_refname(const char **next)
 	return parse_refname(next);
 }
 
+/*
+ * Wrapper around parse_arg which skips the next delimiter.
+ */
+static char *parse_next_arg(const char **next)
+{
+	struct strbuf arg = STRBUF_INIT;
+
+	if (line_termination) {
+		/* Without -z, consume SP and use next argument */
+		if (!**next || **next == line_termination)
+			return NULL;
+		if (**next != ' ')
+			die("expected SP but got: %s", *next);
+	} else {
+		/* With -z, read the next NUL-terminated line */
+		if (**next)
+			return NULL;
+	}
+	/* Skip the delimiter */
+	(*next)++;
+
+	if (line_termination) {
+		/* Without -z, use the next argument */
+		*next = parse_arg(*next, &arg);
+	} else {
+		/* With -z, use everything up to the next NUL */
+		strbuf_addstr(&arg, *next);
+		*next += arg.len;
+	}
+
+	if (arg.len)
+		return strbuf_detach(&arg, NULL);
+	return NULL;
+}
 
 /*
  * The value being parsed is <old-oid> (as opposed to <new-oid>; the
@@ -237,6 +271,61 @@ static void parse_cmd_update(struct ref_transaction *transaction,
 	strbuf_release(&err);
 }
 
+static void parse_cmd_symref_update(struct ref_transaction *transaction,
+				    const char *next, const char *end)
+{
+	char *refname, *new_target, *old_arg;
+	char *old_target = NULL;
+	struct strbuf err = STRBUF_INIT;
+	struct object_id old_oid;
+	int have_old_oid = 0;
+
+	refname = parse_refname(&next);
+	if (!refname)
+		die("symref-update: missing <ref>");
+
+	new_target = parse_next_refname(&next);
+	if (!new_target)
+		die("symref-update %s: missing <new-target>", refname);
+
+	old_arg = parse_next_arg(&next);
+	if (old_arg) {
+		old_target = parse_next_arg(&next);
+		if (!old_target)
+			die("symref-update %s: expected old value", refname);
+
+		if (!strcmp(old_arg, "oid")) {
+			if (repo_get_oid(the_repository, old_target, &old_oid))
+				die("symref-update %s: invalid oid: %s", refname, old_target);
+
+			old_target = NULL;
+			have_old_oid = 1;
+		} else if (!strcmp(old_arg, "ref")) {
+			if (check_refname_format(old_target, REFNAME_ALLOW_ONELEVEL))
+				die("symref-update %s: invalid ref: %s", refname, old_target);
+		} else {
+			die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
+		}
+	}
+
+	if (*next != line_termination)
+		die("symref-update %s: extra input: %s", refname, next);
+
+	if (ref_transaction_update(transaction, refname, NULL,
+				   have_old_oid ? &old_oid : NULL,
+				   new_target, old_target,
+				   update_flags | create_reflog_flag,
+				   msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(old_arg);
+	free(old_target);
+	free(new_target);
+	strbuf_release(&err);
+}
+
 static void parse_cmd_create(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -502,6 +591,7 @@ static const struct parse_cmd {
 	{ "create",        parse_cmd_create,        2, UPDATE_REFS_OPEN },
 	{ "delete",        parse_cmd_delete,        2, UPDATE_REFS_OPEN },
 	{ "verify",        parse_cmd_verify,        2, UPDATE_REFS_OPEN },
+	{ "symref-update", parse_cmd_symref_update, 4, UPDATE_REFS_OPEN },
 	{ "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
 	{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
 	{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 78001f879a..68afe7c228 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1360,6 +1360,7 @@ test_expect_success 'fails with duplicate HEAD update' '
 '
 
 test_expect_success 'fails with duplicate ref update via symref' '
+	test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
 	git branch target2 $A &&
 	git symbolic-ref refs/heads/symref2 refs/heads/target2 &&
 	cat >stdin <<-EOF &&
@@ -1848,6 +1849,208 @@ do
 		git reflog exists refs/heads/symref
 	'
 
+	test_expect_success "stdin $type symref-update fails with too many arguments" '
+		format_command $type "symref-update refs/heads/symref" "$a" "ref" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		if test "$type" = "-z"
+		then
+			grep "fatal: unknown command: $a" err
+		else
+			grep "fatal: symref-update refs/heads/symref: extra input:  $a" err
+		fi
+	'
+
+	test_expect_success "stdin $type symref-update fails with wrong old value argument" '
+		format_command $type "symref-update refs/heads/symref" "$a" "foo" "$a" "$a" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		grep "fatal: symref-update refs/heads/symref: invalid arg ${SQ}foo${SQ} for old value" err
+	'
+
+	test_expect_success "stdin $type symref-update creates with zero old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update creates with no old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update creates dangling" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		format_command $type "symref-update refs/heads/symref" "refs/heads/nonexistent" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo refs/heads/nonexistent >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update fails with wrong old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		format_command $type "symref-update refs/heads/symref" "$m" "ref" "$b" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected $b" err &&
+		test_must_fail git rev-parse --verify -q $c
+	'
+
+	test_expect_success "stdin $type symref-update updates dangling ref" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update updates dangling ref with old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+		format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/nonexistent" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update fails update dangling ref with wrong old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_must_fail git rev-parse refs/heads/nonexistent &&
+		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+		format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/wrongref" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+		echo refs/heads/nonexistent >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update works with right old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		format_command $type "symref-update refs/heads/symref" "$m" "ref" "$a" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo $m >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update works with no old value" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		format_command $type "symref-update refs/heads/symref" "$m" >stdin &&
+		git update-ref --stdin $type --no-deref <stdin &&
+		echo $m >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update fails with empty old ref-target" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref $a &&
+		format_command $type "symref-update refs/heads/symref" "$m" "ref" "" >stdin &&
+		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+		echo $a >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update creates (with deref)" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref --stdin $type <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/symref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+		grep "$Z $(git rev-parse $a)" actual
+	'
+
+	test_expect_success "stdin $type symref-update regular ref to symref with correct old-oid" '
+		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse $a)" >stdin &&
+		git update-ref --stdin $type <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+	'
+
+	test_expect_success "stdin $type symref-update regular ref to symref fails with wrong old-oid" '
+		test_when_finished "git update-ref -d refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
+		test_must_fail git update-ref --stdin $type <stdin 2>err &&
+		grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}: is at $(git rev-parse $a) but expected $(git rev-parse refs/heads/target2)" err &&
+		echo $(git rev-parse $a) >expect &&
+		git rev-parse refs/heads/regularref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update regular ref to symref fails with invalid old-oid" '
+		test_when_finished "git update-ref -d refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		format_command $type "symref-update refs/heads/regularref" "$a" "oid" "not-a-ref-oid" >stdin &&
+		test_must_fail git update-ref --stdin $type <stdin 2>err &&
+		grep "fatal: symref-update refs/heads/regularref: invalid oid: not-a-ref-oid" err &&
+		echo $(git rev-parse $a) >expect &&
+		git rev-parse refs/heads/regularref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update existing symref with zero old-oid" '
+		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/symref" &&
+		git symbolic-ref refs/heads/symref refs/heads/target2 &&
+		format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+		test_must_fail git update-ref --stdin $type <stdin 2>err &&
+		grep "fatal: cannot lock ref ${SQ}refs/heads/symref${SQ}: reference already exists" err &&
+		echo refs/heads/target2 >expect &&
+		git symbolic-ref refs/heads/symref >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "stdin $type symref-update regular ref to symref (with deref)" '
+		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+		test_when_finished "git update-ref -d --no-deref refs/heads/symref2" &&
+		git update-ref refs/heads/symref2 $a &&
+		git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 &&
+		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+		git update-ref $type --stdin <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/symref2 >actual &&
+		test_cmp expect actual &&
+		echo refs/heads/symref2 >expect &&
+		git symbolic-ref --no-recurse refs/heads/symref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+	'
+
+	test_expect_success "stdin $type symref-update regular ref to symref" '
+		test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+		git update-ref --no-deref refs/heads/regularref $a &&
+		format_command $type "symref-update refs/heads/regularref" "$a" >stdin &&
+		git update-ref $type --stdin <stdin &&
+		echo $a >expect &&
+		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+		test_cmp expect actual &&
+		test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+	'
+
 done
 
 test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ff77dcca6b..5a812ca3c0 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -163,6 +163,7 @@ test_expect_success 'hook gets all queued symref updates' '
 	git update-ref refs/heads/branch $POST_OID &&
 	git symbolic-ref refs/heads/symref refs/heads/main &&
 	git symbolic-ref refs/heads/symrefd refs/heads/main &&
+	git symbolic-ref refs/heads/symrefu refs/heads/main &&
 
 	test_hook reference-transaction <<-\EOF &&
 	echo "$*" >>actual
@@ -190,10 +191,12 @@ test_expect_success 'hook gets all queued symref updates' '
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+	ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
 	committed
 	ref:refs/heads/main $ZERO_OID refs/heads/symref
 	ref:refs/heads/main $ZERO_OID refs/heads/symrefd
 	$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+	ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
 	EOF
 
 	git update-ref --no-deref --stdin <<-EOF &&
@@ -201,6 +204,7 @@ test_expect_success 'hook gets all queued symref updates' '
 	symref-verify refs/heads/symref refs/heads/main
 	symref-delete refs/heads/symrefd refs/heads/main
 	symref-create refs/heads/symrefc refs/heads/main
+	symref-update refs/heads/symrefu refs/heads/branch ref refs/heads/main
 	prepare
 	commit
 	EOF
-- 
2.43.GIT

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Junio C Hamano]

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

> The patches 1, 5 fix small issues in the reference backends. The other
> patches 2, 3, 4 & 6, each add one of the new sub-commands.
>
> The series is based off master, with 'kn/ref-transaction-symref' merged
> in. There seem to be no conflicts with 'next' or 'seen'.

Wait.  There is something fishy going on.

> Range diff vs v1:
>  1:  1bc4cc3fc4 =  1:  1bc4cc3fc4 refs: accept symref values in `ref_transaction_update()`
>  2:  57d0b1e2ea =  2:  57d0b1e2ea files-backend: extract out `create_symref_lock()`
>  3:  a8ae923f85 =  3:  a8ae923f85 refs: support symrefs in 'reference-transaction' hook
>  4:  e9965ba477 =  4:  e9965ba477 refs: move `original_update_refname` to 'refs.c'
>  5:  644daf7785 =  5:  644daf7785 refs: add support for transactional symref updates
>  6:  300b38e46f =  6:  300b38e46f refs: use transaction in `refs_create_symref()`
>  7:  f151dfe3c9 =  7:  f151dfe3c9 refs: rename `refs_create_symref()` to `refs_update_symref()`
>  8:  4865707bda =  8:  4865707bda refs: remove `create_symref` and associated dead code
>  9:  4cb67dce7c !  9:  2bbdeff798 refs: create and use `ref_update_ref_must_exist()`

4865707bda has been part of 'next' since 0a7119f2 (Merge branch
'kn/ref-transaction-symref' into next, 2024-05-11) and was merged to
'master' with 4beb7a3b (Merge branch 'kn/ref-transaction-symref',
2024-05-20).

I am confused why we are seeing a total reroll of such an old topic.

Also you have one more patch at the end.  Neither the before or
after version of 9/9.

Is this actually a single patch submission of 9/9 alone?  Patches
1-8/9 are all old ones that are in 'master' already.

Puzzled...

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 43053 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The patches 1, 5 fix small issues in the reference backends. The other
>> patches 2, 3, 4 & 6, each add one of the new sub-commands.
>>
>> The series is based off master, with 'kn/ref-transaction-symref' merged
>> in. There seem to be no conflicts with 'next' or 'seen'.
>
> Wait.  There is something fishy going on.
>
>> Range diff vs v1:
>>  1:  1bc4cc3fc4 =  1:  1bc4cc3fc4 refs: accept symref values in `ref_transaction_update()`
>>  2:  57d0b1e2ea =  2:  57d0b1e2ea files-backend: extract out `create_symref_lock()`
>>  3:  a8ae923f85 =  3:  a8ae923f85 refs: support symrefs in 'reference-transaction' hook
>>  4:  e9965ba477 =  4:  e9965ba477 refs: move `original_update_refname` to 'refs.c'
>>  5:  644daf7785 =  5:  644daf7785 refs: add support for transactional symref updates
>>  6:  300b38e46f =  6:  300b38e46f refs: use transaction in `refs_create_symref()`
>>  7:  f151dfe3c9 =  7:  f151dfe3c9 refs: rename `refs_create_symref()` to `refs_update_symref()`
>>  8:  4865707bda =  8:  4865707bda refs: remove `create_symref` and associated dead code
>>  9:  4cb67dce7c !  9:  2bbdeff798 refs: create and use `ref_update_ref_must_exist()`
>
> 4865707bda has been part of 'next' since 0a7119f2 (Merge branch
> 'kn/ref-transaction-symref' into next, 2024-05-11) and was merged to
> 'master' with 4beb7a3b (Merge branch 'kn/ref-transaction-symref',
> 2024-05-20).
>
> I am confused why we are seeing a total reroll of such an old topic.
>
> Also you have one more patch at the end.  Neither the before or
> after version of 9/9.
>
> Is this actually a single patch submission of 9/9 alone?  Patches
> 1-8/9 are all old ones that are in 'master' already.
>
> Puzzled...

I think this is just a mess up in the range diff, I haven't changed
anything locally. So adding the correct range diff here:

Range diff agains v1:

1:  4cb67dce7c ! 1:  2bbdeff798 refs: create and use
`ref_update_ref_must_exist()`
    @@ Metadata
     Author: Karthik Nayak <karthik.188@gmail.com>

      ## Commit message ##
    -    refs: create and use `ref_update_ref_must_exist()`
    +    refs: create and use `ref_update_expects_existing_old_ref()`

         The files and reftable backend, need to check if a ref must exist, so
         that the required validation can be done. A ref must exist
only when the
    @@ Commit message
         path. As we introduce the 'symref-verify' command in the upcoming
         commits, it is important to fix this.

    -    So let's export this to a function called `ref_update_ref_must_exist()`
    -    and expose it internally via 'refs-internal.h'.
    +    So let's export this to a function called
    +    `ref_update_expects_existing_old_ref()` and expose it internally via
    +    'refs-internal.h'.

         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

    @@ refs.c: int ref_update_check_old_target(const char *referent,
struct ref_update
      	return -1;
      }
     +
    -+int ref_update_ref_must_exist(struct ref_update *update)
    ++int ref_update_expects_existing_old_ref(struct ref_update *update)
     +{
     +	return (update->flags & REF_HAVE_OLD) &&
     +		(!is_null_oid(&update->old_oid) || update->old_target);
    @@ refs/files-backend.c: static int lock_ref_for_update(struct
files_ref_store *ref
      	struct strbuf referent = STRBUF_INIT;
     -	int mustexist = (update->flags & REF_HAVE_OLD) &&
     -		!is_null_oid(&update->old_oid);
    -+	int mustexist = ref_update_ref_must_exist(update);
    ++	int mustexist = ref_update_expects_existing_old_ref(update);
      	int ret = 0;
      	struct ref_lock *lock;

    @@ refs/refs-internal.h: int ref_update_has_null_new_value(struct
ref_update *updat
     + * Check if the ref must exist, this means that the old_oid or
     + * old_target is non NULL.
     + */
    -+int ref_update_ref_must_exist(struct ref_update *update);
    ++int ref_update_expects_existing_old_ref(struct ref_update *update);
     +
      #endif /* REFS_REFS_INTERNAL_H */

    @@ refs/reftable-backend.c: static int
reftable_be_transaction_prepare(struct ref_s
      		if (ret < 0)
      			goto done;
     -		if (ret > 0 && (!(u->flags & REF_HAVE_OLD) ||
is_null_oid(&u->old_oid))) {
    -+		if (ret > 0 && !ref_update_ref_must_exist(u)) {
    ++		if (ret > 0 && !ref_update_expects_existing_old_ref(u)) {
      			/*
      			 * The reference does not exist, and we either have no
      			 * old object ID or expect the reference to not exist.
2:  9fda13b468 ! 2:  f509066cab update-ref: add support for
'symref-verify' command
    @@ Commit message
         point to an object and not a ref and the regular 'verify'
command can be
         used in such situations.

    -    Add required tests for symref support in 'verify' while also adding
    -    reflog checks for the pre-existing 'verify' tests.
    +    Add required tests for symref support in 'verify'. Since we're here,
    +    also add reflog checks for the pre-existing 'verify' tests, there is no
    +    divergence from behavior, but we never tested to ensure that reflog
    +    wasn't affected by the 'verify' command.

         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

    @@ refs.c: int ref_transaction_delete(struct ref_transaction *transaction,
     -		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_oid && old_target)
    ++		BUG("verify called with both old_oid and old_target set");
     +	if (old_target && !(flags & REF_NO_DEREF))
     +		BUG("verify cannot operate on symrefs with deref mode");
      	return ref_transaction_update(transaction, refname,
    @@ t/t1400-update-ref.sh: test_expect_success PIPE 'transaction
flushes status upda
      	test_cmp expected actual
      '

    -+create_stdin_buf () {
    ++format_command () {
     +	if test "$1" = "-z"
     +	then
     +		shift
    -+		printf "$F" "$@" >stdin
    ++		printf "$F" "$@"
     +	else
    -+		echo "$@" >stdin
    ++		echo "$@"
     +	fi
     +}
     +
     +for type in "" "-z"
     +do
     +
    -+	test_expect_success "stdin ${type} symref-verify fails without
--no-deref" '
    ++	test_expect_success "stdin $type symref-verify fails without
--no-deref" '
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
     +		grep "fatal: symref-verify: cannot operate with deref mode" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify fails with too
many arguments" '
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref
<stdin 2>err  &&
    ++	test_expect_success "stdin $type symref-verify fails with too
many arguments" '
    ++		format_command $type "symref-verify refs/heads/symref" "$a"
"$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err  &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: test_expect_success PIPE 'transaction
flushes status upda
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify succeeds for
correct value" '
    ++	test_expect_success "stdin $type symref-verify 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} "symref-verify refs/heads/symref" "$a" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
    ++		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} symref-verify no value is
treated as zero value" '
    ++	test_expect_success "stdin $type symref-verify fails with no value" '
     +		git symbolic-ref refs/heads/symref >expect &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
    ++		format_command $type "symref-verify refs/heads/symref" "" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify succeeds for
dangling reference" '
    ++	test_expect_success "stdin $type symref-verify succeeds for
dangling reference" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
     +		test_must_fail git symbolic-ref refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref2"
"refs/heads/nonexistent" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin
    ++		format_command $type "symref-verify refs/heads/symref2"
"refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-verify fails for
missing reference" '
    ++	test_expect_success "stdin $type symref-verify fails for
missing reference" '
     +		test-tool ref-store main for-each-reflog-ent refs/heads/symref
>before &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/missing"
"refs/heads/unknown" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++		format_command $type "symref-verify refs/heads/missing"
"refs/heads/unknown" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: cannot lock ref ${SQ}refs/heads/missing${SQ}:
unable to resolve reference ${SQ}refs/heads/missing${SQ}" err &&
     +		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} symref-verify fails for wrong value" '
    ++	test_expect_success "stdin $type symref-verify fails for wrong value" '
     +		git symbolic-ref refs/heads/symref >expect &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$b" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-verify refs/heads/symref" "$b" >stdin &&
    ++		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} symref-verify fails for
mistaken null value" '
    ++	test_expect_success "stdin $type symref-verify fails for
mistaken null value" '
     +		git symbolic-ref refs/heads/symref >expect &&
    -+		create_stdin_buf ${type} "symref-verify refs/heads/symref" "$Z" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-verify refs/heads/symref" "$Z" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
3:  d07031827b ! 3:  a11f4c1e48 update-ref: add support for
'symref-delete' command
    @@ refs.c: int ref_transaction_create(struct ref_transaction *transaction,
      {
      	if (old_oid && is_null_oid(old_oid))
      		BUG("delete called with old_oid set to zeros");
    ++	if (old_oid && old_target)
    ++		BUG("delete called with both old_oid and old_target set");
     +	if (old_target && !(flags & REF_NO_DEREF))
     +		BUG("delete cannot operate on symrefs with deref mode");
      	return ref_transaction_update(transaction, refname,
    @@ refs.h: int ref_transaction_create(struct ref_transaction *transaction,
      /*

      ## t/t1400-update-ref.sh ##
    -@@ t/t1400-update-ref.sh: do
    - 		test_cmp before after
    - 	'
    -
    --	test_expect_success "stdin ${type} symref-verify no value is
treated as zero value" '
    -+	test_expect_success "stdin ${type} symref-verify fails with no value" '
    - 		git symbolic-ref refs/heads/symref >expect &&
    - 		create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
    - 		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
     @@ t/t1400-update-ref.sh: do
      		test_cmp expect actual
      	'

    -+	test_expect_success "stdin ${type} symref-delete fails without
--no-deref" '
    ++	test_expect_success "stdin $type symref-delete fails without
--no-deref" '
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
     +		grep "fatal: symref-delete: cannot operate with deref mode" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete fails with no ref" '
    -+		create_stdin_buf ${type} "symref-delete " &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-delete fails with no ref" '
    ++		format_command $type "symref-delete " >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: symref-delete: missing <ref>" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete fails with too
many arguments" '
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" "$a" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-delete fails with too
many arguments" '
    ++		format_command $type "symref-delete refs/heads/symref" "$a"
"$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: do
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete fails with
wrong old value" '
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-delete fails with wrong
old value" '
    ++		format_command $type "symref-delete refs/heads/symref" "$m" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: verifying symref target:
${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main"
err &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete works with
right old value" '
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++	test_expect_success "stdin $type symref-delete works with right
old value" '
    ++		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		test_must_fail git rev-parse --verify -q refs/heads/symref
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete works with
empty old value" '
    -+		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref" "" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++	test_expect_success "stdin $type symref-delete works with empty
old value" '
    ++		git symbolic-ref refs/heads/symref $a >stdin &&
    ++		format_command $type "symref-delete refs/heads/symref" "" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		test_must_fail git rev-parse --verify -q $b
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-delete succeeds for
dangling reference" '
    ++	test_expect_success "stdin $type symref-delete succeeds for
dangling reference" '
     +		test_must_fail git symbolic-ref refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-delete refs/heads/symref2"
"refs/heads/nonexistent" &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-delete refs/heads/symref2"
"refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		test_must_fail git symbolic-ref -d refs/heads/symref2
     +	'
     +
4:  1038e96a44 ! 4:  9b71c9e07b update-ref: add support for
'symref-create' command
    @@ refs.c: int ref_transaction_update(struct ref_transaction *transaction,
      {
     -	if (!new_oid || is_null_oid(new_oid)) {
     -		strbuf_addf(err, "'%s' has a null OID", refname);
    ++	if (new_oid && new_target)
    ++		BUG("create called with both new_oid and new_target set");
     +	if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
    -+		strbuf_addf(err, "'%s' has a null OID or no new target", refname);
    ++		strbuf_addf(err, "'%s' has neither a valid OID nor a target", refname);
      		return 1;
      	}
      	return ref_transaction_update(transaction, refname, new_oid,
    @@ t/t0600-reffiles-backend.sh: test_expect_success POSIXPERM 'git
reflog expire ho
      '

     +test_expect_success SYMLINKS 'symref transaction supports symlinks' '
    -+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
    ++	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
     +	git update-ref refs/heads/new @ &&
     +	test_config core.prefersymlinkrefs true &&
     +	cat >stdin <<-EOF &&
     +	start
    -+	symref-create TESTSYMREFONE refs/heads/new
    ++	symref-create TEST_SYMREF_HEAD refs/heads/new
     +	prepare
     +	commit
     +	EOF
     +	git update-ref --no-deref --stdin <stdin &&
    -+	test_path_is_symlink .git/TESTSYMREFONE &&
    -+	test "$(test_readlink .git/TESTSYMREFONE)" = refs/heads/new
    ++	test_path_is_symlink .git/TEST_SYMREF_HEAD &&
    ++	test "$(test_readlink .git/TEST_SYMREF_HEAD)" = refs/heads/new
     +'
     +
     +test_expect_success 'symref transaction supports false symlink config' '
    -+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
    ++	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
     +	git update-ref refs/heads/new @ &&
     +	test_config core.prefersymlinkrefs false &&
     +	cat >stdin <<-EOF &&
     +	start
    -+	symref-create TESTSYMREFONE refs/heads/new
    ++	symref-create TEST_SYMREF_HEAD refs/heads/new
     +	prepare
     +	commit
     +	EOF
     +	git update-ref --no-deref --stdin <stdin &&
    -+	test_path_is_file .git/TESTSYMREFONE &&
    -+	git symbolic-ref TESTSYMREFONE >actual &&
    ++	test_path_is_file .git/TEST_SYMREF_HEAD &&
    ++	git symbolic-ref TEST_SYMREF_HEAD >actual &&
     +	echo refs/heads/new >expect &&
     +	test_cmp expect actual
     +'
    @@ t/t1400-update-ref.sh: do
      		test_must_fail git symbolic-ref -d refs/heads/symref2
      	'

    -+	test_expect_success "stdin ${type} symref-create fails with too
many arguments" '
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref"
"$a" "$a" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-create fails with too
many arguments" '
    ++		format_command $type "symref-create refs/heads/symref" "$a"
"$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: do
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create fails with no target" '
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
    ++	test_expect_success "stdin $type symref-create fails with no target" '
    ++		format_command $type "symref-create refs/heads/symref" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create fails with
empty target" '
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin
    ++	test_expect_success "stdin $type symref-create fails with empty target" '
    ++		format_command $type "symref-create refs/heads/symref" "" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create works" '
    ++	test_expect_success "stdin $type symref-create works" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref"
"$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create works with --no-deref" '
    ++	test_expect_success "stdin $type symref-create works with --no-deref" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" &&
    -+		git update-ref --stdin ${type} <stdin 2>err
    ++		format_command $type "symref-create refs/heads/symref" "$a" &&
    ++		git update-ref --stdin $type <stdin 2>err
     +	'
     +
    -+	test_expect_success "stdin ${type} create dangling symref ref works" '
    ++	test_expect_success "stdin $type create dangling symref ref works" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref"
"refs/heads/unkown" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/heads/symref"
"refs/heads/unkown" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo refs/heads/unkown >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create does not
create reflogs by default" '
    ++	test_expect_success "stdin $type symref-create does not create
reflogs by default" '
     +		test_when_finished "git symbolic-ref -d refs/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/symref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual &&
     +		test_must_fail git reflog exists refs/symref
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-create reflogs with
--create-reflog" '
    ++	test_expect_success "stdin $type symref-create reflogs with
--create-reflog" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-create refs/heads/symref"
"$a" >stdin &&
    -+		git update-ref --create-reflog --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --create-reflog --stdin $type --no-deref <stdin &&
     +		git symbolic-ref refs/heads/symref >expect &&
     +		echo $a >actual &&
     +		test_cmp expect actual &&
    @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'hook
gets all queued symr
      	prepare
      	commit
      	EOF
    +
    + ## t/t5605-clone-local.sh ##
    +@@ t/t5605-clone-local.sh: test_expect_success REFFILES 'local
clone from repo with corrupt refs fails grac
    + 	echo a >corrupt/.git/refs/heads/topic &&
    +
    + 	test_must_fail git clone corrupt working 2>err &&
    +-	grep "has a null OID" err
    ++	grep "has neither a valid OID nor a target" err
    + '
    +
    + test_done
5:  78dd51b65f = 5:  a9b1a31756 reftable: pick either 'oid' or
'target' for new updates
6:  562e061063 ! 6:  1bbbe86743 update-ref: add support for
'symref-update' command
    @@ Commit message
         OID before the update. This by extension also means that this when a
         zero <old-oid> is provided, it ensures that the ref didn't
exist before.

    +    The divergence in syntax from the regular `update` command is
because if
    +    we don't use a `(ref | oid)` prefix for the old_value, then there is
    +    ambiguity around if the value provided should be treated as an oid or a
    +    reference. This is more so the reason, because we allow anything
    +    committish to be provided as an oid.
    +
         The command allows users to perform symbolic ref updates within a
         transaction. This provides atomicity and allows users to perform a set
         of operations together.

    -    This command will also support deref mode, to ensure that we can update
    +    This command supports deref mode, to ensure that we can update
         dereferenced regular refs to symrefs.

         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
    @@ builtin/update-ref.c: static char *parse_next_refname(const char **next)
     +		return strbuf_detach(&arg, NULL);
     +	return NULL;
     +}
    -+

      /*
       * The value being parsed is <old-oid> (as opposed to <new-oid>; the
    @@ builtin/update-ref.c: static void parse_cmd_update(struct
ref_transaction *trans
     +	char *old_target = NULL;
     +	struct strbuf err = STRBUF_INIT;
     +	struct object_id old_oid;
    -+	int have_old = 0;
    ++	int have_old_oid = 0;
     +
     +	refname = parse_refname(&next);
     +	if (!refname)
    @@ builtin/update-ref.c: static void parse_cmd_update(struct
ref_transaction *trans
     +
     +	old_arg = parse_next_arg(&next);
     +	if (old_arg) {
    -+		old_target = parse_next_refname(&next);
    ++		old_target = parse_next_arg(&next);
     +		if (!old_target)
     +			die("symref-update %s: expected old value", refname);
     +
    -+		if (!strcmp(old_arg, "oid") &&
    -+		    !repo_get_oid(the_repository, old_target, &old_oid)) {
    ++		if (!strcmp(old_arg, "oid")) {
    ++			if (repo_get_oid(the_repository, old_target, &old_oid))
    ++				die("symref-update %s: invalid oid: %s", refname, old_target);
    ++
     +			old_target = NULL;
    -+			have_old = 1;
    -+		} else if (strcmp(old_arg, "ref"))
    ++			have_old_oid = 1;
    ++		} else if (!strcmp(old_arg, "ref")) {
    ++			if (check_refname_format(old_target, REFNAME_ALLOW_ONELEVEL))
    ++				die("symref-update %s: invalid ref: %s", refname, old_target);
    ++		} else {
     +			die("symref-update %s: invalid arg '%s' for old value",
refname, old_arg);
    ++		}
     +	}
     +
     +	if (*next != line_termination)
     +		die("symref-update %s: extra input: %s", refname, next);
     +
     +	if (ref_transaction_update(transaction, refname, NULL,
    -+				   have_old ? &old_oid : NULL,
    ++				   have_old_oid ? &old_oid : NULL,
     +				   new_target, old_target,
    -+				   update_flags |= create_reflog_flag,
    ++				   update_flags | create_reflog_flag,
     +				   msg, &err))
     +		die("%s", err.buf);
     +
    @@ t/t1400-update-ref.sh: do
      		git reflog exists refs/heads/symref
      	'

    -+	test_expect_success "stdin ${type} symref-update fails with too
many arguments" '
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" "ref" "$a" "$a" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-update fails with too
many arguments" '
    ++		format_command $type "symref-update refs/heads/symref" "$a"
"ref" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		if test "$type" = "-z"
     +		then
     +			grep "fatal: unknown command: $a" err
    @@ t/t1400-update-ref.sh: do
     +		fi
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails with
wrong old value argument" '
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" "foo" "$a" "$a" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++	test_expect_success "stdin $type symref-update fails with wrong
old value argument" '
    ++		format_command $type "symref-update refs/heads/symref" "$a"
"foo" "$a" "$a" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: symref-update refs/heads/symref: invalid arg
${SQ}foo${SQ} for old value" err
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates with
zero old value" '
    ++	test_expect_success "stdin $type symref-update creates with
zero old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" "oid" "$Z" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a"
"oid" "$Z" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates with
no old value" '
    ++	test_expect_success "stdin $type symref-update creates with no
old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates dangling" '
    ++	test_expect_success "stdin $type symref-update creates dangling" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"refs/heads/nonexistent" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref"
"refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo refs/heads/nonexistent >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails with
wrong old value" '
    ++	test_expect_success "stdin $type symref-update fails with wrong
old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$m" "ref" "$b" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
    ++		format_command $type "symref-update refs/heads/symref" "$m"
"ref" "$b" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
     +		grep "fatal: verifying symref target:
${SQ}refs/heads/symref${SQ}: is at $a but expected $b" err &&
     +		test_must_fail git rev-parse --verify -q $c
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update updates dangling ref" '
    ++	test_expect_success "stdin $type symref-update updates dangling ref" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update updates
dangling ref with old value" '
    ++	test_expect_success "stdin $type symref-update updates dangling
ref with old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" "ref" "refs/heads/nonexistent" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a"
"ref" "refs/heads/nonexistent" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails update
dangling ref with wrong old value" '
    ++	test_expect_success "stdin $type symref-update fails update
dangling ref with wrong old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_must_fail git rev-parse refs/heads/nonexistent &&
     +		git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" "ref" "refs/heads/wrongref" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a"
"ref" "refs/heads/wrongref" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
     +		echo refs/heads/nonexistent >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update works with
right old value" '
    ++	test_expect_success "stdin $type symref-update works with right
old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$m" "ref" "$a" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$m"
"ref" "$a" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $m >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update works with no
old value" '
    ++	test_expect_success "stdin $type symref-update works with no old value" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$m" >stdin &&
    -+		git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$m" >stdin &&
    ++		git update-ref --stdin $type --no-deref <stdin &&
     +		echo $m >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update fails with
empty old ref-target" '
    ++	test_expect_success "stdin $type symref-update fails with empty
old ref-target" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$m" "ref" "" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$m"
"ref" "" >stdin &&
    ++		test_must_fail git update-ref --stdin $type --no-deref <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update creates (with deref)" '
    ++	test_expect_success "stdin $type symref-update creates (with deref)" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" >stdin &&
    -+		git update-ref --stdin ${type} <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref --stdin $type <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/symref >actual &&
     +		test_cmp expect actual &&
    @@ t/t1400-update-ref.sh: do
     +		grep "$Z $(git rev-parse $a)" actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to
symref with correct old-oid" '
    ++	test_expect_success "stdin $type symref-update regular ref to
symref with correct old-oid" '
     +		test_when_finished "git symbolic-ref -d --no-recurse
refs/heads/regularref" &&
     +		git update-ref --no-deref refs/heads/regularref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/regularref"
"$a" "oid" "$(git rev-parse $a)" >stdin &&
    -+		git update-ref --stdin ${type} <stdin &&
    ++		format_command $type "symref-update refs/heads/regularref"
"$a" "oid" "$(git rev-parse $a)" >stdin &&
    ++		git update-ref --stdin $type <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
     +		test_cmp expect actual &&
    @@ t/t1400-update-ref.sh: do
     +		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to
symref fails with wrong old-oid" '
    ++	test_expect_success "stdin $type symref-update regular ref to
symref fails with wrong old-oid" '
    ++		test_when_finished "git update-ref -d refs/heads/regularref" &&
    ++		git update-ref --no-deref refs/heads/regularref $a &&
    ++		format_command $type "symref-update refs/heads/regularref"
"$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
    ++		grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}:
is at $(git rev-parse $a) but expected $(git rev-parse
refs/heads/target2)" err &&
    ++		echo $(git rev-parse $a) >expect &&
    ++		git rev-parse refs/heads/regularref >actual &&
    ++		test_cmp expect actual
    ++	'
    ++
    ++	test_expect_success "stdin $type symref-update regular ref to
symref fails with invalid old-oid" '
     +		test_when_finished "git update-ref -d refs/heads/regularref" &&
     +		git update-ref --no-deref refs/heads/regularref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/regularref"
"$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-update refs/heads/regularref"
"$a" "oid" "not-a-ref-oid" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
    ++		grep "fatal: symref-update refs/heads/regularref: invalid oid:
not-a-ref-oid" err &&
     +		echo $(git rev-parse $a) >expect &&
     +		git rev-parse refs/heads/regularref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update existing
symref with zero old-oid" '
    ++	test_expect_success "stdin $type symref-update existing symref
with zero old-oid" '
     +		test_when_finished "git symbolic-ref -d --no-recurse
refs/heads/symref" &&
     +		git symbolic-ref refs/heads/symref refs/heads/target2 &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" "oid" "$Z" >stdin &&
    -+		test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
    ++		format_command $type "symref-update refs/heads/symref" "$a"
"oid" "$Z" >stdin &&
    ++		test_must_fail git update-ref --stdin $type <stdin 2>err &&
     +		grep "fatal: cannot lock ref ${SQ}refs/heads/symref${SQ}:
reference already exists" err &&
     +		echo refs/heads/target2 >expect &&
     +		git symbolic-ref refs/heads/symref >actual &&
     +		test_cmp expect actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to
symref (with deref)" '
    ++	test_expect_success "stdin $type symref-update regular ref to
symref (with deref)" '
     +		test_when_finished "git symbolic-ref -d refs/heads/symref" &&
     +		test_when_finished "git update-ref -d --no-deref refs/heads/symref2" &&
     +		git update-ref refs/heads/symref2 $a &&
     +		git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/symref"
"$a" >stdin &&
    -+		git update-ref ${type} --stdin <stdin &&
    ++		format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
    ++		git update-ref $type --stdin <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/symref2 >actual &&
     +		test_cmp expect actual &&
    @@ t/t1400-update-ref.sh: do
     +		grep "$(git rev-parse $a) $(git rev-parse $a)" actual
     +	'
     +
    -+	test_expect_success "stdin ${type} symref-update regular ref to symref" '
    ++	test_expect_success "stdin $type symref-update regular ref to symref" '
     +		test_when_finished "git symbolic-ref -d --no-recurse
refs/heads/regularref" &&
     +		git update-ref --no-deref refs/heads/regularref $a &&
    -+		create_stdin_buf ${type} "symref-update refs/heads/regularref"
"$a" >stdin &&
    -+		git update-ref ${type} --stdin <stdin &&
    ++		format_command $type "symref-update refs/heads/regularref"
"$a" >stdin &&
    ++		git update-ref $type --stdin <stdin &&
     +		echo $a >expect &&
     +		git symbolic-ref --no-recurse refs/heads/regularref >actual &&
     +		test_cmp expect actual &&

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Wait.  There is something fishy going on.
>
>> Range diff vs v1:
>>  1:  1bc4cc3fc4 =  1:  1bc4cc3fc4 refs: accept symref values in `ref_transac...
>>...
>>  8:  4865707bda =  8:  4865707bda refs: remove `create_symref` and associated dead code
>>  9:  4cb67dce7c !  9:  2bbdeff798 refs: create and use `ref_update_ref_must_exist()`
> ...
> I am confused why we are seeing a total reroll of such an old topic.
>
> Also you have one more patch at the end.  Neither the before or
> after version of 9/9.
>
> Is this actually a single patch submission of 9/9 alone?  Patches
> 1-8/9 are all old ones that are in 'master' already.

And then there is a mystery of this v2 being a 6-patch series.
Perhpas a wrong range-diff was pasted into it?  If this were truly a
total reroll of the previous 8-patch series with an extra step
appended to the end, it would have been a 9-patch series, not 6.

Even puzzled...

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> The patches 1, 5 fix small issues in the reference backends. The other
>>> patches 2, 3, 4 & 6, each add one of the new sub-commands.
>>>
>>> The series is based off master, with 'kn/ref-transaction-symref' merged
>>> in. There seem to be no conflicts with 'next' or 'seen'.
>>
>> Wait.  There is something fishy going on.
>> ...
>> Is this actually a single patch submission of 9/9 alone?  Patches
>> 1-8/9 are all old ones that are in 'master' already.
>>
>> Puzzled...
>
> I think this is just a mess up in the range diff, I haven't changed
> anything locally. So adding the correct range diff here:

Quite honestly, I care much less about the range-diff that is almost
unintelligible than the actual patches.  Your title line says 0/6,
your updated range-diff presumably have 1: to 6:?  As a sanity check
mechanism, the list of commits and the overall diffstat is a more
useful part in the cover letter message so that I (or any other
recipients) can use to compare against the list of messages that
appeared on the list.

We may want to teach "format-patch --range-diff" to place the output
of range-diff _below_ the list of commits and the overall diffstat
in the cover letter (and at the end of the patch for a single patch
topic).

I'll ignore the range-diff in the original cover letter and see if
the rest makes sense.

Thanks.

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3056 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> The patches 1, 5 fix small issues in the reference backends. The other
>>>> patches 2, 3, 4 & 6, each add one of the new sub-commands.
>>>>
>>>> The series is based off master, with 'kn/ref-transaction-symref' merged
>>>> in. There seem to be no conflicts with 'next' or 'seen'.
>>>
>>> Wait.  There is something fishy going on.
>>> ...
>>> Is this actually a single patch submission of 9/9 alone?  Patches
>>> 1-8/9 are all old ones that are in 'master' already.
>>>
>>> Puzzled...
>>
>> I think this is just a mess up in the range diff, I haven't changed
>> anything locally. So adding the correct range diff here:
>
> Quite honestly, I care much less about the range-diff that is almost
> unintelligible than the actual patches.  Your title line says 0/6,
> your updated range-diff presumably have 1: to 6:?  As a sanity check
> mechanism, the list of commits and the overall diffstat is a more
> useful part in the cover letter message so that I (or any other
> recipients) can use to compare against the list of messages that
> appeared on the list.
>
> We may want to teach "format-patch --range-diff" to place the output
> of range-diff _below_ the list of commits and the overall diffstat
> in the cover letter (and at the end of the patch for a single patch
> topic).
>

I usually manually add in the range-diff, which is probably where the
error came from. I didn't even know about "format-patch --range-diff".

> I'll ignore the range-diff in the original cover letter and see if
> the rest makes sense.
>
> Thanks.

It does use the same base as the previous revision, I rebased in place
using 'rebase -i' and amended for fixes from the first review.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Wait.  There is something fishy going on.
>>
>>> Range diff vs v1:
>>>  1:  1bc4cc3fc4 =  1:  1bc4cc3fc4 refs: accept symref values in `ref_transac...
>>>...
>>>  8:  4865707bda =  8:  4865707bda refs: remove `create_symref` and associated dead code
>>>  9:  4cb67dce7c !  9:  2bbdeff798 refs: create and use `ref_update_ref_must_exist()`
>> ...
>> I am confused why we are seeing a total reroll of such an old topic.
>>
>> Also you have one more patch at the end.  Neither the before or
>> after version of 9/9.
>>
>> Is this actually a single patch submission of 9/9 alone?  Patches
>> 1-8/9 are all old ones that are in 'master' already.
>
> And then there is a mystery of this v2 being a 6-patch series.
> Perhpas a wrong range-diff was pasted into it?  If this were truly a
> total reroll of the previous 8-patch series with an extra step
> appended to the end, it would have been a 9-patch series, not 6.
>
> Even puzzled...

The v1 of this series is also a 6-patch series, this is not a re-roll of
the earlier series 'kn/ref-transaction-symref' (which is already in
next). This is based on top of it.

Sorry for the confusion though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Eric Sunshine]

On Thu, May 23, 2024 at 12:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> We may want to teach "format-patch --range-diff" to place the output
> of range-diff _below_ the list of commits and the overall diffstat
> in the cover letter

Placing the range-diff below the list of commits and the overall
diffstat in the cover letter is how `format-patch --range-diff`
already works.

> (and at the end of the patch for a single patch topic).

This could indeed lead to less visual clutter for the single-patch topic.

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, May 23, 2024 at 12:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>> We may want to teach "format-patch --range-diff" to place the output
>> of range-diff _below_ the list of commits and the overall diffstat
>> in the cover letter
>
> Placing the range-diff below the list of commits and the overall
> diffstat in the cover letter is how `format-patch --range-diff`
> already works.

Heh, so what Karthik was manually doing made the cover letter harder
to read?

In any case, that is a great news.

>> (and at the end of the patch for a single patch topic).
>
> This could indeed lead to less visual clutter for the single-patch topic.

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Eric Sunshine]

On Thu, May 23, 2024 at 2:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Thu, May 23, 2024 at 12:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> We may want to teach "format-patch --range-diff" to place the output
> >> of range-diff _below_ the list of commits and the overall diffstat
> >> in the cover letter
> >
> > Placing the range-diff below the list of commits and the overall
> > diffstat in the cover letter is how `format-patch --range-diff`
> > already works.
>
> Heh, so what Karthik was manually doing made the cover letter harder
> to read?

Apparently so.

> In any case, that is a great news.

Placement of the range-diff in the cover-letter was a deliberate choice[1].

> >> (and at the end of the patch for a single patch topic).
> >
> > This could indeed lead to less visual clutter for the single-patch topic.

Regarding your experiment[2] to place the range-diff at the end of a
single-patch, apparently that idea had been considered, as well, but
it was noted[3,4] that, by default, some MUIs hide everything after
the "--" line.

[1]: https://lore.kernel.org/git/CAPig+cT9zCcNxn1+DPMQWqJ-hfxb7gE7rKyfbqHjTC+FDNY_mw@mail.gmail.com/
[2]: https://lore.kernel.org/git/xmqqh6ep1pwz.fsf_-_@gitster.g/
[3]: https://lore.kernel.org/git/xmqqmuupogei.fsf@gitster-ct.c.googlers.com/
[4]: https://lore.kernel.org/git/CAPig+cRx5-2TYOm_8oayFfbKGpmTJf=M0cNR3L5UJGGC6vHPDQ@mail.gmail.com/

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Placement of the range-diff in the cover-letter was a deliberate choice[1].

OK, and the reasoning in [1] still makes sense.  Short-and-sweet
stuff with denser information contents first, before a bulky "diff
of diff" we add as an auxiliary piece of information.

> Regarding your experiment[2] to place the range-diff at the end of a
> single-patch, apparently that idea had been considered, as well, but
> it was noted[3,4] that, by default, some MUIs hide everything after
> the "--" line.

That is different from what [3] noted, though.  It was "the stuff
after '-- ' not included in a reply/reply-all when responding".

 (1) As we'd rather want to see the actual patch, not a part of
     range-diff in the cover letter, responded, placing it under the
     "-- " mark and excluded from the response is actually a
     feature.

 (2) If we wanted to, we can show the log message, "---", output of
     "git diff --stat -p", output of "git range-diff" and finally
     the "-- " signature mark.  Receiving end will not mistake the
     range-diff output as part of the last hunk of the last patch.

So, I do not see it as a reason to refrain from placing the range
diff output after the main patch.

Thanks.

Re: [PATCH v2 0/6] update-ref: add symref support for --stdin

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> (and at the end of the patch for a single patch topic).
>
> This could indeed lead to less visual clutter for the single-patch topic.

So here is how such a change looks like.  I actually have this as a
two-patch series in my tree, but here is in squashed-into-one form.

The log-tree.c:show_log() function has a logic to create inter/range
diff at its end.  This function is called early by log_tree_diff(),
which is responsible for showing a single commit (log message,
auxiliary info like diffstat, and the patch, right before the
signature mark "-- " which is given by the format-patch itself).

We move that inter/range logic out into a helper function and call
it at the original place (which is [1/2] step of the two patch
series), which is a no-op refactoring.

In the second step, we remove the call out of show_log(), and
instead call it at the end of the log_tree_commit() after
log_tree_diff() did its thing.  This removes the inter/range diff
out of the "auxiliary info" section between "---" and the patch and
moves it at the end of the patch text, still before the signature
mark "-- ".  As this makes inter/range diff no longer part of the
runs of "commentary block"s, calls to next_commentary_block() is
removed from the show_diff_of_diff() helper.

As expected, this requires adjustment to t/t4014-format-patch.sh but
the fallout is surprisingly small.  It may be either an indication
that our test coverage for the feature is sketchy, or the tests were
written robustly, anticipating that somebody someday may want to
move things around in the output this way.


 log-tree.c              |  7 +++----
 t/t4014-format-patch.sh | 17 +++++++++++------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git c/log-tree.c w/log-tree.c
index e7cd2c491f..f28c4d0bb0 100644
--- c/log-tree.c
+++ w/log-tree.c
@@ -684,7 +684,6 @@ static void show_diff_of_diff(struct rev_info *opt)
 		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
 		DIFF_QUEUE_CLEAR(&diff_queued_diff);
 
-		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->idiff_title);
 		show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,
 			       &opt->diffopt);
@@ -704,7 +703,6 @@ static void show_diff_of_diff(struct rev_info *opt)
 		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
 		DIFF_QUEUE_CLEAR(&diff_queued_diff);
 
-		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
 		/*
 		 * Pass minimum required diff-options to range-diff; others
@@ -903,8 +901,6 @@ void show_log(struct rev_info *opt)
 	strbuf_release(&msgbuf);
 	free(ctx.notes_message);
 	free(ctx.after_subject);
-
-	show_diff_of_diff(opt);
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
@@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	opt->loginfo = NULL;
 	maybe_flush_or_die(opt->diffopt.file, "stdout");
 	opt->diffopt.no_free = no_free;
+	if (shown)
+		show_diff_of_diff(opt);
+
 	diff_free(&opt->diffopt);
 	return shown;
 }
diff --git c/t/t4014-format-patch.sh w/t/t4014-format-patch.sh
index ba85b582c5..c0c5eccb7c 100755
--- c/t/t4014-format-patch.sh
+++ w/t/t4014-format-patch.sh
@@ -2482,13 +2482,18 @@ test_expect_success 'interdiff: reroll-count with a integer' '
 '
 
 test_expect_success 'interdiff: solo-patch' '
-	cat >expect <<-\EOF &&
-	  +fleep
-
-	EOF
 	git format-patch --interdiff=boop~2 -1 boop &&
-	test_grep "^Interdiff:$" 0001-fleep.patch &&
-	sed "1,/^  @@ /d; /^$/q" 0001-fleep.patch >actual &&
+
+	# remove up to the last "patch" output line,
+	# and remove everything below the signature mark.
+	sed -e "1,/^+fleep\$/d" -e "/^-- /,\$d" 0001-fleep.patch >actual &&
+
+	# fabricate Interdiff output.
+	git diff boop~2 boop >inter &&
+	{
+		echo "Interdiff:" &&
+		sed -e "s/^/  /" inter
+	} >expect &&
 	test_cmp expect actual
 '
 

Re: [PATCH v2 6/6] update-ref: add support for 'symref-update' command

[Junio C Hamano]

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

> +static void parse_cmd_symref_update(struct ref_transaction *transaction,
> +				    const char *next, const char *end)
> +{
> +	char *refname, *new_target, *old_arg;
> +	char *old_target = NULL;
> + ...
> +	old_arg = parse_next_arg(&next);

> +	if (old_arg) {
> +		old_target = parse_next_arg(&next);

Now we have an allocated memory we are responsible for freeing in
old_target, obtained from parse_next_arg() ...

> +		if (!old_target)
> +			die("symref-update %s: expected old value", refname);

... and here we know it is not NULL.  We use it to grab the object
name ...

> +		if (!strcmp(old_arg, "oid")) {
> +			if (repo_get_oid(the_repository, old_target, &old_oid))
> +				die("symref-update %s: invalid oid: %s", refname, old_target);
> +
> +			old_target = NULL;

... and then we overwritten the variable, losing the last reference
to the piece of memory without freeing.

Perhaps squashing this in is sufficient to plug this leak, but there
probably are other new leaks around this code.  I ran out of time so
I'll let you take care of the rest ;-)

Thanks.

 builtin/update-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/update-ref.c w/builtin/update-ref.c
index 76d20ca0f1..7d2a419230 100644
--- c/builtin/update-ref.c
+++ w/builtin/update-ref.c
@@ -297,7 +297,7 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
 
 		if (!strcmp(old_arg, "oid") &&
 		    !repo_get_oid(the_repository, old_target, &old_oid)) {
-			old_target = NULL;
+			FREE_AND_NULL(old_target);
 			have_old = 1;
 		} else if (strcmp(old_arg, "ref"))
 			die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);