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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]