refs: implement reference transaction hooks
diff mbox series

Message ID 1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im
State New
Headers show
Series
  • refs: implement reference transaction hooks
Related show

Commit Message

Patrick Steinhardt June 2, 2020, 8:25 a.m. UTC
The low-level reference transactions used to update references are
currently completely opaque to the user. While certainly desirable in
most usecases, there are some which might want to hook into the
transaction to observe all queued reference updates as well as observing
the abortion or commit of a prepared transaction.

One such usecase would be to have a set of replicas of a given Git
repository, where we perform Git operations on all of the repositories
at once and expect the outcome to be the same in all of them. While
there exist hooks already for a certain subset of Git commands that
could be used to implement a voting mechanism for this, many others
currently don't have any mechanism for this.

The above scenario is the motivation for a set of three new hooks that
reach directly into Git's reference transaction. Each of the following
new hooks (currently) doesn't accept any parameters and receives the set
of queued reference updates via stdin:

    - ref-transaction-prepared gets called when all reference updates
      have been queued. At this stage, the hook may decide to abort the
      transaction prematurely by returning a non-zero status code.

    - ref-transaction-committed gets called when a reference transaction
      was transmitted and all queued updates have been persisted.

    - ref-transaction-aborted gets called when a reference transaction
      was aborted and all queued updates have been rolled back.

Given the usecase described above, a voting mechanism can now be
implemented as a "ref-transaction-prepared" hook: as soon as it gets
called, it will take all of stdin and use it to cast a vote to a central
service. When all replicas of the repository agree, the hook will exit
with zero, otherwise it will abort the transaction by returning
non-zero. The most important upside is that this will catch _all_
commands writing references at once, allowing to implement strong
consistency for reference updates via a single mechanism.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/githooks.txt       | 51 ++++++++++++++++++
 refs.c                           | 67 +++++++++++++++++++++++-
 t/t1416-ref-transaction-hooks.sh | 88 ++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 2 deletions(-)
 create mode 100755 t/t1416-ref-transaction-hooks.sh

Comments

Junio C Hamano June 2, 2020, 5:47 p.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> The above scenario is the motivation for a set of three new hooks that
> reach directly into Git's reference transaction. Each of the following
> new hooks (currently) doesn't accept any parameters and receives the set
> of queued reference updates via stdin:

Do we have something (e.g. performance measurement) to convince
ourselves that this won't incur unacceptable levels of overhead in
null cases where there is no hook installed in the repository?

> +static int run_transaction_hook(struct ref_transaction *transaction,
> +				const char *hook_name)
> +{
> +	struct child_process proc = CHILD_PROCESS_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *argv[2];
> +	int code, i;
> +
> +	argv[0] = find_hook(hook_name);
> +	if (!argv[0])
> +		return 0;
> +
> +	argv[1] = NULL;
> +
> +	proc.argv = argv;

Let's use proc.args and argv_push() API in newly introduced code
instead.

> +	proc.in = -1;
> +	proc.stdout_to_stderr = 1;
> +	proc.trace2_hook_name = hook_name;
> +
> +	code = start_command(&proc);
> +	if (code)
> +		return code;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	for (i = 0; i < transaction->nr; i++) {
> +		struct ref_update *update = transaction->updates[i];
> +
> +		strbuf_reset(&buf);
> +		strbuf_addf(&buf, "%s %s %s\n",
> +			    oid_to_hex(&update->old_oid),
> +			    oid_to_hex(&update->new_oid),
> +			    update->refname);
> +
> +		if (write_in_full(proc.in, buf.buf, buf.len) < 0)
> +			break;

We leave the loop early when we detect a write failure here...

> +	}
> +
> +	close(proc.in);
> +	sigchain_pop(SIGPIPE);
> +
> +	strbuf_release(&buf);
> +	return finish_command(&proc);

... but the caller does not get notified.  Intended?

> +}
> +
>  int ref_transaction_prepare(struct ref_transaction *transaction,
>  			    struct strbuf *err)
>  {
>  	struct ref_store *refs = transaction->ref_store;
> +	int ret;
>  
>  	switch (transaction->state) {
>  	case REF_TRANSACTION_OPEN:
> @@ -2012,7 +2060,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
>  		return -1;
>  	}
>  
> -	return refs->be->transaction_prepare(refs, transaction, err);
> +	ret = refs->be->transaction_prepare(refs, transaction, err);
> +	if (ret)
> +		return ret;
> +
> +	ret = run_transaction_hook(transaction, "ref-transaction-prepared");

This caller does care about it, no?

> +	if (ret) {
> +		ref_transaction_abort(transaction, err);
> +		die(_("ref updates aborted by hook"));
> +	}
> +
> +	return 0;
>  }
>  
>  int ref_transaction_abort(struct ref_transaction *transaction,
> @@ -2036,6 +2094,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,
>  		break;
>  	}
>  
> +	run_transaction_hook(transaction, "ref-transaction-aborted");

