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

On Wed, 2015-12-23 at 06:33 +0100, Michael Haggerty wrote:

On 12/03/2015 01:35 AM, David Turner wrote:

quoted

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 <redacted>
---
 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.

quoted

 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.