Re: [PATCH v2 0/6] refs: excessive hook execution with packed refs

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 12498 bytes --]

On Mon, Jan 17, 2022 at 12:37:39PM +0100, Han-Wen Nienhuys wrote:
> On Mon, Jan 17, 2022 at 8:18 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > It might make sense to not introduce a new flag namespace, but use
> > > update->flags instead. You'd have to add your new flag after
> > > REF_SKIP_REFNAME_VERIFICATION.
> > > Bonus is that you can unittest the new flag using the existing
> > > ref-store helper without extra work. (check that a transaction with &
> > > without the flag works as expected.)
> > >
> > > other than that, looks OK to me.
> >
> > Thanks for your feedback!
> >
> > In fact the first version I had locally did exactly that. But I found
> > the end version result harder to reason about, most importantly because
> > it's not a 100% clear to the reader whether all callsites which delete
> > refs in the packed-backend via the files-backend are adapted or whether
> > any of the callsites was missing. By having the flag in a central place
> > it's immediately clear that the hook won't be run at all, which is
> > exactly what we want here.
> 
> If you do it like this, can you find the callsites that take update
> flags (but not transaction flags), and update the signature to say
> "update_flags" rather than "flags", and document appropriately?

All functions which accept "flags" document that they pass the parameter
directly to `ref_transaction_{update,create,delete,verify}` already. And
in the context of those latter functions it cannot possibly be related
to the flags passed to `refs_store_transaction_begin()` because they
already get a transaction as parameter and thus don't have to create
a new one.

So we could apply below patch, and I'm happy to polish and add it to the
series. But it feels to me like it adds churn while only stating things
that are documented already. It could only be me though given that I'm
obviously biased, so please feel free to disagree.

Patrick

-- >8 --

From 91c77a69b6326b1b6cc743ebba6fb0970f0d01c2 Mon Sep 17 00:00:00 2001
Message-Id: <91c77a69b6326b1b6cc743ebba6fb0970f0d01c2.1643008212.git.ps@pks.im>
From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 24 Jan 2022 08:09:45 +0100
Subject: [PATCH] refs: rename generic `flags` field to `update_flags` for
 clarity

---
 refs.c | 50 +++++++++++++++++++++++++-------------------------
 refs.h | 20 ++++++++++----------
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/refs.c b/refs.c
index 526bf5ed97..c15e8bd58d 100644
--- a/refs.c
+++ b/refs.c
@@ -795,7 +795,7 @@ long get_files_ref_lock_timeout_ms(void)
 int refs_delete_ref(struct ref_store *refs, const char *msg,
 		    const char *refname,
 		    const struct object_id *old_oid,
-		    unsigned int flags)
+		    unsigned int update_flags)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
@@ -803,7 +803,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
 	transaction = ref_store_transaction_begin(refs, 0, &err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_oid,
-				   flags, msg, &err) ||
+				   update_flags, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
@@ -816,10 +816,10 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
 }
 
 int delete_ref(const char *msg, const char *refname,
-	       const struct object_id *old_oid, unsigned int flags)
+	       const struct object_id *old_oid, unsigned int update_flags)
 {
 	return refs_delete_ref(get_main_ref_store(the_repository), msg, refname,
-			       old_oid, flags);
+			       old_oid, update_flags);
 }
 
 static void copy_reflog_msg(struct strbuf *sb, const char *msg)
