Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>
>> +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:
signature.asc
Description: PGP signature
