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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 23, 2024, 06:39 UTC
Message-ID: <ZidXrdi7hXdAnDhy@tanuki>
In-Reply-To: <CAOLa=ZSvx4bOmJY+P5j3fJTqfjY37teMHQhOYDjTM4oJtnv_qw@mail.gmail.com>

On Sun, Apr 21, 2024 at 08:50:07AM -0400, Karthik Nayak wrote:

Show 39 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> >
> 
> 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

Show 128 quoted lines

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