Re: [PATCH 06/10] repository: repo_submodule_init to take a submodule struct
On Fri, Oct 26, 2018 at 12:15 PM Jonathan Tan wrote: [snip] > The expected pattern. > > This patch looks good to me. I'll take this as a "Reviewed-by"? Thanks, Stefan
Re: [PATCH 06/10] repository: repo_submodule_init to take a submodule struct
> 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,
> + _oid, path);
[snip]
> - if (repo_submodule_init(, superproject, path)) {
> + if (repo_submodule_init(, superproject, sub)) {
The last argument to repo_submodule_init is now
"submodule_from_path(superproject, _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,
> + _oid, path);
>
> - if (repo_submodule_init(, superproject, path))
> + if (repo_submodule_init(, 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(, the_repository, path))
> + if (repo_submodule_init(, the_repository, sub))
The definition of "sub" is not quoted here in this e-mail, but it is
indeed "submodule_from_path(the_repository, _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, _oid, path);
As expected, this line is removed.
> if (!sub) {
> ret = -1;
> goto out;
> }
>
> - strbuf_repo_worktree_path(, superproject, "%s/.git", path);
> - strbuf_repo_worktree_path(, superproject, "%s", path);
> + strbuf_repo_worktree_path(, superproject, "%s/.git", sub->path);
> + strbuf_repo_worktree_path(, 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
>
[PATCH 06/10] repository: repo_submodule_init to take a submodule struct
When constructing a struct repository for a submodule for some revision
of the superproject where the submodule is not contained in the index,
it may not be present in the working tree currently either. In that
situation giving a 'path' argument is not useful. Upgrade the
repo_submodule_init function to take a struct submodule instead.
The submodule struct can be obtained via submodule_from_{path, name} or
an artificial submodule struct can be passed in.
While we are at it, rename the repository struct in the repo_submodule_init
function, which is to be initialized, to a name that is not confused with
the struct submodule as easily. Perform such renames in similar functions
as well.
Also move its documentation into the header file.
Signed-off-by: Stefan Beller
---
builtin/grep.c | 17 +++-
builtin/ls-files.c | 12 +
builtin/submodule--helper.c | 2 +-
repository.c | 27
repository.h | 12 +++--
t/helper/test-submodule-nested-repo-config.c | 8 +++---
6 files changed, 43 insertions(+), 35 deletions(-)
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,
+ _oid, path);
+
int hit;
/*
@@ -434,12 +437,12 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
return 0;
}
- if (repo_submodule_init(, superproject, path)) {
+ if (repo_submodule_init(, superproject, sub)) {
grep_read_unlock();
return 0;
}
- repo_read_gitmodules();
+ repo_read_gitmodules();
/*
* NEEDSWORK: This adds the submodule's object directory to the list of
@@ -451,7 +454,7 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
* store is no longer global and instead is a member of the repository
* object.
*/
- add_to_alternates_memory(submodule.objects->objectdir);
+ add_to_alternates_memory(subrepo.objects->objectdir);
grep_read_unlock();
if (oid) {
@@ -476,14 +479,14 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
- object->type == OBJ_COMMIT, );
+ object->type == OBJ_COMMIT, );
strbuf_release();
free(data);
} else {
- hit = grep_cache(opt, , pathspec, 1);
+ hit = grep_cache(opt, , pathspec, 1);
}
- repo_clear();
+ repo_clear();
return hit;
}
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,
+ _oid, path);
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, sub))
return;
- if (repo_read_index() < 0)
+ if (repo_read_index() < 0)
die("index file corrupt");
- show_files(, dir);
+ show_files(, dir);
- repo_clear();
+ repo_clear();
}
static void show_ce(struct repository *repo, struct dir_struct *dir,
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(, the_repository, path))
+ if (repo_submodule_init(, the_repository, sub))
die(_("could not get a repository handle for submodule '%s'"),
path);
if (!repo_config_get_string(, "core.worktree", )) {
diff --git a/repository.c b/repository.c
index 5dd1486718..aabe64ee5d 100644
---
