Patrick Steinhardt <ps@xxxxxx> writes:
> On Tue, May 14, 2024 at 02:44:11PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxxxx/
Attachment:
signature.asc
Description: PGP signature
