Re: [PATCH 2/6] update-ref: add support for 'symref-verify' command

> On Tue, May 14, 2024 at 02:44:07PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> 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.
>

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

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

>> +for type in "" "-z"
>> +do
>> +
>> +	test_expect_success "stdin ${type} symref-verify fails without --no-deref" '
>
> We typically avoid curly braces unless required.
>

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