And I presume that the callers of "ref_xn_abort()" would, too, but
the value returned here does not get folded into 'ret'.

>  	ref_transaction_free(transaction);
>  	return ret;
>  }
> @@ -2064,7 +2124,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		break;
>  	}
>  
> -	return refs->be->transaction_finish(refs, transaction, err);
> +	ret = refs->be->transaction_finish(refs, transaction, err);
> +	if (!ret)
> +		run_transaction_hook(transaction, "ref-transaction-committed");
> +	return ret;
>  }
>  
>  int refs_verify_refname_available(struct ref_store *refs,
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> new file mode 100755
> index 0000000000..b6df5fc883
> --- /dev/null
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -0,0 +1,88 @@
> +#!/bin/sh
> +
> +test_description='reference transaction hooks'
> +
> +. ./test-lib.sh
> +
> +create_commit ()
> +{

Style (Documentation/CodingGuidelines):

 - We prefer a space between the function name and the parentheses,
   and no space inside the parentheses. The opening "{" should also
   be on the same line.

> +	test_tick &&
> +	T=$(git write-tree) &&
> +	sha1=$(echo message | git commit-tree $T) &&
> +	echo $sha1

Calling this helper in a subprocess does not have the intended
effect of calling test_tick, which increments the committer
timestamp by 1 second to prepare for the next call...

> +}
> +
> +test_expect_success setup '
> +	mkdir -p .git/hooks
> +'
> +
> +test_expect_success 'prepared hook allows updating ref' '
> +	test_when_finished "rm .git/hooks/ref-transaction-prepared" &&
> +	write_script .git/hooks/ref-transaction-prepared <<-\EOF &&
> +		exit 0
> +	EOF
> +	C=$(create_commit) &&

... like here.

Instead of creating test commits left and right at each test, how
about preparing a pair of them (PRE, POST) upfront in the "setup"
step, reset the refs involved to PRE at the very beginning of each
test, and use POST to operations that would update the refs?  We can
use a couple of calls to test_commit helper to do so, without having
to bother with the low level porcelain calls if we go that route.

> +	git update-ref HEAD $C
> +'
> +
> +test_expect_success 'prepared hook aborts updating ref' '
> +	test_when_finished "rm .git/hooks/ref-transaction-prepared" &&
> +	write_script .git/hooks/ref-transaction-prepared <<-\EOF &&
> +		exit 1
> +	EOF
> +	C=$(create_commit) &&
> +	test_must_fail git update-ref HEAD $C 2>err &&
> +	grep "ref updates aborted by hook" err

Running "make GIT_TEST_GETTEXT_POISON=Yes test" would probably break
this line.  Use test_i18ngrep instead.

> +'
> +
> +test_expect_success 'prepared hook gets all queued updates' '
> +	test_when_finished "rm .git/hooks/ref-transaction-prepared actual" &&
> +	write_script .git/hooks/ref-transaction-prepared <<-\EOF &&
> +		while read -r line; do printf "%s\n" "$line"; done >actual

Style (used in the generated script)?

> +	EOF
> +	C=$(create_commit) &&
> +	cat >expect <<-EOF &&
> +		$ZERO_OID $C HEAD
> +		$ZERO_OID $C refs/heads/master
> +	EOF
> +	git update-ref HEAD $C <<-EOF &&
> +		update HEAD $ZERO_OID $C
> +		update refs/heads/master $ZERO_OID $C
> +	EOF
> +	test_cmp expect actual

OK, good futureproofing for the hash algorithm update ;-).
SZEDER Gábor June 2, 2020, 6:09 p.m. UTC | #2
On Tue, Jun 02, 2020 at 10:25:44AM +0200, Patrick Steinhardt wrote:
> The low-level reference transactions used to update references are
> currently completely opaque to the user. While certainly desirable in
> most usecases, there are some which might want to hook into the
> transaction to observe all queued reference updates as well as observing
> the abortion or commit of a prepared transaction.
> 
> One such usecase would be to have a set of replicas of a given Git
> repository, where we perform Git operations on all of the repositories
> at once and expect the outcome to be the same in all of them. While
> there exist hooks already for a certain subset of Git commands that
> could be used to implement a voting mechanism for this, many others
> currently don't have any mechanism for this.
> 
> The above scenario is the motivation for a set of three new hooks that
> reach directly into Git's reference transaction. Each of the following
> new hooks (currently) doesn't accept any parameters and receives the set
> of queued reference updates via stdin:
> 
>     - ref-transaction-prepared gets called when all reference updates
>       have been queued. At this stage, the hook may decide to abort the
>       transaction prematurely by returning a non-zero status code.
> 
>     - ref-transaction-committed gets called when a reference transaction
>       was transmitted and all queued updates have been persisted.
> 
>     - ref-transaction-aborted gets called when a reference transaction
>       was aborted and all queued updates have been rolled back.

