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

Re: [PATCH v3 3/6] refs: allow skipping the reference-transaction hook

From
Patrick Steinhardt <ps@pks.im>
Date
Jan 17, 2022, 08:03 UTC
Message-ID
<YeUi77PE6MBwTBVQ@ncase>
In-Reply-To
<220113.86zgnzu48q.gmgdl@evledraar.gmail.com>
On Thu, Jan 13, 2022 at 02:34:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 54 quoted lines
> 
> On Thu, Jan 13 2022, Patrick Steinhardt wrote:
> 
> > [[PGP Signed Part:Undecided]]
> > The reference-transaction hook is executing whenever we prepare, commit
> > or abort a reference transaction. While this is mostly intentional, in
> > case of the files backend we're leaking the implementation detail that
> > the store is in fact a composite store with one loose and one packed
> > backend to the caller. So while we want to execute the hook for all
> > logical updates, executing it for such implementation details is
> > unexpected.
> >
> > Prepare for a fix by adding a new flag which allows to skip execution of
> > the hook.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs.c | 3 +++
> >  refs.h | 5 +++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/refs.c b/refs.c
> > index 7415864b62..526bf5ed97 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -2084,6 +2084,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,
> >  	const char *hook;
> >  	int ret = 0, i;
> >  
> > +	if (transaction->flags & REF_TRANSACTION_SKIP_HOOK)
> > +		return 0;
> > +
> >  	hook = find_hook("reference-transaction");
> >  	if (!hook)
> >  		return ret;
> > diff --git a/refs.h b/refs.h
> > index 31f7bf9642..d4056f9fe2 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -568,6 +568,11 @@ enum action_on_err {
> >  	UPDATE_REFS_QUIET_ON_ERR
> >  };
> >  
> > +/*
> > + * Skip executing the reference-transaction hook.
> > + */
> > +#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
> > +
> >  /*
> >   * Begin a reference transaction.  The reference transaction must
> >   * be freed by calling ref_transaction_free().
> 
> This isn't needed in refs.h, so let's put it in refs-internal.h where
> e.g. "enum ref_transaction_state" now lives:

On the other hand we got the flags parameter in the function just two lines later. A reader would thus wonder "which flags?!" if we didn't carry the definition of flags nearby and would have to go search the codebase for the set of supported flags.

Show 49 quoted lines
>     diff --git a/refs.h b/refs.h
>     index d4056f9fe26..31f7bf96424 100644
>     --- a/refs.h
>     +++ b/refs.h
>     @@ -568,11 +568,6 @@ enum action_on_err {
>             UPDATE_REFS_QUIET_ON_ERR
>      };
>      
>     -/*
>     - * Skip executing the reference-transaction hook.
>     - */
>     -#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
>     -
>      /*
>       * Begin a reference transaction.  The reference transaction must
>       * be freed by calling ref_transaction_free().
>     diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>     index a0af63f162f..87da39243f7 100644
>     --- a/refs/refs-internal.h
>     +++ b/refs/refs-internal.h
>     @@ -201,6 +201,11 @@ enum ref_transaction_state {
>             REF_TRANSACTION_CLOSED   = 2
>      };
>      
>     +/*
>     + * Skip executing the reference-transaction hook.
>     + */
>     +#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
>     +
>      /*
>       * Data structure for holding a reference transaction, which can
>       * consist of checks and updates to multiple references, carried out
> 
> A bit more odd is that this series ends up with a
> ref_transaction_begin() that doesn't correspond to its ref_store_*()
> parent, i.e. the others pass the ref store for you, but now we omit the
> flags too.
> 
> I see why you did that, to avoid tweaking every existing
> ref_transaction_begin() caller.
> 
> But isn't something like the below a better approach? We can introduce a
> refs-internal.h-only flag enum, and then just have a new
> ref_store_transaction_begin_no_hook() called from these new
> files-backend.c users.
> 
> The diff on top is a bit verbose, but the exposed API is cleaner
> (presumably no "public" user should be allowed to skip the hook), and
> the overall diff if this is squashed in is smaller.

While the diff does look sensible it makes me wonder how maintainable it is in the future. If we ever were to accept another flag then we might have to duplicate your `ref_store_transaction_begin_no_hook()` to also carry flags, or add `ref_store_transaciton_begin_another_option()` to stay consistent.

So for now I tend to prefer the current version given that it feels more extensible going forward. But I'm happy to change it if others agree with you.

Patrick
Show 185 quoted lines
> diff --git a/refs.c b/refs.c
> index 526bf5ed97a..ffaf34e9710 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -800,7 +800,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
>  	struct ref_transaction *transaction;
>  	struct strbuf err = STRBUF_INIT;
>  
> -	transaction = ref_store_transaction_begin(refs, 0, &err);
> +	transaction = ref_store_transaction_begin(refs, &err);
>  	if (!transaction ||
>  	    ref_transaction_delete(transaction, refname, old_oid,
>  				   flags, msg, &err) ||
> @@ -1004,22 +1004,36 @@ int read_ref_at(struct ref_store *refs, const char *refname,
>  	return 1;
>  }
>  
> -struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
> -						    unsigned int flags,
> -						    struct strbuf *err)
> +static struct ref_transaction *ref_store_transaction_begin_1(struct ref_store *refs,
> +							     unsigned int no_hook,
> +							     struct strbuf *err)
>  {
>  	struct ref_transaction *tr;
>  	assert(err);
>  
>  	CALLOC_ARRAY(tr, 1);
>  	tr->ref_store = refs;
> -	tr->flags = flags;
> +	if (no_hook)
> +		tr->flags &= REF_TRANSACTION_SKIP_HOOK;
>  	return tr;
>  }
>  
>  struct ref_transaction *ref_transaction_begin(struct strbuf *err)
>  {
> -	return ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);
> +	return ref_store_transaction_begin_1(get_main_ref_store(the_repository),
> +					     0, err);
> +}
> +
> +struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
> +						    struct strbuf *err)
> +{
> +	return ref_store_transaction_begin_1(refs, 0, err);
> +}
> +
> +struct ref_transaction *ref_store_transaction_begin_no_hook(struct ref_store *refs,
> +							    struct strbuf *err)
> +{
> +	return ref_store_transaction_begin_1(refs, 1, err);
>  }
>  
>  void ref_transaction_free(struct ref_transaction *transaction)
> @@ -1158,7 +1172,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
>  	struct strbuf err = STRBUF_INIT;
>  	int ret = 0;
>  
> -	t = ref_store_transaction_begin(refs, 0, &err);
> +	t = ref_store_transaction_begin(refs, &err);
>  	if (!t ||
>  	    ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,
>  				   &err) ||
> diff --git a/refs.h b/refs.h
> index 31f7bf96424..3663e0347ff 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -573,8 +573,9 @@ enum action_on_err {
>   * be freed by calling ref_transaction_free().
>   */
>  struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
> -						    unsigned int flags,
>  						    struct strbuf *err);
> +struct ref_transaction *ref_store_transaction_begin_no_hook(struct ref_store *refs,
> +							    struct strbuf *err);
>  struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>  
>  /*
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9a20cb8fa89..f85c8f3a692 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1121,8 +1121,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
>  	if (check_refname_format(r->name, 0))
>  		return;
>  
> -	transaction = ref_store_transaction_begin(&refs->base,
> -						  REF_TRANSACTION_SKIP_HOOK, &err);
> +	transaction = ref_store_transaction_begin_no_hook(&refs->base, &err);
>  	if (!transaction)
>  		goto cleanup;
>  	ref_transaction_add_update(
> @@ -1193,8 +1192,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
>  	struct strbuf err = STRBUF_INIT;
>  	struct ref_transaction *transaction;
>  
> -	transaction = ref_store_transaction_begin(refs->packed_ref_store,
> -						  REF_TRANSACTION_SKIP_HOOK, &err);
> +	transaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, &err);
>  	if (!transaction)
>  		return -1;
>  
> @@ -1261,8 +1259,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
>  	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
>  		goto error;
>  
> -	transaction = ref_store_transaction_begin(refs->packed_ref_store,
> -						  REF_TRANSACTION_SKIP_HOOK, &err);
> +	transaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, &err);
>  	if (!transaction)
>  		goto error;
>  
> @@ -2775,9 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  			 * packed-refs if it exists there.
>  			 */
>  			if (!packed_transaction) {
> -				packed_transaction = ref_store_transaction_begin(
> -						refs->packed_ref_store,
> -						REF_TRANSACTION_SKIP_HOOK, err);
> +				packed_transaction = ref_store_transaction_begin_no_hook(
> +						refs->packed_ref_store, err);
>  				if (!packed_transaction) {
>  					ret = TRANSACTION_GENERIC_ERROR;
>  					goto cleanup;
> @@ -3048,8 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
>  				 &affected_refnames))
>  		BUG("initial ref transaction called with existing refs");
>  
> -	packed_transaction = ref_store_transaction_begin(refs->packed_ref_store,
> -							 REF_TRANSACTION_SKIP_HOOK, err);
> +	packed_transaction = ref_store_transaction_begin_no_hook(refs->packed_ref_store, err);
>  	if (!packed_transaction) {
>  		ret = TRANSACTION_GENERIC_ERROR;
>  		goto cleanup;
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index cfebcd0b46e..e97d67f9321 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1535,7 +1535,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
>  	 * updates into a single transaction.
>  	 */
>  
> -	transaction = ref_store_transaction_begin(ref_store, 0, &err);
> +	transaction = ref_store_transaction_begin(ref_store, &err);
>  	if (!transaction)
>  		return -1;
>  
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 87da39243f7..8ff9e0e6e3a 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -204,7 +204,9 @@ enum ref_transaction_state {
>  /*
>   * Skip executing the reference-transaction hook.
>   */
> -#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
> +enum ref_transaction_flags {
> +	REF_TRANSACTION_SKIP_HOOK = 1 << 0,
> +};
>  
>  /*
>   * Data structure for holding a reference transaction, which can
> @@ -218,7 +220,7 @@ struct ref_transaction {
>  	size_t nr;
>  	enum ref_transaction_state state;
>  	void *backend_data;
> -	unsigned int flags;
> +	enum ref_transaction_flags flags;
>  };
>  
>  /*
> diff --git a/sequencer.c b/sequencer.c
> index 61edd39d7a4..6abd72160cc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3588,7 +3588,7 @@ static int do_label(struct repository *r, const char *name, int len)
>  	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>  	strbuf_addf(&msg, "rebase (label) '%.*s'", len, name);
>  
> -	transaction = ref_store_transaction_begin(refs, 0, &err);
> +	transaction = ref_store_transaction_begin(refs, &err);
>  	if (!transaction) {
>  		error("%s", err.buf);
>  		ret = -1;
Previous: Ævar Arnfjörð BjarmasonNext: Patrick Steinhardt
Message 37 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.