[PATCH 0/6] git-submodule.sh: convert part of cmd_update to C

[PATCH 0/6] git-submodule.sh: convert part of cmd_update to C

[Stefan Beller]

I thought about writing it all in one go, but the series got too large,
so let's chew one bite at a time.

Thanks,
Stefan

Stefan Beller (6):
  git-submodule.sh: align error reporting for update mode to use path
  git-submodule.sh: rename unused variables
  builtin/submodule--helper: factor out submodule updating
  builtin/submodule--helper: store update_clone information in a struct
  builtin/submodule--helper: factor out method to update a single
    submodule
  submodule--helper: introduce new update-module-mode helper

 builtin/submodule--helper.c | 152 ++++++++++++++++++++++++++++--------
 git-submodule.sh            |  22 +-----
 2 files changed, 122 insertions(+), 52 deletions(-)

-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path

[Stefan Beller]

All other error messages in cmd_update are reporting the submodule based
on its path, so let's do that for invalid update modes, too.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 5f9d9f6ea37..8a611865397 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -627,7 +627,7 @@ cmd_update()
 				must_die_on_failure=yes
 				;;
 			*)
-				die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
+				die "$(eval_gettext "Invalid update mode '$update_module' for submodule path '$path'")"
 			esac
 
 			if (sanitize_submodule_env; cd "$sm_path" && $command "$sha1")
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 2/6] git-submodule.sh: rename unused variables

[Stefan Beller]

The 'mode' variable is not used in cmd_update for its original purpose,
rename it to 'dummy' as it only serves the purpose to abort quickly
documenting this knowledge.

The variable 'stage' is also not used any more in cmd_update, so remove it.

This went unnoticed as first each function used the commonly used
submodule listing, which was converted in 74703a1e4df (submodule: rewrite
`module_list` shell function in C, 2015-09-02). When cmd_update was
using its own function starting in 48308681b07 (git submodule update:
have a dedicated helper for cloning, 2016-02-29), its removal was missed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 5 ++---
 git-submodule.sh            | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 20ae9191ca3..ebcfe85bfa9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1571,9 +1571,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	needs_cloning = !file_exists(sb.buf);
 
 	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
-			oid_to_hex(&ce->oid), ce_stage(ce),
-			needs_cloning, ce->name);
+	strbuf_addf(&sb, "dummy %s %d\t%s\n",
+		    oid_to_hex(&ce->oid), needs_cloning, ce->name);
 	string_list_append(&suc->projectlines, sb.buf);
 
 	if (!needs_cloning)
diff --git a/git-submodule.sh b/git-submodule.sh
index 8a611865397..56588aa304d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -531,9 +531,9 @@ cmd_update()
 		"$@" || echo "#unmatched" $?
 	} | {
 	err=
-	while read -r mode sha1 stage just_cloned sm_path
+	while read -r quickabort sha1 just_cloned sm_path
 	do
-		die_if_unmatched "$mode" "$sha1"
+		die_if_unmatched "$quickabort" "$sha1"
 
 		name=$(git submodule--helper name "$sm_path") || exit
 		if ! test -z "$update"
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 3/6] builtin/submodule--helper: factor out submodule updating

[Stefan Beller]

Separate the command line parsing from the actual execution of the command
within the repository. For now there is not a lot of execution as
most of it is still in git-submodule.sh.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 59 +++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ebcfe85bfa9..96929ba1096 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1472,6 +1472,8 @@ struct submodule_update_clone {
 	/* failed clones to be retried again */
 	const struct cache_entry **failed_clones;
 	int failed_clones_nr, failed_clones_alloc;
+
+	int max_jobs;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
 	SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \
@@ -1714,11 +1716,36 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 	return 0;
 }
 