The point of reference transactions is that they are atomic, and these
hooks must work together to ensure that.  This raises the question how
these hooks can be updated in an actively used repository.

Having multiple hooks means that they can't be updated atomically, and
git might invoke the new abort hook after the transaction was prepared
with the old hook.  Now, if there were a single 'ref-transaction' hook
(which gets the phase of the transaction ('prepared', 'committed' or
'aborted') as a parameter), then it could be updated atomically by
mv-ing it to place, but even that update can happen in between git
invokes 'ref-transaction prepared' and 'ref-transaction aborted'.

I suppose this issue could be addressed by a single hook which runs
during the whole transaction and some back-and-forth communication
through stdin/out between git and the hook.  However, this would, I'm
afraid, complicate both Git's handling of this hook and the hook as
well, so let's take a step back first: is this something we should
worry about in the first place?

> Given the usecase described above, a voting mechanism can now be
> implemented as a "ref-transaction-prepared" hook: as soon as it gets
> called, it will take all of stdin and use it to cast a vote to a central
> service. When all replicas of the repository agree, the hook will exit
> with zero, otherwise it will abort the transaction by returning
> non-zero. The most important upside is that this will catch _all_
> commands writing references at once, allowing to implement strong
> consistency for reference updates via a single mechanism.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Documentation/githooks.txt       | 51 ++++++++++++++++++
>  refs.c                           | 67 +++++++++++++++++++++++-
>  t/t1416-ref-transaction-hooks.sh | 88 ++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 2 deletions(-)
>  create mode 100755 t/t1416-ref-transaction-hooks.sh
> 
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 81f2a87e88..48f8446943 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -404,6 +404,57 @@ Both standard output and standard error output are forwarded to
>  `git send-pack` on the other end, so you can simply `echo` messages
>  for the user.
>  
> +ref-transaction-prepared
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by any Git command that performs reference
> +updates. It executes as soon as all reference updates were queued to
> +the transaction and locked on disk. This hook executes for every
> +reference transaction that is being prepared and may thus get called
> +multiple times.
> +
> +It takes no arguments, but for each ref to be updated it receives on
> +standard input a line of the format:
> +
> +  <old-value> SP <new-value> SP <ref-name> LF
> +
> +If the hook exits with a non-zero status, the transaction is aborted
> +and the command exits immediately. The
> +<<ref-transaction-aborted,'ref-transaction-aborted'>> hook is not
> +executed in that case.
> +
> +[[ref-transaction-aborted]]
> +ref-transaction-aborted
> +~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by any Git command that performs reference
> +updates. It executes as soon as a reference transaction is aborted and
> +after all reference locks were released and any changes made to
> +references were rolled back. The hook may get called multiple times or
> +never in case no transaction was aborted.
> +
> +The hook takes no arguments, but for each ref to be updated it

Nit: I found it a bit surprising to read about refs "to be updated" in
the description of the 'aborted' hook, because by the time this hook
is called the update has already been refused.  The same applies to
the 'committed' hook below as well.

