git-submodule.sh: convert part of cmd_update to C

v2:
addressed review comments, renaming the struct, improving the commit message.

v1:
https://public-inbox.org/git/[email protected]/
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

1:  d4e1ec45740 ! 1:  bbc8697a8ca git-submodule.sh: align error reporting for update mode to use path
    @@ -6,7 +6,6 @@
         on its path, so let's do that for invalid update modes, too.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/git-submodule.sh b/git-submodule.sh
     --- a/git-submodule.sh
2:  9c5ec3fccea ! 2:  7e26af17578 git-submodule.sh: rename unused variables
    @@ -14,8 +14,12 @@
         using its own function starting in 48308681b07 (git submodule update:
         have a dedicated helper for cloning, 2016-02-29), its removal was missed.
     
    +    A later patch in this series also touches the communication between
    +    the submodule helper and git-submodule.sh, but let's have this as
    +    a preparatory patch, as it eases the next patch, which stores the
    +    raw data instead of the line printed for this communication.
    +
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
     --- a/builtin/submodule--helper.c
3:  a3fb4e5539f ! 3:  3e8d22b0c70 builtin/submodule--helper: factor out submodule updating
    @@ -7,7 +7,6 @@
         most of it is still in git-submodule.sh.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
     --- a/builtin/submodule--helper.c
4:  e680684139d ! 4:  5e0a39015df builtin/submodule--helper: store update_clone information in a struct
    @@ -11,7 +11,6 @@
         struct.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
     --- a/builtin/submodule--helper.c
    @@ -20,7 +19,7 @@
      	return 0;
      }
      
    -+struct submodule_update_clone_information {
    ++struct update_clone_data {
     +	const struct submodule *sub;
     +	struct object_id oid;
     +	unsigned just_cloned;
    @@ -36,8 +35,8 @@
     -	/* 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;
    ++	struct update_clone_data *update_clone;
    ++	int update_clone_nr; int update_clone_alloc;
      
      	/* If we want to stop as fast as possible and return an error */
      	unsigned quickstop : 1;
    @@ -58,12 +57,12 @@
     -	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++;
    ++	ALLOC_GROW(suc->update_clone, suc->update_clone_nr + 1,
    ++		   suc->update_clone_alloc);
    ++	oidcpy(&suc->update_clone[suc->update_clone_nr].oid, &ce->oid);
    ++	suc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning;
    ++	suc->update_clone[suc->update_clone_nr].sub = sub;
    ++	suc->update_clone_nr++;
      
      	if (!needs_cloning)
      		goto cleanup;
    @@ -83,11 +82,11 @@
      
     -	for_each_string_list_item(item, &suc->projectlines)
     -		fprintf(stdout, "%s", item->string);
    -+	for (i = 0; i < suc->submodule_lines_nr; i++) {
    ++	for (i = 0; i < suc->update_clone_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);
    ++			oid_to_hex(&suc->update_clone[i].oid),
    ++			suc->update_clone[i].just_cloned,
    ++			suc->update_clone[i].sub->path);
     +		fprintf(stdout, "%s", sb.buf);
     +		strbuf_reset(&sb);
     +	}
5:  95409e47b0d ! 5:  ecee68506eb builtin/submodule--helper: factor out method to update a single submodule
    @@ -5,7 +5,6 @@
         In a later patch we'll find this method handy.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
     --- a/builtin/submodule--helper.c
    @@ -14,12 +13,12 @@
      	return 0;
      }
      
    -+static void update_submodule(struct submodule_update_clone_information *suci)
    ++static void update_submodule(struct update_clone_data *ucd)
     +{
     +	fprintf(stdout, "dummy %s %d\t%s\n",
    -+		oid_to_hex(&suci->oid),
    -+		suci->just_cloned,
    -+		suci->sub->path);
    ++		oid_to_hex(&ucd->oid),
    ++		ucd->just_cloned,
    ++		ucd->sub->path);
     +}
     +
      static int update_submodules(struct submodule_update_clone *suc)
    @@ -33,16 +32,16 @@
      	if (suc->quickstop)
      		return 1;
      
    --	for (i = 0; i < suc->submodule_lines_nr; i++) {
    +-	for (i = 0; i < suc->update_clone_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);
    +-			oid_to_hex(&suc->update_clone[i].oid),
    +-			suc->update_clone[i].just_cloned,
    +-			suc->update_clone[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]);
    ++	for (i = 0; i < suc->update_clone_nr; i++)
    ++		update_submodule(&suc->update_clone[i]);
      
     -	strbuf_release(&sb);
      	return 0;
