| Message ID | 20240412095908.1134387-1-knayak@gitlab.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | update-ref: add symref oriented commands | expand |
"make refs.sp" should have told you that you should not write NULL
as just 0. I'll queue a SQUASH??? fix-up on top before merging it
into 'seen' for today's integration.
diff --git c/refs.c w/refs.c
index 010f426def..d578a2823b 100644
--- c/refs.c
+++ w/refs.c
@@ -2758,7 +2758,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,
for_each_string_list_item(item, refnames) {
ret = ref_transaction_delete(transaction, item->string,
- NULL, flags, 0, msg, &err);
+ NULL, flags, NULL, msg, &err);
if (ret) {
warning(_("could not delete reference %s: %s"),
item->string, err.buf);
Also there are quite many whitespace breakages.
$ git am -s ./+kn7-v2-update-ref-symref
Applying: refs: accept symref values in `ref_transaction[_add]_update`
Applying: update-ref: add support for symref-verify
.git/rebase-apply/patch:59: indent with spaces.
if (line_termination) {
.git/rebase-apply/patch:60: indent with spaces.
/* Without -z, consume SP and use next argument */
.git/rebase-apply/patch:61: indent with spaces.
if (!**next || **next == line_termination)
.git/rebase-apply/patch:62: indent with spaces.
return NULL;
.git/rebase-apply/patch:63: indent with spaces.
if (**next != ' ')
warning: squelched 10 whitespace errors
warning: 15 lines applied after fixing whitespace errors.
Applying: update-ref: add support for symref-delete
.git/rebase-apply/patch:95: indent with spaces.
die("symref-delete: cannot operate with deref mode");
.git/rebase-apply/patch:101: indent with spaces.
old_ref = parse_next_refname(&next);
warning: 2 lines applied after fixing whitespace errors.
Applying: files-backend: extract out `create_symref_lock`
Applying: update-ref: add support for symref-create
.git/rebase-apply/patch:81: indent with spaces.
die("symref-create: cannot operate with deref mode");
warning: 1 line applied after fixing whitespace errors.
Applying: update-ref: add support for symref-update
Applying: refs: support symrefs in 'reference-transaction' hook
Junio C Hamano <gitster@pobox.com> writes: > "make refs.sp" should have told you that you should not write NULL > as just 0. I'll queue a SQUASH??? fix-up on top before merging it > into 'seen' for today's integration. > > diff --git c/refs.c w/refs.c > index 010f426def..d578a2823b 100644 > --- c/refs.c > +++ w/refs.c > @@ -2758,7 +2758,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, > > for_each_string_list_item(item, refnames) { > ret = ref_transaction_delete(transaction, item->string, > - NULL, flags, 0, msg, &err); > + NULL, flags, NULL, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > item->string, err.buf); > > Thanks Junio, I'll apply it locally too. > > > Also there are quite many whitespace breakages. > > $ git am -s ./+kn7-v2-update-ref-symref > Applying: refs: accept symref values in `ref_transaction[_add]_update` > Applying: update-ref: add support for symref-verify > .git/rebase-apply/patch:59: indent with spaces. > if (line_termination) { > .git/rebase-apply/patch:60: indent with spaces. > /* Without -z, consume SP and use next argument */ > .git/rebase-apply/patch:61: indent with spaces. > if (!**next || **next == line_termination) > .git/rebase-apply/patch:62: indent with spaces. > return NULL; > .git/rebase-apply/patch:63: indent with spaces. > if (**next != ' ') > warning: squelched 10 whitespace errors > warning: 15 lines applied after fixing whitespace errors. > Applying: update-ref: add support for symref-delete > .git/rebase-apply/patch:95: indent with spaces. > die("symref-delete: cannot operate with deref mode"); > .git/rebase-apply/patch:101: indent with spaces. > old_ref = parse_next_refname(&next); > warning: 2 lines applied after fixing whitespace errors. > Applying: files-backend: extract out `create_symref_lock` > Applying: update-ref: add support for symref-create > .git/rebase-apply/patch:81: indent with spaces. > die("symref-create: cannot operate with deref mode"); > warning: 1 line applied after fixing whitespace errors. > Applying: update-ref: add support for symref-update > Applying: refs: support symrefs in 'reference-transaction' hook Sigh. Even with editorconfig, this seems to be problematic for me on Emacs. I'll just start applying locally from now before sending it. As such I've applied this locally and will refrain from sending a new version until a review happens. 1: 3269d0e91e = 1: 3269d0e91e refs: accept symref values in `ref_transaction[_add]_update` 2: a8cb0e0a1d ! 2: 2293b514a8 update-ref: add support for symref-verify @@ builtin/update-ref.c: static char *parse_refname(const char **next) return strbuf_detach(&ref, NULL); } -+ -+ +/* + * Wrapper around parse_refname which skips the next delimiter. + */ +static char *parse_next_refname(const char **next) +{ -+ if (line_termination) { -+ /* Without -z, consume SP and use next argument */ -+ if (!**next || **next == line_termination) -+ return NULL; -+ if (**next != ' ') -+ die("expected SP but got: %s", *next); -+ } else { -+ /* With -z, read the next NUL-terminated line */ -+ if (**next) -+ return NULL; -+ } -+ /* Skip the delimiter */ -+ (*next)++; ++ if (line_termination) { ++ /* Without -z, consume SP and use next argument */ ++ if (!**next || **next == line_termination) ++ return NULL; ++ if (**next != ' ') ++ die("expected SP but got: %s", *next); ++ } else { ++ /* With -z, read the next NUL-terminated line */ ++ if (**next) ++ return NULL; ++ } ++ /* Skip the delimiter */ ++ (*next)++; + -+ return parse_refname(next); ++ return parse_refname(next); +} + /* @@ builtin/update-ref.c: static void parse_cmd_verify(struct ref_transaction *trans +} + +static void parse_cmd_symref_verify(struct ref_transaction *transaction, -+ const char *next, const char *end) ++ const char *next, const char *end) +{ + struct strbuf err = STRBUF_INIT; + struct object_id old_oid; 3: 37c3e006da ! 3: 62d05e7cb7 update-ref: add support for symref-delete @@ builtin/update-ref.c: static void parse_cmd_delete(struct ref_transaction *trans + char *refname, *old_ref; + + if (!(update_flags & REF_NO_DEREF)) -+ die("symref-delete: cannot operate with deref mode"); ++ die("symref-delete: cannot operate with deref mode"); + + refname = parse_refname(&next); + if (!refname) + die("symref-delete: missing <ref>"); + -+ old_ref = parse_next_refname(&next); ++ old_ref = parse_next_refname(&next); + if (old_ref && read_ref(old_ref, NULL)) + die("symref-delete %s: invalid <old-ref>", refname); + @@ refs.c: int refs_delete_refs(struct ref_store *refs, const char *logmsg, for_each_string_list_item(item, refnames) { ret = ref_transaction_delete(transaction, item->string, - NULL, flags, msg, &err); -+ NULL, flags, 0, msg, &err); ++ NULL, flags, NULL, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); 4: 53fdb408ef = 4: 1bd39a38dc files-backend: extract out `create_symref_lock` 5: 8fa0151f94 ! 5: 732d31b43d update-ref: add support for symref-create @@ builtin/update-ref.c: static void parse_cmd_create(struct ref_transaction *trans + char *refname, *new_ref; + + if (!(update_flags & REF_NO_DEREF)) -+ die("symref-create: cannot operate with deref mode"); ++ die("symref-create: cannot operate with deref mode"); + + refname = parse_refname(&next); + if (!refname) 6: 714492ede3 = 6: eb3d239b4b update-ref: add support for symref-update 7: c483104562 = 7: 3cdfb9e528 refs: support symrefs in 'reference-transaction' hook
On Fri, Apr 12, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: > The 'symref-update' command can be used to update a symref, create a symref, > delete a symref or even convert an existing regular ref to a symref. Wherein > like the regular 'update' command, the zero OID can be used to create/delete > a symref. Is it possible to also convert a symref to a regular ref, like when HEAD becomes detached? > 2. The flow is now changedc to send an old_ref, new_ref pair in supplement to s/changedc/changed/ > the existing old_oid, new_oid pair to the reference backends. This allows the > backends to simply do a combination of changes based on what values are set. I had a number of comments and questions when reading the first few patches in this series. I also took a very quick look at the rest of the series, but I think answering my questions about the first few patches would make reading the rest of the series easier, so I will take a deeper look later. Thanks.
On Fri, Apr 12, 2024 at 11:59:01AM +0200, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > The 'git-update-ref(1)' command allows transactional reference updates. > But currently only supports regular reference updates. Meaning, if one > wants to update HEAD (symbolic ref) in a transaction, there is no tool > to do so. > > One option to obtain transactional updates for the HEAD ref is to > manually create the HEAD.lock file and commit. This is intrusive, where > the user needs to mimic internal git behavior. Also, this only works > when using the files backend. > > At GitLab, we've been using the manual process till date, to allow users > to set and change their default branch. But with the introduction of > reftables as a reference backend, this becomes a necessity to be solved > within git. > > This patch series goes about introducing a set of commands > symref-{create,verify,delete,update} to work with symrefs complimenting > the existing commands for the regular refs within 'git-update-ref(1)'. One more thought crossed my mind this night: is it even necessary to introduce new commands for this? As far as I can see, it should be feasible to introduce symref support for all existing commands without breaking backwards compatibility. This can be achieved by using a prefix that cannot ever be an object ID, like for example "ref:". Thus, all of the following should work (with 1234 being an OID and 0000 being the null OID): update HEAD ref:refs/heads/main ref:refs/heads/master update HEAD ref:refs/heads/main 1234 update HEAD ref:refs/heads/main 0000 update HEAD 1234 ref:refs/heads/main update HEAD 0000 ref:refs/heads/main create HEAD ref:refs/heads/main delete HEAD ref:refs/heads/main verify HEAD ref:refs/heads/mains Parsing is unambiguous because we can use `starts_with("ref:")` for an otherwise invalid object ID. Furthermore, because refs cannot have spaces, we also don't have an issue with the SP separator. I have a hunch that this variant might lead to less code duplication, lead to less confusing behaviour and also makes for an easier user interface. Patrick
Christian Couder <christian.couder@gmail.com> writes: > On Fri, Apr 12, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: > >> The 'symref-update' command can be used to update a symref, create a symref, >> delete a symref or even convert an existing regular ref to a symref. Wherein >> like the regular 'update' command, the zero OID can be used to create/delete >> a symref. > > Is it possible to also convert a symref to a regular ref, like when > HEAD becomes detached? > Currently not, that could be a follow up, since that would involve updating the regular commands. A quick way to think about it is that this series adds commands whose end product is a symref. Since what you're talking about is the inverse, it makes sense to add it to the existing commands. >> 2. The flow is now changedc to send an old_ref, new_ref pair in supplement to > > s/changedc/changed/ > >> the existing old_oid, new_oid pair to the reference backends. This allows the >> backends to simply do a combination of changes based on what values are set. > > I had a number of comments and questions when reading the first few > patches in this series. I also took a very quick look at the rest of > the series, but I think answering my questions about the first few > patches would make reading the rest of the series easier, so I will > take a deeper look later. > > Thanks. Thanks for the review. Really appreciate it.
Patrick Steinhardt <ps@pks.im> writes: > On Fri, Apr 12, 2024 at 11:59:01AM +0200, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> The 'git-update-ref(1)' command allows transactional reference updates. >> But currently only supports regular reference updates. Meaning, if one >> wants to update HEAD (symbolic ref) in a transaction, there is no tool >> to do so. >> >> One option to obtain transactional updates for the HEAD ref is to >> manually create the HEAD.lock file and commit. This is intrusive, where >> the user needs to mimic internal git behavior. Also, this only works >> when using the files backend. >> >> At GitLab, we've been using the manual process till date, to allow users >> to set and change their default branch. But with the introduction of >> reftables as a reference backend, this becomes a necessity to be solved >> within git. >> >> This patch series goes about introducing a set of commands >> symref-{create,verify,delete,update} to work with symrefs complimenting >> the existing commands for the regular refs within 'git-update-ref(1)'. > > One more thought crossed my mind this night: is it even necessary to > introduce new commands for this? As far as I can see, it should be > feasible to introduce symref support for all existing commands without > breaking backwards compatibility. This can be achieved by using a prefix > that cannot ever be an object ID, like for example "ref:". > I couldn't think of any reason that we can't do this. I think it would work. I'll try to patch something with my existing tests and see if breaks something. > Thus, all of the following should work (with 1234 being an OID and 0000 > being the null OID): > > update HEAD ref:refs/heads/main ref:refs/heads/master > update HEAD ref:refs/heads/main 1234 > update HEAD ref:refs/heads/main 0000 > update HEAD 1234 ref:refs/heads/main > update HEAD 0000 ref:refs/heads/main > create HEAD ref:refs/heads/main > delete HEAD ref:refs/heads/main > verify HEAD ref:refs/heads/mains > > Parsing is unambiguous because we can use `starts_with("ref:")` for an > otherwise invalid object ID. Furthermore, because refs cannot have > spaces, we also don't have an issue with the SP separator. > > I have a hunch that this variant might lead to less code duplication, > lead to less confusing behaviour and also makes for an easier user > interface. I'm certain that the duplication in update-ref.c will go away with this. Regarding other code changes, I think they'll stay the same. But I do agree, this seems much nicer in terms of UX and also allows us to introduce `symref => regular ref` conversion in this series itself.
From: Karthik Nayak <karthik.188@gmail.com> The 'git-update-ref(1)' command allows transactional reference updates. But currently only supports regular reference updates. Meaning, if one wants to update HEAD (symbolic ref) in a transaction, there is no tool to do so. One option to obtain transactional updates for the HEAD ref is to manually create the HEAD.lock file and commit. This is intrusive, where the user needs to mimic internal git behavior. Also, this only works when using the files backend. At GitLab, we've been using the manual process till date, to allow users to set and change their default branch. But with the introduction of reftables as a reference backend, this becomes a necessity to be solved within git. This patch series goes about introducing a set of commands symref-{create,verify,delete,update} to work with symrefs complimenting the existing commands for the regular refs within 'git-update-ref(1)'. The 'symref-verify' command can be used to verify if a symref exists and its existing value. The 'symref-create' command can be used to create a new symref. The 'symref-delete' command can be used to delete an existing symref while optionally checking its existing value. The 'symref-update' command can be used to update a symref, create a symref, delete a symref or even convert an existing regular ref to a symref. Wherein like the regular 'update' command, the zero OID can be used to create/delete a symref. V1 of the patch series can be found here: https://lore.kernel.org/git/20240330224623.579457-1-knayak@gitlab.com/ I'm not adding a range diff here, cause I redid the whole series, things which have changed: 1. The earlier series simply propagated a 'symref_target' and only supported the 'symref-update' command, without checks for existing values and such. Now we support the entire fleet of commands with support for checking old_values. 2. The flow is now changedc to send an old_ref, new_ref pair in supplement to the existing old_oid, new_oid pair to the reference backends. This allows the backends to simply do a combination of changes based on what values are set. This allows us to do symref-update's where we change a regular ref to a symref while also validating its old OID. 3. I added a lot more tests to cover reflog checks and also '-z' input. 4. The entered <old-ref> and <new-ref> values are checked within update-ref, to ensure we don't create dangling refs. This could be extended in the future with a flag, maybe "REF_ALLOW_DANGLING_SYMREF" and a corresponding option within update-ref. 5. Removed some commits where reftable backend code was reused for symref creation. This actually caused issues since the reused code also created a reflog along with the symref but we should defer reflog creations only after all ref creations have taken place. There is a bit of DRY here, but I think overall its still much cleaner. Thanks all for the review and discussion in the previous version. Patrick, I did incorporate the changes you suggested, but I just noticed that I didn't reply to your emails. Karthik Nayak (7): refs: accept symref values in `ref_transaction[_add]_update` update-ref: add support for symref-verify update-ref: add support for symref-delete files-backend: extract out `create_symref_lock` update-ref: add support for symref-create update-ref: add support for symref-update refs: support symrefs in 'reference-transaction' hook Documentation/git-update-ref.txt | 25 +++ Documentation/githooks.txt | 13 +- branch.c | 2 +- builtin/clone.c | 2 +- builtin/fast-import.c | 5 +- builtin/fetch.c | 4 +- builtin/receive-pack.c | 4 +- builtin/replace.c | 2 +- builtin/tag.c | 1 + builtin/update-ref.c | 202 +++++++++++++++++-- refs.c | 74 +++++-- refs.h | 15 +- refs/files-backend.c | 143 +++++++++++--- refs/refs-internal.h | 21 ++ refs/reftable-backend.c | 51 ++++- sequencer.c | 9 +- t/t0600-reffiles-backend.sh | 32 ++++ t/t1400-update-ref.sh | 320 ++++++++++++++++++++++++++++++- t/t1416-ref-transaction-hooks.sh | 41 ++++ walker.c | 2 +- 20 files changed, 886 insertions(+), 82 deletions(-)