@@ -1080,12 +1080,12 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int update_flags, const char *msg,
 			   struct strbuf *err)
 {
 	assert(err);
 
-	if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
+	if (!(update_flags & REF_SKIP_REFNAME_VERIFICATION) &&
 	    ((new_oid && !is_null_oid(new_oid)) ?
 		     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
 			   !refname_is_safe(refname))) {
@@ -1094,19 +1094,19 @@ int ref_transaction_update(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
-		BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
+	if (update_flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
+		BUG("illegal flags 0x%x passed to ref_transaction_update()", update_flags);
 
 	/*
 	 * Clear flags outside the allowed set; this should be a noop because
 	 * of the BUG() check above, but it works around a -Wnonnull warning
 	 * with some versions of "gcc -O3".
 	 */
-	flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
+	update_flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
 
-	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
+	update_flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
-	ref_transaction_add_update(transaction, refname, flags,
+	ref_transaction_add_update(transaction, refname, update_flags,
 				   new_oid, old_oid, msg);
 	return 0;
 }
@@ -1114,44 +1114,44 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int update_flags, const char *msg,
 			   struct strbuf *err)
 {
 	if (!new_oid || is_null_oid(new_oid))
 		BUG("create called without valid new_oid");
 	return ref_transaction_update(transaction, refname, new_oid,
-				      null_oid(), flags, msg, err);
+				      null_oid(), update_flags, msg, err);
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int update_flags, const char *msg,
 			   struct strbuf *err)
 {
 	if (old_oid && is_null_oid(old_oid))
 		BUG("delete called with old_oid set to zeros");
 	return ref_transaction_update(transaction, refname,
 				      null_oid(), old_oid,
-				      flags, msg, err);
+				      update_flags, msg, err);
 }
 
 int ref_transaction_verify(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags,
+			   unsigned int update_flags,
 			   struct strbuf *err)
 {
 	if (!old_oid)
 		BUG("verify called with old_oid set to NULL");
 	return ref_transaction_update(transaction, refname,
 				      NULL, old_oid,
-				      flags, NULL, err);
+				      update_flags, NULL, err);
 }
 
 int refs_update_ref(struct ref_store *refs, const char *msg,
 		    const char *refname, const struct object_id *new_oid,
-		    const struct object_id *old_oid, unsigned int flags,
+		    const struct object_id *old_oid, unsigned int update_flags,
 		    enum action_on_err onerr)
 {
 	struct ref_transaction *t = NULL;
@@ -1160,7 +1160,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
 
 	t = ref_store_transaction_begin(refs, 0, &err);
 	if (!t ||
-	    ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,
+	    ref_transaction_update(t, refname, new_oid, old_oid, update_flags, msg,
 				   &err) ||
 	    ref_transaction_commit(t, &err)) {
 		ret = 1;
@@ -1191,10 +1191,10 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
 int update_ref(const char *msg, const char *refname,
 	       const struct object_id *new_oid,
 	       const struct object_id *old_oid,
-	       unsigned int flags, enum action_on_err onerr)
+	       unsigned int update_flags, enum action_on_err onerr)
 {
 	return refs_update_ref(get_main_ref_store(the_repository), msg, refname, new_oid,
-			       old_oid, flags, onerr);
+			       old_oid, update_flags, onerr);
 }
 
 char *refs_shorten_unambiguous_ref(struct ref_store *refs,
@@ -2435,21 +2435,21 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 }
 
 int refs_delete_refs(struct ref_store *refs, const char *logmsg,
-		     struct string_list *refnames, unsigned int flags)
+		     struct string_list *refnames, unsigned int update_flags)
 {
 	char *msg;
 	int retval;
 
 	msg = normalize_reflog_message(logmsg);
-	retval = refs->be->delete_refs(refs, msg, refnames, flags);
+	retval = refs->be->delete_refs(refs, msg, refnames, update_flags);
 	free(msg);
 	return retval;
 }
 
 int delete_refs(const char *msg, struct string_list *refnames,
-		unsigned int flags)
+		unsigned int update_flags)
 {
-	return refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, flags);
+	return refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, update_flags);
 }
 
 int refs_rename_ref(struct ref_store *refs, const char *oldref,
diff --git a/refs.h b/refs.h
index d4056f9fe2..5131db7e48 100644
--- a/refs.h
+++ b/refs.h
@@ -442,9 +442,9 @@ int reflog_exists(const char *refname);
 int refs_delete_ref(struct ref_store *refs, const char *msg,
 		    const char *refname,
 		    const struct object_id *old_oid,
-		    unsigned int flags);
+		    unsigned int update_flags);
 int delete_ref(const char *msg, const char *refname,
-	       const struct object_id *old_oid, unsigned int flags);
+	       const struct object_id *old_oid, unsigned int update_flags);
 
 /*
  * Delete the specified references. If there are any problems, emit
@@ -453,9 +453,9 @@ int delete_ref(const char *msg, const char *refname,
  * ref_transaction_delete().
  */
 int refs_delete_refs(struct ref_store *refs, const char *msg,
-		     struct string_list *refnames, unsigned int flags);
+		     struct string_list *refnames, unsigned int update_flags);
 int delete_refs(const char *msg, struct string_list *refnames,
-		unsigned int flags);
+		unsigned int update_flags);
 
 /** Delete a reflog */
 int refs_delete_reflog(struct ref_store *refs, const char *refname);
@@ -680,7 +680,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int update_flags, const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -695,7 +695,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int update_flags, const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -709,7 +709,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags, const char *msg,
+			   unsigned int update_flags, const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -723,7 +723,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 int ref_transaction_verify(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *old_oid,
-			   unsigned int flags,
+			   unsigned int update_flags,
 			   struct strbuf *err);
 
 /* Naming conflict (for example, the ref names A and A/B conflict). */
@@ -796,10 +796,10 @@ void ref_transaction_free(struct ref_transaction *transaction);
  */
 int refs_update_ref(struct ref_store *refs, const char *msg, const char *refname,
 		    const struct object_id *new_oid, const struct object_id *old_oid,
-		    unsigned int flags, enum action_on_err onerr);
+		    unsigned int update_flags, enum action_on_err onerr);
 int update_ref(const char *msg, const char *refname,
 	       const struct object_id *new_oid, const struct object_id *old_oid,
-	       unsigned int flags, enum action_on_err onerr);
+	       unsigned int update_flags, enum action_on_err onerr);
 
 int parse_hide_refs_config(const char *var, const char *value, const char *);
 
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]