Re: [PATCH v2 5/7] update-ref: add support for symref-create

> On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
> [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.
>

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

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

>> +				/* 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