+static int update_submodules(struct submodule_update_clone *suc)
+{
+	struct string_list_item *item;
+
+	run_processes_parallel(suc->max_jobs,
+			       update_clone_get_next_task,
+			       update_clone_start_failure,
+			       update_clone_task_finished,
+			       suc);
+
+	/*
+	 * We saved the output and put it out all at once now.
+	 * That means:
+	 * - the listener does not have to interleave their (checkout)
+	 *   work with our fetching.  The writes involved in a
+	 *   checkout involve more straightforward sequential I/O.
+	 * - the listener can avoid doing any work if fetching failed.
+	 */
+	if (suc->quickstop)
+		return 1;
+
+	for_each_string_list_item(item, &suc->projectlines)
+		fprintf(stdout, "%s", item->string);
+
+	return 0;
+}
+
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
 	const char *update = NULL;
-	int max_jobs = 1;
-	struct string_list_item *item;
 	struct pathspec pathspec;
 	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -1740,7 +1767,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "depth", &suc.depth, "<depth>",
 			   N_("Create a shallow clone truncated to the "
 			      "specified number of revisions")),
-		OPT_INTEGER('j', "jobs", &max_jobs,
+		OPT_INTEGER('j', "jobs", &suc.max_jobs,
 			    N_("parallel jobs")),
 		OPT_BOOL(0, "recommend-shallow", &suc.recommend_shallow,
 			    N_("whether the initial clone should follow the shallow recommendation")),
@@ -1756,8 +1783,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	};
 	suc.prefix = prefix;
 
-	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
-	git_config(gitmodules_update_clone_config, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, &suc.max_jobs);
+	git_config(gitmodules_update_clone_config, &suc.max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 			     git_submodule_helper_usage, 0);
@@ -1772,27 +1799,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
-	run_processes_parallel(max_jobs,
-			       update_clone_get_next_task,
-			       update_clone_start_failure,
-			       update_clone_task_finished,
-			       &suc);
-
-	/*
-	 * We saved the output and put it out all at once now.
-	 * That means:
-	 * - the listener does not have to interleave their (checkout)
-	 *   work with our fetching.  The writes involved in a
-	 *   checkout involve more straightforward sequential I/O.
-	 * - the listener can avoid doing any work if fetching failed.
-	 */
-	if (suc.quickstop)
-		return 1;
-
-	for_each_string_list_item(item, &suc.projectlines)
-		fprintf(stdout, "%s", item->string);
-
-	return 0;
+	return update_submodules(&suc);
 }
 
 static int resolve_relative_path(int argc, const char **argv, const char *prefix)
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct

[Stefan Beller]

The information that is printed for update_submodules in
'submodule--helper update-clone' and consumed by 'git submodule update'
is stored as a string per submodule. This made sense at the time of
48308681b07 (git submodule update: have a dedicated helper for cloning,
2016-02-29), but as we want to migrate the rest of the submodule update
into C, we're better off having access to the raw information in a helper
struct.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 96929ba1096..c9c3fe2bf28 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1444,6 +1444,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct submodule_update_clone_information {
+	const struct submodule *sub;
+	struct object_id oid;
+	unsigned just_cloned;
+};
+
 struct submodule_update_clone {
 	/* index into 'list', the list of submodules to look into for cloning */
 	int current;
@@ -1463,8 +1469,9 @@ struct submodule_update_clone {
 	const char *recursive_prefix;
 	const char *prefix;
 
-	/* Machine-readable status lines to be consumed by git-submodule.sh */
-	struct string_list projectlines;
+	/* to be consumed by git-submodule.sh */
+	struct submodule_update_clone_information *submodule_lines;
+	int submodule_lines_nr; int submodule_lines_alloc;
 
 	/* If we want to stop as fast as possible and return an error */
 	unsigned quickstop : 1;
@@ -1478,7 +1485,7 @@ struct submodule_update_clone {
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
 	SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \
 	NULL, NULL, NULL, \
-	STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
+	NULL, 0, 0, 0, NULL, 0, 0, 0}
 
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
@@ -1572,10 +1579,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	strbuf_addf(&sb, "%s/.git", ce->name);
 	needs_cloning = !file_exists(sb.buf);
 
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "dummy %s %d\t%s\n",
-		    oid_to_hex(&ce->oid), needs_cloning, ce->name);
-	string_list_append(&suc->projectlines, sb.buf);
+	ALLOC_GROW(suc->submodule_lines, suc->submodule_lines_nr + 1,
+					 suc->submodule_lines_alloc);
+	oidcpy(&suc->submodule_lines[suc->submodule_lines_nr].oid, &ce->oid);
+	suc->submodule_lines[suc->submodule_lines_nr].just_cloned = needs_cloning;
+	suc->submodule_lines[suc->submodule_lines_nr].sub = sub;
+	suc->submodule_lines_nr++;
 
 	if (!needs_cloning)
 		goto cleanup;
@@ -1718,7 +1727,8 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 static int update_submodules(struct submodule_update_clone *suc)
 {
-	struct string_list_item *item;
+	int i;
+	struct strbuf sb = STRBUF_INIT;
 
 	run_processes_parallel(suc->max_jobs,
 			       update_clone_get_next_task,
@@ -1737,9 +1747,16 @@ static int update_submodules(struct submodule_update_clone *suc)
 	if (suc->quickstop)
 		return 1;
 
-	for_each_string_list_item(item, &suc->projectlines)
-		fprintf(stdout, "%s", item->string);
+	for (i = 0; i < suc->submodule_lines_nr; i++) {
+		strbuf_addf(&sb, "dummy %s %d\t%s\n",
+			oid_to_hex(&suc->submodule_lines[i].oid),
+			suc->submodule_lines[i].just_cloned,
+			suc->submodule_lines[i].sub->path);
+		fprintf(stdout, "%s", sb.buf);
+		strbuf_reset(&sb);
+	}
 
+	strbuf_release(&sb);
 	return 0;
 }
 
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 5/6] builtin/submodule--helper: factor out method to update a single submodule

[Stefan Beller]

In a later patch we'll find this method handy.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c9c3fe2bf28..4bbf580df79 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1725,10 +1725,17 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 	return 0;
 }
 
