From: Karthik Nayak <karthik.188@gmail.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: chris.torek@gmail.com, git@vger.kernel.org, gitster@pobox.com, ps@pks.im
Subject: Re: [PATCH v2 2/7] update-ref: add support for symref-verify
Date: Fri, 19 Apr 2024 16:21:26 -0500 [thread overview]
Message-ID: <CAOLa=ZSFXH=xGYF0on3B2YeiudJBGMpjctHazLcn-fyO=UR4yA@mail.gmail.com> (raw)
In-Reply-To: <CAP8UFD2bqdMujHgS9oPn4Xqzu=EhGMPms7ftOB7uf-AHke79AQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5626 bytes --]
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>
>>
>> In the previous commit, we added the required base for adding symref
>> support in transactions provided by the 'git-update-ref(1)'. This commit
>
> s/by the 'git-update-ref(1)'/by 'git-update-ref(1)'/
>
will change.
>> introduces the 'symref-verify' command which is similar to the existing
>> 'verify' command for regular refs.
>>
>> The 'symref-verify' command allows users to verify if a provided <ref>
>> contains the provided <old-ref> without changing the <ref>.
>
> What if <old-ref> looks like an oid? Will it work if <ref> is a
> regular ref that actually contains this oid?
>
This would fail, since we parse and check the ref entered.
>> If <old-ref>
>> is not provided, the command will verify that the <ref> doesn't exist.
>> Since we're checking for symbolic refs,
>
> So if <ref> isn't a symbolic ref, it will fail? I guess the answer is
> yes, but I think it would be better to be clear about this.
Will try and clarify more in the commit message.
>> this command will only work with
>> the 'no-deref' mode. This is because any dereferenced symbolic ref will
>> point to an object and not a ref and the regular 'verify' command can be
>> used in such situations.
>>
>> This commit adds all required helper functions required to also
>> introduce the other symref commands, namely create, delete, and update.
>
> Are these helper functions actually used in this commit? If yes, it
> would be nice to say it explicitly. If not, why is it a good idea to
> add them now?
>
> Also I think we prefer imperative wordings like "Add all required..."
> over wordings like "This commit adds all required..."
>
Will clarify and make it imperative.
>> We also add tests to test the command in both the regular stdin mode and
>> also with the '-z' flag.
>
>> When the user doesn't provide a <old-ref> we need to check that the
>> provided <ref> doesn't exist.
>
> That the provided <ref> doesn't exist or that it isn't a symref?
That it doesn't exist. I don't know if it makes sense to make it 'that
it isn't a symref'.
>> And to do this, we take over the existing
>> understanding that <old-oid> when set to its zero value, it refers to
>> the ref not existing. While this seems like a mix of contexts between
>> using <*-ref> and <*-oid>, this actually works really well, especially
>> considering the fact that we want to eventually also introduce
>>
>> symref-update SP <ref> SP <new-ref> [SP (<old-oid> | <old-rev>)] LF
>>
>> and here, we'd allow the user to update a regular <ref> to a symref and
>> use <old-oid> to check the <ref>'s oid.
>
> This means that the ref actually exists but isn't a symref.
>
> So if I understand correctly, for now it will work only if the ref
> doesn't exist, but in the future we can make it work also if the ref
> exists but isn't a symref.
This comment specifically talks about the `symref-update` command which
is in an upcoming commit, in that commit, we do allow users to update a
regular ref to a symref, while also checking the old_oid value of that
ref.
>> This can be extrapolated to the
>> user using this to create a symref when provided a zero <old-oid>. Which
>> will work given how we're setting it up.
>>
>> We also disable the reference-transaction hook for symref-updates which
>> will be tackled in its own commit.
>
> Why do we need to disable it?
>
Historically, reference transaction didn't support symrefs, so we're not
actually changing functionality here. While we can fix that in this
commit and add tests, it makes more sense to tackle it in its own
commit, which we do at the end.
>> Add required tests for 'symref-verify' while also adding reflog checks for
>> the pre-existing 'verify' tests.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>> +symref-verify::
>> + Verify symbolic <ref> against <old-ref> but do not change it.
>> + If <old-ref> is missing, the ref must not exist.
>
> "must not exist" means that we will need to change this when we make
> it work if the ref exists but isn't a symref. Ok.
>
>> Can only be
>> + used in `no-deref` mode.
>> +
>
>
>> + /*
>> + * old_ref is optional, but we want to differentiate between
>> + * a NULL and zero value.
>> + */
>> + old_ref = parse_next_refname(&next);
>> + if (!old_ref)
>> + old_oid = *null_oid();
>
> So for now we always overwrite old_oid when old_ref is not provided.
> So the ref should not exist and the command will fail if it exists as
> a regular ref. Ok.
>
>> + else if (read_ref(old_ref, NULL))
>> + die("symref-verify %s: invalid <old-ref>", refname);
>
> So I guess we die() if old_ref is the empty string.
>
> It's kind of strange as in the previous patch there was:
>
>> + * If (type & REF_SYMREF_UPDATE), check that the reference
>> + * previously had this value (or didn't previously exist,
>> + * if `old_ref` is an empty string).
>
> So it said the empty string meant the old_ref didn't previously exist,
> but now it's actually NULL that means the old_ref didn't previously
> exist.
>
Good catch, I initially did start with the empty string idea, but
switched to using zero_oid, since it made a lot more sense, especially
because now it sets interoperability. I will fix that comment.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
next prev parent reply other threads:[~2024-04-19 21:21 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 [this message]
2024-04-19 9:40 ` Patrick Steinhardt
2024-04-19 21:53 ` Karthik Nayak
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=ZSFXH=xGYF0on3B2YeiudJBGMpjctHazLcn-fyO=UR4yA@mail.gmail.com' \
--to=karthik.188@gmail.com \
--cc=chris.torek@gmail.com \
--cc=christian.couder@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).