git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C
@ 2018-07-12 19:47 Stefan Beller
  2018-07-12 19:47 ` [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path Stefan Beller
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path
  2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
@ 2018-07-12 19:47 ` Stefan Beller
  2018-07-12 21:12   ` Junio C Hamano
  2018-07-12 19:47 ` [PATCH 2/6] git-submodule.sh: rename unused variables Stefan Beller
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/6] git-submodule.sh: rename unused variables
  2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
  2018-07-12 19:47 ` [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path Stefan Beller
@ 2018-07-12 19:47 ` Stefan Beller
  2018-07-12 21:24   ` Junio C Hamano
  2018-07-12 19:47 ` [PATCH 3/6] builtin/submodule--helper: factor out submodule updating Stefan Beller
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/6] builtin/submodule--helper: factor out submodule updating
  2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
  2018-07-12 19:47 ` [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path Stefan Beller
  2018-07-12 19:47 ` [PATCH 2/6] git-submodule.sh: rename unused variables Stefan Beller
@ 2018-07-12 19:47 ` Stefan Beller
  2018-07-12 19:47 ` [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct Stefan Beller
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
  2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
                   ` (2 preceding siblings ...)
  2018-07-12 19:47 ` [PATCH 3/6] builtin/submodule--helper: factor out submodule updating Stefan Beller
@ 2018-07-12 19:47 ` Stefan Beller
  2018-07-16 19:37   ` Junio C Hamano
  2018-07-12 19:47 ` [PATCH 5/6] builtin/submodule--helper: factor out method to update a single submodule Stefan Beller
  2018-07-12 19:47 ` [PATCH 6/6] submodule--helper: introduce new update-module-mode helper Stefan Beller
  5 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/6] builtin/submodule--helper: factor out method to update a single submodule
  2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
                   ` (3 preceding siblings ...)
  2018-07-12 19:47 ` [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct Stefan Beller
@ 2018-07-12 19:47 ` Stefan Beller
  2018-07-12 19:47 ` [PATCH 6/6] submodule--helper: introduce new update-module-mode helper Stefan Beller
  5 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/6] submodule--helper: introduce new update-module-mode helper
  2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
                   ` (4 preceding siblings ...)
  2018-07-12 19:47 ` [PATCH 5/6] builtin/submodule--helper: factor out method to update a single submodule Stefan Beller
@ 2018-07-12 19:47 ` Stefan Beller
  2018-07-18 15:00   ` Derrick Stolee
  5 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2018-07-12 19:47 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path
  2018-07-12 19:47 ` [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path Stefan Beller
@ 2018-07-12 21:12   ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2018-07-12 21:12 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

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")

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/6] git-submodule.sh: rename unused variables
  2018-07-12 19:47 ` [PATCH 2/6] git-submodule.sh: rename unused variables Stefan Beller
@ 2018-07-12 21:24   ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2018-07-12 21:24 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

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"

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
  2018-07-12 19:47 ` [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct Stefan Beller
@ 2018-07-16 19:37   ` Junio C Hamano
  2018-07-16 23:55     ` Stefan Beller
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2018-07-16 19:37 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

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).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
  2018-07-16 19:37   ` Junio C Hamano
@ 2018-07-16 23:55     ` Stefan Beller
  0 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2018-07-16 23:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 6/6] submodule--helper: introduce new update-module-mode helper
  2018-07-12 19:47 ` [PATCH 6/6] submodule--helper: introduce new update-module-mode helper Stefan Beller
@ 2018-07-18 15:00   ` Derrick Stolee
  2018-07-18 19:41     ` Stefan Beller
  0 siblings, 1 reply; 13+ messages in thread
From: Derrick Stolee @ 2018-07-18 15:00 UTC (permalink / raw)
  To: Stefan Beller, git

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 6/6] submodule--helper: introduce new update-module-mode helper
  2018-07-18 15:00   ` Derrick Stolee
@ 2018-07-18 19:41     ` Stefan Beller
  0 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2018-07-18 19:41 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: git

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/

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-07-18 19:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-12 19:47 [PATCH 0/6] git-submodule.sh: convert part of cmd_update to C Stefan Beller
2018-07-12 19:47 ` [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path Stefan Beller
2018-07-12 21:12   ` Junio C Hamano
2018-07-12 19:47 ` [PATCH 2/6] git-submodule.sh: rename unused variables Stefan Beller
2018-07-12 21:24   ` Junio C Hamano
2018-07-12 19:47 ` [PATCH 3/6] builtin/submodule--helper: factor out submodule updating Stefan Beller
2018-07-12 19:47 ` [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct Stefan Beller
2018-07-16 19:37   ` Junio C Hamano
2018-07-16 23:55     ` Stefan Beller
2018-07-12 19:47 ` [PATCH 5/6] builtin/submodule--helper: factor out method to update a single submodule Stefan Beller
2018-07-12 19:47 ` [PATCH 6/6] submodule--helper: introduce new update-module-mode helper Stefan Beller
2018-07-18 15:00   ` Derrick Stolee
2018-07-18 19:41     ` Stefan Beller

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).