Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Karthik Nayak <karthik.188@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH 2/6] update-ref: add support for 'symref-verify' command
Date: Fri, 17 May 2024 18:21:53 +0200	[thread overview]
Message-ID: <CAOLa=ZSUV1X==x2CBivgu=L7SQryXNZZkLwxgyNth=a+bH9SQg@mail.gmail.com> (raw)
In-Reply-To: <ZkXpYep2MKdsyNyV@tanuki>

[-- Attachment #1: Type: text/plain, Size: 4794 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

  reply	other threads:[~2024-05-17 16:21 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-14 12:44 [PATCH 0/6] update-ref: add symref support for --stdin Karthik Nayak
2024-05-14 12:44 ` [PATCH 1/6] refs: create and use `ref_update_ref_must_exist()` Karthik Nayak
2024-05-16 11:09   ` Patrick Steinhardt
2024-05-17 13:08     ` Karthik Nayak
2024-05-14 12:44 ` [PATCH 2/6] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-05-16 11:09   ` Patrick Steinhardt
2024-05-17 16:21     ` Karthik Nayak [this message]
2024-05-21  6:41       ` Patrick Steinhardt
2024-05-14 12:44 ` [PATCH 3/6] update-ref: add support for 'symref-delete' command Karthik Nayak
2024-05-16 11:09   ` Patrick Steinhardt
2024-05-14 12:44 ` [PATCH 4/6] update-ref: add support for 'symref-create' command Karthik Nayak
2024-05-16 11:09   ` Patrick Steinhardt
2024-05-19 14:01     ` Karthik Nayak
2024-05-14 12:44 ` [PATCH 5/6] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-05-14 12:44 ` [PATCH 6/6] update-ref: add support for 'symref-update' command Karthik Nayak
2024-05-16 11:09   ` Patrick Steinhardt
2024-05-21  9:49     ` Karthik Nayak
2024-05-22  7:59       ` Karthik Nayak
2024-05-22  9:03 ` [PATCH v2 0/6] update-ref: add symref support for --stdin Karthik Nayak
2024-05-22  9:03   ` [PATCH v2 1/6] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
2024-05-22  9:03   ` [PATCH v2 2/6] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-05-22  9:03   ` [PATCH v2 3/6] update-ref: add support for 'symref-delete' command Karthik Nayak
2024-05-22  9:03   ` [PATCH v2 4/6] update-ref: add support for 'symref-create' command Karthik Nayak
2024-05-22  9:03   ` [PATCH v2 5/6] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-05-22  9:03   ` [PATCH v2 6/6] update-ref: add support for 'symref-update' command Karthik Nayak
2024-05-25 23:00     ` Junio C Hamano
2024-05-29  8:29       ` Karthik Nayak
2024-05-23 15:02   ` [PATCH v2 0/6] update-ref: add symref support for --stdin Junio C Hamano
2024-05-23 15:52     ` Karthik Nayak
2024-05-23 16:29       ` Junio C Hamano
2024-05-23 17:50         ` Karthik Nayak
2024-05-23 17:59         ` Eric Sunshine
2024-05-23 18:08           ` Junio C Hamano
2024-05-23 18:50             ` Eric Sunshine
2024-05-23 19:06               ` Junio C Hamano
2024-05-23 21:46           ` Junio C Hamano
2024-05-23 16:03     ` Junio C Hamano
2024-05-30 12:09 ` [PATCH v3 " Karthik Nayak
2024-06-05  8:02   ` Patrick Steinhardt
2024-05-30 12:09 ` [PATCH v3 1/6] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
2024-05-30 12:09 ` [PATCH v3 2/6] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-05-30 12:09 ` [PATCH v3 3/6] update-ref: add support for 'symref-delete' command Karthik Nayak
2024-06-05  8:02   ` Patrick Steinhardt
2024-06-05  9:31     ` Karthik Nayak
2024-06-05 16:22     ` Junio C Hamano
2024-05-30 12:09 ` [PATCH v3 4/6] update-ref: add support for 'symref-create' command Karthik Nayak
2024-06-05  8:02   ` Patrick Steinhardt
2024-05-30 12:09 ` [PATCH v3 5/6] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-05-30 12:09 ` [PATCH v3 6/6] update-ref: add support for 'symref-update' command Karthik Nayak
2024-06-05  8:02   ` Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOLa=ZSUV1X==x2CBivgu=L7SQryXNZZkLwxgyNth=a+bH9SQg@mail.gmail.com' \
    --to=karthik.188@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).