On Thu, Jan 13, 2022 at 02:34:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> 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.

>     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

> 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;