Message ID | 20240530120940.456817-4-knayak@gitlab.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | None | expand |
On Thu, May 30, 2024 at 03:09:37PM +0300, Karthik Nayak wrote: > diff --git a/refs.c b/refs.c > index cdc4d25557..e29abebe1d 100644 > --- a/refs.c > +++ b/refs.c > @@ -950,7 +950,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); > @@ -1283,14 +1283,20 @@ 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_target, The old target is somewhat weirdly placed here, as I'd have expected it to live next to `old_oid`. That's only a minor nit, nothing that's worth a reroll in my opinion. > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index 52801be07d..848d6fc42e 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -1731,6 +1731,60 @@ do > test_cmp expect actual > ' > > + test_expect_success "stdin $type symref-delete fails without --no-deref" ' > + git symbolic-ref refs/heads/symref $a && > + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && > + 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 symref-delete fails with no ref" ' > + format_command $type "symref-delete " >stdin && > + 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 symref-delete fails with too many arguments" ' > + format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin && > + 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 fails with wrong old value" ' > + format_command $type "symref-delete refs/heads/symref" "$m" >stdin && > + test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err && > + grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err && > + git symbolic-ref refs/heads/symref >expect && > + echo $a >actual && > + test_cmp expect actual > + ' > + > + test_expect_success "stdin $type symref-delete works with right old value" ' > + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && > + git update-ref --stdin $type --no-deref <stdin && > + test_must_fail git rev-parse --verify -q refs/heads/symref > + ' > + > + test_expect_success "stdin $type symref-delete works with empty old value" ' > + git symbolic-ref refs/heads/symref $a >stdin && > + format_command $type "symref-delete refs/heads/symref" "" >stdin && > + git update-ref --stdin $type --no-deref <stdin && > + test_must_fail git rev-parse --verify -q $b > + ' > + > + test_expect_success "stdin $type symref-delete succeeds for dangling reference" ' > + test_must_fail git symbolic-ref refs/heads/nonexistent && > + git symbolic-ref refs/heads/symref2 refs/heads/nonexistent && > + format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin && > + git update-ref --stdin $type --no-deref <stdin && > + test_must_fail git symbolic-ref -d refs/heads/symref2 > + ' > + Not sure whether I overlooked it, but one test I missed was when trying to delete a non-symbolic-ref. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Thu, May 30, 2024 at 03:09:37PM +0300, Karthik Nayak wrote: >> diff --git a/refs.c b/refs.c >> index cdc4d25557..e29abebe1d 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -950,7 +950,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); >> @@ -1283,14 +1283,20 @@ 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_target, > > The old target is somewhat weirdly placed here, as I'd have expected it > to live next to `old_oid`. That's only a minor nit, nothing that's worth > a reroll in my opinion. > This is a good point. >> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh >> index 52801be07d..848d6fc42e 100755 >> --- a/t/t1400-update-ref.sh >> +++ b/t/t1400-update-ref.sh >> @@ -1731,6 +1731,60 @@ do >> test_cmp expect actual >> ' >> >> + test_expect_success "stdin $type symref-delete fails without --no-deref" ' >> + git symbolic-ref refs/heads/symref $a && >> + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && >> + 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 symref-delete fails with no ref" ' >> + format_command $type "symref-delete " >stdin && >> + 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 symref-delete fails with too many arguments" ' >> + format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin && >> + 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 fails with wrong old value" ' >> + format_command $type "symref-delete refs/heads/symref" "$m" >stdin && >> + test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err && >> + grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err && >> + git symbolic-ref refs/heads/symref >expect && >> + echo $a >actual && >> + test_cmp expect actual >> + ' >> + >> + test_expect_success "stdin $type symref-delete works with right old value" ' >> + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && >> + git update-ref --stdin $type --no-deref <stdin && >> + test_must_fail git rev-parse --verify -q refs/heads/symref >> + ' >> + >> + test_expect_success "stdin $type symref-delete works with empty old value" ' >> + git symbolic-ref refs/heads/symref $a >stdin && >> + format_command $type "symref-delete refs/heads/symref" "" >stdin && >> + git update-ref --stdin $type --no-deref <stdin && >> + test_must_fail git rev-parse --verify -q $b >> + ' >> + >> + test_expect_success "stdin $type symref-delete succeeds for dangling reference" ' >> + test_must_fail git symbolic-ref refs/heads/nonexistent && >> + git symbolic-ref refs/heads/symref2 refs/heads/nonexistent && >> + format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin && >> + git update-ref --stdin $type --no-deref <stdin && >> + test_must_fail git symbolic-ref -d refs/heads/symref2 >> + ' >> + > > Not sure whether I overlooked it, but one test I missed was when trying > to delete a non-symbolic-ref. > > Patrick I think this point is worthwhile of a reroll, not because of the test itself, but because while testing it, I realized that currently when we try to delete a regular ref using `symref-delete` and providing an `old_target`, the error message is quite generic. I've added a new commit to make this more specific and will send in a new version today.
Patrick Steinhardt <ps@pks.im> writes: >> + test_expect_success "stdin $type symref-delete fails with wrong old value" ' >> + format_command $type "symref-delete refs/heads/symref" "$m" >stdin && >> + test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err && >> + grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err && >> + git symbolic-ref refs/heads/symref >expect && >> + echo $a >actual && >> + test_cmp expect actual >> + ' >> + >> + test_expect_success "stdin $type symref-delete works with right old value" ' >> + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && >> + git update-ref --stdin $type --no-deref <stdin && >> + test_must_fail git rev-parse --verify -q refs/heads/symref >> + ' >> + >> + test_expect_success "stdin $type symref-delete works with empty old value" ' >> + git symbolic-ref refs/heads/symref $a >stdin && >> + format_command $type "symref-delete refs/heads/symref" "" >stdin && >> + git update-ref --stdin $type --no-deref <stdin && >> + test_must_fail git rev-parse --verify -q $b >> + ' >> + >> + test_expect_success "stdin $type symref-delete succeeds for dangling reference" ' >> + test_must_fail git symbolic-ref refs/heads/nonexistent && >> + git symbolic-ref refs/heads/symref2 refs/heads/nonexistent && >> + format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin && >> + git update-ref --stdin $type --no-deref <stdin && >> + test_must_fail git symbolic-ref -d refs/heads/symref2 >> + ' >> + > > Not sure whether I overlooked it, but one test I missed was when trying > to delete a non-symbolic-ref. Hmph, so we want to see an attempt to run symref-delete of refs/heads/main when refs/heads/main is *not* a symref to fail? That is a reasonable test to have. When you invent a shiny new toy, it is hard to make sure you covered cases where it should not kick in or positively diagnose a failure. A review with a sharp eye to spot missing tests is very much appreciated. THanks.
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9fe78b3501..16e02f6979 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-target>] LF symref-verify SP <ref> [SP <old-target>] 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-target>] NUL symref-verify SP <ref> [NUL <old-target>] NUL option SP <opt> NUL start NUL @@ -119,6 +121,9 @@ 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-target>, if given. + symref-verify:: Verify symbolic <ref> against <old-target> but do not change it. If <old-target> is missing, the ref must not exist. Can only be diff --git a/builtin/fetch.c b/builtin/fetch.c index 75255dc600..90a80cf016 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1387,7 +1387,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 01c1f04ece..9430a50fde 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 50f5472160..a78c2f9c85 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -293,7 +293,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; @@ -301,6 +301,36 @@ 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_target; + + 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_target = parse_next_refname(&next); + + if (*next != line_termination) + die("symref-delete %s: extra input: %s", refname, next); + + if (ref_transaction_delete(transaction, refname, NULL, + update_flags, old_target, msg, &err)) + die("%s", err.buf); + + update_flags = default_flags; + free(refname); + free(old_target); + strbuf_release(&err); +} + + static void parse_cmd_verify(struct ref_transaction *transaction, const char *next, const char *end) { @@ -443,6 +473,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 cdc4d25557..e29abebe1d 100644 --- a/refs.c +++ b/refs.c @@ -950,7 +950,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); @@ -1283,14 +1283,20 @@ 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_target, + const char *msg, struct strbuf *err) { if (old_oid && is_null_oid(old_oid)) BUG("delete called with old_oid set to zeros"); + if (old_oid && old_target) + BUG("delete called with both old_oid and old_target set"); + if (old_target && !(flags & REF_NO_DEREF)) + BUG("delete cannot operate on symrefs with deref mode"); return ref_transaction_update(transaction, refname, null_oid(), old_oid, - NULL, NULL, flags, + NULL, old_target, flags, msg, err); } @@ -2599,7 +2605,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, NULL, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); diff --git a/refs.h b/refs.h index 906299351c..a054a4f998 100644 --- a/refs.h +++ b/refs.h @@ -722,7 +722,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_target, + const char *msg, struct strbuf *err); /* diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 52801be07d..848d6fc42e 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1731,6 +1731,60 @@ do test_cmp expect actual ' + test_expect_success "stdin $type symref-delete fails without --no-deref" ' + git symbolic-ref refs/heads/symref $a && + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && + 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 symref-delete fails with no ref" ' + format_command $type "symref-delete " >stdin && + 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 symref-delete fails with too many arguments" ' + format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin && + 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 fails with wrong old value" ' + format_command $type "symref-delete refs/heads/symref" "$m" >stdin && + test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err && + grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err && + git symbolic-ref refs/heads/symref >expect && + echo $a >actual && + test_cmp expect actual + ' + + test_expect_success "stdin $type symref-delete works with right old value" ' + format_command $type "symref-delete refs/heads/symref" "$a" >stdin && + git update-ref --stdin $type --no-deref <stdin && + test_must_fail git rev-parse --verify -q refs/heads/symref + ' + + test_expect_success "stdin $type symref-delete works with empty old value" ' + git symbolic-ref refs/heads/symref $a >stdin && + format_command $type "symref-delete refs/heads/symref" "" >stdin && + git update-ref --stdin $type --no-deref <stdin && + test_must_fail git rev-parse --verify -q $b + ' + + test_expect_success "stdin $type symref-delete succeeds for dangling reference" ' + test_must_fail git symbolic-ref refs/heads/nonexistent && + git symbolic-ref refs/heads/symref2 refs/heads/nonexistent && + format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin && + git update-ref --stdin $type --no-deref <stdin && + test_must_fail git symbolic-ref -d refs/heads/symref2 + ' + done test_done diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index fd58b902f4..ccde1b944b 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -162,6 +162,7 @@ test_expect_success 'hook gets all queued symref updates' ' git update-ref refs/heads/branch $POST_OID && git symbolic-ref refs/heads/symref refs/heads/main && + git symbolic-ref refs/heads/symrefd refs/heads/main && test_hook reference-transaction <<-\EOF && echo "$*" >>actual @@ -171,16 +172,32 @@ test_expect_success 'hook gets all queued symref updates' ' done >>actual EOF - cat >expect <<-EOF && + # In the files backend, "delete" also triggers an additional transaction + # update on the packed-refs backend, which constitutes additional reflog + # entries. + if test_have_prereq REFFILES + then + cat >expect <<-EOF + aborted + $ZERO_OID $ZERO_OID refs/heads/symrefd + EOF + else + >expect + fi && + + cat >>expect <<-EOF && prepared ref:refs/heads/main $ZERO_OID refs/heads/symref + ref:refs/heads/main $ZERO_OID refs/heads/symrefd committed ref:refs/heads/main $ZERO_OID refs/heads/symref + ref:refs/heads/main $ZERO_OID refs/heads/symrefd EOF git update-ref --no-deref --stdin <<-EOF && start symref-verify refs/heads/symref refs/heads/main + symref-delete refs/heads/symrefd refs/heads/main prepare commit EOF