From: Karthik Nayak <karthik.188@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: chris.torek@gmail.com, git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v2 2/7] update-ref: add support for symref-verify
Date: Fri, 19 Apr 2024 14:53:01 -0700 [thread overview]
Message-ID: <CAOLa=ZQJmxrQ+n060P0X8BW4ay2XMvOHHyJ3vxrW7C+j+5kWSA@mail.gmail.com> (raw)
In-Reply-To: <ZiI8DmqrbvdCDWt0@tanuki>
[-- Attachment #1: Type: text/plain, Size: 9081 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
>> +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
>> + used in `no-deref` mode.
>
> Should this say "is zero or missing", like the comment for "verify"
> does?
>
We don't allow users to enter OID here, we do convert it to zero OID
internally. But the user input is expected to be old_ref or nothing.
> [snip]
>> @@ -297,11 +321,48 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
>> die("verify %s: extra input: %s", refname, next);
>>
>> if (ref_transaction_verify(transaction, refname, &old_oid,
>> - update_flags, &err))
>> + NULL, update_flags, &err))
>> + die("%s", err.buf);
>> +
>> + update_flags = default_flags;
>> + free(refname);
>> + strbuf_release(&err);
>> +}
>> +
>> +static void parse_cmd_symref_verify(struct ref_transaction *transaction,
>> + const char *next, const char *end)
>> +{
>> + struct strbuf err = STRBUF_INIT;
>> + struct object_id old_oid;
>> + char *refname, *old_ref;
>> +
>> + if (!(update_flags & REF_NO_DEREF))
>> + die("symref-verify: cannot operate with deref mode");
>
> This feels quite restrictive to me. Wouldn't it be preferable to simply
> ignore `REF_NO_DEREF` here? It basically means that this command can't
> ever be used in a normal `git update-ref --stdin` session.
>
We do support 'option' with the '--stdin' flag. So technically a user
should be able to do.
$ git update-ref --stdin
no-deref
symref-verify refs/heads/symref refs/heads/master
update-ref refs/heads/branch 0b3b55ad0e593ead604f80fe3f621239b34cce7e
I guess we could make it implicit, but I thought it's better to keep it
explicit so the user knows that there is no dereferencing taking place
here, eventhough the default option is to dereference.
[snip]
>>
>> update->flags = flags;
>>
>> - if (flags & REF_HAVE_NEW)
>> - oidcpy(&update->new_oid, new_oid);
>> - if (flags & REF_HAVE_OLD)
>> - oidcpy(&update->old_oid, old_oid);
>> + /*
>> + * The ref values are to be considered over the oid values when we're
>> + * doing symref operations.
>> + */
>
> I feel like this is a statement that should be backed up by a deeper
> explanation of why that is. I'm still wondering here why we cannot
> assert that the old value is an object ID when I want to update it to a
> symref, or alternatively why it would even be possible to have both
> `REF_SYMREF_UPDATE` and a set of other, incompatible fields set. It
> feels like this should be a `BUG()` instead if this is supposedly an
> unsupported configuration rather than silently ignoring it.
>
> In any case, I feel like it would be easier to reason about if this was
> introduced together with the actual user. As far as I can see this code
> shouldn't ever be hit for "verify-symref", right? Currently, the reader
> is forced to figure out what is and isn't related to the new command.
>
I've changed this now to no longer have this condition and also added
'BUG' for cases where both old_{ref,target} and new_{ref,target} exist.
[snip]
>> @@ -2464,8 +2495,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>> struct strbuf *err)
>> {
>> struct strbuf referent = STRBUF_INIT;
>> - int mustexist = (update->flags & REF_HAVE_OLD) &&
>> - !is_null_oid(&update->old_oid);
>> + int mustexist = (update->flags & REF_HAVE_OLD) && !is_null_oid(&update->old_oid);
>
> This change is a no-op, right? If so, let's rather drop it.
>
Yeah, will do.
>> int ret = 0;
>> struct ref_lock *lock;
>>
>> @@ -2514,6 +2544,18 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>> ret = TRANSACTION_GENERIC_ERROR;
>> goto out;
>> }
>> + }
>> +
>> + /*
>> + * For symref verification, we need to check the referent value
>> + * rather than the oid. If we're dealing with regular refs or we're
>> + * verifying a dereferenced symref, we then check the oid.
>> + */
>> + if (update->flags & REF_SYMREF_UPDATE && update->old_ref) {
>> + if (check_old_ref(update, referent.buf, err)) {
>> + ret = TRANSACTION_GENERIC_ERROR;
>> + goto out;
>> + }
>> } else if (check_old_oid(update, &lock->old_oid, err)) {
>> ret = TRANSACTION_GENERIC_ERROR;
>> goto out;
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index 4c5fe02687..21c6b940d8 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -749,4 +749,11 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
>> */
>> struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
>>
>> +/*
>> + * Helper function to check if the new value is null, this
>> + * takes into consideration that the update could be a regular
>> + * ref or a symbolic ref.
>> + */
>> +int null_new_value(struct ref_update *update);
>
> When adding it to the header we should probably prefix this to avoid
> name collisions. `ref_update_is_null_new_value()` might be a mouth full,
> but feels preferable to me.
>
Makes sense.
>> #endif /* REFS_REFS_INTERNAL_H */
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index 6104471199..7a03922c7b 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -938,7 +938,28 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>> * individual refs. But the error messages match what the files
>> * backend returns, which keeps our tests happy.
>> */
>> - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) {
>> + if ((u->flags & REF_HAVE_OLD) &&
>> + (u->flags & REF_SYMREF_UPDATE) &&
>> + u->old_ref) {
>> + if (strcmp(referent.buf, u->old_ref)) {
>
> s/ / /
>
>> + if (!strcmp(u->old_ref, ""))
>> + strbuf_addf(err, "cannot lock ref '%s': "
>> + "reference already exists",
>> + original_update_refname(u));
>> + else if (!strcmp(referent.buf, ""))
>> + strbuf_addf(err, "cannot lock ref '%s': "
>> + "reference is missing but expected %s",
>> + original_update_refname(u),
>> + u->old_ref);
>> + else
>> + strbuf_addf(err, "cannot lock ref '%s': "
>> + "is at %s but expected %s",
>> + original_update_refname(u),
>> + referent.buf, u->old_ref);
>
> I'd use better-matching error messages here. I know that we talk about
> "cannot lock ref" in the next branch, as well. But the only reason we
> did this is to have the same error messages as the "files" backend.
> Semantically, those errors don't make much sense as the "reftable"
> backend never locks specific refs, but only the complete stack.
>
Fair enough, will change.
>> + ret = -1;
>> + goto done;
>> + }
>> + } else if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) {
>> if (is_null_oid(&u->old_oid))
>> strbuf_addf(err, _("cannot lock ref '%s': "
>> "reference already exists"),
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index ec3443cc87..d8ffda4096 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -890,17 +890,23 @@ test_expect_success 'stdin update/create/verify combination works' '
>> '
>>
>> test_expect_success 'stdin verify succeeds for correct value' '
>> + test-tool ref-store main for-each-reflog-ent $m >before &&
>> git rev-parse $m >expect &&
>> echo "verify $m $m" >stdin &&
>> git update-ref --stdin <stdin &&
>> git rev-parse $m >actual &&
>> - test_cmp expect actual
>> + test_cmp expect actual &&
>> + test-tool ref-store main for-each-reflog-ent $m >after &&
>> + test_cmp before after
>> '
>>
>> test_expect_success 'stdin verify succeeds for missing reference' '
>> + test-tool ref-store main for-each-reflog-ent $m >before &&
>> echo "verify refs/heads/missing $Z" >stdin &&
>> git update-ref --stdin <stdin &&
>> - test_must_fail git rev-parse --verify -q refs/heads/missing
>> + test_must_fail git rev-parse --verify -q refs/heads/missing &&
>> + test-tool ref-store main for-each-reflog-ent $m >after &&
>> + test_cmp before after
>> '
>
> The updated tests merely assert that the refs didn't change, right?
>
Yes, also that we didn't add anything unexpected to the reflog.
>> test_expect_success 'stdin verify treats no value as missing' '
>> @@ -1641,4 +1647,74 @@ test_expect_success PIPE 'transaction flushes status updates' '
>> test_cmp expected actual
>> '
>>
>> +create_stdin_buf ()
>> +{
>
> The curly brace should go on the same line as the function name.
>
Right, will change.
>> + if test "$1" = "-z"
>> + then
>> + shift
>> + printf "$F" "$@" >stdin
>> + else
>> + echo "$@" >stdin
>> + fi
>> +}
>> +
>> +for type in "" "-z"
>> +do
>
> We should probably indent all of the tests to make it easier to see that
> they run in a loop.
>
> Patrick
>
I was a bit confused about this, I saw smaller tests with loops
indented, while some larger ones not indented. I think its better to do
so too, let me do that.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
next prev parent reply other threads:[~2024-04-19 21:53 UTC|newest]
Thread overview: 146+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-30 22:46 [PATCH 0/8] update-ref: add support for update-symref option Karthik Nayak
2024-03-30 22:46 ` [PATCH 1/8] files-backend: extract out `create_symref_lock` Karthik Nayak
2024-04-02 12:20 ` Patrick Steinhardt
2024-04-03 14:52 ` Karthik Nayak
2024-03-30 22:46 ` [PATCH 2/8] reftable-backend: extract out `write_symref_with_log` Karthik Nayak
2024-04-02 12:20 ` Patrick Steinhardt
2024-03-30 22:46 ` [PATCH 3/8] reftable-backend: move `write_symref_with_log` up Karthik Nayak
2024-03-30 22:46 ` [PATCH 4/8] refs: accept symref in `ref_transaction_add_update` Karthik Nayak
2024-03-30 22:46 ` [PATCH 5/8] refs/files-backend: add support for symref updates Karthik Nayak
2024-04-02 12:20 ` Patrick Steinhardt
2024-03-30 22:46 ` [PATCH 6/8] refs/reftable-backend: " Karthik Nayak
2024-04-02 12:20 ` Patrick Steinhardt
2024-03-30 22:46 ` [PATCH 7/8] refs: add 'update-symref' command to 'update-ref' Karthik Nayak
2024-03-31 22:08 ` Junio C Hamano
2024-03-31 22:27 ` Chris Torek
2024-03-31 23:14 ` Junio C Hamano
2024-04-01 1:31 ` Junio C Hamano
2024-04-02 12:20 ` Patrick Steinhardt
2024-04-02 16:40 ` Junio C Hamano
2024-04-09 11:55 ` Patrick Steinhardt
2024-04-09 16:15 ` Karthik Nayak
2024-04-10 4:20 ` Patrick Steinhardt
2024-04-10 16:06 ` Junio C Hamano
2024-04-10 17:31 ` Patrick Steinhardt
2024-04-01 10:38 ` Karthik Nayak
2024-04-01 11:48 ` Karthik Nayak
2024-04-01 16:17 ` Junio C Hamano
2024-04-01 20:40 ` Junio C Hamano
2024-04-01 22:37 ` Karthik Nayak
2024-03-30 22:46 ` [PATCH 8/8] refs: support symrefs in 'reference-transaction' hook Karthik Nayak
2024-04-02 12:20 ` Patrick Steinhardt
2024-04-12 9:59 ` [PATCH v2 0/7] update-ref: add symref oriented commands Karthik Nayak
2024-04-12 9:59 ` [PATCH v2 1/7] refs: accept symref values in `ref_transaction[_add]_update` Karthik Nayak
2024-04-18 14:25 ` Christian Couder
2024-04-19 10:28 ` Karthik Nayak
2024-04-18 15:08 ` Phillip Wood
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-19 15:47 ` Karthik Nayak
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-19 18:09 ` Karthik Nayak
2024-04-23 6:31 ` Patrick Steinhardt
2024-04-23 10:48 ` Karthik Nayak
2024-04-12 9:59 ` [PATCH v2 2/7] update-ref: add support for symref-verify Karthik Nayak
2024-04-18 14:26 ` Christian Couder
2024-04-19 21:21 ` Karthik Nayak
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-19 21:53 ` Karthik Nayak [this message]
2024-04-12 9:59 ` [PATCH v2 3/7] update-ref: add support for symref-delete Karthik Nayak
2024-04-18 14:52 ` Christian Couder
2024-04-21 10:43 ` Karthik Nayak
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-21 10:45 ` Karthik Nayak
2024-04-12 9:59 ` [PATCH v2 4/7] files-backend: extract out `create_symref_lock` Karthik Nayak
2024-04-12 9:59 ` [PATCH v2 5/7] update-ref: add support for symref-create Karthik Nayak
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-19 15:48 ` Junio C Hamano
2024-04-21 12:50 ` Karthik Nayak
2024-04-21 15:57 ` Karthik Nayak
2024-04-23 6:39 ` Patrick Steinhardt
2024-04-23 10:52 ` Karthik Nayak
2024-04-12 9:59 ` [PATCH v2 6/7] update-ref: add support for symref-update Karthik Nayak
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-21 19:00 ` Karthik Nayak
2024-04-23 6:49 ` Patrick Steinhardt
2024-04-23 11:30 ` Karthik Nayak
2024-04-12 9:59 ` [PATCH v2 7/7] refs: support symrefs in 'reference-transaction' hook Karthik Nayak
2024-04-12 18:01 ` [PATCH v2 0/7] update-ref: add symref oriented commands Junio C Hamano
2024-04-12 18:49 ` Karthik Nayak
2024-04-18 15:05 ` Christian Couder
2024-04-21 19:06 ` Karthik Nayak
2024-04-20 6:16 ` Patrick Steinhardt
2024-04-21 19:11 ` Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 0/8] refs: add symref support to 'git-update-ref' Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 1/8] refs: accept symref values in `ref_transaction[_add]_update` Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 2/8] update-ref: support parsing ref targets in `parse_next_oid` Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 3/8] files-backend: extract out `create_symref_lock` Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 4/8] update-ref: support symrefs in the verify command Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 5/8] update-ref: support symrefs in the delete command Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 6/8] update-ref: support symrefs in the create command Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 7/8] update-ref: support symrefs in the update command Karthik Nayak
2024-04-23 21:28 ` [PATCH v3 8/8] ref: support symrefs in 'reference-transaction' hook Karthik Nayak
2024-04-23 22:03 ` [PATCH v3 0/8] refs: add symref support to 'git-update-ref' Jeff King
2024-04-24 1:17 ` Junio C Hamano
2024-04-24 16:25 ` Karthik Nayak
2024-04-25 6:40 ` Patrick Steinhardt
2024-04-25 21:12 ` Karthik Nayak
2024-04-25 18:01 ` Junio C Hamano
2024-04-25 21:14 ` Karthik Nayak
2024-04-25 21:55 ` Junio C Hamano
2024-04-26 12:48 ` Karthik Nayak
2024-04-26 20:41 ` Jeff King
2024-04-25 17:09 ` Junio C Hamano
2024-04-25 21:07 ` Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 0/7] add symref-* commands to 'git-update-ref --stdin' Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 1/7] refs: accept symref values in `ref_transaction[_add]_update` Karthik Nayak
2024-04-26 19:31 ` Junio C Hamano
2024-04-26 21:15 ` Jeff King
2024-04-29 7:02 ` Patrick Steinhardt
2024-04-29 7:55 ` Jeff King
2024-04-29 9:29 ` phillip.wood123
2024-04-29 9:32 ` phillip.wood123
2024-04-29 16:18 ` Junio C Hamano
2024-04-30 10:33 ` Jeff King
2024-04-30 10:30 ` Jeff King
2024-04-28 19:36 ` Karthik Nayak
2024-04-29 13:38 ` Phillip Wood
2024-04-29 14:01 ` Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 2/7] files-backend: extract out `create_symref_lock` Karthik Nayak
2024-04-26 21:39 ` Junio C Hamano
2024-04-28 19:57 ` Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-04-26 22:51 ` Junio C Hamano
2024-04-28 22:28 ` Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 6/7] update-ref: add support for 'symref-update' command Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 7/7] ref: support symrefs in 'reference-transaction' hook Karthik Nayak
2024-04-30 10:14 ` [PATCH v4 0/7] add symref-* commands to 'git-update-ref --stdin' Karthik Nayak
2024-05-01 20:22 ` [PATCH v5 0/7] refs: add support for transactional symref updates Karthik Nayak
2024-05-01 20:22 ` [PATCH v5 1/7] refs: accept symref values in `ref_transaction_update()` Karthik Nayak
2024-05-01 20:22 ` [PATCH v5 2/7] files-backend: extract out `create_symref_lock()` Karthik Nayak
2024-05-01 22:06 ` Junio C Hamano
2024-05-02 7:47 ` Patrick Steinhardt
2024-05-02 11:05 ` Karthik Nayak
2024-05-02 16:49 ` Junio C Hamano
2024-05-01 20:22 ` [PATCH v5 3/7] refs: support symrefs in 'reference-transaction' hook Karthik Nayak
2024-05-01 23:05 ` Junio C Hamano
2024-05-02 5:32 ` Karthik Nayak
2024-05-01 20:22 ` [PATCH v5 4/7] refs: add support for transactional symref updates Karthik Nayak
2024-05-01 23:52 ` Junio C Hamano
2024-05-02 5:50 ` Karthik Nayak
2024-05-02 7:47 ` Patrick Steinhardt
2024-05-02 11:10 ` Karthik Nayak
2024-05-02 16:51 ` Junio C Hamano
2024-05-02 16:00 ` Junio C Hamano
2024-05-02 17:53 ` Junio C Hamano
2024-05-01 20:22 ` [PATCH v5 5/7] refs: use transaction in `refs_create_symref()` Karthik Nayak
2024-05-02 7:47 ` Patrick Steinhardt
2024-05-01 20:22 ` [PATCH v5 6/7] refs: rename `refs_create_symref()` to `refs_update_symref()` Karthik Nayak
2024-05-02 7:47 ` Patrick Steinhardt
2024-05-02 11:34 ` Karthik Nayak
2024-05-01 20:22 ` [PATCH v5 7/7] refs: remove `create_symref` and associated dead code Karthik Nayak
2024-05-02 7:47 ` Patrick Steinhardt
2024-05-02 16:53 ` Junio C Hamano
2024-05-02 0:20 ` [PATCH v5 0/7] refs: add support for transactional symref updates Junio C Hamano
2024-05-02 5:53 ` Karthik Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOLa=ZQJmxrQ+n060P0X8BW4ay2XMvOHHyJ3vxrW7C+j+5kWSA@mail.gmail.com' \
--to=karthik.188@gmail.com \
--cc=chris.torek@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).