| Message ID | 20241012230428.3259229-1-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,1/6] refs: atomically record overwritten ref in update_symref | expand |
Hi Bence On 13/10/2024 00:03, Bence Ferdinandy wrote: > When updating a symref it's currently not possible to know for sure what > was the previous value that was overwritten. It is if you use a ref transaction rather than call refs_update_symref() and query the ref after calling ref_transaction_prepare() and before calling ref_transaction_commit() which is what the code below does. > Record the value after the > ref has been locked if the caller of refs_update_symref requests it via > a new variable in the function call. To me this patch and patch 5 feel quite disruptive to all the existing callers which don't need this specialized functionality. I think it would be less disruptive over all if you used a ref transaction rather than calling refs_update_symref() in the final patch. That would enable us to keep the simpler interface for refs_update_symref(). I'm also not sure about the proposed interface I would have thought it would be simpler to take a "char**" rather than an "struct strbuf*" if we do decide that it is useful for callers of refs_update_symref() to query the old value. Best Wishes Phillip > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > --- > > Notes: > v4: new patch > > v5: - added before_target to reftables backend > - added an extra safety check for transaction's existence in refs.c > > v6: - no change > > v7: - remove the whole before_target concept from the backends and > handle checking it in refs.c instead (thanks Karthik) > - rename the before_target to referent which is how the same concept > is called in the backends > - change commit prefix to be more in line with project standards > > 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 | 16 +++++++++++----- > refs.h | 3 ++- > reset.c | 2 +- > sequencer.c | 2 +- > setup.c | 2 +- > t/helper/test-ref-store.c | 2 +- > 13 files changed, 29 insertions(+), 22 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index fd1611ebf5..6c87690b58 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, logmsg, NULL)) > ret = error(_("HEAD of working tree %s is not updated"), > worktrees[i]->path); > } > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 9c30000d3a..356ee9bcde 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1015,7 +1015,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, msg.buf, NULL) < 0) > die(_("unable to update HEAD")); > if (!opts->quiet) { > if (old_branch_info->path && !strcmp(new_branch_info->path, old_branch_info->path)) { > @@ -1479,7 +1479,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, "checkout -b", NULL); > 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 e77339c847..ead2af20ea 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -661,7 +661,7 @@ static void update_remote_refs(const struct ref *refs, > 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, > - msg) < 0) > + msg, NULL) < 0) > die(_("unable to update %s"), head_ref.buf); > strbuf_release(&head_ref); > } > @@ -673,7 +673,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, NULL, NULL) < 0) > die(_("unable to update HEAD")); > if (!option_bare) { > refs_update_ref(get_main_ref_store(the_repository), > @@ -702,7 +702,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, NULL, 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 8c26e45526..ba646f06ff 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -980,7 +980,7 @@ static int merge(int argc, const char **argv, const char *prefix) > die(_("a notes merge into %s is already in-progress at %s"), > notes_ref, wt->path); > free_worktrees(worktrees); > - if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", notes_ref, NULL)) > + if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", notes_ref, NULL, NULL)) > die(_("failed to store link to current notes ref (%s)"), > notes_ref); > fprintf(stderr, _("Automatic notes merge failed. Fix conflicts in %s " > diff --git a/builtin/remote.c b/builtin/remote.c > index 76670ddd8b..d8ff440027 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -244,7 +244,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, "remote add", NULL)) > result = error(_("Could not setup master '%s'"), master); > } > > @@ -864,7 +864,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, buf3.buf, NULL)) > die(_("creating '%s' failed"), buf.buf); > display_progress(progress, ++refs_renamed_nr); > } > @@ -1444,7 +1444,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, "remote set-head", NULL)) > 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 299d23d76a..7728fbc3c1 100644 > --- a/builtin/symbolic-ref.c > +++ b/builtin/symbolic-ref.c > @@ -88,7 +88,7 @@ int cmd_symbolic_ref(int argc, > 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], msg, NULL); > break; > default: > usage_with_options(git_symbolic_ref_usage, options); > diff --git a/builtin/worktree.c b/builtin/worktree.c > index fc31d072a6..a7ab4193c1 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, NULL, NULL); > if (ret) > goto done; > > diff --git a/refs.c b/refs.c > index 5f729ed412..b964ac44d0 100644 > --- a/refs.c > +++ b/refs.c > @@ -2114,7 +2114,8 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct > } > > int refs_update_symref(struct ref_store *refs, const char *ref, > - const char *target, const char *logmsg) > + const char *target, const char *logmsg, > + struct strbuf *referent) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > @@ -2122,17 +2123,23 @@ 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, > + ref_transaction_update(transaction, ref, NULL, NULL, > target, NULL, REF_NO_DEREF, > logmsg, &err) || > - ref_transaction_commit(transaction, &err)) { > + ref_transaction_prepare(transaction, &err)) { > ret = error("%s", err.buf); > + goto cleanup; > } > + if (referent) > + refs_read_symbolic_ref(refs, ref, referent); > + > + if (ref_transaction_commit(transaction, &err)) > + ret = error("%s", err.buf); > > +cleanup: > strbuf_release(&err); > if (transaction) > ref_transaction_free(transaction); > - > return ret; > } > > @@ -2948,4 +2955,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 108dfc93b3..b09a3a4384 100644 > --- a/refs.h > +++ b/refs.h > @@ -571,7 +571,8 @@ 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 char *logmsg, > + struct strbuf *referent); > > enum action_on_err { > UPDATE_REFS_MSG_ON_ERR, > diff --git a/reset.c b/reset.c > index b22b1be792..cc36a9ed56 100644 > --- a/reset.c > +++ b/reset.c > @@ -76,7 +76,7 @@ static int update_refs(const struct reset_head_opts *opts, > if (!ret) > ret = refs_update_symref(get_main_ref_store(the_repository), > "HEAD", switch_to_branch, > - reflog_head); > + reflog_head, NULL); > } > if (!ret && run_hook) > run_hooks_l(the_repository, "post-checkout", > diff --git a/sequencer.c b/sequencer.c > index 8d01cd50ac..23b162924c 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, msg, NULL)) { > res = error(_("could not update HEAD to %s"), > head_ref.buf); > goto cleanup_head_ref; > diff --git a/setup.c b/setup.c > index 94e79b2e48..d95f051465 100644 > --- a/setup.c > +++ b/setup.c > @@ -2275,7 +2275,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, NULL, NULL) < 0) > exit(1); > free(ref); > } > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index 65346dee55..a911302bea 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, logmsg, NULL); > } > > static struct flag_definition transaction_flags[] = {
On Sun Oct 13, 2024 at 15:52, Phillip Wood <phillip.wood123@gmail.com> wrote: > Hi Bence > > On 13/10/2024 00:03, Bence Ferdinandy wrote: >> When updating a symref it's currently not possible to know for sure what >> was the previous value that was overwritten. > > It is if you use a ref transaction rather than call refs_update_symref() > and query the ref after calling ref_transaction_prepare() and before > calling ref_transaction_commit() which is what the code below does. Yeah, it would be more clear if that sentence would say "when using update_symref". > >> Record the value after the >> ref has been locked if the caller of refs_update_symref requests it via >> a new variable in the function call. > > To me this patch and patch 5 feel quite disruptive to all the existing > callers which don't need this specialized functionality. I think it > would be less disruptive over all if you used a ref transaction rather > than calling refs_update_symref() in the final patch. That would enable > us to keep the simpler interface for refs_update_symref(). The extra parameter introduced here is actually used in two places by the end of the series, in remote set-head and fetch (of course you could make a similar argument for the functionality added in 5/6 which is only used in fetch by the final patch). To avoid code duplication I think even if we did not touch refs_update_symref() it would make sense to create a "refs_update_symref_extended()" and make refs_update_symref() a wrapper around that with a few less parameters. That would be similar to how refs_update_symref() and refs_update_ref() predetermine a couple of parameters to say transaction_update(). Currently there are 15 calls to refs_update_symref() in total, of these 5 do not use the complete functionality of the function (they pass NULL as logmsg), so the current implementation would not be completely unprecedented. (This tally did make me catch an error on my side: the logmsg in fetch's set_head should be "fetch" not "remote set-head", I'll fix that in a v8). Imho, even if I manage to come up with a better name than "refs_update_symref_extended()" wouldn't it be more confusing to have two ways to update symrefs via a function call, rather than have one, where _usually_ you pass two NULL-s at the end? > > I'm also not sure about the proposed interface I would have thought it > would be simpler to take a "char**" rather than an "struct strbuf*" if > we do decide that it is useful for callers of refs_update_symref() to > query the old value. refs_read_symbolic_ref requires a strbuf, so one would need to be created anyway and I also sort of got the feeling that the project likes to handle refs in strbufs (which may be wrong). Are there any downsides I'm not seeing? Thanks, Bence
Hi Bence On 13/10/2024 22:24, Bence Ferdinandy wrote: > > On Sun Oct 13, 2024 at 15:52, Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 13/10/2024 00:03, Bence Ferdinandy wrote: >>> Record the value after the >>> ref has been locked if the caller of refs_update_symref requests it via >>> a new variable in the function call. >> >> To me this patch and patch 5 feel quite disruptive to all the existing >> callers which don't need this specialized functionality. I think it >> would be less disruptive over all if you used a ref transaction rather >> than calling refs_update_symref() in the final patch. That would enable >> us to keep the simpler interface for refs_update_symref(). > > The extra parameter introduced here is actually used in two places by the end > of the series, in remote set-head and fetch (of course you could make a similar > argument for the functionality added in 5/6 which is only used in fetch by the > final patch). To avoid code duplication I think even if we did not touch > refs_update_symref() it would make sense to create > a "refs_update_symref_extended()" and make refs_update_symref() a wrapper > around that with a few less parameters. That would be similar to how > refs_update_symref() and refs_update_ref() predetermine a couple of parameters > to say transaction_update(). > > Currently there are 15 calls to refs_update_symref() in total, of these > 5 do not use the complete functionality of the function (they pass NULL as > logmsg), so the current implementation would not be completely unprecedented. As those figures show it's pretty unusual not to pass a reflog message when updating a ref, on the other hand it is very unusual to want the old value so I don't think the two are comparable. At a high level the two callers that want to be able to check the old value are both doing essentially the same thing so can we create a specialized function that encapsulates the functionality needed by --set-head and uses a ref transaction? > (This tally did make me catch an error on my side: the logmsg in fetch's > set_head should be "fetch" not "remote set-head", I'll fix that in a v8). > > Imho, even if I manage to come up with a better name than > "refs_update_symref_extended()" wouldn't it be more confusing to have two ways > to update symrefs via a function call, rather than have one, where _usually_ > you pass two NULL-s at the end? > >> >> I'm also not sure about the proposed interface I would have thought it >> would be simpler to take a "char**" rather than an "struct strbuf*" if >> we do decide that it is useful for callers of refs_update_symref() to >> query the old value. > > refs_read_symbolic_ref requires a strbuf, so one would need to be created > anyway and I also sort of got the feeling that the project likes to handle refs > in strbufs (which may be wrong). Are there any downsides I'm not seeing? It's true that refs_read_symbolic_ref takes and strbuf. I'd argue that's a mistake for a function that is just returning a string in an "out" parameter as I think it is more continent for the caller not to have to initialize an strbuf just to retrieve the target of a symbolic ref. I alse think that it is inconsistent with functions like refs_resolve_refdup() that return a string. Best Wishes Phillip
On Tue Oct 15, 2024 at 16:05, Phillip Wood <phillip.wood123@gmail.com> wrote: > Hi Bence > > On 13/10/2024 22:24, Bence Ferdinandy wrote: >> >> On Sun Oct 13, 2024 at 15:52, Phillip Wood <phillip.wood123@gmail.com> wrote: >>> On 13/10/2024 00:03, Bence Ferdinandy wrote: >>>> Record the value after the >>>> ref has been locked if the caller of refs_update_symref requests it via >>>> a new variable in the function call. >>> >>> To me this patch and patch 5 feel quite disruptive to all the existing >>> callers which don't need this specialized functionality. I think it >>> would be less disruptive over all if you used a ref transaction rather >>> than calling refs_update_symref() in the final patch. That would enable >>> us to keep the simpler interface for refs_update_symref(). >> >> The extra parameter introduced here is actually used in two places by the end >> of the series, in remote set-head and fetch (of course you could make a similar >> argument for the functionality added in 5/6 which is only used in fetch by the >> final patch). To avoid code duplication I think even if we did not touch >> refs_update_symref() it would make sense to create >> a "refs_update_symref_extended()" and make refs_update_symref() a wrapper >> around that with a few less parameters. That would be similar to how >> refs_update_symref() and refs_update_ref() predetermine a couple of parameters >> to say transaction_update(). >> >> Currently there are 15 calls to refs_update_symref() in total, of these >> 5 do not use the complete functionality of the function (they pass NULL as >> logmsg), so the current implementation would not be completely unprecedented. > > As those figures show it's pretty unusual not to pass a reflog message > when updating a ref, on the other hand it is very unusual to want the > old value so I don't think the two are comparable. At a high level the > two callers that want to be able to check the old value are both doing > essentially the same thing so can we create a specialized function that > encapsulates the functionality needed by --set-head and uses a ref > transaction? Ok, so let's not change the signature of refs_update_symref(). On the other hand the best approach here is still not quite clear to me. As it is now, the series added two extra parameters to refs_update_symref(), the referent and the create_only option. Obviously any new function would also take all the parameters refs_update_symref takes. Should I rename the current (v8) refs_update_symref to something else (name suggestion would be welcome, refs_update_symref_extended?, refs_update_symref_check_current? checking the currently existing symref is the common theme for the two new parameters) and leave refs_update_symref as it is? Or is this really about just not having to change all the already existing callers? Because in that case I'd just wrap the new function, something like int refs_update_symref(struct ref_store *refs, const char *ref, const char *target, const char *logmsg) return refs_update_symref_extended(*refs, *ref, *target, *logmsg, NULL, 0); Let me know what you think, I'll hold off on a v9 until I see more clearly here on what would be preferred. > >> (This tally did make me catch an error on my side: the logmsg in fetch's >> set_head should be "fetch" not "remote set-head", I'll fix that in a v8). >> >> Imho, even if I manage to come up with a better name than >> "refs_update_symref_extended()" wouldn't it be more confusing to have two ways >> to update symrefs via a function call, rather than have one, where _usually_ >> you pass two NULL-s at the end? >> >>> >>> I'm also not sure about the proposed interface I would have thought it >>> would be simpler to take a "char**" rather than an "struct strbuf*" if >>> we do decide that it is useful for callers of refs_update_symref() to >>> query the old value. >> >> refs_read_symbolic_ref requires a strbuf, so one would need to be created >> anyway and I also sort of got the feeling that the project likes to handle refs >> in strbufs (which may be wrong). Are there any downsides I'm not seeing? > > It's true that refs_read_symbolic_ref takes and strbuf. I'd argue that's > a mistake for a function that is just returning a string in an "out" > parameter as I think it is more continent for the caller not to have to > initialize an strbuf just to retrieve the target of a symbolic ref. I > alse think that it is inconsistent with functions like > refs_resolve_refdup() that return a string. Ok, I'll change this to use **char. On the other hand, if refs_read_symbolic_ref is inconsistent would it make sense to change it to also use a **char instead of strbuf? There's only four calls to it including the one I just added. Although I might rather do that _after_ this series is resolved :) Thanks (to everyone) for your time! Best, Bence
On Tue Oct 15, 2024 at 19:25, Bence Ferdinandy <bence@ferdinandy.com> wrote: > > On Tue Oct 15, 2024 at 16:05, Phillip Wood <phillip.wood123@gmail.com> wrote: [snip] >>>> >>>> I'm also not sure about the proposed interface I would have thought it >>>> would be simpler to take a "char**" rather than an "struct strbuf*" if >>>> we do decide that it is useful for callers of refs_update_symref() to >>>> query the old value. >>> >>> refs_read_symbolic_ref requires a strbuf, so one would need to be created >>> anyway and I also sort of got the feeling that the project likes to handle refs >>> in strbufs (which may be wrong). Are there any downsides I'm not seeing? >> >> It's true that refs_read_symbolic_ref takes and strbuf. I'd argue that's >> a mistake for a function that is just returning a string in an "out" >> parameter as I think it is more continent for the caller not to have to >> initialize an strbuf just to retrieve the target of a symbolic ref. I >> alse think that it is inconsistent with functions like >> refs_resolve_refdup() that return a string. > > Ok, I'll change this to use **char. On the other hand, if > refs_read_symbolic_ref is inconsistent would it make sense to change it to also > use a **char instead of strbuf? There's only four calls to it including the one > I just added. Although I might rather do that _after_ this series is resolved :) I started doing this change, but ran into two things which I would yet again, bring up in defence of strbuf. So not only refs_read_symbolic_ref makes use of strbuf, but a) I also use it in refs_update_symref[_extended] to check if the caller actually wants that referent or not (passing a NULL or a strbuf). b) the consumer, report_set_head_auto check for !buf->len Both of these sound way more convenient with strbuf than with *char. Ofc I'm not exactly a C guru so ... Anyhow, v9 does not have this change. Best, Bence
diff --git a/builtin/branch.c b/builtin/branch.c index fd1611ebf5..6c87690b58 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, logmsg, NULL)) ret = error(_("HEAD of working tree %s is not updated"), worktrees[i]->path); } diff --git a/builtin/checkout.c b/builtin/checkout.c index 9c30000d3a..356ee9bcde 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1015,7 +1015,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, msg.buf, NULL) < 0) die(_("unable to update HEAD")); if (!opts->quiet) { if (old_branch_info->path && !strcmp(new_branch_info->path, old_branch_info->path)) { @@ -1479,7 +1479,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, "checkout -b", NULL); 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 e77339c847..ead2af20ea 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -661,7 +661,7 @@ static void update_remote_refs(const struct ref *refs, 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, - msg) < 0) + msg, NULL) < 0) die(_("unable to update %s"), head_ref.buf); strbuf_release(&head_ref); } @@ -673,7 +673,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, NULL, NULL) < 0) die(_("unable to update HEAD")); if (!option_bare) { refs_update_ref(get_main_ref_store(the_repository), @@ -702,7 +702,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, NULL, 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 8c26e45526..ba646f06ff 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -980,7 +980,7 @@ static int merge(int argc, const char **argv, const char *prefix) die(_("a notes merge into %s is already in-progress at %s"), notes_ref, wt->path); free_worktrees(worktrees); - if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", notes_ref, NULL)) + if (refs_update_symref(get_main_ref_store(the_repository), "NOTES_MERGE_REF", notes_ref, NULL, NULL)) die(_("failed to store link to current notes ref (%s)"), notes_ref); fprintf(stderr, _("Automatic notes merge failed. Fix conflicts in %s " diff --git a/builtin/remote.c b/builtin/remote.c index 76670ddd8b..d8ff440027 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -244,7 +244,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, "remote add", NULL)) result = error(_("Could not setup master '%s'"), master); } @@ -864,7 +864,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, buf3.buf, NULL)) die(_("creating '%s' failed"), buf.buf); display_progress(progress, ++refs_renamed_nr); } @@ -1444,7 +1444,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, "remote set-head", NULL)) 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 299d23d76a..7728fbc3c1 100644 --- a/builtin/symbolic-ref.c +++ b/builtin/symbolic-ref.c @@ -88,7 +88,7 @@ int cmd_symbolic_ref(int argc, 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], msg, NULL); break; default: usage_with_options(git_symbolic_ref_usage, options); diff --git a/builtin/worktree.c b/builtin/worktree.c index fc31d072a6..a7ab4193c1 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, NULL, NULL); if (ret) goto done; diff --git a/refs.c b/refs.c index 5f729ed412..b964ac44d0 100644 --- a/refs.c +++ b/refs.c @@ -2114,7 +2114,8 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct } int refs_update_symref(struct ref_store *refs, const char *ref, - const char *target, const char *logmsg) + const char *target, const char *logmsg, + struct strbuf *referent) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -2122,17 +2123,23 @@ 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, + ref_transaction_update(transaction, ref, NULL, NULL, target, NULL, REF_NO_DEREF, logmsg, &err) || - ref_transaction_commit(transaction, &err)) { + ref_transaction_prepare(transaction, &err)) { ret = error("%s", err.buf); + goto cleanup; } + if (referent) + refs_read_symbolic_ref(refs, ref, referent); + + if (ref_transaction_commit(transaction, &err)) + ret = error("%s", err.buf); +cleanup: strbuf_release(&err); if (transaction) ref_transaction_free(transaction); - return ret; } @@ -2948,4 +2955,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 108dfc93b3..b09a3a4384 100644 --- a/refs.h +++ b/refs.h @@ -571,7 +571,8 @@ 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 char *logmsg, + struct strbuf *referent); enum action_on_err { UPDATE_REFS_MSG_ON_ERR, diff --git a/reset.c b/reset.c index b22b1be792..cc36a9ed56 100644 --- a/reset.c +++ b/reset.c @@ -76,7 +76,7 @@ static int update_refs(const struct reset_head_opts *opts, if (!ret) ret = refs_update_symref(get_main_ref_store(the_repository), "HEAD", switch_to_branch, - reflog_head); + reflog_head, NULL); } if (!ret && run_hook) run_hooks_l(the_repository, "post-checkout", diff --git a/sequencer.c b/sequencer.c index 8d01cd50ac..23b162924c 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, msg, NULL)) { res = error(_("could not update HEAD to %s"), head_ref.buf); goto cleanup_head_ref; diff --git a/setup.c b/setup.c index 94e79b2e48..d95f051465 100644 --- a/setup.c +++ b/setup.c @@ -2275,7 +2275,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, NULL, NULL) < 0) exit(1); free(ref); } diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 65346dee55..a911302bea 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, logmsg, NULL); } static struct flag_definition transaction_flags[] = {
When updating a symref it's currently not possible to know for sure what was the previous value that was overwritten. Record the value after the ref has been locked if the caller of refs_update_symref requests it via a new variable in the function call. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- Notes: v4: new patch v5: - added before_target to reftables backend - added an extra safety check for transaction's existence in refs.c v6: - no change v7: - remove the whole before_target concept from the backends and handle checking it in refs.c instead (thanks Karthik) - rename the before_target to referent which is how the same concept is called in the backends - change commit prefix to be more in line with project standards 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 | 16 +++++++++++----- refs.h | 3 ++- reset.c | 2 +- sequencer.c | 2 +- setup.c | 2 +- t/helper/test-ref-store.c | 2 +- 13 files changed, 29 insertions(+), 22 deletions(-)