Re: [PATCH v2 2/7] update-ref: add support for symref-verify

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 18, 2024, 14:26 UTC
Message-ID: <CAP8UFD2bqdMujHgS9oPn4Xqzu=EhGMPms7ftOB7uf-AHke79AQ@mail.gmail.com>
In-Reply-To: <20240412095908.1134387-3-knayak@gitlab.com>

On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@gmail.com> wrote:

Show 5 quoted lines

>
> 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)'/

Show 5 quoted lines

> 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?

> 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.

Show 7 quoted lines

> 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..."

> 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?

Show 10 quoted lines

> 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.

Show 6 quoted lines

> 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?

> 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.
> +

Show 7 quoted lines

> +       /*
> +        * 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.

Show 12 quoted lines

> +       if (*next != line_termination)
> +               die("symref-verify %s: extra input: %s", refname, next);
> +
> +       if (ref_transaction_verify(transaction, refname, old_ref ? NULL : &old_oid,
> +                                  old_ref, update_flags | REF_SYMREF_UPDATE, &err))
>                 die("%s", err.buf);
>
>         update_flags = default_flags;
>         free(refname);
> +       free(old_ref);
>         strbuf_release(&err);
>  }