> +receives on standard input a line of the format:
> +
> +  <old-value> SP <new-value> SP <ref-name> LF
> +
> +The hook's exit code is discarded by Git.
> +
> +ref-transaction-committed
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by any Git command that performs reference
> +updates. It executes as soon as a reference transaction is committed,
> +persisting all changes to disk and releasing any locks. The hook may
> +get called multiple times or never in case no transaction was aborted.
> +
> +The hook takes no arguments, but for each ref to be updated it
> +receives on standard input a line of the format:
> +
> +  <old-value> SP <new-value> SP <ref-name> LF
> +
> +The hook's exit code is discarded by Git.
> +
>  push-to-checkout
>  ~~~~~~~~~~~~~~~~
>
Patrick Steinhardt June 3, 2020, 9:46 a.m. UTC | #3
On Tue, Jun 02, 2020 at 08:09:00PM +0200, SZEDER Gábor wrote:
> On Tue, Jun 02, 2020 at 10:25:44AM +0200, Patrick Steinhardt wrote:
> > The low-level reference transactions used to update references are
> > currently completely opaque to the user. While certainly desirable in
> > most usecases, there are some which might want to hook into the
> > transaction to observe all queued reference updates as well as observing
> > the abortion or commit of a prepared transaction.
> > 
> > One such usecase would be to have a set of replicas of a given Git
> > repository, where we perform Git operations on all of the repositories
> > at once and expect the outcome to be the same in all of them. While
> > there exist hooks already for a certain subset of Git commands that
> > could be used to implement a voting mechanism for this, many others
> > currently don't have any mechanism for this.
> > 
> > The above scenario is the motivation for a set of three new hooks that
> > reach directly into Git's reference transaction. Each of the following
> > new hooks (currently) doesn't accept any parameters and receives the set
> > of queued reference updates via stdin:
> > 
> >     - ref-transaction-prepared gets called when all reference updates
> >       have been queued. At this stage, the hook may decide to abort the
> >       transaction prematurely by returning a non-zero status code.
> > 
> >     - ref-transaction-committed gets called when a reference transaction
> >       was transmitted and all queued updates have been persisted.
> > 
> >     - ref-transaction-aborted gets called when a reference transaction
> >       was aborted and all queued updates have been rolled back.
> 
> The point of reference transactions is that they are atomic, and these
> hooks must work together to ensure that.  This raises the question how
> these hooks can be updated in an actively used repository.
> 
> Having multiple hooks means that they can't be updated atomically, and
> git might invoke the new abort hook after the transaction was prepared
> with the old hook.  Now, if there were a single 'ref-transaction' hook
> (which gets the phase of the transaction ('prepared', 'committed' or
> 'aborted') as a parameter), then it could be updated atomically by
> mv-ing it to place, but even that update can happen in between git
> invokes 'ref-transaction prepared' and 'ref-transaction aborted'.
> 
> I suppose this issue could be addressed by a single hook which runs
> during the whole transaction and some back-and-forth communication
> through stdin/out between git and the hook.  However, this would, I'm
> afraid, complicate both Git's handling of this hook and the hook as
> well, so let's take a step back first: is this something we should
> worry about in the first place?

Very good point about which I didn't previously think, thanks a lot for
raising it!

I agree that using a single long-lived hook would complicate the logic
by quite a bit. Given that the ref-transaction mechanism is such a
central piece to Git, I'd be wary of introducing such complexity. But
merging the current three hooks into a single hook accepting a parameter
sounds like a fair compromise to me that would at least allow users to
replace them atomically, even though it doesn't mean all stages were run
with the same version of the hook.

If we want to go further and also ensure the same script's run across
all hook invocations, we could also open the hook's file descriptor on
first invocation and then use `fexecve` on all subsequent invocations.
As long as the user doesn't do inline rewrites of the file, we'd thus
always use the same file. I'm not sure how portable that syscall is,
though, but it's at least part of POSIX-2008.

> > Given the usecase described above, a voting mechanism can now be
> > implemented as a "ref-transaction-prepared" hook: as soon as it gets
> > called, it will take all of stdin and use it to cast a vote to a central
> > service. When all replicas of the repository agree, the hook will exit
> > with zero, otherwise it will abort the transaction by returning
> > non-zero. The most important upside is that this will catch _all_
> > commands writing references at once, allowing to implement strong
> > consistency for reference updates via a single mechanism.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  Documentation/githooks.txt       | 51 ++++++++++++++++++
> >  refs.c                           | 67 +++++++++++++++++++++++-
> >  t/t1416-ref-transaction-hooks.sh | 88 ++++++++++++++++++++++++++++++++
> >  3 files changed, 204 insertions(+), 2 deletions(-)
> >  create mode 100755 t/t1416-ref-transaction-hooks.sh
> > 
> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> > index 81f2a87e88..48f8446943 100644
> > --- a/Documentation/githooks.txt
> > +++ b/Documentation/githooks.txt
> > @@ -404,6 +404,57 @@ Both standard output and standard error output are forwarded to
> >  `git send-pack` on the other end, so you can simply `echo` messages
> >  for the user.
> >  
> > +ref-transaction-prepared
> > +~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +This hook is invoked by any Git command that performs reference
> > +updates. It executes as soon as all reference updates were queued to
> > +the transaction and locked on disk. This hook executes for every
> > +reference transaction that is being prepared and may thus get called
> > +multiple times.
> > +
> > +It takes no arguments, but for each ref to be updated it receives on
> > +standard input a line of the format:
> > +
> > +  <old-value> SP <new-value> SP <ref-name> LF
> > +
> > +If the hook exits with a non-zero status, the transaction is aborted
> > +and the command exits immediately. The
> > +<<ref-transaction-aborted,'ref-transaction-aborted'>> hook is not
> > +executed in that case.
> > +
> > +[[ref-transaction-aborted]]
> > +ref-transaction-aborted
> > +~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +This hook is invoked by any Git command that performs reference
> > +updates. It executes as soon as a reference transaction is aborted and
> > +after all reference locks were released and any changes made to
> > +references were rolled back. The hook may get called multiple times or
> > +never in case no transaction was aborted.
> > +
> > +The hook takes no arguments, but for each ref to be updated it
> 
> Nit: I found it a bit surprising to read about refs "to be updated" in
> the description of the 'aborted' hook, because by the time this hook
> is called the update has already been refused.  The same applies to
> the 'committed' hook below as well.

