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

> 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?
>

>> 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?
>