Patrick Steinhardt <ps@xxxxxx> writes:
> On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
> [snip]
>> @@ -268,6 +268,39 @@ 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_ref;
>> +
>> + if (!(update_flags & REF_NO_DEREF))
>> + die("symref-create: cannot operate with deref mode");
>> +
>> + refname = parse_refname(&next);
>> + if (!refname)
>> + die("symref-create: missing <ref>");
>> +
>> + new_ref = parse_next_refname(&next);
>> + if (!new_ref)
>> + die("symref-create %s: missing <new-ref>", refname);
>> + if (read_ref(new_ref, NULL))
>> + die("symref-create %s: invalid <new-ref>", refname);
>
> This restricts the creation of dangling symrefs, right? I think we might
> want to lift this restriction, in which case we can eventually get rid
> of the `create_symref` callback in ref backends completely.
>
Yes it does. I thought it'd be more consistent with what update-ref does
with regular ref updates. We verify that the object exists. Also this
could be an added option 'allow-dangling'.
I'm not sure I understand what you mean 'the `create_symref` callback in
ref backends completely.'.
>> + if (*next != line_termination)
>> + die("symref-create %s: extra input: %s", refname, next);
>> +
>> + if (ref_transaction_create(transaction, refname, NULL, new_ref,
>> + update_flags | create_reflog_flag |
>> + REF_SYMREF_UPDATE, msg, &err))
>> + die("%s", err.buf);
>> +
>> + update_flags = default_flags;
>> + free(refname);
>> + free(new_ref);
>> + strbuf_release(&err);
>> +}
>> +
>> static void parse_cmd_delete(struct ref_transaction *transaction,
>> const char *next, const char *end)
>> {
>> @@ -476,6 +509,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 6d98d9652d..e62c0f4aca 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1305,15 +1305,20 @@ 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_ref,
>> unsigned int flags, const char *msg,
>> struct strbuf *err)
>> {
>> - if (!new_oid || is_null_oid(new_oid)) {
>> + if ((flags & REF_SYMREF_UPDATE) && !new_ref) {
>> + strbuf_addf(err, "'%s' has a no new ref", refname);
>> + return 1;
>> + }
>> + if (!(flags & REF_SYMREF_UPDATE) && (!new_oid || is_null_oid(new_oid))) {
>> strbuf_addf(err, "'%s' has a null OID", refname);
>> return 1;
>> }
>> return ref_transaction_update(transaction, refname, new_oid,
>> - null_oid(), NULL, NULL, flags,
>> + null_oid(), new_ref, NULL, flags,
>> msg, err);
>> }
>>
>> diff --git a/refs.h b/refs.h
>> index 60e6a21a31..c01a517e40 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -744,6 +744,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_ref,
>> unsigned int flags, const char *msg,
>> struct strbuf *err);
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 7c894ebe65..59d438878a 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2609,6 +2609,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>> }
>> }
>>
>> + if (update->flags & REF_SYMREF_UPDATE && update->new_ref) {
>
> Let's add braces around `(updaet->flags & REF_SYMREF_UPDATE)` to make
> this easier to read.
>
This should now be cleaned up as we removed the flag entirely.
>> + if (create_symref_lock(refs, lock, update->refname, update->new_ref)) {
>> + ret = TRANSACTION_GENERIC_ERROR;
>> + goto out;
>> + }
>> +
>> + if (close_ref_gently(lock)) {
>> + strbuf_addf(err, "couldn't close '%s.lock'",
>> + update->refname);
>> + ret = TRANSACTION_GENERIC_ERROR;
>> + goto out;
>> + }
>> +
>> + /*
>> + * Once we have created the symref lock, the commit
>> + * phase of the transaction only needs to commit the lock.
>> + */
>> + update->flags |= REF_NEEDS_COMMIT;
>> + }
>> +
>> +
>> if ((update->flags & REF_HAVE_NEW) &&
>> !(update->flags & REF_DELETING) &&
>> !(update->flags & REF_LOG_ONLY)) {
>> @@ -2904,6 +2925,14 @@ static int files_transaction_finish(struct ref_store *ref_store,
>>
>> if (update->flags & REF_NEEDS_COMMIT ||
>> update->flags & REF_LOG_ONLY) {
>> + if (update->flags & REF_SYMREF_UPDATE && update->new_ref) {
>
> Here, as well.
>
Same here.
>> + /* for dangling symrefs we gracefully set the oid to zero */
>> + if (!refs_resolve_ref_unsafe(&refs->base, update->new_ref,
>> + RESOLVE_REF_READING, &update->new_oid, NULL)) {
>> + update->new_oid = *null_oid();
>> + }
>
> Can this actually happenn right now? I thought that the `read_ref()`
> further up forbids this case.
>
> Patrick
With update-ref, it won't happen anymore, because as you mentioned, we
use `read_ref()`. I thought it was still worthwhile to have. But I guess
its cleaner to remove this.
Attachment:
signature.asc
Description: PGP signature
