Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data

From: Jeff King <peff@peff.net>
Date: Sep 8, 2017, 07:02 UTC
Message-ID: <20170908070205.3hwa7tjgok7kgknc@sigill.intra.peff.net>
In-Reply-To: <5b8c8f9ab0ccd5f85eb43a7a1535af89b0af85e3.1503993268.git.mhagger@alum.mit.edu>

On Tue, Aug 29, 2017 at 10:20:26AM +0200, Michael Haggerty wrote:

> `packed_ref_store` is going to want to store some transaction-wide
> data, so make a place for it.

That makes sense, although...

Show 10 quoted lines

> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index b02dc5a7e3..d7d344de73 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -242,6 +242,7 @@ struct ref_transaction {
>  	size_t alloc;
>  	size_t nr;
>  	enum ref_transaction_state state;
> +	void *backend_data;
>  };

This is just one pointer. Once we start layering ref backends (and already we're moving towards a "files" layer which sits atop loose and packed backends, right?), how do we avoid backends stomping on each other (or worse, dereferencing somebody else's data as their own struct)?

I don't know that we necessarily need to answer that question right now, but I'm worried that this pattern might need adjustment eventually.

I guess the hand-wavy answer is that whatever is doing the layering would need to manage the pointers. So if you imagine that we had a "union" backend that took two other arbitrary backends, it would probably have something like:

  struct union_backend_data {
	void *data_a;
	void *data_b;
  }

and when it forwarded calls to the separate backends, it would give them a view of the transaction with only their data. Something like:

  void union_backend_foo(void *be, struct ref_transaction *transaction)
  {
	struct union_backend *me = be;
	struct union_backend_data *my_data = transaction->backend_data;

        /* "a" sees only it's data, and we remember any modifications */
	transaction->backend_data = my_data->data_a;
	me->backend_a->foo(me->backend_a, transaction);
	my_data->data_a = transaction->backend_data;

        /* ditto for "b" */
	transaction->backend_data = my_data->data_b;
	me->backend_b->foo(me->backend_b, transaction);
	my_data->data_b = transaction->backend_data;

	/* and then we restore our own view */
	transaction->backend_data = my_data;
  }

-Peff