Fair. This should rather read "that would have been updated" and "that
were updated", respectively.

Patrick
Patrick Steinhardt June 3, 2020, 11:26 a.m. UTC | #4
On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The above scenario is the motivation for a set of three new hooks that
> > reach directly into Git's reference transaction. Each of the following
> > new hooks (currently) doesn't accept any parameters and receives the set
> > of queued reference updates via stdin:
> 
> Do we have something (e.g. performance measurement) to convince
> ourselves that this won't incur unacceptable levels of overhead in
> null cases where there is no hook installed in the repository?

Not yet, but I'll try to come up with a benchmark in the next iteration.
I guess the best way to test is to directly exercise git-update-refs, as
it's nearly a direct wrapper around reference transactions.

> > +	proc.in = -1;
> > +	proc.stdout_to_stderr = 1;
> > +	proc.trace2_hook_name = hook_name;
> > +
> > +	code = start_command(&proc);
> > +	if (code)
> > +		return code;
> > +
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> > +
> > +	for (i = 0; i < transaction->nr; i++) {
> > +		struct ref_update *update = transaction->updates[i];
> > +
> > +		strbuf_reset(&buf);
> > +		strbuf_addf(&buf, "%s %s %s\n",
> > +			    oid_to_hex(&update->old_oid),
> > +			    oid_to_hex(&update->new_oid),
> > +			    update->refname);
> > +
> > +		if (write_in_full(proc.in, buf.buf, buf.len) < 0)
> > +			break;
> 
> We leave the loop early when we detect a write failure here...
> 
> > +	}
> > +
> > +	close(proc.in);
> > +	sigchain_pop(SIGPIPE);
> > +
> > +	strbuf_release(&buf);
> > +	return finish_command(&proc);
> 
> ... but the caller does not get notified.  Intended?

This is semi-intended. In case the hook doesn't fully consume stdin and
exits early, writing to its stdin would fail as we ignore SIGPIPE. We
don't want to force the hook to care about consuming all of stdin,
though.

We could improve error handling here by ignoring EPIPE, but making every
other write error fatal. If there's any other abnormal error condition
then we certainly don't want the hook to act on incomplete data and
pretend everything's fine.

