[v6,6/6] submodule: call parallel code from serial status

Commit Message

Calvin Wan Jan. 17, 2023, 7:30 p.m. UTC

Remove the serial implementation of status inside of
is_submodule_modified since the parallel implementation of status with
one job accomplishes the same task.

Combine parse_status_porcelain and parse_status_porcelain_strbuf since
the only other caller of parse_status_porcelain was in
is_submodule_modified

Signed-off-by: Calvin Wan <[e-mail]>
---
 submodule.c | 146 ++++++++++++++++++----------------------------------
 1 file changed, 51 insertions(+), 95 deletions(-)

Comments

Glen Choo Jan. 26, 2023, 8:09 a.m. UTC | #1

Calvin Wan <[e-mail]> writes:

> Remove the serial implementation of status inside of
> is_submodule_modified since the parallel implementation of status with
> one job accomplishes the same task.
>
> Combine parse_status_porcelain and parse_status_porcelain_strbuf since
> the only other caller of parse_status_porcelain was in
> is_submodule_modified

I see that this is in direct response to Jonathan's earlier comment [1]
that we should have only one implementation. Thanks, this is helpful.
Definitely a step in the right direction.

That said, I don't think this patch's position in the series makes
sense. I would have expected a patch like this to come before 5/6. I.e.
this series duplicates code in 5/6 and deletes it in 6/6 so that we only
have one implementation for both serial and parallel submodule status.

Instead, I would have expected we would refactor out the serial
implementation, then use the refactored code for the parallel
implementation. Not having duplicated code in 5/6 would shrink the line
count a lot and make it easier to review.

[1] https://lore.kernel.org/git/[e-mail]/

Glen Choo Jan. 26, 2023, 8:45 a.m. UTC | #2

Glen Choo <[e-mail]> writes:

> Calvin Wan <[e-mail]> writes:
>
>> Remove the serial implementation of status inside of
>> is_submodule_modified since the parallel implementation of status with
>> one job accomplishes the same task.
>
> I see that this is in direct response to Jonathan's earlier comment [1]
> that we should have only one implementation. Thanks, this is helpful.
> Definitely a step in the right direction.
>
> That said, I don't think this patch's position in the series makes
> sense. I would have expected a patch like this to come before 5/6. I.e.
> this series duplicates code in 5/6 and deletes it in 6/6 so that we only
> have one implementation for both serial and parallel submodule status.
>
> Instead, I would have expected we would refactor out the serial
> implementation, then use the refactored code for the parallel
> implementation. Not having duplicated code in 5/6 would shrink the line
> count a lot and make it easier to review.
>
> [1] https://lore.kernel.org/git/[e-mail]/

Ah, I realize I completely misunderstood this patch. I thought that this
was deleting code that was duplicated between the serial and parallel
implementations in 5/6 such that both ended up sharing just one copy of
the code.

Instead, this patch deletes the serial implementation altogether and
replaces it with the parallel one. As such, this patch can't come
earlier than 5/6, because we need the parallel implementation to exist
before we can use it.

