On Sun, Apr 21, 2024 at 08:50:07AM -0400, Karthik Nayak wrote: > Patrick Steinhardt writes: > > > On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote: > >> From: Karthik Nayak > > [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 "); > >> + > >> + new_ref = parse_next_refname(&next); > >> + if (!new_ref) > >> + die("symref-create %s: missing ", refname); > >> + if (read_ref(new_ref, NULL)) > >> + die("symref-create %s: invalid ", 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.