> > +}
> > +
> >  int ref_transaction_prepare(struct ref_transaction *transaction,
> >  			    struct strbuf *err)
> >  {
> >  	struct ref_store *refs = transaction->ref_store;
> > +	int ret;
> >  
> >  	switch (transaction->state) {
> >  	case REF_TRANSACTION_OPEN:
> > @@ -2012,7 +2060,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
> >  		return -1;
> >  	}
> >  
> > -	return refs->be->transaction_prepare(refs, transaction, err);
> > +	ret = refs->be->transaction_prepare(refs, transaction, err);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = run_transaction_hook(transaction, "ref-transaction-prepared");
> 
> This caller does care about it, no?

This caller does as it may abort the transaction, but...

> > +	if (ret) {
> > +		ref_transaction_abort(transaction, err);
> > +		die(_("ref updates aborted by hook"));
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  int ref_transaction_abort(struct ref_transaction *transaction,
> > @@ -2036,6 +2094,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,
> >  		break;
> >  	}
> >  
> > +	run_transaction_hook(transaction, "ref-transaction-aborted");
> 
> And I presume that the callers of "ref_xn_abort()" would, too, but
> the value returned here does not get folded into 'ret'.

... this one doesn't. The thing is that the reference transaction hook
for the "aborted" case can't do anything about an aborted transaction
after the fact anyway. That's why I chose to ignore errors here, same
for the "committed" case.

Patrick
SZEDER Gábor June 7, 2020, 8:12 p.m. UTC | #5
On Wed, Jun 03, 2020 at 01:26:04PM +0200, Patrick Steinhardt wrote:
> On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > The above scenario is the motivation for a set of three new hooks that
> > > reach directly into Git's reference transaction. Each of the following
> > > new hooks (currently) doesn't accept any parameters and receives the set
> > > of queued reference updates via stdin:
> > 
> > Do we have something (e.g. performance measurement) to convince
> > ourselves that this won't incur unacceptable levels of overhead in
> > null cases where there is no hook installed in the repository?
> 
> Not yet, but I'll try to come up with a benchmark in the next iteration.
> I guess the best way to test is to directly exercise git-update-refs, as
> it's nearly a direct wrapper around reference transactions.
> 
> > > +	proc.in = -1;
> > > +	proc.stdout_to_stderr = 1;
> > > +	proc.trace2_hook_name = hook_name;
> > > +
> > > +	code = start_command(&proc);
> > > +	if (code)
> > > +		return code;
> > > +
> > > +	sigchain_push(SIGPIPE, SIG_IGN);
> > > +
> > > +	for (i = 0; i < transaction->nr; i++) {
> > > +		struct ref_update *update = transaction->updates[i];
> > > +
> > > +		strbuf_reset(&buf);
> > > +		strbuf_addf(&buf, "%s %s %s\n",
> > > +			    oid_to_hex(&update->old_oid),
> > > +			    oid_to_hex(&update->new_oid),
> > > +			    update->refname);
> > > +
> > > +		if (write_in_full(proc.in, buf.buf, buf.len) < 0)
> > > +			break;
> > 
> > We leave the loop early when we detect a write failure here...
> > 
> > > +	}
> > > +
> > > +	close(proc.in);
> > > +	sigchain_pop(SIGPIPE);
> > > +
> > > +	strbuf_release(&buf);
> > > +	return finish_command(&proc);
> > 
> > ... but the caller does not get notified.  Intended?
> 
> This is semi-intended. In case the hook doesn't fully consume stdin and
> exits early, writing to its stdin would fail as we ignore SIGPIPE. We
> don't want to force the hook to care about consuming all of stdin,
> though.

Why?  How could the prepared hook properly initialize the voting
mechanism for the transaction without reading all the refs to be
updated?

> We could improve error handling here by ignoring EPIPE, but making every
> other write error fatal. If there's any other abnormal error condition
> then we certainly don't want the hook to act on incomplete data and
> pretend everything's fine.

As I read v2 of this patch, a prepared hook can exit(0) early without
reading all the refs to be updated, cause EPIPE in the git process
invoking the hook, and that process would interpret that as success.
I haven't though it through how such a voting mechanism would work,
but I have a gut feeling that this can't be good.
Patrick Steinhardt June 8, 2020, 5:36 a.m. UTC | #6
On Sun, Jun 07, 2020 at 10:12:33PM +0200, SZEDER Gábor wrote:
> On Wed, Jun 03, 2020 at 01:26:04PM +0200, Patrick Steinhardt wrote:
> > On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:
> > > Patrick Steinhardt <ps@pks.im> writes:
> > > 
> > > > The above scenario is the motivation for a set of three new hooks that
> > > > reach directly into Git's reference transaction. Each of the following
> > > > new hooks (currently) doesn't accept any parameters and receives the set
> > > > of queued reference updates via stdin:
> > > 
> > > Do we have something (e.g. performance measurement) to convince
> > > ourselves that this won't incur unacceptable levels of overhead in
> > > null cases where there is no hook installed in the repository?
> > 
> > Not yet, but I'll try to come up with a benchmark in the next iteration.
> > I guess the best way to test is to directly exercise git-update-refs, as
> > it's nearly a direct wrapper around reference transactions.
> > 
> > > > +	proc.in = -1;
> > > > +	proc.stdout_to_stderr = 1;
> > > > +	proc.trace2_hook_name = hook_name;
> > > > +
> > > > +	code = start_command(&proc);
> > > > +	if (code)
> > > > +		return code;
> > > > +
> > > > +	sigchain_push(SIGPIPE, SIG_IGN);
> > > > +
> > > > +	for (i = 0; i < transaction->nr; i++) {
> > > > +		struct ref_update *update = transaction->updates[i];
> > > > +
> > > > +		strbuf_reset(&buf);
> > > > +		strbuf_addf(&buf, "%s %s %s\n",
> > > > +			    oid_to_hex(&update->old_oid),
> > > > +			    oid_to_hex(&update->new_oid),
> > > > +			    update->refname);
> > > > +
> > > > +		if (write_in_full(proc.in, buf.buf, buf.len) < 0)
> > > > +			break;
> > > 
> > > We leave the loop early when we detect a write failure here...
> > > 
> > > > +	}
> > > > +
> > > > +	close(proc.in);
> > > > +	sigchain_pop(SIGPIPE);
> > > > +
> > > > +	strbuf_release(&buf);
> > > > +	return finish_command(&proc);
> > > 
> > > ... but the caller does not get notified.  Intended?
> > 
> > This is semi-intended. In case the hook doesn't fully consume stdin and
> > exits early, writing to its stdin would fail as we ignore SIGPIPE. We
> > don't want to force the hook to care about consuming all of stdin,
> > though.
> 
> Why?  How could the prepared hook properly initialize the voting
> mechanism for the transaction without reading all the refs to be
> updated?

Because the hook might not want to implement a voting mechanism after
all but something entirely different which we're currently not
foreseeing as a valid usecase. We don't enforce this anywhere else
either, like e.g. for the pre-receive hook. If that one exits early
without consuming its stdin then that's totally fine.

> > We could improve error handling here by ignoring EPIPE, but making every
> > other write error fatal. If there's any other abnormal error condition
> > then we certainly don't want the hook to act on incomplete data and
> > pretend everything's fine.
> 
> As I read v2 of this patch, a prepared hook can exit(0) early without
> reading all the refs to be updated, cause EPIPE in the git process
> invoking the hook, and that process would interpret that as success.
> I haven't though it through how such a voting mechanism would work,
> but I have a gut feeling that this can't be good.

As said, I lean towards allowing more flexibility for the hook
implementation to also cater for other usecases. But I agree that in a
voting implementation, not reading all of stdin is a bad thing and may
point to a buggy hook implementation. Aborting the transaction if the
hook didn't read all of stdin would be a nice safeguard in that case.

With the current implementation of using a single hook for "prepared",
"committed" and "aborted", it'd also force the hook implementation to do
something in cases it doesn't care about. E.g.

    #!/bin/sh
    case "$1" in
        prepared)
            VOTE=$(sha1sum <&0)
            cast $VOTE
            ;;
        aborted|committed)
            cat <&0 >/dev/null
            ;;
    esac

