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

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

[Stefan Beller]

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

v1:
https://public-inbox.org/git/20180712194754.71979-1-sbeller@google.com/
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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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;
     @@

[PATCH v2 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 v2 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.

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 <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 v2 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 v2 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..fb54936efc7 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 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

[PATCH v2 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 fb54936efc7..034ba1bb2e0 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 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

[PATCH v2 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..d4cb7c72e33 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 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.203.gfac676dfb9-goog

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

[SZEDER Gábor]

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 034ba1bb2e0..d4cb7c72e33 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c


> +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));

Various compilers warn about the potential insecurity of the above
call:

      CC builtin/submodule--helper.o
  builtin/submodule--helper.c: In function ‘module_update_module_mode’:
  builtin/submodule--helper.c:1502:2: error: format not a string literal and no format arguments [-Werror=format-security]
    fprintf(stdout, submodule_strategy_to_string(&update_strategy));
    ^
  cc1: all warnings being treated as errors
  Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed
  make: *** [builtin/submodule--helper.o] Error 1

I think it should either use an explicit format string:

  fprintf(stdout, "%s", submodule_strategy_to_string(&update_strategy));

or, perhaps better yet, simply use fputs().


> +
> +	return 0;
> +}

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> v2:
>> addressed review comments, renaming the struct, improving the commit message.
>>
>> v1:
>> https://public-inbox.org/git/20180712194754.71979-1-sbeller@google.com/
>> 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(-)
>>
>> -- 

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.

>> 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 <sbeller@google.com>
>>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>      
>>      diff --git a/git-submodule.sh b/git-submodule.sh
>>      --- a/git-submodule.sh

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.

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

[Stefan Beller]

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

+cc Eric who needs to think about this tangent, then.
https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/

>
> >> 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 <sbeller@google.com>
> >>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >>
> >>      diff --git a/git-submodule.sh b/git-submodule.sh
> >>      --- a/git-submodule.sh
>
> 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.

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

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

[Eric Sunshine]

On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller <sbeller@google.com> wrote:
> > 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.
>
> +cc Eric who needs to think about this tangent, then.
> https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/

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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

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

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

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.

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

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.

> (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?

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

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

[Stefan Beller]

On Tue, Jul 17, 2018 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:

> > (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).
>
> 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"),

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.

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

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)

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

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

>  One side
> having a sign-off while the other side does not is inherent to what
> we actively want,

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

> and you are letting your tail wag your dog by
> suggesting to discard it, which is disappointing.

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.

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

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.

> > (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?
>
> That indicates the same direction as I alluded to in the message you
> are responding to, I guess, which is a good thing.

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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> 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
>
> Ah, that is an interesting workflow. Do you keep patch files/emails
> around locally, only to (long after) add a message and resend it?

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.

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> +	fprintf(stdout, submodule_strategy_to_string(&update_strategy));
>
> Various compilers warn about the potential insecurity of the above
> call:
>
>       CC builtin/submodule--helper.o
>   builtin/submodule--helper.c: In function ‘module_update_module_mode’:
>   builtin/submodule--helper.c:1502:2: error: format not a string literal and no format arguments [-Werror=format-security]
>     fprintf(stdout, submodule_strategy_to_string(&update_strategy));
>     ^
>   cc1: all warnings being treated as errors
>   Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed
>   make: *** [builtin/submodule--helper.o] Error 1
>
> I think it should either use an explicit format string:
>
>   fprintf(stdout, "%s", submodule_strategy_to_string(&update_strategy));
>
> or, perhaps better yet, simply use fputs().

Sounds good.

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

[Stefan Beller]

Hi Eric,
On Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller <sbeller@google.com> wrote:
> > > 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.
> >
> > +cc Eric who needs to think about this tangent, then.
> > https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/
>
> 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.)

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

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

[Eric Sunshine]

On Wed, Jul 18, 2018 at 3:34 PM Stefan Beller <sbeller@google.com> wrote:
> On Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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.
>
> 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)

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/

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

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

[Johannes Schindelin]

Hi Stefan,

On Tue, 17 Jul 2018, Stefan Beller wrote:

> > 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.
> 
> +cc Eric who needs to think about this tangent, then.
> https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/

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.

> > >> 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 <sbeller@google.com>
> > >>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > >>
> > >>      diff --git a/git-submodule.sh b/git-submodule.sh
> > >>      --- a/git-submodule.sh
> >
> > 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.
> 
> 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).

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.

> 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?

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

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

[Stefan Beller]

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

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.