From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: peff@peff.net, me@ttaylorr.com, gitster@pobox.com
Subject: Re: [PATCH] refs: Always pass old object name to reftx hook
Date: Fri, 4 Dec 2020 09:37:21 +0100 [thread overview]
Message-ID: <X8n1Qf7TJyqIOE/l@ncase> (raw)
In-Reply-To: <d255c7a5f95635c2e7ae36b9689c3efd07b4df5d.1604501894.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5363 bytes --]
On Wed, Nov 04, 2020 at 03:58:40PM +0100, Patrick Steinhardt wrote:
> Inputs of the reference-transaction hook currently depends on the
> command which is being run. For example if the command `git update-ref
> $REF $A $B` is executed, it will receive "$B $A $REF" as input, but if
> the command `git update-ref $REF $A` is executed without providing the
> old value, then it will receive "0*40 $A $REF" as input. This is due to
> the fact that we directly write queued transaction updates into the
> hook's standard input, which will not contain the old object value in
> case it wasn't provided.
>
> While this behaviour reflects what is happening as part of the
> repository, it doesn't feel like it is useful. The main intent of the
> reference-transaction hook is to be able to completely audit all
> reference updates, no matter where they come from. As such, it makes a
> lot more sense to always provide actual values instead of what the user
> wanted. Furthermore, it's impossible for the hook to distinguish whether
> this is intended to be a branch creation or a branch update without
> doing additional digging with the current format.
>
> Fix the issue by storing the old object value into the queued
> transaction update operation if it wasn't provided by the caller.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
Quick ping on this patch. Is there any interest or shall I just drop it?
Patrick
> Documentation/githooks.txt | 6 ++++++
> refs/files-backend.c | 8 ++++++++
> refs/packed-backend.c | 2 ++
> t/t1416-ref-transaction-hooks.sh | 12 ++++++------
> 4 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 4e097dc4e9..8f3540e2f6 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -492,6 +492,12 @@ receives on standard input a line of the format:
>
> <old-value> SP <new-value> SP <ref-name> LF
>
> +where `<old-value>` is the old object name stored in the ref,
> +`<new-value>` is the new object name to be stored in the ref and
> +`<ref-name>` is the full name of the ref.
> +When creating a new ref, `<old-value>` is 40 `0`.
> +When deleting an old ref, `<new-value>` is 40 `0`.
> +
> The exit status of the hook is ignored for any state except for the
> "prepared" state. In the "prepared" state, a non-zero exit status will
> cause the transaction to be aborted. The hook will not be called with
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 04e85e7002..5b10d3822b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2452,6 +2452,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> ret = TRANSACTION_GENERIC_ERROR;
> goto out;
> }
> +
> + if (!(update->flags & REF_HAVE_OLD))
> + oidcpy(&update->old_oid, &lock->old_oid);
> } else {
> /*
> * Create a new update for the reference this
> @@ -2474,6 +2477,9 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> goto out;
> }
>
> + if (!(update->flags & REF_HAVE_OLD))
> + oidcpy(&update->old_oid, &lock->old_oid);
> +
> /*
> * If this update is happening indirectly because of a
> * symref update, record the old OID in the parent
> @@ -2484,6 +2490,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> parent_update = parent_update->parent_update) {
> struct ref_lock *parent_lock = parent_update->backend_data;
> oidcpy(&parent_lock->old_oid, &lock->old_oid);
> + if (!(parent_update->flags & REF_HAVE_OLD))
> + oidcpy(&parent_update->old_oid, &lock->old_oid);
> }
> }
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index b912f2505f..08f0feee3d 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1178,6 +1178,8 @@ static int write_with_updates(struct packed_ref_store *refs,
> oid_to_hex(&update->old_oid));
> goto error;
> }
> + } else {
> + oidcpy(&update->old_oid, iter->oid);
> }
>
> /* Now figure out what to use for the new value: */
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index f6e741c6c0..111533682a 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -52,12 +52,12 @@ test_expect_success 'hook gets all queued updates in prepared state' '
> fi
> EOF
> cat >expect <<-EOF &&
> - $ZERO_OID $POST_OID HEAD
> - $ZERO_OID $POST_OID refs/heads/master
> + $PRE_OID $POST_OID HEAD
> + $PRE_OID $POST_OID refs/heads/master
> EOF
> git update-ref HEAD POST <<-EOF &&
> - update HEAD $ZERO_OID $POST_OID
> - update refs/heads/master $ZERO_OID $POST_OID
> + update HEAD $PRE_OID $POST_OID
> + update refs/heads/master $PRE_OID $POST_OID
> EOF
> test_cmp expect actual
> '
> @@ -75,8 +75,8 @@ test_expect_success 'hook gets all queued updates in committed state' '
> fi
> EOF
> cat >expect <<-EOF &&
> - $ZERO_OID $POST_OID HEAD
> - $ZERO_OID $POST_OID refs/heads/master
> + $PRE_OID $POST_OID HEAD
> + $PRE_OID $POST_OID refs/heads/master
> EOF
> git update-ref HEAD POST &&
> test_cmp expect actual
> --
> 2.29.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-12-04 8:40 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-04 14:58 [PATCH] refs: Always pass old object name to reftx hook Patrick Steinhardt
2020-12-04 8:37 ` Patrick Steinhardt [this message]
2021-01-12 21:07 ` Taylor Blau
2021-01-13 11:22 ` Patrick Steinhardt
2021-01-13 15:09 ` Taylor Blau
2021-01-13 20:11 ` Junio C Hamano
2021-01-18 12:44 ` Patrick Steinhardt
2021-01-18 12:49 ` [PATCH RESEND] " Patrick Steinhardt
2021-01-18 22:45 ` Junio C Hamano
2021-01-20 6:28 ` Patrick Steinhardt
2021-01-20 7:06 ` Junio C Hamano
2021-01-22 6:44 ` Patrick Steinhardt
2021-01-22 18:33 ` Junio C Hamano
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=X8n1Qf7TJyqIOE/l@ncase \
--to=ps@pks.im \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).