6:  05bb02e6ea8 ! 6:  b80e60a9d11 submodule--helper: introduce new update-module-mode helper
    @@ -10,7 +10,6 @@
         for arbitrary repositories.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
     --- a/builtin/submodule--helper.c
    @@ -79,7 +78,7 @@
     +	return 0;
     +}
     +
    - struct submodule_update_clone_information {
    + struct update_clone_data {
      	const struct submodule *sub;
      	struct object_id oid;
     @@

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.

---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5f9d9f6ea37..8a611865397 100755
@@ -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

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.

A later patch in this series also touches the communication between the submodule helper and git-submodule.sh, but let's have this as a preparatory patch, as it eases the next patch, which stores the raw data instead of the line printed for this communication.

---
 builtin/submodule--helper.c | 5 ++---
 git-submodule.sh            | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

index 20ae9191ca3..ebcfe85bfa9 100644
@@ -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)

index 8a611865397..56588aa304d 100755
@@ -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

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.

---
 builtin/submodule--helper.c | 59 +++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

index ebcfe85bfa9..96929ba1096 100644
@@ -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

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.

---
 builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

index 96929ba1096..fb54936efc7 100644
@@ -1444,6 +1444,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct update_clone_data {
+	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 update_clone_data *update_clone;
+	int update_clone_nr; int update_clone_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->update_clone, suc->update_clone_nr + 1,
+		   suc->update_clone_alloc);
+	oidcpy(&suc->update_clone[suc->update_clone_nr].oid, &ce->oid);
+	suc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning;
+	suc->update_clone[suc->update_clone_nr].sub = sub;
+	suc->update_clone_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->update_clone_nr; i++) {
+		strbuf_addf(&sb, "dummy %s %d\t%s\n",
+			oid_to_hex(&suc->update_clone[i].oid),
+			suc->update_clone[i].just_cloned,
+			suc->update_clone[i].sub->path);
+		fprintf(stdout, "%s", sb.buf);
+		strbuf_reset(&sb);
+	}
 
+	strbuf_release(&sb);
 	return 0;
 }
 

2.18.0.203.gfac676dfb9-goog

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

---
 builtin/submodule--helper.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

index fb54936efc7..034ba1bb2e0 100644
@@ -1725,10 +1725,17 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 	return 0;
 }
 
+static void update_submodule(struct update_clone_data *ucd)
+{
+	fprintf(stdout, "dummy %s %d\t%s\n",
+		oid_to_hex(&ucd->oid),
+		ucd->just_cloned,
+		ucd->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->update_clone_nr; i++) {
-		strbuf_addf(&sb, "dummy %s %d\t%s\n",
-			oid_to_hex(&suc->update_clone[i].oid),
-			suc->update_clone[i].just_cloned,
-			suc->update_clone[i].sub->path);
-		fprintf(stdout, "%s", sb.buf);
-		strbuf_reset(&sb);
-	}
+	for (i = 0; i < suc->update_clone_nr; i++)
+		update_submodule(&suc->update_clone[i]);
 
-	strbuf_release(&sb);
 	return 0;
 }
 

2.18.0.203.gfac676dfb9-goog

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.

---
 builtin/submodule--helper.c | 61 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 +---------
 2 files changed, 62 insertions(+), 15 deletions(-)

index 034ba1bb2e0..d4cb7c72e33 100644
@@ -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 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},

