git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] refs: implement reference transaction hooks

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 2, 2020, 17:47 UTC
Message-ID
<xmqq4krttl4k.fsf@gitster.c.googlers.com>
In-Reply-To
<1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im>
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?

Show 15 quoted lines
> +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.

Show 21 quoted lines
> +	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...
Show 7 quoted lines
> +	}
> +
> +	close(proc.in);
> +	sigchain_pop(SIGPIPE);
> +
> +	strbuf_release(&buf);
> +	return finish_command(&proc);
... but the caller does not get notified.  Intended?
Show 20 quoted lines
> +}
> +
>  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?
Show 14 quoted lines
> +	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'.

Show 29 quoted lines
>  	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...

Show 12 quoted lines
> +}
> +
> +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.

Show 11 quoted lines
> +	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.

Show 6 quoted lines
> +'
> +
> +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)?
Show 11 quoted lines
> +	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 ;-).
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 2 of 26 in “refs: implement reference transaction hooks”
  1. refs: implement reference transaction hooksPatrick Steinhardt, Jun 2, 2020
  2. Junio C HamanoJun 2, 2020
  3. Patrick SteinhardtJun 3, 2020
  4. SZEDER GáborJun 7, 2020
  5. Patrick SteinhardtJun 8, 2020
  6. SZEDER GáborJun 2, 2020
  7. Patrick SteinhardtJun 3, 2020
  8. refs: implement reference transaction hookPatrick Steinhardt, Jun 3, 2020
  9. Taylor BlauJun 3, 2020
  10. Patrick SteinhardtJun 4, 2020
  11. Taylor BlauJun 15, 2020
  12. Patrick SteinhardtJun 16, 2020
  13. Han-Wen NienhuysJun 3, 2020
  14. Junio C HamanoJun 3, 2020
  15. refs: implement reference transaction hookPatrick Steinhardt, Jun 18, 2020
  16. Junio C HamanoJun 18, 2020
  17. refs: implement reference transaction hookPatrick Steinhardt, Jun 19, 2020
  18. Jeff KingOct 23, 2020
  19. Junio C HamanoOct 23, 2020
  20. Taylor BlauOct 23, 2020
  21. Taylor BlauOct 23, 2020
  22. Patrick SteinhardtOct 26, 2020
  23. Taylor BlauOct 26, 2020
  24. Jeff KingOct 27, 2020
  25. Patrick SteinhardtOct 28, 2020
  26. Taylor BlauOct 23, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.