Re: [PATCH 08/16] refs: add methods to init refs backend and db

[David Turner <dturner@twopensource.com>]

On Wed, 2015-12-23 at 06:33 +0100, Michael Haggerty wrote:
> On 12/03/2015 01:35 AM, David Turner wrote:
> > Alternate refs backends might not need the refs/heads directory and
> > so
> > on, so we make ref db initialization part of the backend.  We also
> > might need to initialize ref backends themselves, so we'll add a
> > method for that as well.
> > 
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> >  builtin/init-db.c    | 14 ++++----------
> >  refs.c               |  8 +++++++-
> >  refs.h               |  4 +++-
> >  refs/files-backend.c | 23 +++++++++++++++++++++++
> >  refs/refs-internal.h |  4 ++++
> >  5 files changed, 41 insertions(+), 12 deletions(-)
> > 
> > [...]
> > diff --git a/refs.c b/refs.c
> > index 9a2fed7..bdeb276 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -22,13 +22,14 @@ 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 *data)
> 
> The purpose of the data argument is rather mysterious at this point,
> especially since set_refs_backend() still doesn't have any callers. I
> hope that will become clearer later in the series. Nevertheless, it
> would be nice for its use to be described in the docstring (which
> should
> preferably be moved to the header file).

Will fix.

> >  struct ref_be {
> >  	struct ref_be *next;
> >  	const char *name;
> > +	ref_backend_init_fn *init_backend;
> > +	ref_backend_init_db_fn *init_db;
> >  	ref_transaction_commit_fn *transaction_commit;
> >  	ref_transaction_commit_fn *initial_transaction_commit;
> >  
> > 
> 
> Your naming seems inconsistent here. I would have expected a
> "files_init_backend()" function to satisfy the typedef
> "ref_backend_init_backend_fn" or "ref_init_backend_fn", not
> "ref_backend_init_fn". Or, conversely, for the function implementing
> "ref_backend_init_fn" to be called "files_init()".
> 
> More generally, it would be nice to have a consistent naming pattern
> between (1) the name of the typedef, (2) the name of the member in
> "struct ref_be", (3) the names of concrete, backend-specific
> implementations of the functions.

I'll change this to "refs_init_backend_fn *init_backend" and
"refs_init_db_fn *init_db" (since we already have an init_db function),
and make any other similar changes I happen to notice.

On the naming in general, I am somewhat at the mercy of history here. 
 For example, git presently has functions named ref_transaction_commit
(object_verb) and create_reflog (verb_object).  So nothing I do will be
consistent with everything.  I could, of course, do some initial
commits to clean up the naming, but that would create code churn.