| Message ID | 20240919121335.298856-2-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fetch: set remote/HEAD if missing | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > Add a new REF_CREATE_ONLY flag for use by the files backend which will > only update the symref if it doesn't already exist. Add the possibility > to pass extra flags to refs_update_symref so that it can utilize this > new flag. If I wanted to create a symref that points at A, there are three cases: (1) the symref does not exist. (2) the symref exists and points at A. (3) the symref exists and points at B. I'll see a symref that points at A at the end in the first two cases, and my request is silently ignored in the third case. I'd expect that the caller can tell the failing case apart from the successful case with the return value or something. The caller might want to tell between the first two cases for reporting purposes, but I do not care as much as I would care about detecting true failures. Nobody actually passes the flag yet, so we would not be able to tell if any of the added code is buggy from this step alone. Let's see what happens in the next patch ;-). > diff --git a/refs.c b/refs.c > index ceb72d4bd7..7afe46cadc 100644 > --- a/refs.c > +++ b/refs.c > @@ -2085,8 +2085,9 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct > return peel_object(r, base, peeled) ? -1 : 0; > } > > + > int refs_update_symref(struct ref_store *refs, const char *ref, > - const char *target, const char *logmsg) > + const char *target, const unsigned int extra_flags, const char *logmsg) While it is not _wrong_ per-se to mark an "unsigned int" parameter as "const", it is a bit unusual in this code base. The only thing it prevents us from doing is to mutate it until this function returns, which does not help all that much in making the code safer, as opposed to marking a parameter of a pointer type as a const pointer.
On Fri Sep 20, 2024 at 00:46, Junio C Hamano <gitster@pobox.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: [ snip] > If I wanted to create a symref that points at A, there are three cases: > > (1) the symref does not exist. > (2) the symref exists and points at A. > (3) the symref exists and points at B. > > I'll see a symref that points at A at the end in the first two > cases, and my request is silently ignored in the third case. > > I'd expect that the caller can tell the failing case apart from the > successful case with the return value or something. The caller > might want to tell between the first two cases for reporting > purposes, but I do not care as much as I would care about detecting > true failures. Hmm. So in case I'm passing REF_CREATE_ONLY I would not expect the above cases to be error in the sense that transaction_finish should not report a failure and thus have all callers assume things went wrong. On the other hand it's a valid concern, that the caller may want to check what happened. Actually, the idea that I mentioned in https://lore.kernel.org/git/D4AK4USDVP5T.10INJOFE2I8LE@ferdinandy.com/ may actually be useful here as well. We could record the state of the reference atomically before the transaction in update, and then if the caller is interested, they can match this against what they requested. That way they can figure out which of the 3 cases they were in without raceconditions after the situation. Actually, this way any feedback could be given to the user post transaction here as well. If this sounds sensible, then I guess it would make sense to rejoin the set-head patch into this thread as well ... [snip] > > > diff --git a/refs.c b/refs.c > > index ceb72d4bd7..7afe46cadc 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -2085,8 +2085,9 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct > > return peel_object(r, base, peeled) ? -1 : 0; > > } > > > > + > > int refs_update_symref(struct ref_store *refs, const char *ref, > > - const char *target, const char *logmsg) > > + const char *target, const unsigned int extra_flags, const char *logmsg) > > While it is not _wrong_ per-se to mark an "unsigned int" parameter > as "const", it is a bit unusual in this code base. The only thing > it prevents us from doing is to mutate it until this function > returns, which does not help all that much in making the code safer, > as opposed to marking a parameter of a pointer type as a const > pointer. Makes sense, I'll drop it. Thanks very much for your patience! Best, Bence
On 19/09/2024 13:13, Bence Ferdinandy wrote: > Add a new REF_CREATE_ONLY flag for use by the files backend which will > only update the symref if it doesn't already exist. Add the possibility > to pass extra flags to refs_update_symref so that it can utilize this > new flag. I'm not sure we need a new flag to do this as it is already supported by the ref transaction api. struct ref_transaction *t; struct strbuf err = STRBUF_INIT; struct ref_store refs = get_main_ref_store(the_repository); int ret = 0; t = ref_transaction_begin(refs, &err); if (!t || ref_transaction_create(t, b_head.buf, NULL, b_remote_head.buf, REF_NO_DEREF, "fetch", &err) || ref_transaction_commit(t, &err)) ret = error(_("%s", err.buf)); ref_transaction_free(transaction); strbuf_release(&err); return ret; Best Wishes Phillip > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > --- > > Notes: > v3: new patch, passes all tests on it's own > > builtin/branch.c | 2 +- > builtin/checkout.c | 4 ++-- > builtin/clone.c | 6 +++--- > builtin/notes.c | 2 +- > builtin/remote.c | 6 +++--- > builtin/symbolic-ref.c | 2 +- > builtin/worktree.c | 2 +- > refs.c | 6 +++--- > refs.h | 12 ++++++++++-- > refs/files-backend.c | 8 ++++++++ > reset.c | 2 +- > sequencer.c | 2 +- > setup.c | 2 +- > t/helper/test-ref-store.c | 2 +- > 14 files changed, 37 insertions(+), 21 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index c98601c6fe..6025bca45e 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -559,7 +559,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, > continue; > > refs = get_worktree_ref_store(worktrees[i]); > - if (refs_update_symref(refs, "HEAD", newref, logmsg)) > + if (refs_update_symref(refs, "HEAD", newref, 0, logmsg)) > ret = error(_("HEAD of working tree %s is not updated"), > worktrees[i]->path); > } > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 4cfe6fab50..23e28321d6 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1011,7 +1011,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts, > describe_detached_head(_("HEAD is now at"), new_branch_info->commit); > } > } else if (new_branch_info->path) { /* Switch branches. */ > - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", new_branch_info->path, msg.buf) < 0) > + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", new_branch_info->path, 0, msg.buf) < 0) > die(_("unable to update HEAD")); > if (!opts->quiet) { > if (old_branch_info->path && !strcmp(new_branch_info->path, old_branch_info->path)) { > @@ -1475,7 +1475,7 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts) > die(_("You are on a branch yet to be born")); > strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch); > status = refs_update_symref(get_main_ref_store(the_repository), > - "HEAD", branch_ref.buf, "checkout -b"); > + "HEAD", branch_ref.buf, 0, "checkout -b"); > strbuf_release(&branch_ref); > if (!opts->quiet) > fprintf(stderr, _("Switched to a new branch '%s'\n"), > diff --git a/builtin/clone.c b/builtin/clone.c > index 269b6e18a4..43b7878a79 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -659,7 +659,7 @@ static void update_remote_refs(const struct ref *refs, > strbuf_addstr(&head_ref, branch_top); > strbuf_addstr(&head_ref, "HEAD"); > if (refs_update_symref(get_main_ref_store(the_repository), head_ref.buf, > - remote_head_points_at->peer_ref->name, > + remote_head_points_at->peer_ref->name, 0, > msg) < 0) > die(_("unable to update %s"), head_ref.buf); > strbuf_release(&head_ref); > @@ -672,7 +672,7 @@ static void update_head(const struct ref *our, const struct ref *remote, > const char *head; > if (our && skip_prefix(our->name, "refs/heads/", &head)) { > /* Local default branch link */ > - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", our->name, NULL) < 0) > + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", our->name, 0, NULL) < 0) > die(_("unable to update HEAD")); > if (!option_bare) { > refs_update_ref(get_main_ref_store(the_repository), > @@ -701,7 +701,7 @@ static void update_head(const struct ref *our, const struct ref *remote, > * Unborn head from remote; same as "our" case above except > * that we have no ref to update. > */ > - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", unborn, NULL) < 0) > + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", unborn, 0, NULL) < 0) > die(_("unable to update HEAD")); > if (!option_bare) > install_branch_config(0, head, remote_name, unborn); > diff --git a/builtin/notes.c b/builtin/notes.c > index 04f9dfb7fb..6b42d1139f 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -978,7 +978,7 @@ static int merge(int argc, const char **argv, const char *prefix) > die(_("a notes merge into %s is already in-progress at %s"), > default_notes_ref(), wt->path); > free_worktrees(worktrees); > - if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", default_notes_ref(), NULL)) > + if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", default_notes_ref(), 0, NULL)) > die(_("failed to store link to current notes ref (%s)"), > default_notes_ref()); > fprintf(stderr, _("Automatic notes merge failed. Fix conflicts in %s " > diff --git a/builtin/remote.c b/builtin/remote.c > index 0acc547d69..d28c65599d 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -243,7 +243,7 @@ static int add(int argc, const char **argv, const char *prefix) > strbuf_reset(&buf2); > strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master); > > - if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote add")) > + if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, 0, "remote add")) > result = error(_("Could not setup master '%s'"), master); > } > > @@ -863,7 +863,7 @@ static int mv(int argc, const char **argv, const char *prefix) > strbuf_reset(&buf3); > strbuf_addf(&buf3, "remote: renamed %s to %s", > item->string, buf.buf); > - if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf)) > + if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, 0, buf3.buf)) > die(_("creating '%s' failed"), buf.buf); > display_progress(progress, ++refs_renamed_nr); > } > @@ -1443,7 +1443,7 @@ static int set_head(int argc, const char **argv, const char *prefix) > /* make sure it's valid */ > if (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf)) > result |= error(_("Not a valid ref: %s"), buf2.buf); > - else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote set-head")) > + else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, 0, "remote set-head")) > result |= error(_("Could not setup %s"), buf.buf); > else if (opt_a) > printf("%s/HEAD set to %s\n", argv[0], head_name); > diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c > index 81abdd170f..52de3e02af 100644 > --- a/builtin/symbolic-ref.c > +++ b/builtin/symbolic-ref.c > @@ -84,7 +84,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix) > if (check_refname_format(argv[1], REFNAME_ALLOW_ONELEVEL) < 0) > die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]); > ret = !!refs_update_symref(get_main_ref_store(the_repository), > - argv[0], argv[1], msg); > + argv[0], argv[1], 0, msg); > break; > default: > usage_with_options(git_symbolic_ref_usage, options); > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 41e7f6a327..71434737e8 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -517,7 +517,7 @@ static int add_worktree(const char *path, const char *refname, > ret = refs_update_ref(wt_refs, NULL, "HEAD", &commit->object.oid, > NULL, 0, UPDATE_REFS_MSG_ON_ERR); > else > - ret = refs_update_symref(wt_refs, "HEAD", symref.buf, NULL); > + ret = refs_update_symref(wt_refs, "HEAD", symref.buf, 0, NULL); > if (ret) > goto done; > > diff --git a/refs.c b/refs.c > index ceb72d4bd7..7afe46cadc 100644 > --- a/refs.c > +++ b/refs.c > @@ -2085,8 +2085,9 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct > return peel_object(r, base, peeled) ? -1 : 0; > } > > + > int refs_update_symref(struct ref_store *refs, const char *ref, > - const char *target, const char *logmsg) > + const char *target, const unsigned int extra_flags, const char *logmsg) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > @@ -2095,7 +2096,7 @@ int refs_update_symref(struct ref_store *refs, const char *ref, > transaction = ref_store_transaction_begin(refs, &err); > if (!transaction || > ref_transaction_update(transaction, ref, NULL, NULL, > - target, NULL, REF_NO_DEREF, > + target, NULL, REF_NO_DEREF | extra_flags, > logmsg, &err) || > ref_transaction_commit(transaction, &err)) { > ret = error("%s", err.buf); > @@ -2920,4 +2921,3 @@ int ref_update_expects_existing_old_ref(struct ref_update *update) > return (update->flags & REF_HAVE_OLD) && > (!is_null_oid(&update->old_oid) || update->old_target); > } > - > diff --git a/refs.h b/refs.h > index f8b919a138..d907451d13 100644 > --- a/refs.h > +++ b/refs.h > @@ -569,7 +569,7 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref, > const char *newref, const char *logmsg); > > int refs_update_symref(struct ref_store *refs, const char *refname, > - const char *target, const char *logmsg); > + const char *target, const unsigned int extra_flags, const char *logmsg); > > enum action_on_err { > UPDATE_REFS_MSG_ON_ERR, > @@ -672,13 +672,21 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, > */ > #define REF_SKIP_CREATE_REFLOG (1 << 12) > > +/* > + * If the reference has already been created do not touch it. > + */ > + > +#define REF_CREATE_ONLY (1 << 13) > + > + > /* > * Bitmask of all of the flags that are allowed to be passed in to > * ref_transaction_update() and friends: > */ > #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \ > (REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \ > - REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG) > + REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG | \ > + REF_CREATE_ONLY) > > /* > * Add a reference update to transaction. `new_oid` is the value that > diff --git a/refs/files-backend.c b/refs/files-backend.c > index c7f3f4e591..1440b69b87 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2993,6 +2993,8 @@ static int files_transaction_finish(struct ref_store *ref_store, > struct ref_update *update = transaction->updates[i]; > struct ref_lock *lock = update->backend_data; > > + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) > + continue; > if (update->flags & REF_NEEDS_COMMIT || > update->flags & REF_LOG_ONLY) { > if (parse_and_write_reflog(refs, update, lock, err)) { > @@ -3031,6 +3033,8 @@ static int files_transaction_finish(struct ref_store *ref_store, > */ > for (i = 0; i < transaction->nr; i++) { > struct ref_update *update = transaction->updates[i]; > + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) > + continue; > if (update->flags & REF_DELETING && > !(update->flags & REF_LOG_ONLY) && > !(update->flags & REF_IS_PRUNING)) { > @@ -3061,6 +3065,8 @@ static int files_transaction_finish(struct ref_store *ref_store, > for (i = 0; i < transaction->nr; i++) { > struct ref_update *update = transaction->updates[i]; > struct ref_lock *lock = update->backend_data; > + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) > + continue; > > if (update->flags & REF_DELETING && > !(update->flags & REF_LOG_ONLY)) { > @@ -3085,6 +3091,8 @@ static int files_transaction_finish(struct ref_store *ref_store, > > for (i = 0; i < transaction->nr; i++) { > struct ref_update *update = transaction->updates[i]; > + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) > + continue; > > if (update->flags & REF_DELETED_RMDIR) { > /* > diff --git a/reset.c b/reset.c > index b22b1be792..8dce5f2133 100644 > --- a/reset.c > +++ b/reset.c > @@ -75,7 +75,7 @@ static int update_refs(const struct reset_head_opts *opts, > UPDATE_REFS_MSG_ON_ERR); > if (!ret) > ret = refs_update_symref(get_main_ref_store(the_repository), > - "HEAD", switch_to_branch, > + "HEAD", switch_to_branch, 0, > reflog_head); > } > if (!ret && run_hook) > diff --git a/sequencer.c b/sequencer.c > index 8d01cd50ac..924a78dab8 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5107,7 +5107,7 @@ static int pick_commits(struct repository *r, > } > msg = reflog_message(opts, "finish", "returning to %s", > head_ref.buf); > - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", head_ref.buf, msg)) { > + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", head_ref.buf, 0, msg)) { > res = error(_("could not update HEAD to %s"), > head_ref.buf); > goto cleanup_head_ref; > diff --git a/setup.c b/setup.c > index 29f8673921..6a1fdef2c3 100644 > --- a/setup.c > +++ b/setup.c > @@ -2169,7 +2169,7 @@ void create_reference_database(enum ref_storage_format ref_storage_format, > die(_("invalid initial branch name: '%s'"), > initial_branch); > > - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", ref, NULL) < 0) > + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", ref, 0, NULL) < 0) > exit(1); > free(ref); > } > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index 65346dee55..90af41edce 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -120,7 +120,7 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv) > const char *target = notnull(*argv++, "target"); > const char *logmsg = *argv++; > > - return refs_update_symref(refs, refname, target, logmsg); > + return refs_update_symref(refs, refname, target, 0, logmsg); > } > > static struct flag_definition transaction_flags[] = {
On Sat Sep 21, 2024 at 15:40, Phillip Wood <phillip.wood123@gmail.com> wrote: > On 19/09/2024 13:13, Bence Ferdinandy wrote: > > Add a new REF_CREATE_ONLY flag for use by the files backend which will > > only update the symref if it doesn't already exist. Add the possibility > > to pass extra flags to refs_update_symref so that it can utilize this > > new flag. > > I'm not sure we need a new flag to do this as it is already supported by > the ref transaction api. Thanks, I was not aware of ref_transaction_create. It also seems to return with TRANSACTION_NAME_CONFLICT so we should be able to see from the error code if indeed the existence was the problem or something else went wrong. Best, Bence
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > On Sat Sep 21, 2024 at 15:40, Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 19/09/2024 13:13, Bence Ferdinandy wrote: >> > Add a new REF_CREATE_ONLY flag for use by the files backend which will >> > only update the symref if it doesn't already exist. Add the possibility >> > to pass extra flags to refs_update_symref so that it can utilize this >> > new flag. >> >> I'm not sure we need a new flag to do this as it is already supported by >> the ref transaction api. > > Thanks, I was not aware of ref_transaction_create. It also seems to return with > TRANSACTION_NAME_CONFLICT so we should be able to see from the error code if > indeed the existence was the problem or something else went wrong. Thank you for working on this, and thanks, Phillip, for a useful suggestion.
On Sun Sep 22, 2024 at 00:19, Bence Ferdinandy <bence@ferdinandy.com> wrote: > > On Sat Sep 21, 2024 at 15:40, Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 19/09/2024 13:13, Bence Ferdinandy wrote: >> > Add a new REF_CREATE_ONLY flag for use by the files backend which will >> > only update the symref if it doesn't already exist. Add the possibility >> > to pass extra flags to refs_update_symref so that it can utilize this >> > new flag. >> >> I'm not sure we need a new flag to do this as it is already supported by >> the ref transaction api. > > Thanks, I was not aware of ref_transaction_create. It also seems to return with > TRANSACTION_NAME_CONFLICT so we should be able to see from the error code if > indeed the existence was the problem or something else went wrong. Unfortunately, it seems that my reading of the code did not pass practice. When using ref_transaction_create ref_transaction_commit will return with -2 if the reference already exists, but it also returns with -2 for various other issues, like if the lock file already exists. I could parse the error message to see what was the cause, but that doesn't feel like a robust solution. Since fetch should _not_ error out on this, I think the REF_CREATE_ONLY flag is warranted. As it stands, it would serve a different purpose than ref_transaction_create, i.e. a "silent" create-only. I'll send a v4 tomorrow hopefully. Best, Bence
On Mon, Sep 30, 2024 at 12:58:05AM +0200, Bence Ferdinandy wrote: > > On Sun Sep 22, 2024 at 00:19, Bence Ferdinandy <bence@ferdinandy.com> wrote: > > > > On Sat Sep 21, 2024 at 15:40, Phillip Wood <phillip.wood123@gmail.com> wrote: > >> On 19/09/2024 13:13, Bence Ferdinandy wrote: > >> > Add a new REF_CREATE_ONLY flag for use by the files backend which will > >> > only update the symref if it doesn't already exist. Add the possibility > >> > to pass extra flags to refs_update_symref so that it can utilize this > >> > new flag. > >> > >> I'm not sure we need a new flag to do this as it is already supported by > >> the ref transaction api. > > > > Thanks, I was not aware of ref_transaction_create. It also seems to return with > > TRANSACTION_NAME_CONFLICT so we should be able to see from the error code if > > indeed the existence was the problem or something else went wrong. > > Unfortunately, it seems that my reading of the code did not pass practice. When > using ref_transaction_create ref_transaction_commit will return with -2 if the > reference already exists, but it also returns with -2 for various other issues, > like if the lock file already exists. I could parse the error message to see > what was the cause, but that doesn't feel like a robust solution. Since fetch > should _not_ error out on this, I think the REF_CREATE_ONLY flag is warranted. > As it stands, it would serve a different purpose than ref_transaction_create, > i.e. a "silent" create-only. > > I'll send a v4 tomorrow hopefully. I don't think that is a good reason to introduce this new flag though. If we need to have a proper way to identify this specific failure case we should rather update the already-existing mechanism to give us useful signals, shouldn't we? The problem with this flag is that it basically duplicates functionality that already exists, and it needs to be wired up by every ref backend that we have and that we're adding in the future. Your patch for example only implements the functionality for the "files" backend, but it must also be wired up for the "reftable" backend or otherwise it would be broken. Another issue is that it gives you more ways to create nonsensical ref updates. With it you could for example create requests with a non-zero old object ID, and if it has `REF_CREATE_ONLY` set it would never be possible to fulfill the request. There's probably other cases where you can create nonsensical ref updates already, but we shouldn't add more ways of doing that. Mind you, if we go the way I propose and improve the error reporting we'd also have to adapt both backends to do so. But that would be plugging a gap for which we have no proper solution right now instead of circumventing the current design by duplicating the functionality that we already have in a way that makes us able to handle this. Patrick
On Mon Sep 30, 2024 at 08:40, Patrick Steinhardt <ps@pks.im> wrote: > I don't think that is a good reason to introduce this new flag though. > If we need to have a proper way to identify this specific failure case > we should rather update the already-existing mechanism to give us useful > signals, shouldn't we? > > The problem with this flag is that it basically duplicates functionality > that already exists, and it needs to be wired up by every ref backend > that we have and that we're adding in the future. Your patch for example > only implements the functionality for the "files" backend, but it must > also be wired up for the "reftable" backend or otherwise it would be > broken. > > Another issue is that it gives you more ways to create nonsensical ref > updates. With it you could for example create requests with a non-zero > old object ID, and if it has `REF_CREATE_ONLY` set it would never be > possible to fulfill the request. There's probably other cases where you > can create nonsensical ref updates already, but we shouldn't add more > ways of doing that. > > Mind you, if we go the way I propose and improve the error reporting > we'd also have to adapt both backends to do so. But that would be > plugging a gap for which we have no proper solution right now instead of > circumventing the current design by duplicating the functionality that > we already have in a way that makes us able to handle this. > > Patrick You make a convincing argument :) I'll try to see if I can add another transaction error code for when you only want to create not overwrite and the ref already exists and pass it up (for both files and reftables, you also don't mention it, but I think this would not concern packed after a quick glance at the code). Best, Bence
diff --git a/builtin/branch.c b/builtin/branch.c index c98601c6fe..6025bca45e 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -559,7 +559,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, continue; refs = get_worktree_ref_store(worktrees[i]); - if (refs_update_symref(refs, "HEAD", newref, logmsg)) + if (refs_update_symref(refs, "HEAD", newref, 0, logmsg)) ret = error(_("HEAD of working tree %s is not updated"), worktrees[i]->path); } diff --git a/builtin/checkout.c b/builtin/checkout.c index 4cfe6fab50..23e28321d6 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1011,7 +1011,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts, describe_detached_head(_("HEAD is now at"), new_branch_info->commit); } } else if (new_branch_info->path) { /* Switch branches. */ - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", new_branch_info->path, msg.buf) < 0) + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", new_branch_info->path, 0, msg.buf) < 0) die(_("unable to update HEAD")); if (!opts->quiet) { if (old_branch_info->path && !strcmp(new_branch_info->path, old_branch_info->path)) { @@ -1475,7 +1475,7 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts) die(_("You are on a branch yet to be born")); strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch); status = refs_update_symref(get_main_ref_store(the_repository), - "HEAD", branch_ref.buf, "checkout -b"); + "HEAD", branch_ref.buf, 0, "checkout -b"); strbuf_release(&branch_ref); if (!opts->quiet) fprintf(stderr, _("Switched to a new branch '%s'\n"), diff --git a/builtin/clone.c b/builtin/clone.c index 269b6e18a4..43b7878a79 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -659,7 +659,7 @@ static void update_remote_refs(const struct ref *refs, strbuf_addstr(&head_ref, branch_top); strbuf_addstr(&head_ref, "HEAD"); if (refs_update_symref(get_main_ref_store(the_repository), head_ref.buf, - remote_head_points_at->peer_ref->name, + remote_head_points_at->peer_ref->name, 0, msg) < 0) die(_("unable to update %s"), head_ref.buf); strbuf_release(&head_ref); @@ -672,7 +672,7 @@ static void update_head(const struct ref *our, const struct ref *remote, const char *head; if (our && skip_prefix(our->name, "refs/heads/", &head)) { /* Local default branch link */ - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", our->name, NULL) < 0) + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", our->name, 0, NULL) < 0) die(_("unable to update HEAD")); if (!option_bare) { refs_update_ref(get_main_ref_store(the_repository), @@ -701,7 +701,7 @@ static void update_head(const struct ref *our, const struct ref *remote, * Unborn head from remote; same as "our" case above except * that we have no ref to update. */ - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", unborn, NULL) < 0) + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", unborn, 0, NULL) < 0) die(_("unable to update HEAD")); if (!option_bare) install_branch_config(0, head, remote_name, unborn); diff --git a/builtin/notes.c b/builtin/notes.c index 04f9dfb7fb..6b42d1139f 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -978,7 +978,7 @@ static int merge(int argc, const char **argv, const char *prefix) die(_("a notes merge into %s is already in-progress at %s"), default_notes_ref(), wt->path); free_worktrees(worktrees); - if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", default_notes_ref(), NULL)) + if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", default_notes_ref(), 0, NULL)) die(_("failed to store link to current notes ref (%s)"), default_notes_ref()); fprintf(stderr, _("Automatic notes merge failed. Fix conflicts in %s " diff --git a/builtin/remote.c b/builtin/remote.c index 0acc547d69..d28c65599d 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -243,7 +243,7 @@ static int add(int argc, const char **argv, const char *prefix) strbuf_reset(&buf2); strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master); - if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote add")) + if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, 0, "remote add")) result = error(_("Could not setup master '%s'"), master); } @@ -863,7 +863,7 @@ static int mv(int argc, const char **argv, const char *prefix) strbuf_reset(&buf3); strbuf_addf(&buf3, "remote: renamed %s to %s", item->string, buf.buf); - if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf)) + if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, 0, buf3.buf)) die(_("creating '%s' failed"), buf.buf); display_progress(progress, ++refs_renamed_nr); } @@ -1443,7 +1443,7 @@ static int set_head(int argc, const char **argv, const char *prefix) /* make sure it's valid */ if (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf)) result |= error(_("Not a valid ref: %s"), buf2.buf); - else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote set-head")) + else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, 0, "remote set-head")) result |= error(_("Could not setup %s"), buf.buf); else if (opt_a) printf("%s/HEAD set to %s\n", argv[0], head_name); diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c index 81abdd170f..52de3e02af 100644 --- a/builtin/symbolic-ref.c +++ b/builtin/symbolic-ref.c @@ -84,7 +84,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix) if (check_refname_format(argv[1], REFNAME_ALLOW_ONELEVEL) < 0) die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]); ret = !!refs_update_symref(get_main_ref_store(the_repository), - argv[0], argv[1], msg); + argv[0], argv[1], 0, msg); break; default: usage_with_options(git_symbolic_ref_usage, options); diff --git a/builtin/worktree.c b/builtin/worktree.c index 41e7f6a327..71434737e8 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -517,7 +517,7 @@ static int add_worktree(const char *path, const char *refname, ret = refs_update_ref(wt_refs, NULL, "HEAD", &commit->object.oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR); else - ret = refs_update_symref(wt_refs, "HEAD", symref.buf, NULL); + ret = refs_update_symref(wt_refs, "HEAD", symref.buf, 0, NULL); if (ret) goto done; diff --git a/refs.c b/refs.c index ceb72d4bd7..7afe46cadc 100644 --- a/refs.c +++ b/refs.c @@ -2085,8 +2085,9 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct return peel_object(r, base, peeled) ? -1 : 0; } + int refs_update_symref(struct ref_store *refs, const char *ref, - const char *target, const char *logmsg) + const char *target, const unsigned int extra_flags, const char *logmsg) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -2095,7 +2096,7 @@ int refs_update_symref(struct ref_store *refs, const char *ref, transaction = ref_store_transaction_begin(refs, &err); if (!transaction || ref_transaction_update(transaction, ref, NULL, NULL, - target, NULL, REF_NO_DEREF, + target, NULL, REF_NO_DEREF | extra_flags, logmsg, &err) || ref_transaction_commit(transaction, &err)) { ret = error("%s", err.buf); @@ -2920,4 +2921,3 @@ int ref_update_expects_existing_old_ref(struct ref_update *update) return (update->flags & REF_HAVE_OLD) && (!is_null_oid(&update->old_oid) || update->old_target); } - diff --git a/refs.h b/refs.h index f8b919a138..d907451d13 100644 --- a/refs.h +++ b/refs.h @@ -569,7 +569,7 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref, const char *newref, const char *logmsg); int refs_update_symref(struct ref_store *refs, const char *refname, - const char *target, const char *logmsg); + const char *target, const unsigned int extra_flags, const char *logmsg); enum action_on_err { UPDATE_REFS_MSG_ON_ERR, @@ -672,13 +672,21 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, */ #define REF_SKIP_CREATE_REFLOG (1 << 12) +/* + * If the reference has already been created do not touch it. + */ + +#define REF_CREATE_ONLY (1 << 13) + + /* * Bitmask of all of the flags that are allowed to be passed in to * ref_transaction_update() and friends: */ #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \ (REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \ - REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG) + REF_SKIP_REFNAME_VERIFICATION | REF_SKIP_CREATE_REFLOG | \ + REF_CREATE_ONLY) /* * Add a reference update to transaction. `new_oid` is the value that diff --git a/refs/files-backend.c b/refs/files-backend.c index c7f3f4e591..1440b69b87 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2993,6 +2993,8 @@ static int files_transaction_finish(struct ref_store *ref_store, struct ref_update *update = transaction->updates[i]; struct ref_lock *lock = update->backend_data; + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) + continue; if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { if (parse_and_write_reflog(refs, update, lock, err)) { @@ -3031,6 +3033,8 @@ static int files_transaction_finish(struct ref_store *ref_store, */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) + continue; if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY) && !(update->flags & REF_IS_PRUNING)) { @@ -3061,6 +3065,8 @@ static int files_transaction_finish(struct ref_store *ref_store, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; struct ref_lock *lock = update->backend_data; + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) + continue; if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY)) { @@ -3085,6 +3091,8 @@ static int files_transaction_finish(struct ref_store *ref_store, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; + if (update->flags & REF_CREATE_ONLY && refs_ref_exists(ref_store, update->refname)) + continue; if (update->flags & REF_DELETED_RMDIR) { /* diff --git a/reset.c b/reset.c index b22b1be792..8dce5f2133 100644 --- a/reset.c +++ b/reset.c @@ -75,7 +75,7 @@ static int update_refs(const struct reset_head_opts *opts, UPDATE_REFS_MSG_ON_ERR); if (!ret) ret = refs_update_symref(get_main_ref_store(the_repository), - "HEAD", switch_to_branch, + "HEAD", switch_to_branch, 0, reflog_head); } if (!ret && run_hook) diff --git a/sequencer.c b/sequencer.c index 8d01cd50ac..924a78dab8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5107,7 +5107,7 @@ static int pick_commits(struct repository *r, } msg = reflog_message(opts, "finish", "returning to %s", head_ref.buf); - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", head_ref.buf, msg)) { + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", head_ref.buf, 0, msg)) { res = error(_("could not update HEAD to %s"), head_ref.buf); goto cleanup_head_ref; diff --git a/setup.c b/setup.c index 29f8673921..6a1fdef2c3 100644 --- a/setup.c +++ b/setup.c @@ -2169,7 +2169,7 @@ void create_reference_database(enum ref_storage_format ref_storage_format, die(_("invalid initial branch name: '%s'"), initial_branch); - if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", ref, NULL) < 0) + if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", ref, 0, NULL) < 0) exit(1); free(ref); } diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 65346dee55..90af41edce 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -120,7 +120,7 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv) const char *target = notnull(*argv++, "target"); const char *logmsg = *argv++; - return refs_update_symref(refs, refname, target, logmsg); + return refs_update_symref(refs, refname, target, 0, logmsg); } static struct flag_definition transaction_flags[] = {
Add a new REF_CREATE_ONLY flag for use by the files backend which will only update the symref if it doesn't already exist. Add the possibility to pass extra flags to refs_update_symref so that it can utilize this new flag. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- Notes: v3: new patch, passes all tests on it's own builtin/branch.c | 2 +- builtin/checkout.c | 4 ++-- builtin/clone.c | 6 +++--- builtin/notes.c | 2 +- builtin/remote.c | 6 +++--- builtin/symbolic-ref.c | 2 +- builtin/worktree.c | 2 +- refs.c | 6 +++--- refs.h | 12 ++++++++++-- refs/files-backend.c | 8 ++++++++ reset.c | 2 +- sequencer.c | 2 +- setup.c | 2 +- t/helper/test-ref-store.c | 2 +- 14 files changed, 37 insertions(+), 21 deletions(-)