That being said, I'm not opposed to enforce this and not treat EPIPE
differently.

Patrick

Patch
diff mbox series

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 81f2a87e88..48f8446943 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -404,6 +404,57 @@  Both standard output and standard error output are forwarded to
 `git send-pack` on the other end, so you can simply `echo` messages
 for the user.
 
+ref-transaction-prepared
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by any Git command that performs reference
+updates. It executes as soon as all reference updates were queued to
+the transaction and locked on disk. This hook executes for every
+reference transaction that is being prepared and may thus get called
+multiple times.
+
+It takes no arguments, but for each ref to be updated it receives on
+standard input a line of the format:
+
+  <old-value> SP <new-value> SP <ref-name> LF
+
+If the hook exits with a non-zero status, the transaction is aborted
+and the command exits immediately. The
+<<ref-transaction-aborted,'ref-transaction-aborted'>> hook is not
+executed in that case.
+
+[[ref-transaction-aborted]]
+ref-transaction-aborted
+~~~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by any Git command that performs reference
+updates. It executes as soon as a reference transaction is aborted and
+after all reference locks were released and any changes made to
+references were rolled back. The hook may get called multiple times or
+never in case no transaction was aborted.
+
+The hook takes no arguments, but for each ref to be updated it
+receives on standard input a line of the format:
+
+  <old-value> SP <new-value> SP <ref-name> LF
+
+The hook's exit code is discarded by Git.
+
+ref-transaction-committed
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by any Git command that performs reference
+updates. It executes as soon as a reference transaction is committed,
+persisting all changes to disk and releasing any locks. The hook may
+get called multiple times or never in case no transaction was aborted.
+
+The hook takes no arguments, but for each ref to be updated it
+receives on standard input a line of the format:
+
+  <old-value> SP <new-value> SP <ref-name> LF
+
+The hook's exit code is discarded by Git.
+
 push-to-checkout
 ~~~~~~~~~~~~~~~~
 
diff --git a/refs.c b/refs.c
index 224ff66c7b..e41fa7ea55 100644
--- a/refs.c
+++ b/refs.c
@@ -9,6 +9,7 @@ 
 #include "iterator.h"
 #include "refs.h"
 #include "refs/refs-internal.h"
+#include "run-command.h"
 #include "object-store.h"
 #include "object.h"
 #include "tag.h"
@@ -16,6 +17,7 @@ 
 #include "worktree.h"
 #include "argv-array.h"
 #include "repository.h"
