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

[Michael Haggerty]

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 
> ---
>  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 = _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).

> [...]
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index e769242..6600c02 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3313,6 +3313,11 @@ static int ref_present(const char *refname,
>   return string_list_has_string(affected_refnames, refname);
>  }
>  
> +void files_init_backend(void *data)
> +{
> + /* do nothing */
> +}
> +
>  static int files_initial_transaction_commit(struct ref_transaction 
> *transaction,
>   struct strbuf *err)
>  {
> @@ -3534,9 +3539,27 @@ static int files_reflog_expire(const char *refname, 
> const unsigned char *sha1,
>   return -1;
>  }
>  
> +static int files_init_db(struct strbuf *err, int shared)
> +{
> + /*
> +  * Create .git/refs/{heads,tags}
> +  */
> + safe_create_dir(git_path("refs"), 1);
> + safe_create_dir(git_path("refs/heads"), 1);
> + safe_create_dir(git_path("refs/tags"), 1);
> + if (shared) {
> + adjust_shared_perm(git_path("refs"));
> + adjust_shared_perm(git_path("refs/heads"));
> + adjust_shared_perm(git_path("refs/tags"));
> + }
> + return 0;
> +}
> +
>  struct ref_be refs_be_files = {
>   NULL,
>   "files",
> + files_init_backend,
> + files_init_db,
>   files_transaction_commit,
>   files_initial_transaction_commit,
>  
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 478ad54..85a0b91 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -208,6 +208,8 @@ const char *find_descendant_ref(const char *dirname,
>  int rename_ref_available(const char *oldname, const char *newname);
>  
>  /* refs backends */
> +typedef void ref_backend_init_fn(void *data);
> +typedef int ref_backend_init_db_fn(struct strbuf *err, int shared);
>  typedef int ref_transaction_commit_fn(struct ref_transaction *transaction,
> struct strbuf *err);
>  
> @@ -267,6 +269,8 @@ typedef int for_each_replace_ref_fn(each_ref_fn fn, void 
> *cb_data);
>  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.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Turner]

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 
> > ---
> >  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 = _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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Turner]

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 
---
 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/builtin/init-db.c b/builtin/init-db.c
index 26e1cc3..4771e7e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -178,13 +178,7 @@ static int create_default_files(const char *template_path)
char junk[2];
int reinit;
int filemode;
-
-   /*
-* Create .git/refs/{heads,tags}
-*/
-   safe_create_dir(git_path_buf(, "refs"), 1);
-   safe_create_dir(git_path_buf(, "refs/heads"), 1);
-   safe_create_dir(git_path_buf(, "refs/tags"), 1);
+   struct strbuf err = STRBUF_INIT;
 
/* Just look for `init.templatedir` */
git_config(git_init_db_config, NULL);
@@ -208,11 +202,11 @@ static int create_default_files(const char *template_path)
 */
if (shared_repository) {
adjust_shared_perm(get_git_dir());
-   adjust_shared_perm(git_path_buf(, "refs"));
-   adjust_shared_perm(git_path_buf(, "refs/heads"));
-   adjust_shared_perm(git_path_buf(, "refs/tags"));
}
 
+   if (refs_init_db(, shared_repository))
+   die("failed to set up refs db: %s", err.buf);
+
/*
 * Create the default symlink from ".git/HEAD" to the "master"
 * branch, if it does not exist yet.
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 = _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)
 {
struct ref_be *be;
 
for (be = refs_backends; be; be = be->next)
if (!strcmp(be->name, name)) {
the_refs_backend = be;
+   be->init_backend(data);
return 0;
}
return 1;
@@ -1109,6 +1110,11 @@ int rename_ref_available(const char *oldname, const char 
*newname)
 }
 
 /* backend functions */
+int refs_init_db(struct strbuf *err, int shared)
+{
+   return the_refs_backend->init_db(err, shared);
+}
+
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
diff --git a/refs.h b/refs.h
index 4e5477d..c211b9e 100644
--- a/refs.h
+++ b/refs.h
@@ -66,6 +66,8 @@ extern int ref_exists(const char *refname);
 
 extern int is_branch(const char *refname);
 
+extern int refs_init_db(struct strbuf *err, int shared);
+
 /*
  * If refname is a non-symbolic reference that refers to a tag object,
  * and the tag can be (recursively) dereferenced to a non-tag object,
@@ -508,6 +510,6 @@ extern int reflog_expire(const char *refname, const 
unsigned char *sha1,
 reflog_expiry_cleanup_fn cleanup_fn,
 void *policy_cb_data);
 
-int set_refs_backend(const char *name);
+int set_refs_backend(const char *name, void *data);
 
 #endif /* REFS_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index e769242..6600c02 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3313,6 +3313,11 @@ static int ref_present(const char *refname,
return string_list_has_string(affected_refnames, refname);
 }
 
+void files_init_backend(void *data)
+{
+   /* do nothing */
+}
+
 static int files_initial_transaction_commit(struct ref_transaction 
*transaction,
struct strbuf *err)
 {
@@ -3534,9 +3539,27 @@ static int files_reflog_expire(const char *refname, 
const unsigned char *sha1,
return -1;
 }
 
+static int files_init_db(struct strbuf *err, int shared)
+{
+   /*
+* Create .git/refs/{heads,tags}
+*/
+   safe_create_dir(git_path("refs"), 1);
+   safe_create_dir(git_path("refs/heads"), 1);
+   safe_create_dir(git_path("refs/tags"), 1);
+   if (shared) {
+   adjust_shared_perm(git_path("refs"));
+   adjust_shared_perm(git_path("refs/heads"));
+   adjust_shared_perm(git_path("refs/tags"));
+   }
+   return 0;
+}
+
 struct ref_be refs_be_files = {
NULL,
"files",
+   files_init_backend,
+   files_init_db,
files_transaction_commit,
files_initial_transaction_commit,
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 478ad54..85a0b91 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -208,6 +208,8 @@ const char *find_descendant_ref(const char