For reviewability of 5/6, I'd still strongly prefer that we refactor out
functions (I'll leave more specific comments on that patch). We could
still consider replacing the serial implementation with "parallel with a
single job", though I suspect that it will be unnecessary if we do the
refactoring well. I'm also not sure how idiomatic it is to call
run_processes_parallel() with a hardcoded value of 1, but I don't feel
too strongly about that.

Patch

diff --git a/submodule.c b/submodule.c
index da95ea1f5e..2009748d9f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1887,46 +1887,7 @@  int fetch_submodules(struct repository *r,
 	return spf.result;
 }
 
-static int parse_status_porcelain(char *str, size_t len,
-				  unsigned *dirty_submodule,
-				  int ignore_untracked)
-{
-	/* regular untracked files */
-	if (str[0] == '?')
-		*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-
-	if (str[0] == 'u' ||
-	    str[0] == '1' ||
-	    str[0] == '2') {
-		/* T = line type, XY = status, SSSS = submodule state */
-		if (len < strlen("T XY SSSS"))
-			BUG("invalid status --porcelain=2 line %s",
-			    str);
-
-		if (str[5] == 'S' && str[8] == 'U')
-			/* nested untracked file */
-			*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-
-		if (str[0] == 'u' ||
-		    str[0] == '2' ||
-		    memcmp(str + 5, "S..U", 4))
-			/* other change */
-			*dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-	}
-
-	if ((*dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
-	    ((*dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
-	     ignore_untracked)) {
-		/*
-		* We're not interested in any further information from
-		* the child any more, neither output nor its exit code.
-		*/
-		return 1;
-	}
-	return 0;
-}
-
-static void parse_status_porcelain_strbuf(struct strbuf *buf,
+static void parse_status_porcelain(struct strbuf *buf,
 				   unsigned *dirty_submodule,
 				   int ignore_untracked)
 {
@@ -1936,65 +1897,60 @@  static void parse_status_porcelain_strbuf(struct strbuf *buf,
 	string_list_split(&list, buf->buf, '\n', -1);
 
 	for_each_string_list_item(item, &list) {
-		if (parse_status_porcelain(item->string,
-					   strlen(item->string),
-					   dirty_submodule,
-					   ignore_untracked))
+		char *str = item->string;
+		/* regular untracked files */
+		if (str[0] == '?')
+			*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+		if (str[0] == 'u' ||
+		str[0] == '1' ||
+		str[0] == '2') {
+			/* T = line type, XY = status, SSSS = submodule state */
+			if (strlen(str) < strlen("T XY SSSS"))
+				BUG("invalid status --porcelain=2 line %s",
+				str);
+
+			if (str[5] == 'S' && str[8] == 'U')
+				/* nested untracked file */
+				*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+			if (str[0] == 'u' ||
+			str[0] == '2' ||
+			memcmp(str + 5, "S..U", 4))
+				/* other change */
+				*dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+		}
+
+		if ((*dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+		    ((*dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
+		    ignore_untracked)) {
+			/*
+			* We're not interested in any further information from
+			* the child any more, neither output nor its exit code.
+			*/
 			break;
+		}
 	}
 	string_list_clear(&list, 0);
 }
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	struct strbuf buf = STRBUF_INIT;
-	FILE *fp;
-	unsigned dirty_submodule = 0;
-	const char *git_dir;
-	int ignore_cp_exit_code = 0;
-
-	strbuf_addf(&buf, "%s/.git", path);
-	git_dir = read_gitfile(buf.buf);
-	if (!git_dir)
-		git_dir = buf.buf;
-	if (!is_git_directory(git_dir)) {
-		if (is_directory(git_dir))
-			die(_("'%s' not recognized as a git repository"), git_dir);
-		strbuf_release(&buf);
-		/* The submodule is not checked out, so it is not modified */
-		return 0;
-	}
-	strbuf_reset(&buf);
-
-	strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
-	if (ignore_untracked)
-		strvec_push(&cp.args, "-uno");
-
-	prepare_submodule_repo_env(&cp.env);
-	cp.git_cmd = 1;
-	cp.no_stdin = 1;
-	cp.out = -1;
-	cp.dir = path;
-	if (start_command(&cp))
-		die(_("Could not run 'git status --porcelain=2' in submodule %s"), path);
-
-	fp = xfdopen(cp.out, "r");
-	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
-		char *str = buf.buf;
-		const size_t len = buf.len;
-
-		ignore_cp_exit_code = parse_status_porcelain(str, len, &dirty_submodule,
-							     ignore_untracked);
-		if (ignore_cp_exit_code)
-			break;
-	}
-	fclose(fp);
-
-	if (finish_command(&cp) && !ignore_cp_exit_code)
-		die(_("'git status --porcelain=2' failed in submodule %s"), path);
-
-	strbuf_release(&buf);
+	struct submodule_status_util util = {
+		.dirty_submodule = 0,
+		.ignore_untracked = ignore_untracked,
+		.path = path,
+	};
+	struct string_list sub = STRING_LIST_INIT_NODUP;
+	struct string_list_item *item;
+	int dirty_submodule;
+
+	item = string_list_append(&sub, path);
+	item->util = &util;
+	if (get_submodules_status(&sub, 1))
+		die(_("submodule status failed"));
+	dirty_submodule = util.dirty_submodule;
+	string_list_clear(&sub, 0);
 	return dirty_submodule;
 }
 
@@ -2096,9 +2052,9 @@  static int status_finish(int retvalue, struct strbuf *err,
 		    task->path);
 	}
 
-	parse_status_porcelain_strbuf(&task->out,
-			      &util->dirty_submodule,
-			      util->ignore_untracked);
+	parse_status_porcelain(&task->out,
+			       &util->dirty_submodule,
+			       util->ignore_untracked);
 
 	strbuf_release(&task->out);
 	free(task);