Re: [PATCH v3 1/9] setup: split up concerns of `init_db()`

[Justin Tobler <jltobler@gmail.com>]

On 26/09/07 10:25AM, Patrick Steinhardt wrote:
> The function `init_db()` is responsible for creating the on-disk
> directory structure required for a Git repository. It is used by both
> git-init(1) and git-clone(1), and because their expected behaviour is
> different we support a couple of flags:
> 
>   - The `QUIET` flag controls whether the command is quiet or not. For
>     git-init(1) this is user-controllable, whereas for git-clone(1)
>     we're always quiet.
> 
>   - The `EXIST_OK` flag controls whether a preexisting repository is
>     okay or not. For git-init(1) it is, for git-clone(1) it's not.
> 
>   - The `SKIP_REFDB` flag controls whether the reference database should
>     already be created or not. For git-init(1) we do, but for
>     git-clone(1) we don't because it does not yet know about the default
>     branch and about the remote object hash.
> 
> Furthermore, we're about to add another divergence in behaviour, where
> we have to also skip creation of the object database in git-clone(1).
> This is becoming quite cumbersome though.
> 
> Instead of introducing another flag, start to split up concerns of the
> function so that we never create the reference or object database. This
> becomes the responsibility of the caller, which is thus free to defer
> their creation to a later point in time. This lets us get rid of most of
> the divergent behaviour:
> 
>   - We don't need the `SKIP_REFDB` and a potential `SKIP_ODB` flags
>     anymore.
> 
>   - We don't need the `QUIET` flag anymore, as nothing prints output
>     except for the final status message that tells the user that the
>     repository has been (re)initialized. But as this message is specific
>     to git-init(1), we can easily move it there.
> 
> The only piece of information we still have to convey is whether or not
> reinitialization of a preexisting repository is okay. This is handled by
> a new `reinit_ok` pointer that, if non-`NULL`, indicates that it is okay
> to reinitialize the repository. Furthermore, the pointer will be written
> to to indicate whether the repository was reinitialized or not, which we
> need in git-init(1) to print the correct initialization message.

I like the proposed direction here much more than the previous version.
:)

