Re: [PATCH 2/6] update-ref: add support for 'symref-verify' command
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 16, 2024, 11:09 UTC
- Message-ID
- <ZkXpYep2MKdsyNyV@tanuki>
- In-Reply-To
- <20240514124411.1037019-3-knayak@gitlab.com>
On Tue, May 14, 2024 at 02:44:07PM +0200, Karthik Nayak wrote:
Show 18 quoted lines
> 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]
Show 18 quoted lines
> 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`?
Show 13 quoted lines
> @@ -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]
Show 39 quoted lines
> 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