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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 23, 2020, 19:57 UTC
Message-ID: <X5M1oe4lfkUy9lAh@nand.local>
In-Reply-To: <xmqqpn59k2xw.fsf@gitster.c.googlers.com>

On Thu, Oct 22, 2020 at 08:59:23PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> [...]
> I think that makes a lot of sense.  In addition,
>
> > ...  But we'd possibly want to actually change the behavior
> > to always send the actual ref value to the hook.
>
> I offhand do not see a reason why we shouldn't do that.

I can't see a reason either, but teaching the new ref transaction hook about these updates gets a little tricky. In particular, we update the old_oid for ref updates that were split off with something like:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 04e85e7002..9c105a376b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2484,9 +2484,20 @@ 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);
+			/*
+			 * Now the parent knows its old OID too;
+			 * record that fact for logging.
+			 */
+			parent_update->flags |= REF_HAVE_OLD;
 		}
 	}

+	/* Now we know the old value. Record that fact for logging. */
+	if (!(update->flags & REF_HAVE_OLD)) {
+		oidcpy(&update->old_oid, &lock->old_oid);
+		update->flags |= REF_HAVE_OLD;
+	}
+
 	if ((update->flags & REF_HAVE_NEW) &&
 	    !(update->flags & REF_DELETING) &&
 	    !(update->flags & REF_LOG_ONLY)) {

...but by that time we have already recorded via an oidcpy the old and
new state of HEAD. So, Peff's patch passes in isolation, but applying
this on top produces failures in t1416 like:

--- expect	2020-10-23 19:56:15.649101024 +0000
+++ actual	2020-10-23 19:56:15.657100941 +0000
@@ -1,2 +1,2 @@
-63ac8e7bcdb882293465435909f54a96de17d4f7 99d53161c3a0a903b6561b9f6c0c665b3a476401 HEAD
+0000000000000000000000000000000000000000 99d53161c3a0a903b6561b9f6c0c665b3a476401 HEAD
 63ac8e7bcdb882293465435909f54a96de17d4f7 99d53161c3a0a903b6561b9f6c0c665b3a476401 refs/heads/master

It should be possible to keep track of the old and new OIDs via a
non-copied pointer, but I have to untangle this code a little bit more
before I can be sure.

> Thanks for carefully thinking things through.

Thanks,
Taylor