+static void update_submodule(struct submodule_update_clone_information *suci)
+{
+	fprintf(stdout, "dummy %s %d\t%s\n",
+		oid_to_hex(&suci->oid),
+		suci->just_cloned,
+		suci->sub->path);
+}
+
 static int update_submodules(struct submodule_update_clone *suc)
 {
 	int i;
-	struct strbuf sb = STRBUF_INIT;
 
 	run_processes_parallel(suc->max_jobs,
 			       update_clone_get_next_task,
@@ -1747,16 +1754,9 @@ static int update_submodules(struct submodule_update_clone *suc)
 	if (suc->quickstop)
 		return 1;
 
-	for (i = 0; i < suc->submodule_lines_nr; i++) {
-		strbuf_addf(&sb, "dummy %s %d\t%s\n",
-			oid_to_hex(&suc->submodule_lines[i].oid),
-			suc->submodule_lines[i].just_cloned,
-			suc->submodule_lines[i].sub->path);
-		fprintf(stdout, "%s", sb.buf);
-		strbuf_reset(&sb);
-	}
+	for (i = 0; i < suc->submodule_lines_nr; i++)
+		update_submodule(&suc->submodule_lines[i]);
 
-	strbuf_release(&sb);
 	return 0;
 }
 
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 6/6] submodule--helper: introduce new update-module-mode helper

[Stefan Beller]

