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

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

From
Patrick Steinhardt <ps@pks.im>
Date
Jan 24, 2022, 07:13 UTC
Message-ID
<Ye5Rgx71QSpkIgT7@ncase>
In-Reply-To
<CAFQ2z_M+2D-5qPXN=0FEveZ36Yq0rps3HxRkgCmiupq1DD7Kig@mail.gmail.com>
On Mon, Jan 17, 2022 at 12:37:39PM +0100, Han-Wen Nienhuys wrote:
Show 23 quoted lines
> 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
Previous: Han-Wen NienhuysNext: Patrick Steinhardt
Message 29 of 51 in “refs: excessive hook execution with packed refs”
  1. 0/6 refs: excessive hook execution with packed refsPatrick Steinhardt, Dec 7, 2021
  2. 1/6 refs: open-code deletion of packed refsPatrick Steinhardt, Dec 7, 2021
  3. 2/6 refs: allow passing flags when beginning transactionsPatrick Steinhardt, Dec 7, 2021
  4. 3/6 refs: allow skipping the reference-transaction hookPatrick Steinhardt, Dec 7, 2021
  5. 4/6 refs: demonstrate excessive execution of the reference-transaction hookPatrick Steinhardt, Dec 7, 2021
  6. 5/6 refs: do not execute reference-transaction hook on packing refsPatrick Steinhardt, Dec 7, 2021
  7. 6/6 refs: skip hooks when deleting uncovered packed refsPatrick Steinhardt, Dec 7, 2021
  8. 0/6 refs: excessive hook execution with packed refsPatrick Steinhardt, Jan 7, 2022
  9. 1/6 refs: open-code deletion of packed refsPatrick Steinhardt, Jan 7, 2022
  10. Junio C HamanoJan 8, 2022
  11. Junio C HamanoJan 8, 2022
  12. Junio C HamanoJan 13, 2022
  13. 2/6 refs: allow passing flags when beginning transactionsPatrick Steinhardt, Jan 7, 2022
  14. Junio C HamanoJan 8, 2022
  15. 3/6 refs: allow skipping the reference-transaction hookPatrick Steinhardt, Jan 7, 2022
  16. 4/6 refs: demonstrate excessive execution of the reference-transaction hookPatrick Steinhardt, Jan 7, 2022
  17. Junio C HamanoJan 8, 2022
  18. Patrick SteinhardtJan 10, 2022
  19. Eric SunshineJan 8, 2022
  20. 6/6 refs: skip hooks when deleting uncovered packed refsPatrick Steinhardt, Jan 7, 2022
  21. Junio C HamanoJan 8, 2022
  22. Patrick SteinhardtJan 10, 2022
  23. Junio C HamanoJan 10, 2022
  24. 5/6 refs: do not execute reference-transaction hook on packing refsPatrick Steinhardt, Jan 7, 2022
  25. Junio C HamanoJan 7, 2022
  26. Han-Wen NienhuysJan 13, 2022
  27. Patrick SteinhardtJan 17, 2022
  28. Han-Wen NienhuysJan 17, 2022
  29. Patrick SteinhardtJan 24, 2022
  30. 0/6 refs: excessive hook execution with packed refsPatrick Steinhardt, Jan 13, 2022
  31. 1/6 refs: extract packed_refs_delete_refs() to allow control of transactionPatrick Steinhardt, Jan 13, 2022
  32. Ævar Arnfjörð BjarmasonJan 13, 2022
  33. Patrick SteinhardtJan 17, 2022
  34. 2/6 refs: allow passing flags when beginning transactionsPatrick Steinhardt, Jan 13, 2022
  35. 3/6 refs: allow skipping the reference-transaction hookPatrick Steinhardt, Jan 13, 2022
  36. Ævar Arnfjörð BjarmasonJan 13, 2022
  37. Patrick SteinhardtJan 17, 2022
  38. 4/6 refs: demonstrate excessive execution of the reference-transaction hookPatrick Steinhardt, Jan 13, 2022
  39. 5/6 refs: do not execute reference-transaction hook on packing refsPatrick Steinhardt, Jan 13, 2022
  40. Ævar Arnfjörð BjarmasonJan 13, 2022
  41. Patrick SteinhardtJan 17, 2022
  42. 6/6 refs: skip hooks when deleting uncovered packed refsPatrick Steinhardt, Jan 13, 2022
  43. Ævar Arnfjörð BjarmasonJan 13, 2022
  44. Patrick SteinhardtJan 17, 2022
  45. 0/6 refs: excessive hook execution with packed refsPatrick Steinhardt, Jan 17, 2022
  46. 1/6 refs: extract packed_refs_delete_refs() to allow control of transactionPatrick Steinhardt, Jan 17, 2022
  47. 2/6 refs: allow passing flags when beginning transactionsPatrick Steinhardt, Jan 17, 2022
  48. 3/6 refs: allow skipping the reference-transaction hookPatrick Steinhardt, Jan 17, 2022
  49. 4/6 refs: demonstrate excessive execution of the reference-transaction hookPatrick Steinhardt, Jan 17, 2022
  50. 5/6 refs: do not execute reference-transaction hook on packing refsPatrick Steinhardt, Jan 17, 2022
  51. 6/6 refs: skip hooks when deleting uncovered packed refsPatrick Steinhardt, Jan 17, 2022

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.