| Message ID | 20240412095908.1134387-4-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | update-ref: add symref oriented commands | expand |
On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@gmail.com> wrote: > > From: Karthik Nayak <karthik.188@gmail.com> > > Similar to the previous commit, add 'symref-delete' to allow deletions > of symbolic refs in a transaction via the 'git-update-ref' command. The > 'symref-delete' command can when given with an <old-ref>, deletes the > provided <ref> only when it points to <old-ref>. I have a similar question as with the previous patch about what happens if <old-ref> looks like an oid and <ref> is a regular ref pointing to it. > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt > index 749aaa7892..ef22a1a2f4 100644 > --- a/Documentation/git-update-ref.txt > +++ b/Documentation/git-update-ref.txt > @@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form: > create SP <ref> SP <new-oid> LF > delete SP <ref> [SP <old-oid>] LF > verify SP <ref> [SP <old-oid>] LF > + symref-delete SP <ref> [SP <old-ref>] LF > symref-verify SP <ref> [SP <old-ref>] LF > option SP <opt> LF > start LF > @@ -87,6 +88,7 @@ quoting: > create SP <ref> NUL <new-oid> NUL > delete SP <ref> NUL [<old-oid>] NUL > verify SP <ref> NUL [<old-oid>] NUL > + symref-delete SP <ref> [NUL <old-ref>] NUL > symref-verify SP <ref> [NUL <old-ref>] NUL > option SP <opt> NUL > start NUL Also I wonder if there is a test where <old-ref> is an empty string, so where: symref-delete SP <ref> SP LF or: symref-delete SP <ref> NUL NUL are used?
On Fri, Apr 12, 2024 at 11:59:04AM +0200, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> [snip] > @@ -302,6 +302,37 @@ static void parse_cmd_delete(struct ref_transaction *transaction, > strbuf_release(&err); > } > > +static void parse_cmd_symref_delete(struct ref_transaction *transaction, > + const char *next, const char *end) > +{ > + struct strbuf err = STRBUF_INIT; > + char *refname, *old_ref; > + > + if (!(update_flags & REF_NO_DEREF)) > + die("symref-delete: cannot operate with deref mode"); Again, I'm a bit on the fence regarding this restriction. I feel like it ought to be possible to delete both plain and symbolic refs in a single git-update-ref(1) command. > + refname = parse_refname(&next); > + if (!refname) > + die("symref-delete: missing <ref>"); > + > + old_ref = parse_next_refname(&next); This line is indented with spaces and not tabs. [snip] > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -1715,6 +1715,45 @@ test_expect_success "stdin ${type} symref-verify fails for mistaken null value" > test_cmp expect actual > ' > > +test_expect_success "stdin ${type} symref-delete fails without --no-deref" ' > + git symbolic-ref refs/heads/symref $a && > + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" && > + test_must_fail git update-ref --stdin ${type} <stdin 2>err && > + grep "fatal: symref-delete: cannot operate with deref mode" err > +' > + > +test_expect_success "stdin ${type} fails symref-delete with no ref" ' > + create_stdin_buf ${type} "symref-delete " && > + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && > + grep "fatal: symref-delete: missing <ref>" err > +' > + > +test_expect_success "stdin ${type} fails symref-delete with too many arguments" ' > + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" "$a" && > + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && > + if test "$type" = "-z" > + then > + grep "fatal: unknown command: $a" err > + else > + grep "fatal: symref-delete refs/heads/symref: extra input: $a" err > + fi > +' > + > +test_expect_success "stdin ${type} symref-delete ref fails with wrong old value" ' > + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" && > + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && > + grep "fatal: cannot lock ref '"'"'refs/heads/symref'"'"'" err && You can use "${SQ}" to insert single quotes. Patrick > + git symbolic-ref refs/heads/symref >expect && > + echo $a >actual && > + test_cmp expect actual > +' > + > +test_expect_success "stdin ${type} symref-delete ref works with right old value" ' > + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" && > + git update-ref --stdin ${type} --no-deref <stdin && > + test_must_fail git rev-parse --verify -q $b > +' > + > done > > test_done > -- > 2.43.GIT >
Christian Couder <christian.couder@gmail.com> writes: > On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@gmail.com> wrote: >> >> From: Karthik Nayak <karthik.188@gmail.com> >> >> Similar to the previous commit, add 'symref-delete' to allow deletions >> of symbolic refs in a transaction via the 'git-update-ref' command. The >> 'symref-delete' command can when given with an <old-ref>, deletes the >> provided <ref> only when it points to <old-ref>. > > I have a similar question as with the previous patch about what > happens if <old-ref> looks like an oid and <ref> is a regular ref > pointing to it. > We parse refs passed as <old-ref> and this would fail. >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > >> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt >> index 749aaa7892..ef22a1a2f4 100644 >> --- a/Documentation/git-update-ref.txt >> +++ b/Documentation/git-update-ref.txt >> @@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form: >> create SP <ref> SP <new-oid> LF >> delete SP <ref> [SP <old-oid>] LF >> verify SP <ref> [SP <old-oid>] LF >> + symref-delete SP <ref> [SP <old-ref>] LF >> symref-verify SP <ref> [SP <old-ref>] LF >> option SP <opt> LF >> start LF >> @@ -87,6 +88,7 @@ quoting: >> create SP <ref> NUL <new-oid> NUL >> delete SP <ref> NUL [<old-oid>] NUL >> verify SP <ref> NUL [<old-oid>] NUL >> + symref-delete SP <ref> [NUL <old-ref>] NUL >> symref-verify SP <ref> [NUL <old-ref>] NUL >> option SP <opt> NUL >> start NUL > > Also I wonder if there is a test where <old-ref> is an empty string, so where: > > symref-delete SP <ref> SP LF > > or: > > symref-delete SP <ref> NUL NUL > > are used? I didn't add such tests, will add.
Patrick Steinhardt <ps@pks.im> writes: > On Fri, Apr 12, 2024 at 11:59:04AM +0200, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> > [snip] >> @@ -302,6 +302,37 @@ static void parse_cmd_delete(struct ref_transaction *transaction, >> strbuf_release(&err); >> } >> >> +static void parse_cmd_symref_delete(struct ref_transaction *transaction, >> + const char *next, const char *end) >> +{ >> + struct strbuf err = STRBUF_INIT; >> + char *refname, *old_ref; >> + >> + if (!(update_flags & REF_NO_DEREF)) >> + die("symref-delete: cannot operate with deref mode"); > > Again, I'm a bit on the fence regarding this restriction. I feel like it > ought to be possible to delete both plain and symbolic refs in a single > git-update-ref(1) command. > Yup this is still possible since we have the 'no-deref' option. >> + refname = parse_refname(&next); >> + if (!refname) >> + die("symref-delete: missing <ref>"); >> + >> + old_ref = parse_next_refname(&next); > > This line is indented with spaces and not tabs. > There was a bunch of this, I'll have them all fixed. > [snip] >> --- a/t/t1400-update-ref.sh >> +++ b/t/t1400-update-ref.sh >> @@ -1715,6 +1715,45 @@ test_expect_success "stdin ${type} symref-verify fails for mistaken null value" >> test_cmp expect actual >> ' >> >> +test_expect_success "stdin ${type} symref-delete fails without --no-deref" ' >> + git symbolic-ref refs/heads/symref $a && >> + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" && >> + test_must_fail git update-ref --stdin ${type} <stdin 2>err && >> + grep "fatal: symref-delete: cannot operate with deref mode" err >> +' >> + >> +test_expect_success "stdin ${type} fails symref-delete with no ref" ' >> + create_stdin_buf ${type} "symref-delete " && >> + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && >> + grep "fatal: symref-delete: missing <ref>" err >> +' >> + >> +test_expect_success "stdin ${type} fails symref-delete with too many arguments" ' >> + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" "$a" && >> + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && >> + if test "$type" = "-z" >> + then >> + grep "fatal: unknown command: $a" err >> + else >> + grep "fatal: symref-delete refs/heads/symref: extra input: $a" err >> + fi >> +' >> + >> +test_expect_success "stdin ${type} symref-delete ref fails with wrong old value" ' >> + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" && >> + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && >> + grep "fatal: cannot lock ref '"'"'refs/heads/symref'"'"'" err && > > You can use "${SQ}" to insert single quotes. > > Patrick > Neat, this is much better, thanks!
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 749aaa7892..ef22a1a2f4 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form: create SP <ref> SP <new-oid> LF delete SP <ref> [SP <old-oid>] LF verify SP <ref> [SP <old-oid>] LF + symref-delete SP <ref> [SP <old-ref>] LF symref-verify SP <ref> [SP <old-ref>] LF option SP <opt> LF start LF @@ -87,6 +88,7 @@ quoting: create SP <ref> NUL <new-oid> NUL delete SP <ref> NUL [<old-oid>] NUL verify SP <ref> NUL [<old-oid>] NUL + symref-delete SP <ref> [NUL <old-ref>] NUL symref-verify SP <ref> [NUL <old-ref>] NUL option SP <opt> NUL start NUL @@ -119,6 +121,10 @@ verify:: Verify <ref> against <old-oid> but do not change it. If <old-oid> is zero or missing, the ref must not exist. +symref-delete:: + Delete <ref> after verifying it exists with <old-ref>, if + given. + symref-verify:: Verify symbolic <ref> against <old-ref> but do not change it. If <old-ref> is missing, the ref must not exist. Can only be diff --git a/builtin/fetch.c b/builtin/fetch.c index 66840b7c5b..d02592efca 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1383,7 +1383,7 @@ static int prune_refs(struct display_state *display_state, if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { result = ref_transaction_delete(transaction, ref->name, NULL, 0, - "fetch: prune", &err); + NULL, "fetch: prune", &err); if (result) goto cleanup; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index ebea1747cb..6b728baaac 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1576,7 +1576,8 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (ref_transaction_delete(transaction, namespaced_name, old_oid, - 0, "push", &err)) { + 0, NULL, + "push", &err)) { rp_error("%s", err.buf); ret = "failed to delete"; } else { diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 4ae6bdcb12..3be9ae0d00 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -294,7 +294,7 @@ static void parse_cmd_delete(struct ref_transaction *transaction, if (ref_transaction_delete(transaction, refname, have_old ? &old_oid : NULL, - update_flags, msg, &err)) + update_flags, NULL, msg, &err)) die("%s", err.buf); update_flags = default_flags; @@ -302,6 +302,37 @@ static void parse_cmd_delete(struct ref_transaction *transaction, strbuf_release(&err); } +static void parse_cmd_symref_delete(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf err = STRBUF_INIT; + char *refname, *old_ref; + + if (!(update_flags & REF_NO_DEREF)) + 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); + if (old_ref && read_ref(old_ref, NULL)) + die("symref-delete %s: invalid <old-ref>", refname); + + if (*next != line_termination) + die("symref-delete %s: extra input: %s", refname, next); + + if (ref_transaction_delete(transaction, refname, NULL, + update_flags | REF_SYMREF_UPDATE, + old_ref, msg, &err)) + die("%s", err.buf); + + update_flags = default_flags; + free(refname); + free(old_ref); + strbuf_release(&err); +} + static void parse_cmd_verify(struct ref_transaction *transaction, const char *next, const char *end) { @@ -445,6 +476,7 @@ static const struct parse_cmd { { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN }, { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN }, { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN }, + { "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN }, { "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN }, { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN }, { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, diff --git a/refs.c b/refs.c index 124b294c9f..6d98d9652d 100644 --- a/refs.c +++ b/refs.c @@ -981,7 +981,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, transaction = ref_store_transaction_begin(refs, &err); if (!transaction || ref_transaction_delete(transaction, refname, old_oid, - flags, msg, &err) || + flags, NULL, msg, &err) || ref_transaction_commit(transaction, &err)) { error("%s", err.buf); ref_transaction_free(transaction); @@ -1220,6 +1220,7 @@ void ref_transaction_free(struct ref_transaction *transaction) for (i = 0; i < transaction->nr; i++) { free(transaction->updates[i]->msg); free((void *)transaction->updates[i]->old_ref); + free((void *)transaction->updates[i]->new_ref); free(transaction->updates[i]); } free(transaction->updates); @@ -1252,6 +1253,8 @@ struct ref_update *ref_transaction_add_update( if (update->flags & REF_SYMREF_UPDATE) { if (old_ref) update->old_ref = xstrdup(old_ref); + if (new_ref) + update->new_ref = xstrdup(new_ref); } else { if (flags & REF_HAVE_NEW) oidcpy(&update->new_oid, new_oid); @@ -1317,14 +1320,17 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const struct object_id *old_oid, - unsigned int flags, const char *msg, + unsigned int flags, + const char *old_ref, + const char *msg, struct strbuf *err) { - if (old_oid && is_null_oid(old_oid)) + if (!(flags & REF_SYMREF_UPDATE) && old_oid && + is_null_oid(old_oid)) BUG("delete called with old_oid set to zeros"); return ref_transaction_update(transaction, refname, null_oid(), old_oid, - NULL, NULL, flags, + NULL, old_ref, flags, msg, err); } @@ -2748,7 +2754,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, msg, &err); + NULL, flags, 0, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); diff --git a/refs.h b/refs.h index a988e672ff..60e6a21a31 100644 --- a/refs.h +++ b/refs.h @@ -758,7 +758,9 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const struct object_id *old_oid, - unsigned int flags, const char *msg, + unsigned int flags, + const char *old_ref, + const char *msg, struct strbuf *err); /* diff --git a/refs/files-backend.c b/refs/files-backend.c index 8421530bde..f74ea308b5 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2501,7 +2501,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, files_assert_main_repository(refs, "lock_ref_for_update"); - if ((update->flags & REF_HAVE_NEW) && is_null_oid(&update->new_oid)) + if ((update->flags & REF_HAVE_NEW) && null_new_value(update)) update->flags |= REF_DELETING; if (head_ref) { diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 7a03922c7b..935bf407df 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1122,7 +1122,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data if (u->flags & REF_LOG_ONLY) continue; - if (u->flags & REF_HAVE_NEW && is_null_oid(&u->new_oid)) { + if (u->flags & REF_HAVE_NEW && null_new_value(u)) { struct reftable_ref_record ref = { .refname = (char *)u->refname, .update_index = ts, diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index d8ffda4096..cf01c5d867 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1715,6 +1715,45 @@ test_expect_success "stdin ${type} symref-verify fails for mistaken null value" test_cmp expect actual ' +test_expect_success "stdin ${type} symref-delete fails without --no-deref" ' + git symbolic-ref refs/heads/symref $a && + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" && + test_must_fail git update-ref --stdin ${type} <stdin 2>err && + grep "fatal: symref-delete: cannot operate with deref mode" err +' + +test_expect_success "stdin ${type} fails symref-delete with no ref" ' + create_stdin_buf ${type} "symref-delete " && + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && + grep "fatal: symref-delete: missing <ref>" err +' + +test_expect_success "stdin ${type} fails symref-delete with too many arguments" ' + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" "$a" && + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && + if test "$type" = "-z" + then + grep "fatal: unknown command: $a" err + else + grep "fatal: symref-delete refs/heads/symref: extra input: $a" err + fi +' + +test_expect_success "stdin ${type} symref-delete ref fails with wrong old value" ' + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" && + test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err && + grep "fatal: cannot lock ref '"'"'refs/heads/symref'"'"'" err && + git symbolic-ref refs/heads/symref >expect && + echo $a >actual && + test_cmp expect actual +' + +test_expect_success "stdin ${type} symref-delete ref works with right old value" ' + create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" && + git update-ref --stdin ${type} --no-deref <stdin && + test_must_fail git rev-parse --verify -q $b +' + done test_done