From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 05 Oct 2015 08:37:59 GMT
Subject: Re: [PATCH v2 28/43] refs.c: add ref backend init function
Message-ID: <561236E7.1050508@alum.mit.edu>
In-Reply-To: <1443477738-32023-29-git-send-email-dturner@twopensource.com>

On 09/29/2015 12:02 AM, David Turner wrote:
> The file backend doesn't need this function, but other backends might.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Ronnie Sahlberg <rsahlberg@google.com>
> ---
>  refs-be-files.c | 1 +
>  refs.c          | 4 +++-
>  refs.h          | 4 +++-
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/refs-be-files.c b/refs-be-files.c
> index 37e244a..eaa74b6 100644
> --- a/refs-be-files.c
> +++ b/refs-be-files.c
> @@ -3737,6 +3737,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>  struct ref_be refs_be_files = {
>  	NULL,
>  	"files",
> +	NULL,
>  	files_transaction_begin,
>  	files_transaction_update,
>  	files_transaction_create,
> diff --git a/refs.c b/refs.c
> index 769574d..9ce10b7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -19,13 +19,15 @@ struct ref_be *refs_backends = &refs_be_files;
>  /*
>   * This function is used to switch to an alternate backend.
>   */
> -int set_refs_backend(const char *name)
> +int set_refs_backend(const char *name, void *init_data)
>  {
>  	struct ref_be *be;
>  
>  	for (be = refs_backends; be; be = be->next)
>  		if (!strcmp(be->name, name)) {
>  			the_refs_backend = be;
> +			if (be->init_backend)
> +				be->init_backend(init_data);

I don't like that this virtual function, alone among all of them
introduced so far, is allowed to be NULL. That seems non-obvious and
something extra that devs have to remember.

I think it would be better for the files backend to define a do-nothing
function that can be stuck in this slot.

If you are opposed to that for some reason, then please at least add a
comment where this function pointer is added to `struct ref_be`, warning
that it can be NULL.

>  			return 0;
>  		}
>  	return 1;
> diff --git a/refs.h b/refs.h
> index 0b407b2..0dc626e 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -586,6 +586,7 @@ extern int reflog_expire(const char *refname, const unsigned char *sha1,
>  			 void *policy_cb_data);
>  
>  /* refs backends */
> +typedef void (*ref_backend_init_fn)(void *data);
>  typedef struct ref_transaction *(*ref_transaction_begin_fn)(struct strbuf *err);
>  typedef int (*ref_transaction_update_fn)(struct ref_transaction *transaction,
>  		const char *refname, const unsigned char *new_sha1,
> @@ -641,6 +642,7 @@ typedef int (*for_each_reftype_fullpath_fn)(each_ref_fn fn, char *type,
>  struct ref_be {
>  	struct ref_be *next;
>  	const char *name;
> +	ref_backend_init_fn init_backend;
>  	ref_transaction_begin_fn transaction_begin;
>  	ref_transaction_update_fn transaction_update;
>  	ref_transaction_create_fn transaction_create;
> @@ -669,6 +671,6 @@ struct ref_be {
>  
>  
>  extern struct ref_be refs_be_files;
> -int set_refs_backend(const char *name);
> +int set_refs_backend(const char *name, void *init_data);
>  
>  #endif /* REFS_H */
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu