Re: [PATCH 08/16] refs: add methods to init refs backend and db
- From
David Turner <dturner@twopensource.com>
- Date
- Dec 23, 2015, 06:54 UTC
- Message-ID
- <1450853697.3892.21.camel@twopensource.com>
- In-Reply-To
- <567A3228.2060407@alum.mit.edu>
On Wed, 2015-12-23 at 06:33 +0100, Michael Haggerty wrote:
Show 34 quoted lines
> 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.
Show 20 quoted lines
> > 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.