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