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

Commit Message

Karthik Nayak April 12, 2024, 9:59 a.m. UTC

From: Karthik Nayak <[e-mail]>

Add 'symref-create' to allow creation of symbolic refs in a transaction
via the 'git-update-ref' command. The 'symref-create' command takes in a
<new-ref>, which the created <ref> will point to.

We also support the 'core.prefersymlinkrefs', wherein if the flag is set
and the filesystem supports symlinks, we create the symbolic ref as a
symlink.

Signed-off-by: Karthik Nayak <[e-mail]>
---
 Documentation/git-update-ref.txt |  6 ++++
 builtin/clone.c                  |  2 +-
 builtin/update-ref.c             | 36 +++++++++++++++++++-
 refs.c                           |  9 +++--
 refs.h                           |  1 +
 refs/files-backend.c             | 38 +++++++++++++++++++++
 refs/reftable-backend.c          | 19 +++++++++--
 t/t0600-reffiles-backend.sh      | 32 ++++++++++++++++++
 t/t1400-update-ref.sh            | 58 ++++++++++++++++++++++++++++++++
 9 files changed, 194 insertions(+), 7 deletions(-)

Comments

Patrick Steinhardt April 19, 2024, 9:40 a.m. UTC | #1

On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <[e-mail]>
[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

Junio C Hamano April 19, 2024, 3:48 p.m. UTC | #2

Patrick Steinhardt <[e-mail]> writes:

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

True.

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

Yup, the language may not require the extra pair of parentheses, but
human eyes are helped by them.

Thanks, again and as always, for carefully reading.

Karthik Nayak April 21, 2024, 12:50 p.m. UTC | #3

Patrick Steinhardt <[e-mail]> writes:

> On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <[e-mail]>
> [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.

Karthik Nayak April 21, 2024, 3:57 p.m. UTC | #4

Karthik Nayak <karthik.188@gmail.

[snip]
>>> +				/* 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.

Well, just realized that this needs to stay because we also want the
resolved OID for the reflogs. Will modify the comment though.

Patrick Steinhardt April 23, 2024, 6:39 a.m. UTC | #5

On Sun, Apr 21, 2024 at 08:50:07AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> > On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
> >> From: Karthik Nayak <[e-mail]>
> > [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.'.

Well, once normal reference transactions learn to update symrefs we can
do the following:

    diff --git a/refs/refs-internal.h b/refs/refs-internal.h
    index 56641aa57a..2302311282 100644
    --- a/refs/refs-internal.h
    +++ b/refs/refs-internal.h
    @@ -676,7 +676,6 @@ struct ref_storage_be {
        ref_transaction_commit_fn *initial_transaction_commit;
     
        pack_refs_fn *pack_refs;
    -	create_symref_fn *create_symref;
        rename_ref_fn *rename_ref;
        copy_ref_fn *copy_ref;

There would be no need to specifically have this as a separate callback
anymore as we can now provide a generic wrapper `refs_create_symref()`
(in pseudo-code):

```
int refs_create_symref(struct ref_store *refs, const char *refname,
                       const char *target)
{
    tx = ref_store_transaction_begin(refs);
    ref_transaction_create_symref(tx, refname, target);
    ref_transaction_commit(tx);
}
```

Patrick

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

Karthik Nayak April 23, 2024, 10:52 a.m. UTC | #6

Patrick Steinhardt <[e-mail]> writes:

> On Sun, Apr 21, 2024 at 08:50:07AM -0400, Karthik Nayak wrote:
>> Patrick Steinhardt <[e-mail]> writes:
>>
>> > On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
>> >> From: Karthik Nayak <[e-mail]>
>> > [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.'.
>
> Well, once normal reference transactions learn to update symrefs we can
> do the following:
>
>     diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>     index 56641aa57a..2302311282 100644
>     --- a/refs/refs-internal.h
>     +++ b/refs/refs-internal.h
>     @@ -676,7 +676,6 @@ struct ref_storage_be {
>         ref_transaction_commit_fn *initial_transaction_commit;
>
>         pack_refs_fn *pack_refs;
>     -	create_symref_fn *create_symref;
>         rename_ref_fn *rename_ref;
>         copy_ref_fn *copy_ref;
>
> There would be no need to specifically have this as a separate callback
> anymore as we can now provide a generic wrapper `refs_create_symref()`
> (in pseudo-code):
>
> ```
> int refs_create_symref(struct ref_store *refs, const char *refname,
>                        const char *target)
> {
>     tx = ref_store_transaction_begin(refs);
>     ref_transaction_create_symref(tx, refname, target);
>     ref_transaction_commit(tx);
> }
> ```
>
> Patrick
>

Indeed, this is pretty nice side-effect. Thanks for explaining, I'll
remove the explicit check and allow dangling refs.

Patch

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index ef22a1a2f4..a5b1f42728 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@  performs all modifications together.  Specify commands of the form:
 	create SP <ref> SP <new-oid> LF
 	delete SP <ref> [SP <old-oid>] LF
 	verify SP <ref> [SP <old-oid>] LF
+	symref-create SP <ref> SP <new-ref> LF
 	symref-delete SP <ref> [SP <old-ref>] LF
 	symref-verify SP <ref> [SP <old-ref>] LF
 	option SP <opt> LF
@@ -88,6 +89,7 @@  quoting:
 	create SP <ref> NUL <new-oid> NUL
 	delete SP <ref> NUL [<old-oid>] NUL
 	verify SP <ref> NUL [<old-oid>] NUL
+	symref-create SP <ref> NUL <new-ref> NUL
 	symref-delete SP <ref> [NUL <old-ref>] NUL
 	symref-verify SP <ref> [NUL <old-ref>] NUL
 	option SP <opt> NUL
@@ -121,6 +123,10 @@  verify::
 	Verify <ref> against <old-oid> but do not change it.  If
 	<old-oid> is zero or missing, the ref must not exist.
 
+symref-create::
+	Create symbolic ref <ref> with <new-ref> after verifying
+	it does not exist.  Can only be used in `no-deref` mode.
+
 symref-delete::
 	Delete <ref> after verifying it exists with <old-ref>, if
 	given.
diff --git a/builtin/clone.c b/builtin/clone.c
index 74ec14542e..c0eed8e795 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -547,7 +547,7 @@  static void write_remote_refs(const struct ref *local_refs)
 		if (!r->peer_ref)
 			continue;
 		if (ref_transaction_create(t, r->peer_ref->name, &r->old_oid,
-					   0, NULL, &err))
+					   NULL, 0, NULL, &err))
 			die("%s", err.buf);
 	}
 
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3be9ae0d00..24556a28a8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,7 +258,7 @@  static void parse_cmd_create(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	if (ref_transaction_create(transaction, refname, &new_oid,
+	if (ref_transaction_create(transaction, refname, &new_oid, NULL,
 				   update_flags | create_reflog_flag,
 				   msg, &err))
 		die("%s", err.buf);
@@ -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);
+
+	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) {
+		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) {
+				/* 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();
+				}
+			}
+
 			if (files_log_ref_write(refs,
 						lock->ref_name,
 						&lock->old_oid,
@@ -2921,6 +2950,15 @@  static int files_transaction_finish(struct ref_store *ref_store,
 				goto cleanup;
 			}
 		}
+
+		/*
+		 * We try creating a symlink, if that succeeds we continue to the
+		 * next updated. If not, we try and create a regular symref.
+		 */
+		if (update->flags & REF_SYMREF_UPDATE && prefer_symlink_refs)
+			if (!create_ref_symlink(lock, update->new_ref))
+				continue;
+
 		if (update->flags & REF_NEEDS_COMMIT) {
 			clear_loose_ref_cache(refs);
 			if (commit_ref(lock)) {
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 935bf407df..6d42838e15 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -856,7 +856,7 @@  static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 			 * There is no need to write the reference deletion
 			 * when the reference in question doesn't exist.
 			 */
-			 if (u->flags & REF_HAVE_NEW && !is_null_oid(&u->new_oid)) {
+			 if (u->flags & REF_HAVE_NEW && !null_new_value(u)) {
 				 ret = queue_transaction_update(refs, tx_data, u,
 								&current_oid, err);
 				 if (ret)
@@ -1064,7 +1064,7 @@  static int write_transaction_table(struct reftable_writer *writer, void *cb_data
 		 * - `core.logAllRefUpdates` tells us to create the reflog for
 		 *   the given ref.
 		 */
-		if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) {
+		if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && null_new_value(u)) {
 			struct reftable_log_record log = {0};
 			struct reftable_iterator it = {0};
 
@@ -1122,7 +1122,20 @@  static int write_transaction_table(struct reftable_writer *writer, void *cb_data
 		if (u->flags & REF_LOG_ONLY)
 			continue;
 
-		if (u->flags & REF_HAVE_NEW && null_new_value(u)) {
+		if (u->flags & REF_SYMREF_UPDATE &&
+		    u->flags & REF_HAVE_NEW &&
+		    !null_new_value(u)) {
+			struct reftable_ref_record ref = {
+				.refname = (char *)u->refname,
+				.value_type = REFTABLE_REF_SYMREF,
+				.value.symref = (char *)u->new_ref,
+				.update_index = ts,
+			};
+
+			ret = reftable_writer_add_ref(writer, &ref);
+			if (ret < 0)
+				goto done;
+		} else if (u->flags & REF_HAVE_NEW && null_new_value(u)) {
 			struct reftable_ref_record ref = {
 				.refname = (char *)u->refname,
 				.update_index = ts,
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 64214340e7..c5061c26cf 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -472,4 +472,36 @@  test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	esac
 '
 
+test_expect_success SYMLINKS 'symref transaction supports symlinks' '
+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
+	git update-ref refs/heads/new @ &&
+	test_config core.prefersymlinkrefs true &&
+	cat >stdin <<-EOF &&
+	start
+	symref-create TESTSYMREFONE refs/heads/new
+	prepare
+	commit
+	EOF
+	git update-ref --no-deref --stdin <stdin &&
+	test_path_is_symlink .git/TESTSYMREFONE &&
+	test "$(test_readlink .git/TESTSYMREFONE)" = refs/heads/new
+'
+
+test_expect_success 'symref transaction supports false symlink config' '
+	test_when_finished "git symbolic-ref -d TESTSYMREFONE" &&
+	git update-ref refs/heads/new @ &&
+	test_config core.prefersymlinkrefs false &&
+	cat >stdin <<-EOF &&
+	start
+	symref-create TESTSYMREFONE refs/heads/new
+	prepare
+	commit
+	EOF
+	git update-ref --no-deref --stdin <stdin &&
+	test_path_is_file .git/TESTSYMREFONE &&
+	git symbolic-ref TESTSYMREFONE >actual &&
+	echo refs/heads/new >expect &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index cf01c5d867..f4e63fae6e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1754,6 +1754,64 @@  test_expect_success "stdin ${type} symref-delete ref works with right old value"
 	test_must_fail git rev-parse --verify -q $b
 '
 
+test_expect_success "stdin ${type} symref-create fails without --no-deref" '
+	create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" &&
+	test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+	grep "fatal: symref-create: cannot operate with deref mode" err
+'
+
+test_expect_success "stdin ${type} fails symref-create with no ref" '
+	create_stdin_buf ${type} "symref-create " >stdin &&
+	test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+	grep "fatal: symref-create: missing <ref>" err
+'
+
+test_expect_success "stdin ${type} fails symref-create with no new value" '
+	create_stdin_buf ${type} "symref-create refs/heads/symref" >stdin &&
+	test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+	grep "fatal: symref-create refs/heads/symref: missing <new-ref>" err
+'
+
+test_expect_success "stdin ${type} fails symref-create with too many arguments" '
+	create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" "$a" >stdin &&
+	test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+	if test "$type" = "-z"
+	then
+		grep "fatal: unknown command: $a" err
+	else
+		grep "fatal: symref-create refs/heads/symref: extra input:  $a" err
+	fi
+'
+
+test_expect_success "stdin ${type} symref-create ref works" '
+	test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+	create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" >stdin &&
+	git update-ref --stdin ${type} --no-deref <stdin &&
+	git symbolic-ref refs/heads/symref >expect &&
+	echo $a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success "stdin ${type} symref-create does not create reflogs by default" '
+	test_when_finished "git symbolic-ref -d refs/symref" &&
+	create_stdin_buf ${type} "symref-create refs/symref" "$a" >stdin &&
+	git update-ref --stdin ${type} --no-deref <stdin &&
+	git symbolic-ref refs/symref >expect &&
+	echo $a >actual &&
+	test_cmp expect actual &&
+	test_must_fail git reflog exists refs/symref
+'
+
+test_expect_success "stdin ${type} symref-create reflogs with --create-reflog" '
+	test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+	create_stdin_buf ${type} "symref-create refs/heads/symref" "$a" >stdin &&
+	git update-ref --create-reflog --stdin ${type} --no-deref <stdin &&
+	git symbolic-ref refs/heads/symref >expect &&
+	echo $a >actual &&
+	test_cmp expect actual &&
+	git reflog exists refs/heads/symref
+'
+
 done
 
 test_done