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@xxxxxxxxxxxxxxxx>
> ---
> 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).
> [...]
> 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
mhagger@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html