index 56588aa304d..215760898ce 100755
@@ -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

Sounds good.

A tangent.

Because this "-- " is a conventional signature separator, MUAs like Emacs message-mode seems to omit everything below it from the quote while responding, making it cumbersome to comment on the tbdiff.

Something to think about if somebody is contemplating on adding more to format-patch's cover letter.

This is quite unfortunate. I wonder if it is easy to tell range-diff that certain differences in the log message are to be ignored so that we can show that the first patch is unchanged in a case like this. This series has 4 really changed ones with 2 otherwise unchanged ones shown all as changed, which is not too bad, but for a series like sb/diff-colro-move-more reroll that has 9 patches, out of only two have real updated patches, showing otherwise unchanged 7 as changed like this hunk does would make the cover letter useless. It is a shame that adding range-diff to the cover does have so much potential.

+cc Eric who needs to think about this tangent, then.
https://public-inbox.org/git/[email protected]/

Actually I thought it was really cool, i.e. when using your queued branch instead of my last sent branch, I can see any edits *you* did (including fixing up typos or applying at slightly different bases).

The sign offs are a bit unfortunate as they are repetitive. I have two conflicting points of view on that:

(A) This sign off is inherent to the workflow. So we could change the workflow, i.e. you pull series instead of applying them. I think this "more in git, less in email" workflow would find supporters, such as DScho (cc'd).

The downside is that (1) you'd have to change your
workflow, i.e. instead of applying the patches at the base you think is best for maintenance you'd have to tell the author "please rebase to $X"; but that also has upsides, such as "If you want to have your series integrated please merge with $Y and $Z" (looking at the object store stuff).

The other (2) downside is that everyone else (authors, reviewers) have to adapt as well. For authors this might be easy to adapt (push instead of sending email sounds like a win). For reviewers we'd need to have an easy way to review things "stored in git" and not exposed via email, which is not obvious how to do.

(B) The other point of view that I can offer is that we teach range-diff to ignore certain patterns. Maybe in combination with interpret-trailers this can be an easy configurable thing, or even a default to ignore all sign offs?

Thanks,
Stefan

The "git-format-patch --range-diff" option implemented by that patch series (and its upcoming re-roll) place the range-diff before the "-- " signature line, so this isn't a problem.

I think Junio's tangent was targeted more at humans blindly plopping the copy/pasted range-diff at the end of the cover letter without taking the "-- " signature line into account. (Indeed, Gmail hides everything below the "-- " line by default, as well.)

Hi Eric,

Awesome thanks! (I missed that hint given by Junio)

Now that I grow more accustomed to range-diffs, I wonder if we want to have them even *before* the short stat in the cover letter. (I usually scroll over the short stat just to take it as a FYI on what to expect, whereas the range-diff can already be reviewed, hence seems more useful that the stats)

Thanks,
Stefan

I did consider placing the range-diff before the diffstat, however, what convinced me to position range-diff last was that the diffstat is usually short and easy to skip over both visually and via scrolling, whereas the range-diff often is long and noisy, thus more difficult to skip for someone more interested in the diffstat.

So, the choice was deliberate. However, I also considered making it configurable, but that's probably something that can be done later, if needed, once we get more experience with the feature. It's also worth taking Duy's wish[1] for customization into account, as well, before designing such capability.

[1]: https://public-inbox.org/git/CACsJy8D=6fAEpO5m4cc7KZyggAW1AosSkUWaunQBFH0nr-YrdA@mail.gmail.com/

I find that quite sensible, from a reader's point of view.

Absolutely. I did not say that there needs a mode to ignore log message.

Sign-off is inherent to the project, in the sense that we want to see how the change flowed recorded in the commits.

With a pull-request based workflow, until Git is somehow improved so that a "pull" becomes "fetch and rebase what got fetched on top of my stuff, and add my sign-off while at it" (which is the opposite of the usual "pull --rebase"), we would lose the ability to fully "use" Git to run this project, as we would lose most sign-offs, unlike the e-mail based workflow, which we can use Git fully to have it do its job of recording necessary information.

And much more importantly, when "pull-request" based workflow is improved enough so that your original without my sign-off (and you shouldn't, unless you are relaying my changes) becomes what I pulled, which does have my sign-off, range-diff that compares both histories does need to deal with a pair of commits with only one side having a sign-off. So switching the tool is not a proper solution to work around the "sign-off noise" we observed. One side having a sign-off while the other side does not is inherent to what we actively want, and you are letting your tail wag your dog by suggesting to discard it, which is disappointing.

As I most often edit the log message and material below three-dash lines (long) _after_ format-patch produced files, I do not think it is a win to force me to push and ask to pull.

That indicates the same direction as I alluded to in the message you are responding to, I guess, which is a good thing.

and here is where our thoughts did not align.
I imagined a git-pull as of today (fetch + merge), where you in the role of the maintainer tells me in the role of an author to base my series on top of $X, such that there is no need for rebase on your end, (you would also not sign off each commit, but only the resulting merge) Even merge conflicts could be handed off to the authors instead of burdening you.

I think all needed information would still be there, but there would be an actual change as authors would be committers (as they commit locally and we keep it all in Git to get it to you and you keep it in Git to put it into the blessed repository)

I do not view it as work around, but "another proper workflow that has advantages and disadvantages, one of the advantages is that it would enable us to work with this tool".

[in the current workflow that has proven good for 10 years]

I am suggesting to continue thinking about workflows in general, as there are many; all of them having advantages and disadvantages.
I am not sure if workflows can be adapted easily via improving the current workflow continually or if sometimes a workflow has to be rethought to to changes in the landscape of available tools.

When the Git project started, an email based workflow was chosen, precisely because Git was not available.

Now that it has gained wide spread adoption (among its developers at least) the workflow could adapt to that.

Ah, that is an interesting workflow. Do you keep patch files/emails around locally, only to (long after) add a message and resend it? I try to keep any contribution of mine in Git as long as possible as that helps me tracking and fixing errors in my code and log messages.

Yes, I imagined this is the approach we'll be taking. I think we would want to give %(trailers:none) or rather "ignore-sign-offs" to the inner diffs.

Thanks,
Stefan

The time I "finish" working on a series and commit is typically much closer to the time I format-patch the result, but it will take time for me to actually mail out these files. It will take time to re-review and re-think if the explanation in them makes sense to readers, and sending half-edited patch is something I try to avoid as it would be a waste of time for everybody involved (including me who would then need to respond to messages that helpfully point out silly typoes, in addition to those who spots them).

I am trained myself not to touch these files after sending them out, as comparing them with a newer iteration of the correspoinding files was one way to see what has (and hasn't) changed between iterations before I learned "git tbdiff", and that habit persists.

Hi Stefan,

I think this is just a natural fall-out from the users' choice of mail program. Personally, I have no difficulty commenting on anything below the -- separator.

FWIW GitGitGadget follows the example of the base-commit footer and places this information *above* the -- separator.

This is probably a good indicator that the practice on insisting on signing off on every patch, rather than just the merge commit, is something to re-think.

Those are real changes relative to the original commit, after all, and if they are not desired, they should not be made.

I thought about that myself.

The reason: I was surprised, a couple of times, when I realized long after the fact, that some of my patches were changed without my knowledge nor blessing before being merged into master.

To allow me to protest in a timely manner, I wanted to teach GitGitGadget (which is the main reason I work on range-diff, as you undoubtedly suspect by now) to warn me about such instances.

The range-diff patch series has simmered too long at this stage, though, and I did not try to address such a "ignore <regex>" feature *specifically* so that the range-diff command could be available sooner than later. I already missed one major version, please refrain from forcing me to miss another one.

Ciao,
Dscho

I did not suspect that GGG is the prime motivation for range diff; as it proves useful (a) on its own, e.g. looking at the updates in pu and (b) is helping the current workflow very much.