Patrick Steinhardt <ps@xxxxxx> 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:
signature.asc
Description: PGP signature
