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

Patrick Steinhardt [off-list ref] writes:

On Tue, May 14, 2024 at 02:44:11PM +0200, Karthik Nayak wrote:

quoted

From: Karthik Nayak <redacted>

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.

quoted

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.

quoted

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.

quoted

 /*
  * 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()`.

quoted

+		} 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.

quoted

+	}
+
+	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/ (local)