Re: [PATCH] refs: implement reference transaction hooks

[Junio C Hamano <gitster@pobox.com>]

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