+#include "sigchain.h"
 
 /*
  * List of all available backends
@@ -1986,10 +1988,56 @@  int ref_update_reject_duplicates(struct string_list *refnames,
 	return 0;
 }
 
+static int run_transaction_hook(struct ref_transaction *transaction,
+				const char *hook_name)
+{
+	struct child_process proc = CHILD_PROCESS_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	const char *argv[2];
+	int code, i;
+
+	argv[0] = find_hook(hook_name);
+	if (!argv[0])
+		return 0;
+
+	argv[1] = NULL;
+
+	proc.argv = argv;
+	proc.in = -1;
+	proc.stdout_to_stderr = 1;
+	proc.trace2_hook_name = hook_name;
+
+	code = start_command(&proc);
+	if (code)
+		return code;
+
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = transaction->updates[i];
+
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%s %s %s\n",
+			    oid_to_hex(&update->old_oid),
+			    oid_to_hex(&update->new_oid),
+			    update->refname);
+
+		if (write_in_full(proc.in, buf.buf, buf.len) < 0)
+			break;
+	}
+
+	close(proc.in);
+	sigchain_pop(SIGPIPE);
+
+	strbuf_release(&buf);
+	return finish_command(&proc);
+}
+
 int ref_transaction_prepare(struct ref_transaction *transaction,
 			    struct strbuf *err)
 {
 	struct ref_store *refs = transaction->ref_store;
+	int ret;
 
 	switch (transaction->state) {
 	case REF_TRANSACTION_OPEN:
@@ -2012,7 +2060,17 @@  int ref_transaction_prepare(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	return refs->be->transaction_prepare(refs, transaction, err);
+	ret = refs->be->transaction_prepare(refs, transaction, err);
+	if (ret)
+		return ret;
+
+	ret = run_transaction_hook(transaction, "ref-transaction-prepared");
+	if (ret) {
+		ref_transaction_abort(transaction, err);
+		die(_("ref updates aborted by hook"));
+	}
+
+	return 0;
 }
 
 int ref_transaction_abort(struct ref_transaction *transaction,
@@ -2036,6 +2094,8 @@  int ref_transaction_abort(struct ref_transaction *transaction,
 		break;
 	}
 
+	run_transaction_hook(transaction, "ref-transaction-aborted");
+
 	ref_transaction_free(transaction);
 	return ret;
 }
@@ -2064,7 +2124,10 @@  int ref_transaction_commit(struct ref_transaction *transaction,
 		break;
 	}
 
-	return refs->be->transaction_finish(refs, transaction, err);
+	ret = refs->be->transaction_finish(refs, transaction, err);
+	if (!ret)
+		run_transaction_hook(transaction, "ref-transaction-committed");
+	return ret;
 }
 
 int refs_verify_refname_available(struct ref_store *refs,
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
new file mode 100755
index 0000000000..b6df5fc883
--- /dev/null
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -0,0 +1,88 @@ 
+#!/bin/sh
+
+test_description='reference transaction hooks'
+
+. ./test-lib.sh
+
+create_commit ()
+{
+	test_tick &&
+	T=$(git write-tree) &&
+	sha1=$(echo message | git commit-tree $T) &&
+	echo $sha1
+}
+
+test_expect_success setup '
+	mkdir -p .git/hooks
+'
+
+test_expect_success 'prepared hook allows updating ref' '
+	test_when_finished "rm .git/hooks/ref-transaction-prepared" &&
+	write_script .git/hooks/ref-transaction-prepared <<-\EOF &&
+		exit 0
+	EOF
+	C=$(create_commit) &&
+	git update-ref HEAD $C
+'
+
+test_expect_success 'prepared hook aborts updating ref' '
+	test_when_finished "rm .git/hooks/ref-transaction-prepared" &&
+	write_script .git/hooks/ref-transaction-prepared <<-\EOF &&
+		exit 1
+	EOF
+	C=$(create_commit) &&
+	test_must_fail git update-ref HEAD $C 2>err &&
+	grep "ref updates aborted by hook" err
+'
+
+test_expect_success 'prepared hook gets all queued updates' '
+	test_when_finished "rm .git/hooks/ref-transaction-prepared actual" &&
+	write_script .git/hooks/ref-transaction-prepared <<-\EOF &&
+		while read -r line; do printf "%s\n" "$line"; done >actual
+	EOF
+	C=$(create_commit) &&
+	cat >expect <<-EOF &&
+		$ZERO_OID $C HEAD
+		$ZERO_OID $C refs/heads/master
+	EOF
+	git update-ref HEAD $C <<-EOF &&
+		update HEAD $ZERO_OID $C
+		update refs/heads/master $ZERO_OID $C
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'committed hook gets all queued updates' '
+	test_when_finished "rm .git/hooks/ref-transaction-committed actual" &&
+	write_script .git/hooks/ref-transaction-committed <<-\EOF &&
+		while read -r line; do printf "%s\n" "$line"; done >actual
+	EOF
+	C=$(create_commit) &&
+	cat >expect <<-EOF &&
+		$ZERO_OID $C HEAD
+		$ZERO_OID $C refs/heads/master
+	EOF
+	git update-ref HEAD $C &&
+	test_cmp expect actual
+'
+
+test_expect_success 'aborted hook gets all queued updates' '
+	test_when_finished "rm .git/hooks/ref-transaction-aborted actual" &&
+	write_script .git/hooks/ref-transaction-aborted <<-\EOF &&
+		while read -r line; do printf "%s\n" "$line"; done >actual
+	EOF
+	C=$(create_commit) &&
+	cat >expect <<-EOF &&
+		$ZERO_OID $C HEAD
+		$ZERO_OID $C refs/heads/master
+	EOF
+	git update-ref --stdin <<-EOF &&
+		start
+		update HEAD $C $ZERO_OID
+		update refs/heads/master $C $ZERO_OID
+		abort
+	EOF
+	test_cmp expect actual
+'
+
+test_done