> With these refactorings, `init_db()` is named quite misleadingly though,
> as we don't create any of the reference or object databases anymore.
> Rename it to `create_repository()`.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/clone.c   |  9 +++++----
>  builtin/init-db.c | 32 ++++++++++++++++++++++++--------
>  setup.c           | 54 +++++++++++++++++-------------------------------------
>  setup.h           | 22 ++++++++++------------
>  4 files changed, 56 insertions(+), 61 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5b25cca510..904d2d859f 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1185,10 +1185,10 @@ int cmd_clone(int argc,
>  	 * repository, and reference backends may persist that information into
>  	 * their on-disk data structures.
>  	 */
> -	init_db(the_repository, git_dir, real_git_dir, work_tree, option_template,
> -		GIT_HASH_UNKNOWN, ref_storage_format, NULL,
> -		do_not_override_repo_unix_permissions,
> -		INIT_DB_QUIET | INIT_DB_SKIP_REFDB);
> +	create_repository(the_repository, git_dir, real_git_dir, work_tree,
> +			  option_template, GIT_HASH_UNKNOWN, ref_storage_format,
> +			  do_not_override_repo_unix_permissions, NULL);
> +	create_object_database(the_repository);

The implicit creation of the ref DB was already skipped so we only need
to explicitly add `create_object_database()`. Makes sense.

>  	if (real_git_dir) {
>  		free((char *)git_dir);
> @@ -1445,6 +1445,7 @@ int cmd_clone(int argc,
>  	initialize_repository_version(the_repository, hash_algo, the_repository->ref_storage_format, 1);
>  	repo_set_hash_algo(the_repository, hash_algo);
>  	create_reference_database(the_repository, NULL, 1);
> +	startup_info->have_repository = 1;

Ok IIUC, we now have to explicitly set "have_repository" because we
don't want to set it if the ODB has not been created.

>  	/*
>  	 * Before fetching from the remote, download and install bundle
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index e96b1283b7..f2c7e3be6d 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -80,7 +80,7 @@ int cmd_init_db(int argc,
>  	char *work_tree = NULL;
>  	const char *template_dir = NULL;
>  	char *template_dir_to_free = NULL;
> -	unsigned int flags = 0;
> +	int quiet = 0;
>  	int bare = startup_info->force_bare_repository ? 1 : -1;
>  	const char *object_format = NULL;
>  	const char *ref_format = NULL;
> @@ -102,7 +102,7 @@ int cmd_init_db(int argc,
>  			.flags = PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>  			.callback = shared_callback
>  		},
> -		OPT_BIT('q', "quiet", &flags, N_("be quiet"), INIT_DB_QUIET),
> +		OPT_BOOL('q', "quiet", &quiet, N_("be quiet")),
>  		OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
>  			   N_("separate git dir from working tree")),
>  		OPT_STRING('b', "initial-branch", &initial_branch, N_("name"),
> @@ -113,7 +113,7 @@ int cmd_init_db(int argc,
>  			   N_("specify the reference format to use")),
>  		OPT_END()
>  	};
> -	int ret;
> +	int reinit;
>  
>  	argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
>  
> @@ -247,14 +247,30 @@ int cmd_init_db(int argc,
>  		die(_("--separate-git-dir incompatible with bare repository"));
>  	}
>  
> -	flags |= INIT_DB_EXIST_OK;
> -	ret = init_db(the_repository, git_dir, real_git_dir, work_tree,
> -		      template_dir, hash_algo, ref_storage_format, initial_branch,
> -		      init_shared_repository, flags);
> +	create_repository(the_repository, git_dir, real_git_dir, work_tree,
> +			  template_dir, hash_algo, ref_storage_format,
> +			  init_shared_repository, &reinit);
> +	create_reference_database(the_repository, initial_branch, quiet);
> +	create_object_database(the_repository);
> +
> +	if (!quiet) {
> +		int len = strlen(git_dir);
> +
> +		if (reinit)
> +			printf(repo_settings_get_shared_repository(the_repository)
> +			       ? _("Reinitialized existing shared Git repository in %s%s\n")
> +			       : _("Reinitialized existing Git repository in %s%s\n"),
> +			       git_dir, len && git_dir[len-1] != '/' ? "/" : "");
> +		else
> +			printf(repo_settings_get_shared_repository(the_repository)
> +			       ? _("Initialized empty shared Git repository in %s%s\n")
> +			       : _("Initialized empty Git repository in %s%s\n"),
> +			       git_dir, len && git_dir[len-1] != '/' ? "/" : "");
> +	}

Previously all of this logic was specific to git-init(1), so now its
moved out of the generic function accordingly to furthur simplify the
interface. Nice.

[snip]
> diff --git a/setup.h b/setup.h
> index 763fd384e8..c4aa464caa 100644
> --- a/setup.h
> +++ b/setup.h
> @@ -256,23 +256,21 @@ int apply_repository_format(struct repository *repo,
>  
>  const char *get_template_dir(const char *option_template);
>  
> -#define INIT_DB_QUIET      (1 << 0)
> -#define INIT_DB_EXIST_OK   (1 << 1)
> -#define INIT_DB_SKIP_REFDB (1 << 2)
> -
> -int init_db(struct repository *repo,
> -	    const char *git_dir,
> -	    const char *real_git_dir,
> -	    const char *worktree,
> -	    const char *template_dir, int hash_algo,
> -	    enum ref_storage_format ref_storage_format,
> -	    const char *initial_branch, int init_shared_repository,
> -	    unsigned int flags);
> +void create_repository(struct repository *repo,
> +		       const char *git_dir,
> +		       const char *real_git_dir,
> +		       const char *worktree,
> +		       const char *template_dir,
> +		       int hash_algo,
> +		       enum ref_storage_format ref_storage_format,
> +		       int init_shared_repository,
> +		       int *reinit_ok);

While we are here, it might be nice to document these functions a little
bit. The NULL/non-NULL behavior of `reinit_ok` may not be entirely
obvious to future readers.

>  void initialize_repository_version(struct repository *repo,
>  				   int hash_algo,
>  				   enum ref_storage_format ref_storage_format,
>  				   int reinit);
>  void create_reference_database(struct repository *repo, const char *initial_branch, int quiet);
> +void create_object_database(struct repository *repo);

It might also be nice to document these functions to explain exactly
what we are "creating".

-Justin