This chews off a bit of the shell part of the update command in
git-submodule.sh. When writing the C code, keep in mind that the
submodule--helper part will go away eventually and we want to have
a C function that is able to determine the submodule update strategy,
it as a nicety, make determine_submodule_update_strategy accessible
for arbitrary repositories.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 61 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 +---------
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4bbf580df79..e53231cf286 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1444,6 +1444,66 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void determine_submodule_update_strategy(struct repository *r,
+						int just_cloned,
+						const char *path,
+						const char *update,
+						struct submodule_update_strategy *out)
+{
+	const struct submodule *sub = submodule_from_path(r, &null_oid, path);
+	char *key;
+	const char *val;
+
+	key = xstrfmt("submodule.%s.update", sub->name);
+
+	if (update) {
+		trace_printf("parsing update");
+		if (parse_submodule_update_strategy(update, out) < 0)
+			die(_("Invalid update mode '%s' for submodule path '%s'"),
+				update, path);
+	} else if (!repo_config_get_string_const(r, key, &val)) {
+		if (parse_submodule_update_strategy(val, out) < 0)
+			die(_("Invalid update mode '%s' configured for submodule path '%s'"),
+				val, path);
+	} else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
+		trace_printf("loaded thing");
+		out->type = sub->update_strategy.type;
+		out->command = sub->update_strategy.command;
+	} else
+		out->type = SM_UPDATE_CHECKOUT;
+
+	if (just_cloned &&
+	    (out->type == SM_UPDATE_MERGE ||
+	     out->type == SM_UPDATE_REBASE ||
+	     out->type == SM_UPDATE_NONE))
+		out->type = SM_UPDATE_CHECKOUT;
+
+	free(key);
+}
+
+static int module_update_module_mode(int argc, const char **argv, const char *prefix)
+{
+	const char *path, *update = NULL;
+	int just_cloned;
+	struct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT };
+
+	if (argc < 3 || argc > 4)
+		die("submodule--helper update-module-clone expects <just-cloned> <path> [<update>]");
+
+	just_cloned = git_config_int("just_cloned", argv[1]);
+	path = argv[2];
+
+	if (argc == 4)
+		update = argv[3];
+
+	determine_submodule_update_strategy(the_repository,
+					    just_cloned, path, update,
+					    &update_strategy);
+	fprintf(stdout, submodule_strategy_to_string(&update_strategy));
+
+	return 0;
+}
+
 struct submodule_update_clone_information {
 	const struct submodule *sub;
 	struct object_id oid;
@@ -2039,6 +2099,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"relative-path", resolve_relative_path, 0},
 	{"resolve-relative-url", resolve_relative_url, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 56588aa304d..215760898ce 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -535,27 +535,13 @@ cmd_update()
 	do
 		die_if_unmatched "$quickabort" "$sha1"
 
-		name=$(git submodule--helper name "$sm_path") || exit
-		if ! test -z "$update"
-		then
-			update_module=$update
-		else
-			update_module=$(git config submodule."$name".update)
-			if test -z "$update_module"
-			then
-				update_module="checkout"
-			fi
-		fi
+		update_module=$(git submodule--helper update-module-mode $just_cloned "$sm_path" $update)
 
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 
 		if test $just_cloned -eq 1
 		then
 			subsha1=
-			case "$update_module" in
-			merge | rebase | none)
-				update_module=checkout ;;
-			esac
 		else
 			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> All other error messages in cmd_update are reporting the submodule based
> on its path, so let's do that for invalid update modes, too.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Makes sense.

>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 5f9d9f6ea37..8a611865397 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -627,7 +627,7 @@ cmd_update()
>  				must_die_on_failure=yes
>  				;;
>  			*)
> -				die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
> +				die "$(eval_gettext "Invalid update mode '$update_module' for submodule path '$path'")"
>  			esac
>  
>  			if (sanitize_submodule_env; cd "$sm_path" && $command "$sha1")

Re: [PATCH 2/6] git-submodule.sh: rename unused variables

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The 'mode' variable is not used in cmd_update for its original purpose,
> rename it to 'dummy' as it only serves the purpose to abort quickly
> documenting this knowledge.

It seems that

 (1) the while loop in git-submodule.sh we see in this patch is the
     only thing that read from submodule--helper update-clone; and

 (2) the mode/sha1/stage output from prepare_to_clone_next_submodule()
     is shown only when update_clone_get_next_task which in turn is
     run only during update-clone task

so this conversion will not have unintended bad effect on other
codepaths that use similarly formatted (but already different)
output used by the users of module_list.  

Which tells us that this step is safe.

I am not sure how much it buys us not having to format mode into hex
and not having to call ce_stage(), when we have cache entry anyway
in the codeflow, though.  IOW, it is unclear at this second step in
the six-patch series if we get to the end of the tunnel sooner by
having this step here.  Let's keep reading ;-).




