Re: [PATCH 06/10] repository: repo_submodule_init to take a submodule struct

[Jonathan Tan <jonathantanmy@google.com>]

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 7da8fef31a..ba7634258a 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -418,7 +418,10 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
>  			  const struct object_id *oid,
>  			  const char *filename, const char *path)
>  {
> -	struct repository submodule;
> +	struct repository subrepo;
> +	const struct submodule *sub = submodule_from_path(superproject,
> +							  &null_oid, path);

[snip]

> -	if (repo_submodule_init(&submodule, superproject, path)) {
> +	if (repo_submodule_init(&subrepo, superproject, sub)) {

The last argument to repo_submodule_init is now
"submodule_from_path(superproject, &null_oid, path)" instead of "path",
and looking forward into the patch, we do not need a NULL check because
repo_submodule_init() tolerates NULL in that argument.

Let's see if the rest of the code follows the pattern - a call to
submodule_from_path() with the 3 expected arguments (repo, null OID,
path).

> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 7f9919a362..4d1649c1b3 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct dir_struct *dir);
>  static void show_submodule(struct repository *superproject,
>  			   struct dir_struct *dir, const char *path)
>  {
> -	struct repository submodule;
> +	struct repository subrepo;
> +	const struct submodule *sub = submodule_from_path(superproject,
> +							  &null_oid, path);
>  
> -	if (repo_submodule_init(&submodule, superproject, path))
> +	if (repo_submodule_init(&subrepo, superproject, sub))

So far so good.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5f8a804a6e..015aa1471f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2037,7 +2037,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
>  	if (!sub)
>  		BUG("We could get the submodule handle before?");
>  
> -	if (repo_submodule_init(&subrepo, the_repository, path))
> +	if (repo_submodule_init(&subrepo, the_repository, sub))

The definition of "sub" is not quoted here in this e-mail, but it is
indeed "submodule_from_path(the_repository, &null_oid, path)".
("the_repository" in the invocation to submodule_from_path() is correct
because the 2nd argument to the invocation of repo_submodule_init() is
"the_repository".)

> -int repo_submodule_init(struct repository *submodule,
> +int repo_submodule_init(struct repository *subrepo,
>  			struct repository *superproject,
> -			const char *path)
> +			const struct submodule *sub)
>  {
> -	const struct submodule *sub;
>  	struct strbuf gitdir = STRBUF_INIT;
>  	struct strbuf worktree = STRBUF_INIT;
>  	int ret = 0;
>  
> -	sub = submodule_from_path(superproject, &null_oid, path);

As expected, this line is removed.

>  	if (!sub) {
>  		ret = -1;
>  		goto out;
>  	}
>  
> -	strbuf_repo_worktree_path(&gitdir, superproject, "%s/.git", path);
> -	strbuf_repo_worktree_path(&worktree, superproject, "%s", path);
> +	strbuf_repo_worktree_path(&gitdir, superproject, "%s/.git", sub->path);
> +	strbuf_repo_worktree_path(&worktree, superproject, "%s", sub->path);

path and sub->path are the same, so this is fine. (This can be seen from
cache_put_path() and cache_lookup_path() in submodule-config.c.)

> -	submodule->submodule_prefix = xstrfmt("%s%s/",
> -					      superproject->submodule_prefix ?
> -					      superproject->submodule_prefix :
> -					      "", path);
> +	subrepo->submodule_prefix = xstrfmt("%s%s/",
> +					    superproject->submodule_prefix ?
> +					    superproject->submodule_prefix :
> +					    "", sub->path);

Likewise.

> +/*
> + * Initialize the repository 'subrepo' as the submodule given by the
> + * struct submodule 'sub' in parent repository 'superproject'.
> + * Return 0 upon success and a non-zero value upon failure, which may happen
> + * if the submodule is not found, or 'sub' is NULL.
> + */
> +struct submodule;
> +int repo_submodule_init(struct repository *subrepo,
>  			struct repository *superproject,
> -			const char *path);
> +			const struct submodule *sub);

Here is where it says that the last argument can be NULL.

> diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c
> index a31e2a9bea..bc97929bbc 100644
> --- a/t/helper/test-submodule-nested-repo-config.c
> +++ b/t/helper/test-submodule-nested-repo-config.c
> @@ -10,19 +10,21 @@ static void die_usage(int argc, const char **argv, const char *msg)
>  
>  int cmd__submodule_nested_repo_config(int argc, const char **argv)
>  {
> -	struct repository submodule;
> +	struct repository subrepo;
> +	const struct submodule *sub;
>  
>  	if (argc < 3)
>  		die_usage(argc, argv, "Wrong number of arguments.");
>  
>  	setup_git_directory();
>  
> -	if (repo_submodule_init(&submodule, the_repository, argv[1])) {
> +	sub = submodule_from_path(the_repository, &null_oid, argv[1]);
> +	if (repo_submodule_init(&subrepo, the_repository, sub)) {

The expected pattern.

This patch looks good to me.