Patrick Steinhardt <ps@xxxxxx> writes:
> On Tue, May 14, 2024 at 02:44:07PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> The 'symref-verify' command allows users to verify if a provided <ref>
>> contains the provided <old-target> without changing the <ref>. If
>> <old-target> is not provided, the command will verify that the <ref>
>> doesn't exist.
>>
>> The command allows users to verify symbolic refs within a transaction,
>> and this means users can perform a set of changes in a transaction only
>> when the verification holds good.
>>
>> Since we're checking for symbolic refs, 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.
>>
>> Add required tests for symref support in 'verify' while also adding
>> reflog checks for the pre-existing 'verify' tests.
>
> I'm a bit surprised that you add reflog-related tests, and you don't
> really explain why you do it. Do we change any behaviour relating to
> reflogs here? If there is a particular reason that is independent of the
> new "symref-verify" command, then I'd expect this to be part of a
> separate commit.
>
Ah! There is no divergence in behavior, rather this is behavior which is
never captured in tests. So I thought it makes to have tests around it.
> [snip]
>> diff --git a/refs.c b/refs.c
>> index 59858fafdb..ee4c6ed99c 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1331,14 +1331,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>> int ref_transaction_verify(struct ref_transaction *transaction,
>> const char *refname,
>> const struct object_id *old_oid,
>> + const char *old_target,
>> unsigned int flags,
>> struct strbuf *err)
>> {
>> - if (!old_oid)
>> - BUG("verify called with old_oid set to NULL");
>> + if (!old_target && !old_oid)
>> + BUG("verify called with old_oid and old_target set to NULL");
>> + if (old_target && !(flags & REF_NO_DEREF))
>> + BUG("verify cannot operate on symrefs with deref mode");
>
> Should we also BUG on `old_target && old_oid`?
>
I didn't do this, because `ref_transaction_add_update` downstream from
this already does that. But I guess no harm in adding it here too.
>> @@ -1641,4 +1647,88 @@ test_expect_success PIPE 'transaction flushes status updates' '
>> test_cmp expected actual
>> '
>>
>> +create_stdin_buf () {
>> + if test "$1" = "-z"
>> + then
>> + shift
>> + printf "$F" "$@" >stdin
>> + else
>> + echo "$@" >stdin
>> + fi
>> +}
>
> I think this would be easier to use if you didn't handle the redirect to
> "stdin" over here, but at the calling site. Otherwise, the caller needs
> to be aware of the inner workings.
>
Not sure what you mean by easier here, but I think it would be nicer to
read, since the client would now determine the destination of the
formatting and this would align with what the test needs to do. Will
change!
>> +for type in "" "-z"
>> +do
>> +
>> + test_expect_success "stdin ${type} symref-verify fails without --no-deref" '
>
> We typically avoid curly braces unless required.
>
Will change, thanks!
> [snip]
>> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
>> index 067fd57290..fd58b902f4 100755
>> --- a/t/t1416-ref-transaction-hooks.sh
>> +++ b/t/t1416-ref-transaction-hooks.sh
>> @@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'hook gets all queued symref updates' '
>> + test_when_finished "rm actual" &&
>> +
>> + git update-ref refs/heads/branch $POST_OID &&
>> + git symbolic-ref refs/heads/symref refs/heads/main &&
>> +
>> + test_hook reference-transaction <<-\EOF &&
>> + echo "$*" >>actual
>> + while read -r line
>> + do
>> + printf "%s\n" "$line"
>> + done >>actual
>> + EOF
>> +
>> + cat >expect <<-EOF &&
>> + prepared
>> + ref:refs/heads/main $ZERO_OID refs/heads/symref
>> + committed
>> + ref:refs/heads/main $ZERO_OID refs/heads/symref
>> + EOF
>> +
>> + git update-ref --no-deref --stdin <<-EOF &&
>> + start
>> + symref-verify refs/heads/symref refs/heads/main
>> + prepare
>> + commit
>> + EOF
>> + test_cmp expect actual
>> +'
>> +
>> test_done
>
> So the reference-transaction hook executes even for "symref-verify"?
> This feels quite unexpected to me. Do we do the same for "verify"?
>
> Patrick
Yes this is the same for verify as well. I was surprised to find this
too. It's just the way ref update code is written, all updates land
trigger the hook. This means verify, which is also a form of update,
with just the new value not set, also triggers the hook. I've kept the
same behavior with symref-verify.
Attachment:
signature.asc
Description: PGP signature
