diff mbox series

[07/10] submodule: migrate get_next_submodule to use repository structs

Message ID 20181025233231.102245-8-sbeller@google.com (mailing list archive)
State New, archived
Headers show
Series Resending sb/submodule-recursive-fetch-gets-the-tip | expand

Commit Message

Stefan Beller Oct. 25, 2018, 11:32 p.m. UTC 
We used to recurse into submodules, even if they were broken having
only an objects directory. The child process executed in the submodule
would fail though if the submodule was broken.

This patch tightens the check upfront, such that we do not need
to spawn a child process to find out if the submodule is broken.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 51 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 12 deletions(-)

Comments

Jonathan Tan Oct. 26, 2018, 7:26 p.m. UTC | #1 
> We used to recurse into submodules, even if they were broken having
> only an objects directory. The child process executed in the submodule
> would fail though if the submodule was broken.
> 
> This patch tightens the check upfront, such that we do not need
> to spawn a child process to find out if the submodule is broken.

Thanks for the clear commit message. Also mention which tests currently
exercise this.

> +static struct repository *get_submodule_repo_for(struct repository *r,
> +						 const struct submodule *sub)
> +{
> +	struct repository *ret = xmalloc(sizeof(*ret));
> +
> +	if (repo_submodule_init(ret, r, sub)) {
> +		/*
> +		 * No entry in .gitmodules? Technically not a submodule,
> +		 * but historically we supported repositories that happen to be
> +		 * in-place where a gitlink is. Keep supporting them.
> +		 */
> +		struct strbuf gitdir = STRBUF_INIT;
> +		strbuf_repo_worktree_path(&gitdir, r, "%s/.git", sub->path);
> +		if (repo_init(ret, gitdir.buf, NULL)) {
> +			strbuf_release(&gitdir);

You should also free ret here.

> +			return NULL;
> +		}
> +		strbuf_release(&gitdir);
> +	}
> +
> +	return ret;
> +}

The above is the rest of get_submodule_repo_for(), so that we can see
that gitdir is indeed freed.

> -		if (!git_dir)
> -			git_dir = submodule_git_dir.buf;
> -		if (is_directory(git_dir)) {
> +		repo = get_submodule_repo_for(spf->r, submodule);
> +		if (repo) {
>  			child_process_init(cp);
> -			cp->dir = strbuf_detach(&submodule_path, NULL);
>  			prepare_submodule_repo_env(&cp->env_array);
> +			cp->dir = xstrdup(repo->worktree);

Move the cp->dir one line up for a cleaner diff.

[snip]

> +			repo_clear(repo);
> +			free(repo);
>  			ret = 1;
> +		} else {
> +			/*
> +			 * An empty directory is normal,
> +			 * the submodule is not initialized
> +			 */
> +			if (S_ISGITLINK(ce->ce_mode) &&
> +			    !is_empty_dir(ce->name))
> +				die(_("Could not access submodule '%s'"), ce->name);

Previously, a failed fetch would just set spf->result = 1 (in
fetch_finish()), allowing other fetches to still proceed. This sounds
like a better idea to me instead of die-ing outright. (Also remember to
print a warning message - since we no longer spawn a child process to
try a fetch that will fail, we need to print a message ourselves.)
diff mbox series

Patch

diff --git a/submodule.c b/submodule.c
index e4b494af7b..a1a32cab7d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1240,6 +1240,29 @@  static int get_fetch_recurse_config(const struct submodule *submodule,
 	return spf->default_option;
 }
 
+static struct repository *get_submodule_repo_for(struct repository *r,
+						 const struct submodule *sub)
+{
+	struct repository *ret = xmalloc(sizeof(*ret));
+
+	if (repo_submodule_init(ret, r, sub)) {
+		/*
+		 * No entry in .gitmodules? Technically not a submodule,
+		 * but historically we supported repositories that happen to be
+		 * in-place where a gitlink is. Keep supporting them.
+		 */
+		struct strbuf gitdir = STRBUF_INIT;
+		strbuf_repo_worktree_path(&gitdir, r, "%s/.git", sub->path);
+		if (repo_init(ret, gitdir.buf, NULL)) {
+			strbuf_release(&gitdir);
+			return NULL;
+		}
+		strbuf_release(&gitdir);
+	}
+
+	return ret;
+}
+
 static int get_next_submodule(struct child_process *cp,
 			      struct strbuf *err, void *data, void **task_cb)
 {
@@ -1247,12 +1270,11 @@  static int get_next_submodule(struct child_process *cp,
 	struct submodule_parallel_fetch *spf = data;
 
 	for (; spf->count < spf->r->index->cache_nr; spf->count++) {
-		struct strbuf submodule_path = STRBUF_INIT;
-		struct strbuf submodule_git_dir = STRBUF_INIT;
 		struct strbuf submodule_prefix = STRBUF_INIT;
 		const struct cache_entry *ce = spf->r->index->cache[spf->count];
-		const char *git_dir, *default_argv;
+		const char *default_argv;
 		const struct submodule *submodule;
+		struct repository *repo;
 		struct submodule default_submodule = SUBMODULE_INIT;
 
 		if (!S_ISGITLINK(ce->ce_mode))
@@ -1287,16 +1309,12 @@  static int get_next_submodule(struct child_process *cp,
 			continue;
 		}
 
-		strbuf_repo_worktree_path(&submodule_path, spf->r, "%s", ce->name);
-		strbuf_addf(&submodule_git_dir, "%s/.git", submodule_path.buf);
 		strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, ce->name);
-		git_dir = read_gitfile(submodule_git_dir.buf);
-		if (!git_dir)
-			git_dir = submodule_git_dir.buf;
-		if (is_directory(git_dir)) {
+		repo = get_submodule_repo_for(spf->r, submodule);
+		if (repo) {
 			child_process_init(cp);
-			cp->dir = strbuf_detach(&submodule_path, NULL);
 			prepare_submodule_repo_env(&cp->env_array);
+			cp->dir = xstrdup(repo->worktree);
 			cp->git_cmd = 1;
 			if (!spf->quiet)
 				strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -1306,10 +1324,19 @@  static int get_next_submodule(struct child_process *cp,
 			argv_array_push(&cp->args, default_argv);
 			argv_array_push(&cp->args, "--submodule-prefix");
 			argv_array_push(&cp->args, submodule_prefix.buf);
+
+			repo_clear(repo);
+			free(repo);
 			ret = 1;
+		} else {
+			/*
+			 * An empty directory is normal,
+			 * the submodule is not initialized
+			 */
+			if (S_ISGITLINK(ce->ce_mode) &&
+			    !is_empty_dir(ce->name))
+				die(_("Could not access submodule '%s'"), ce->name);
 		}
-		strbuf_release(&submodule_path);
-		strbuf_release(&submodule_git_dir);
 		strbuf_release(&submodule_prefix);
 		if (ret) {
 			spf->count++;