> The variable 'stage' is also not used any more in cmd_update, so remove it.
>
> This went unnoticed as first each function used the commonly used
> submodule listing, which was converted in 74703a1e4df (submodule: rewrite
> `module_list` shell function in C, 2015-09-02). When cmd_update was
> using its own function starting in 48308681b07 (git submodule update:
> have a dedicated helper for cloning, 2016-02-29), its removal was missed.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 5 ++---
>  git-submodule.sh            | 4 ++--
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 20ae9191ca3..ebcfe85bfa9 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1571,9 +1571,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  	needs_cloning = !file_exists(sb.buf);
>  
>  	strbuf_reset(&sb);
> -	strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
> -			oid_to_hex(&ce->oid), ce_stage(ce),
> -			needs_cloning, ce->name);
> +	strbuf_addf(&sb, "dummy %s %d\t%s\n",
> +		    oid_to_hex(&ce->oid), needs_cloning, ce->name);
>  	string_list_append(&suc->projectlines, sb.buf);
>  
>  	if (!needs_cloning)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8a611865397..56588aa304d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -531,9 +531,9 @@ cmd_update()
>  		"$@" || echo "#unmatched" $?
>  	} | {
>  	err=
> -	while read -r mode sha1 stage just_cloned sm_path
> +	while read -r quickabort sha1 just_cloned sm_path
>  	do
> -		die_if_unmatched "$mode" "$sha1"
> +		die_if_unmatched "$quickabort" "$sha1"
>  
>  		name=$(git submodule--helper name "$sm_path") || exit
>  		if ! test -z "$update"

Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The information that is printed for update_submodules in
> 'submodule--helper update-clone' and consumed by 'git submodule update'
> is stored as a string per submodule. This made sense at the time of
> 48308681b07 (git submodule update: have a dedicated helper for cloning,
> 2016-02-29), but as we want to migrate the rest of the submodule update
> into C, we're better off having access to the raw information in a helper
> struct.

The reasoning above makes sense, but I have a few niggles on the
naming.

 * Anything you'd place to keep track of the state is "information",
   so a whole "_information" suffix to the structure name does not
   add much value.  We've seen our fair share of (meaningless)
   "_data" suffix used in many places but I think the overly long
   "_information" that is equally meaningless trumps them.  I think
   we also have "_info", but if we are not going to have a beter
   name, let's not be original and stick to "_data" like other
   existing codepath.  An alternative with more meaningful name is
   of course better, though, but it is not all that much worth to
   spend too many braincycles on it.

 * Is the fact that these parameters necessary to help populating
   the submodule repository are sent on a line to external process
   the most important aspect of the submodule_lines[] array?  As
   this step is a preparation to migrate out of that line/text
   oriented IPC, I think line-ness is the least important and
   interesting thing to name the variable with.


If I were writing this patch, perhaps I'd do

	struct update_clone_data *update_clone;
	int update_clone_nr, update_clone_alloc;

following my gut, but since you've been thinking about submodule
longer than I have, perhaps you can come up with a better name.

> @@ -1463,8 +1469,9 @@ struct submodule_update_clone {
>  	const char *recursive_prefix;
>  	const char *prefix;
>  
> -	/* Machine-readable status lines to be consumed by git-submodule.sh */
> -	struct string_list projectlines;
> +	/* to be consumed by git-submodule.sh */
> +	struct submodule_update_clone_information *submodule_lines;
> +	int submodule_lines_nr; int submodule_lines_alloc;
>  
>  	/* If we want to stop as fast as possible and return an error */
>  	unsigned quickstop : 1;
> @@ -1478,7 +1485,7 @@ struct submodule_update_clone {
>  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
>  	SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \
>  	NULL, NULL, NULL, \
> -	STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
> +	NULL, 0, 0, 0, NULL, 0, 0, 0}

The structure definition and this macro definition are nearby, so it
is not crucial, but its probably not a bad idea to switch to C99
initializers for a thing like this to make it more readable, once
the code around this area stabilizes back again sufficiently (IOW,
let's not distract ourselves in the middle of adding a new feature
like this one).

Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct

[Stefan Beller]

On Mon, Jul 16, 2018 at 12:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > The information that is printed for update_submodules in
> > 'submodule--helper update-clone' and consumed by 'git submodule update'
> > is stored as a string per submodule. This made sense at the time of
> > 48308681b07 (git submodule update: have a dedicated helper for cloning,
> > 2016-02-29), but as we want to migrate the rest of the submodule update
> > into C, we're better off having access to the raw information in a helper
> > struct.
>
> The reasoning above makes sense, but I have a few niggles on the
> naming.
>
>  * Anything you'd place to keep track of the state is "information",
>    so a whole "_information" suffix to the structure name does not
>    add much value.  We've seen our fair share of (meaningless)
>    "_data" suffix used in many places but I think the overly long
>    "_information" that is equally meaningless trumps them.  I think
>    we also have "_info", but if we are not going to have a beter
>    name, let's not be original and stick to "_data" like other
>    existing codepath.  An alternative with more meaningful name is
>    of course better, though, but it is not all that much worth to
>    spend too many braincycles on it.

agreed.

>  * Is the fact that these parameters necessary to help populating
>    the submodule repository are sent on a line to external process
>    the most important aspect of the submodule_lines[] array?  As

In terms of what the submodule--helper does, it is.
It is not the most important aspect in the big picture, or once
we finish the migration to not have shell interpret its output.

>    this step is a preparation to migrate out of that line/text
>    oriented IPC, I think line-ness is the least important and
>    interesting thing to name the variable with.

ok.

> If I were writing this patch, perhaps I'd do
>
>         struct update_clone_data *update_clone;
>         int update_clone_nr, update_clone_alloc;
>
> following my gut, but since you've been thinking about submodule
> longer than I have, perhaps you can come up with a better name.

That makes sense. We do not need to mention 'submodule' as that
ought to be obvious from the file name already and 'update_clone'
is just enough to describe what we are doing.
Although there is already struct submodule_update_clone, which
would be the better candidate for 'update_clone' and this new
struct would be used as a helper in that struct, so update_clone_data

I'll think of adding a patch to rename that already existing struct
(submodule_update_clone -> update_clone) and renaming
this to update_clone_data.



>
> > @@ -1463,8 +1469,9 @@ struct submodule_update_clone {
> >       const char *recursive_prefix;
> >       const char *prefix;
> >
> > -     /* Machine-readable status lines to be consumed by git-submodule.sh */
> > -     struct string_list projectlines;
> > +     /* to be consumed by git-submodule.sh */
> > +     struct submodule_update_clone_information *submodule_lines;
> > +     int submodule_lines_nr; int submodule_lines_alloc;
> >
> >       /* If we want to stop as fast as possible and return an error */
> >       unsigned quickstop : 1;
> > @@ -1478,7 +1485,7 @@ struct submodule_update_clone {
> >  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
> >       SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \
> >       NULL, NULL, NULL, \
> > -     STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
> > +     NULL, 0, 0, 0, NULL, 0, 0, 0}
>
> The structure definition and this macro definition are nearby, so it
> is not crucial, but its probably not a bad idea to switch to C99
> initializers for a thing like this to make it more readable, once
> the code around this area stabilizes back again sufficiently (IOW,
> let's not distract ourselves in the middle of adding a new feature
> like this one).

Are we still in the phase of "test balloon" or do we now accept
C99 initializers all over the code base?

But I agree to defer the conversion for now.

Thanks,
Stefan

Re: [PATCH 6/6] submodule--helper: introduce new update-module-mode helper

[Derrick Stolee]

On 7/12/2018 3:47 PM, Stefan Beller wrote:
> +	fprintf(stdout, submodule_strategy_to_string(&update_strategy));

This line is causing build failures on 'pu' for certain setups:

2018-07-18T14:53:03.6857987Z builtin/submodule--helper.c: In function 
‘module_update_module_mode’:
2018-07-18T14:53:03.6874886Z builtin/submodule--helper.c:1504:2: error: 
format not a string literal and no format arguments 
[-Werror=format-security]
2018-07-18T14:53:03.6890814Z   fprintf(stdout, 
submodule_strategy_to_string(&update_strategy));
2018-07-18T14:53:03.6905905Z   ^

Thanks,

-Stolee

Re: [PATCH 6/6] submodule--helper: introduce new update-module-mode helper

[Stefan Beller]

On Wed, Jul 18, 2018 at 8:00 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 7/12/2018 3:47 PM, Stefan Beller wrote:
> > +     fprintf(stdout, submodule_strategy_to_string(&update_strategy));
>
> This line is causing build failures on 'pu' for certain setups:
>

yeah, will fix in a resend.
originally reported at
https://public-inbox.org/git/20180717075959.30594-1-szeder.dev@gmail.com/

[PATCH 6/6] submodule--helper: introduce new update-module-mode helper

[Stefan Beller]

This chews off a bit of the shell part of the update command in
git-submodule.sh. When writing the C code, keep in mind that the
submodule--helper part will go away eventually and we want to have
a C function that is able to determine the submodule update strategy,
it as a nicety, make determine_submodule_update_strategy accessible
for arbitrary repositories.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 61 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 +---------
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 034ba1bb2e0..c7de5d1e0a0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1444,6 +1444,66 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void determine_submodule_update_strategy(struct repository *r,
+						int just_cloned,
+						const char *path,
+						const char *update,
+						struct submodule_update_strategy *out)
+{
+	const struct submodule *sub = submodule_from_path(r, &null_oid, path);
+	char *key;
+	const char *val;
+
+	key = xstrfmt("submodule.%s.update", sub->name);
+
+	if (update) {
+		trace_printf("parsing update");
+		if (parse_submodule_update_strategy(update, out) < 0)
+			die(_("Invalid update mode '%s' for submodule path '%s'"),
+				update, path);
+	} else if (!repo_config_get_string_const(r, key, &val)) {
+		if (parse_submodule_update_strategy(val, out) < 0)
+			die(_("Invalid update mode '%s' configured for submodule path '%s'"),
+				val, path);
+	} else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
+		trace_printf("loaded thing");
+		out->type = sub->update_strategy.type;
+		out->command = sub->update_strategy.command;
+	} else
+		out->type = SM_UPDATE_CHECKOUT;
+
+	if (just_cloned &&
+	    (out->type == SM_UPDATE_MERGE ||
+	     out->type == SM_UPDATE_REBASE ||
+	     out->type == SM_UPDATE_NONE))
+		out->type = SM_UPDATE_CHECKOUT;
+
+	free(key);
+}
+
+static int module_update_module_mode(int argc, const char **argv, const char *prefix)
+{
+	const char *path, *update = NULL;
+	int just_cloned;
+	struct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT };
+
+	if (argc < 3 || argc > 4)
+		die("submodule--helper update-module-clone expects <just-cloned> <path> [<update>]");
+
+	just_cloned = git_config_int("just_cloned", argv[1]);
+	path = argv[2];
+
+	if (argc == 4)
+		update = argv[3];
+
+	determine_submodule_update_strategy(the_repository,
+					    just_cloned, path, update,
+					    &update_strategy);
+	fputs(submodule_strategy_to_string(&update_strategy), stdout);
+
+	return 0;
+}
+
 struct update_clone_data {
 	const struct submodule *sub;
 	struct object_id oid;
@@ -2039,6 +2099,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"relative-path", resolve_relative_path, 0},
 	{"resolve-relative-url", resolve_relative_url, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 56588aa304d..215760898ce 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -535,27 +535,13 @@ cmd_update()
 	do
 		die_if_unmatched "$quickabort" "$sha1"
 
-		name=$(git submodule--helper name "$sm_path") || exit
-		if ! test -z "$update"
-		then
-			update_module=$update
-		else
-			update_module=$(git config submodule."$name".update)
-			if test -z "$update_module"
-			then
-				update_module="checkout"
-			fi
-		fi
+		update_module=$(git submodule--helper update-module-mode $just_cloned "$sm_path" $update)
 
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 
 		if test $just_cloned -eq 1
 		then
 			subsha1=
-			case "$update_module" in
-			merge | rebase | none)
-				update_module=checkout ;;
-			esac
 		else
 			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
-- 
2.18.0.233.g985f88cf7e-goog