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

Re: [PATCH v4] refs: implement reference transaction hook

From
Jeff King <peff@peff.net>
Date
Oct 23, 2020, 01:03 UTC
Message-ID
<20201023010315.GA1542721@coredump.intra.peff.net>
In-Reply-To
<55c58e9b09691dc11dea911f26424594fb3922c9.1592549570.git.ps@pks.im>
On Fri, Jun 19, 2020 at 08:56:14AM +0200, Patrick Steinhardt wrote:
Show 8 quoted lines
> The above scenario is the motivation for the new "reference-transaction"
> hook that reaches directly into Git's reference transaction mechanism.
> The hook receives as parameter the current state the transaction was
> moved to ("prepared", "committed" or "aborted") and gets via its
> standard input all queued reference updates. While the exit code gets
> ignored in the "committed" and "aborted" states, a non-zero exit code in
> the "prepared" state will cause the transaction to be aborted
> prematurely.

I know this has been merged for a while, but Taylor and I were looking at it today and came across a question. The docs say:

> +For each reference update that was added to the transaction, the hook
> +receives on standard input a line of the format:
> +
> +  <old-value> SP <new-value> SP <ref-name> LF

but that doesn't define <old-value>. I take it to mean the current value of the ref before the proposed update. But in the tests:

Show 22 quoted lines
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> new file mode 100755
> index 0000000000..da58d867a5
> --- /dev/null
> +++ b/t/t1416-ref-transaction-hooks.sh
> [...]
> +test_expect_success 'hook gets all queued updates in prepared state' '
> +	test_when_finished "rm .git/hooks/reference-transaction actual" &&
> +	git reset --hard PRE &&
> +	write_script .git/hooks/reference-transaction <<-\EOF &&
> +		if test "$1" = prepared
> +		then
> +			while read -r line
> +			do
> +				printf "%s\n" "$line"
> +			done >actual
> +		fi
> +	EOF
> +	cat >expect <<-EOF &&
> +		$ZERO_OID $POST_OID HEAD
> +		$ZERO_OID $POST_OID refs/heads/master
> +	EOF

We are expecting to see $ZERO_OID in that slot, even though the current value of the ref is "PRE" due to our reset above.

Should this be $PRE_OID (we don't have that variable, but it would be the result of "git rev-parse PRE")?

I could alternatively see an argument that <old-value> is the old-value that the caller asked for. So seeing $ZERO_OID is saying that the caller wants to move from _anything_ to $POST_OID, and we're not willing to tell the hook what the current value actually is.

We could actually fill in the current value for zero cost. The reason we found this is that we have a custom patch at GitHub that fills in these values when we open the ref after locking.

In real usage, I'm not sure how much the distinction would matter, because any careful caller would provide a non-zero "old" value. And if that doesn't match the current value, we'd reject the transaction before we even hit the hook, I think. It's only the fact that the update-ref calls are sloppy and do not provide an expected old value that it even matters.

So I wonder if:
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index f6e741c6c0..8155522a1a 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -9,6 +9,7 @@ test_expect_success setup '
 	test_commit PRE &&
 	PRE_OID=$(git rev-parse PRE) &&
 	test_commit POST &&
+	PRE_OID=$(git rev-parse PRE) &&
 	POST_OID=$(git rev-parse POST)
 '
 
@@ -52,10 +53,10 @@ 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 &&
+	git update-ref HEAD POST POST <<-EOF &&
 		update HEAD $ZERO_OID $POST_OID
 		update refs/heads/master $ZERO_OID $POST_OID
 	EOF
@@ -75,10 +76,10 @@ 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 &&
+	git update-ref HEAD POST PRE &&
 	test_cmp expect actual
 '
 

would be a step forward. This isn't changing the actual behavior,
obviously. It's just tweaking the test so that it tests the more likely
real-world case.  But we'd possibly want to actually change the behavior
to always send the actual ref value to the hook.

-Peff
Previous: Patrick SteinhardtNext: Junio C Hamano
Message 18 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.