{"thread":{"id":"48893","subject":"[PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","startedAt":"2018-07-17T00:27:01Z","lastAt":"2018-07-26T18:15:05Z","messageCount":20,"participants":["Stefan Beller","SZEDER Gábor","Junio C Hamano","Eric Sunshine","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"352730","messageId":"20180717002654.120375-1-sbeller@google.com","threadId":"48893","inReplyTo":null,"subject":"[PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:48Z","receivedAt":"2018-07-17T00:27:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\naddressed review comments, renaming the struct, improving the commit message.\n\nv1:\nhttps://public-inbox.org/git/20180712194754.71979-1-sbeller@google.com/\nI thought about writing it all in one go, but the series got too large,\nso let's chew one bite at a time.\n\nThanks,\nStefan\n\nStefan Beller (6):\n  git-submodule.sh: align error reporting for update mode to use path\n  git-submodule.sh: rename unused variables\n  builtin/submodule--helper: factor out submodule updating\n  builtin/submodule--helper: store update_clone information in a struct\n  builtin/submodule--helper: factor out method to update a single\n    submodule\n  submodule--helper: introduce new update-module-mode helper\n\n builtin/submodule--helper.c | 152 ++++++++++++++++++++++++++++--------\n git-submodule.sh            |  22 +-----\n 2 files changed, 122 insertions(+), 52 deletions(-)\n\n-- \n2.18.0.203.gfac676dfb9-goog\n\n1:  d4e1ec45740 ! 1:  bbc8697a8ca git-submodule.sh: align error reporting for update mode to use path\n    @@ -6,7 +6,6 @@\n         on its path, so let's do that for invalid update modes, too.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n     diff --git a/git-submodule.sh b/git-submodule.sh\n     --- a/git-submodule.sh\n2:  9c5ec3fccea ! 2:  7e26af17578 git-submodule.sh: rename unused variables\n    @@ -14,8 +14,12 @@\n         using its own function starting in 48308681b07 (git submodule update:\n         have a dedicated helper for cloning, 2016-02-29), its removal was missed.\n     \n    +    A later patch in this series also touches the communication between\n    +    the submodule helper and git-submodule.sh, but let's have this as\n    +    a preparatory patch, as it eases the next patch, which stores the\n    +    raw data instead of the line printed for this communication.\n    +\n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n     --- a/builtin/submodule--helper.c\n3:  a3fb4e5539f ! 3:  3e8d22b0c70 builtin/submodule--helper: factor out submodule updating\n    @@ -7,7 +7,6 @@\n         most of it is still in git-submodule.sh.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n     --- a/builtin/submodule--helper.c\n4:  e680684139d ! 4:  5e0a39015df builtin/submodule--helper: store update_clone information in a struct\n    @@ -11,7 +11,6 @@\n         struct.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n     --- a/builtin/submodule--helper.c\n    @@ -20,7 +19,7 @@\n      \treturn 0;\n      }\n      \n    -+struct submodule_update_clone_information {\n    ++struct update_clone_data {\n     +\tconst struct submodule *sub;\n     +\tstruct object_id oid;\n     +\tunsigned just_cloned;\n    @@ -36,8 +35,8 @@\n     -\t/* Machine-readable status lines to be consumed by git-submodule.sh */\n     -\tstruct string_list projectlines;\n     +\t/* to be consumed by git-submodule.sh */\n    -+\tstruct submodule_update_clone_information *submodule_lines;\n    -+\tint submodule_lines_nr; int submodule_lines_alloc;\n    ++\tstruct update_clone_data *update_clone;\n    ++\tint update_clone_nr; int update_clone_alloc;\n      \n      \t/* If we want to stop as fast as possible and return an error */\n      \tunsigned quickstop : 1;\n    @@ -58,12 +57,12 @@\n     -\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n     -\t\t    oid_to_hex(&ce->oid), needs_cloning, ce->name);\n     -\tstring_list_append(&suc->projectlines, sb.buf);\n    -+\tALLOC_GROW(suc->submodule_lines, suc->submodule_lines_nr + 1,\n    -+\t\t\t\t\t suc->submodule_lines_alloc);\n    -+\toidcpy(&suc->submodule_lines[suc->submodule_lines_nr].oid, &ce->oid);\n    -+\tsuc->submodule_lines[suc->submodule_lines_nr].just_cloned = needs_cloning;\n    -+\tsuc->submodule_lines[suc->submodule_lines_nr].sub = sub;\n    -+\tsuc->submodule_lines_nr++;\n    ++\tALLOC_GROW(suc->update_clone, suc->update_clone_nr + 1,\n    ++\t\t   suc->update_clone_alloc);\n    ++\toidcpy(&suc->update_clone[suc->update_clone_nr].oid, &ce->oid);\n    ++\tsuc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning;\n    ++\tsuc->update_clone[suc->update_clone_nr].sub = sub;\n    ++\tsuc->update_clone_nr++;\n      \n      \tif (!needs_cloning)\n      \t\tgoto cleanup;\n    @@ -83,11 +82,11 @@\n      \n     -\tfor_each_string_list_item(item, &suc->projectlines)\n     -\t\tfprintf(stdout, \"%s\", item->string);\n    -+\tfor (i = 0; i < suc->submodule_lines_nr; i++) {\n    ++\tfor (i = 0; i < suc->update_clone_nr; i++) {\n     +\t\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n    -+\t\t\toid_to_hex(&suc->submodule_lines[i].oid),\n    -+\t\t\tsuc->submodule_lines[i].just_cloned,\n    -+\t\t\tsuc->submodule_lines[i].sub->path);\n    ++\t\t\toid_to_hex(&suc->update_clone[i].oid),\n    ++\t\t\tsuc->update_clone[i].just_cloned,\n    ++\t\t\tsuc->update_clone[i].sub->path);\n     +\t\tfprintf(stdout, \"%s\", sb.buf);\n     +\t\tstrbuf_reset(&sb);\n     +\t}\n5:  95409e47b0d ! 5:  ecee68506eb builtin/submodule--helper: factor out method to update a single submodule\n    @@ -5,7 +5,6 @@\n         In a later patch we'll find this method handy.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n     --- a/builtin/submodule--helper.c\n    @@ -14,12 +13,12 @@\n      \treturn 0;\n      }\n      \n    -+static void update_submodule(struct submodule_update_clone_information *suci)\n    ++static void update_submodule(struct update_clone_data *ucd)\n     +{\n     +\tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n    -+\t\toid_to_hex(&suci->oid),\n    -+\t\tsuci->just_cloned,\n    -+\t\tsuci->sub->path);\n    ++\t\toid_to_hex(&ucd->oid),\n    ++\t\tucd->just_cloned,\n    ++\t\tucd->sub->path);\n     +}\n     +\n      static int update_submodules(struct submodule_update_clone *suc)\n    @@ -33,16 +32,16 @@\n      \tif (suc->quickstop)\n      \t\treturn 1;\n      \n    --\tfor (i = 0; i < suc->submodule_lines_nr; i++) {\n    +-\tfor (i = 0; i < suc->update_clone_nr; i++) {\n     -\t\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n    --\t\t\toid_to_hex(&suc->submodule_lines[i].oid),\n    --\t\t\tsuc->submodule_lines[i].just_cloned,\n    --\t\t\tsuc->submodule_lines[i].sub->path);\n    +-\t\t\toid_to_hex(&suc->update_clone[i].oid),\n    +-\t\t\tsuc->update_clone[i].just_cloned,\n    +-\t\t\tsuc->update_clone[i].sub->path);\n     -\t\tfprintf(stdout, \"%s\", sb.buf);\n     -\t\tstrbuf_reset(&sb);\n     -\t}\n    -+\tfor (i = 0; i < suc->submodule_lines_nr; i++)\n    -+\t\tupdate_submodule(&suc->submodule_lines[i]);\n    ++\tfor (i = 0; i < suc->update_clone_nr; i++)\n    ++\t\tupdate_submodule(&suc->update_clone[i]);\n      \n     -\tstrbuf_release(&sb);\n      \treturn 0;\n6:  05bb02e6ea8 ! 6:  b80e60a9d11 submodule--helper: introduce new update-module-mode helper\n    @@ -10,7 +10,6 @@\n         for arbitrary repositories.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n     diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n     --- a/builtin/submodule--helper.c\n    @@ -79,7 +78,7 @@\n     +\treturn 0;\n     +}\n     +\n    - struct submodule_update_clone_information {\n    + struct update_clone_data {\n      \tconst struct submodule *sub;\n      \tstruct object_id oid;\n     @@\n"},{"id":"352731","messageId":"20180717002654.120375-2-sbeller@google.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"[PATCH v2 1/6] git-submodule.sh: align error reporting for update mode to use path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:49Z","receivedAt":"2018-07-17T00:27:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All other error messages in cmd_update are reporting the submodule based\non its path, so let's do that for invalid update modes, too.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5f9d9f6ea37..8a611865397 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -627,7 +627,7 @@ cmd_update()\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n+\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n \t\t\tesac\n \n \t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352732","messageId":"20180717002654.120375-3-sbeller@google.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"[PATCH v2 2/6] git-submodule.sh: rename unused variables","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:50Z","receivedAt":"2018-07-17T00:27:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 'mode' variable is not used in cmd_update for its original purpose,\nrename it to 'dummy' as it only serves the purpose to abort quickly\ndocumenting this knowledge.\n\nThe variable 'stage' is also not used any more in cmd_update, so remove it.\n\nThis went unnoticed as first each function used the commonly used\nsubmodule listing, which was converted in 74703a1e4df (submodule: rewrite\n`module_list` shell function in C, 2015-09-02). When cmd_update was\nusing its own function starting in 48308681b07 (git submodule update:\nhave a dedicated helper for cloning, 2016-02-29), its removal was missed.\n\nA later patch in this series also touches the communication between\nthe submodule helper and git-submodule.sh, but let's have this as\na preparatory patch, as it eases the next patch, which stores the\nraw data instead of the line printed for this communication.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 5 ++---\n git-submodule.sh            | 4 ++--\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 20ae9191ca3..ebcfe85bfa9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1571,9 +1571,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tneeds_cloning = !file_exists(sb.buf);\n \n \tstrbuf_reset(&sb);\n-\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n-\t\t\toid_to_hex(&ce->oid), ce_stage(ce),\n-\t\t\tneeds_cloning, ce->name);\n+\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n+\t\t    oid_to_hex(&ce->oid), needs_cloning, ce->name);\n \tstring_list_append(&suc->projectlines, sb.buf);\n \n \tif (!needs_cloning)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8a611865397..56588aa304d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -531,9 +531,9 @@ cmd_update()\n \t\t\"$@\" || echo \"#unmatched\" $?\n \t} | {\n \terr=\n-\twhile read -r mode sha1 stage just_cloned sm_path\n+\twhile read -r quickabort sha1 just_cloned sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n+\t\tdie_if_unmatched \"$quickabort\" \"$sha1\"\n \n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\tif ! test -z \"$update\"\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352733","messageId":"20180717002654.120375-4-sbeller@google.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"[PATCH v2 3/6] builtin/submodule--helper: factor out submodule updating","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:51Z","receivedAt":"2018-07-17T00:27:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Separate the command line parsing from the actual execution of the command\nwithin the repository. For now there is not a lot of execution as\nmost of it is still in git-submodule.sh.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 59 +++++++++++++++++++++----------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ebcfe85bfa9..96929ba1096 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1472,6 +1472,8 @@ struct submodule_update_clone {\n \t/* failed clones to be retried again */\n \tconst struct cache_entry **failed_clones;\n \tint failed_clones_nr, failed_clones_alloc;\n+\n+\tint max_jobs;\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n@@ -1714,11 +1716,36 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static int update_submodules(struct submodule_update_clone *suc)\n+{\n+\tstruct string_list_item *item;\n+\n+\trun_processes_parallel(suc->max_jobs,\n+\t\t\t       update_clone_get_next_task,\n+\t\t\t       update_clone_start_failure,\n+\t\t\t       update_clone_task_finished,\n+\t\t\t       suc);\n+\n+\t/*\n+\t * We saved the output and put it out all at once now.\n+\t * That means:\n+\t * - the listener does not have to interleave their (checkout)\n+\t *   work with our fetching.  The writes involved in a\n+\t *   checkout involve more straightforward sequential I/O.\n+\t * - the listener can avoid doing any work if fetching failed.\n+\t */\n+\tif (suc->quickstop)\n+\t\treturn 1;\n+\n+\tfor_each_string_list_item(item, &suc->projectlines)\n+\t\tfprintf(stdout, \"%s\", item->string);\n+\n+\treturn 0;\n+}\n+\n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n \tconst char *update = NULL;\n-\tint max_jobs = 1;\n-\tstruct string_list_item *item;\n \tstruct pathspec pathspec;\n \tstruct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;\n \n@@ -1740,7 +1767,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &suc.depth, \"<depth>\",\n \t\t\t   N_(\"Create a shallow clone truncated to the \"\n \t\t\t      \"specified number of revisions\")),\n-\t\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\tOPT_INTEGER('j', \"jobs\", &suc.max_jobs,\n \t\t\t    N_(\"parallel jobs\")),\n \t\tOPT_BOOL(0, \"recommend-shallow\", &suc.recommend_shallow,\n \t\t\t    N_(\"whether the initial clone should follow the shallow recommendation\")),\n@@ -1756,8 +1783,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t};\n \tsuc.prefix = prefix;\n \n-\tconfig_from_gitmodules(gitmodules_update_clone_config, &max_jobs);\n-\tgit_config(gitmodules_update_clone_config, &max_jobs);\n+\tconfig_from_gitmodules(gitmodules_update_clone_config, &suc.max_jobs);\n+\tgit_config(gitmodules_update_clone_config, &suc.max_jobs);\n \n \targc = parse_options(argc, argv, prefix, module_update_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\n@@ -1772,27 +1799,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tif (pathspec.nr)\n \t\tsuc.warn_if_uninitialized = 1;\n \n-\trun_processes_parallel(max_jobs,\n-\t\t\t       update_clone_get_next_task,\n-\t\t\t       update_clone_start_failure,\n-\t\t\t       update_clone_task_finished,\n-\t\t\t       &suc);\n-\n-\t/*\n-\t * We saved the output and put it out all at once now.\n-\t * That means:\n-\t * - the listener does not have to interleave their (checkout)\n-\t *   work with our fetching.  The writes involved in a\n-\t *   checkout involve more straightforward sequential I/O.\n-\t * - the listener can avoid doing any work if fetching failed.\n-\t */\n-\tif (suc.quickstop)\n-\t\treturn 1;\n-\n-\tfor_each_string_list_item(item, &suc.projectlines)\n-\t\tfprintf(stdout, \"%s\", item->string);\n-\n-\treturn 0;\n+\treturn update_submodules(&suc);\n }\n \n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352734","messageId":"20180717002654.120375-5-sbeller@google.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"[PATCH v2 4/6] builtin/submodule--helper: store update_clone information in a struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:52Z","receivedAt":"2018-07-17T00:27:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The information that is printed for update_submodules in\n'submodule--helper update-clone' and consumed by 'git submodule update'\nis stored as a string per submodule. This made sense at the time of\n48308681b07 (git submodule update: have a dedicated helper for cloning,\n2016-02-29), but as we want to migrate the rest of the submodule update\ninto C, we're better off having access to the raw information in a helper\nstruct.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++----------\n 1 file changed, 27 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 96929ba1096..fb54936efc7 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1444,6 +1444,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct update_clone_data {\n+\tconst struct submodule *sub;\n+\tstruct object_id oid;\n+\tunsigned just_cloned;\n+};\n+\n struct submodule_update_clone {\n \t/* index into 'list', the list of submodules to look into for cloning */\n \tint current;\n@@ -1463,8 +1469,9 @@ struct submodule_update_clone {\n \tconst char *recursive_prefix;\n \tconst char *prefix;\n \n-\t/* Machine-readable status lines to be consumed by git-submodule.sh */\n-\tstruct string_list projectlines;\n+\t/* to be consumed by git-submodule.sh */\n+\tstruct update_clone_data *update_clone;\n+\tint update_clone_nr; int update_clone_alloc;\n \n \t/* If we want to stop as fast as possible and return an error */\n \tunsigned quickstop : 1;\n@@ -1478,7 +1485,7 @@ struct submodule_update_clone {\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n \tNULL, NULL, NULL, \\\n-\tSTRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n+\tNULL, 0, 0, 0, NULL, 0, 0, 0}\n \n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n@@ -1572,10 +1579,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n \tneeds_cloning = !file_exists(sb.buf);\n \n-\tstrbuf_reset(&sb);\n-\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n-\t\t    oid_to_hex(&ce->oid), needs_cloning, ce->name);\n-\tstring_list_append(&suc->projectlines, sb.buf);\n+\tALLOC_GROW(suc->update_clone, suc->update_clone_nr + 1,\n+\t\t   suc->update_clone_alloc);\n+\toidcpy(&suc->update_clone[suc->update_clone_nr].oid, &ce->oid);\n+\tsuc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning;\n+\tsuc->update_clone[suc->update_clone_nr].sub = sub;\n+\tsuc->update_clone_nr++;\n \n \tif (!needs_cloning)\n \t\tgoto cleanup;\n@@ -1718,7 +1727,8 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \n static int update_submodules(struct submodule_update_clone *suc)\n {\n-\tstruct string_list_item *item;\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \trun_processes_parallel(suc->max_jobs,\n \t\t\t       update_clone_get_next_task,\n@@ -1737,9 +1747,16 @@ static int update_submodules(struct submodule_update_clone *suc)\n \tif (suc->quickstop)\n \t\treturn 1;\n \n-\tfor_each_string_list_item(item, &suc->projectlines)\n-\t\tfprintf(stdout, \"%s\", item->string);\n+\tfor (i = 0; i < suc->update_clone_nr; i++) {\n+\t\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n+\t\t\toid_to_hex(&suc->update_clone[i].oid),\n+\t\t\tsuc->update_clone[i].just_cloned,\n+\t\t\tsuc->update_clone[i].sub->path);\n+\t\tfprintf(stdout, \"%s\", sb.buf);\n+\t\tstrbuf_reset(&sb);\n+\t}\n \n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352735","messageId":"20180717002654.120375-6-sbeller@google.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"[PATCH v2 5/6] builtin/submodule--helper: factor out method to update a single submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:53Z","receivedAt":"2018-07-17T00:27:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we'll find this method handy.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fb54936efc7..034ba1bb2e0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1725,10 +1725,17 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static void update_submodule(struct update_clone_data *ucd)\n+{\n+\tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n+\t\toid_to_hex(&ucd->oid),\n+\t\tucd->just_cloned,\n+\t\tucd->sub->path);\n+}\n+\n static int update_submodules(struct submodule_update_clone *suc)\n {\n \tint i;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \trun_processes_parallel(suc->max_jobs,\n \t\t\t       update_clone_get_next_task,\n@@ -1747,16 +1754,9 @@ static int update_submodules(struct submodule_update_clone *suc)\n \tif (suc->quickstop)\n \t\treturn 1;\n \n-\tfor (i = 0; i < suc->update_clone_nr; i++) {\n-\t\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n-\t\t\toid_to_hex(&suc->update_clone[i].oid),\n-\t\t\tsuc->update_clone[i].just_cloned,\n-\t\t\tsuc->update_clone[i].sub->path);\n-\t\tfprintf(stdout, \"%s\", sb.buf);\n-\t\tstrbuf_reset(&sb);\n-\t}\n+\tfor (i = 0; i < suc->update_clone_nr; i++)\n+\t\tupdate_submodule(&suc->update_clone[i]);\n \n-\tstrbuf_release(&sb);\n \treturn 0;\n }\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352736","messageId":"20180717002654.120375-7-sbeller@google.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"[PATCH v2 6/6] submodule--helper: introduce new update-module-mode helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T00:26:54Z","receivedAt":"2018-07-17T00:27:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This chews off a bit of the shell part of the update command in\ngit-submodule.sh. When writing the C code, keep in mind that the\nsubmodule--helper part will go away eventually and we want to have\na C function that is able to determine the submodule update strategy,\nit as a nicety, make determine_submodule_update_strategy accessible\nfor arbitrary repositories.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 61 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 16 +---------\n 2 files changed, 62 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 034ba1bb2e0..d4cb7c72e33 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1444,6 +1444,66 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void determine_submodule_update_strategy(struct repository *r,\n+\t\t\t\t\t\tint just_cloned,\n+\t\t\t\t\t\tconst char *path,\n+\t\t\t\t\t\tconst char *update,\n+\t\t\t\t\t\tstruct submodule_update_strategy *out)\n+{\n+\tconst struct submodule *sub = submodule_from_path(r, &null_oid, path);\n+\tchar *key;\n+\tconst char *val;\n+\n+\tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n+\n+\tif (update) {\n+\t\ttrace_printf(\"parsing update\");\n+\t\tif (parse_submodule_update_strategy(update, out) < 0)\n+\t\t\tdie(_(\"Invalid update mode '%s' for submodule path '%s'\"),\n+\t\t\t\tupdate, path);\n+\t} else if (!repo_config_get_string_const(r, key, &val)) {\n+\t\tif (parse_submodule_update_strategy(val, out) < 0)\n+\t\t\tdie(_(\"Invalid update mode '%s' configured for submodule path '%s'\"),\n+\t\t\t\tval, path);\n+\t} else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n+\t\ttrace_printf(\"loaded thing\");\n+\t\tout->type = sub->update_strategy.type;\n+\t\tout->command = sub->update_strategy.command;\n+\t} else\n+\t\tout->type = SM_UPDATE_CHECKOUT;\n+\n+\tif (just_cloned &&\n+\t    (out->type == SM_UPDATE_MERGE ||\n+\t     out->type == SM_UPDATE_REBASE ||\n+\t     out->type == SM_UPDATE_NONE))\n+\t\tout->type = SM_UPDATE_CHECKOUT;\n+\n+\tfree(key);\n+}\n+\n+static int module_update_module_mode(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *path, *update = NULL;\n+\tint just_cloned;\n+\tstruct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT };\n+\n+\tif (argc < 3 || argc > 4)\n+\t\tdie(\"submodule--helper update-module-clone expects <just-cloned> <path> [<update>]\");\n+\n+\tjust_cloned = git_config_int(\"just_cloned\", argv[1]);\n+\tpath = argv[2];\n+\n+\tif (argc == 4)\n+\t\tupdate = argv[3];\n+\n+\tdetermine_submodule_update_strategy(the_repository,\n+\t\t\t\t\t    just_cloned, path, update,\n+\t\t\t\t\t    &update_strategy);\n+\tfprintf(stdout, submodule_strategy_to_string(&update_strategy));\n+\n+\treturn 0;\n+}\n+\n struct update_clone_data {\n \tconst struct submodule *sub;\n \tstruct object_id oid;\n@@ -2039,6 +2099,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n+\t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 56588aa304d..215760898ce 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -535,27 +535,13 @@ cmd_update()\n \tdo\n \t\tdie_if_unmatched \"$quickabort\" \"$sha1\"\n \n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tif ! test -z \"$update\"\n-\t\tthen\n-\t\t\tupdate_module=$update\n-\t\telse\n-\t\t\tupdate_module=$(git config submodule.\"$name\".update)\n-\t\t\tif test -z \"$update_module\"\n-\t\t\tthen\n-\t\t\t\tupdate_module=\"checkout\"\n-\t\t\tfi\n-\t\tfi\n+\t\tupdate_module=$(git submodule--helper update-module-mode $just_cloned \"$sm_path\" $update)\n \n \t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n \n \t\tif test $just_cloned -eq 1\n \t\tthen\n \t\t\tsubsha1=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tmerge | rebase | none)\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n \t\telse\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352745","messageId":"20180717075959.30594-1-szeder.dev@gmail.com","threadId":"48893","inReplyTo":"20180717002654.120375-7-sbeller@google.com","subject":"Re: [PATCH v2 6/6] submodule--helper: introduce new update-module-mode helper","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-17T07:59:59Z","receivedAt":"2018-07-17T08:00:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 034ba1bb2e0..d4cb7c72e33 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n\n\n> +static int module_update_module_mode(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *path, *update = NULL;\n> +\tint just_cloned;\n> +\tstruct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT };\n> +\n> +\tif (argc < 3 || argc > 4)\n> +\t\tdie(\"submodule--helper update-module-clone expects <just-cloned> <path> [<update>]\");\n> +\n> +\tjust_cloned = git_config_int(\"just_cloned\", argv[1]);\n> +\tpath = argv[2];\n> +\n> +\tif (argc == 4)\n> +\t\tupdate = argv[3];\n> +\n> +\tdetermine_submodule_update_strategy(the_repository,\n> +\t\t\t\t\t    just_cloned, path, update,\n> +\t\t\t\t\t    &update_strategy);\n> +\tfprintf(stdout, submodule_strategy_to_string(&update_strategy));\n\nVarious compilers warn about the potential insecurity of the above\ncall:\n\n      CC builtin/submodule--helper.o\n  builtin/submodule--helper.c: In function ‘module_update_module_mode’:\n  builtin/submodule--helper.c:1502:2: error: format not a string literal and no format arguments [-Werror=format-security]\n    fprintf(stdout, submodule_strategy_to_string(&update_strategy));\n    ^\n  cc1: all warnings being treated as errors\n  Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed\n  make: *** [builtin/submodule--helper.o] Error 1\n\nI think it should either use an explicit format string:\n\n  fprintf(stdout, \"%s\", submodule_strategy_to_string(&update_strategy));\n\nor, perhaps better yet, simply use fputs().\n\n\n> +\n> +\treturn 0;\n> +}\n"},{"id":"352814","messageId":"xmqqmuupogei.fsf@gitster-ct.c.googlers.com","threadId":"48893","inReplyTo":"20180717002654.120375-1-sbeller@google.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T18:39:49Z","receivedAt":"2018-07-17T18:39:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> v2:\n>> addressed review comments, renaming the struct, improving the commit message.\n>>\n>> v1:\n>> https://public-inbox.org/git/20180712194754.71979-1-sbeller@google.com/\n>> I thought about writing it all in one go, but the series got too large,\n>> so let's chew one bite at a time.\n>>\n>> Thanks,\n>> Stefan\n>>\n>> Stefan Beller (6):\n>>   git-submodule.sh: align error reporting for update mode to use path\n>>   git-submodule.sh: rename unused variables\n>>   builtin/submodule--helper: factor out submodule updating\n>>   builtin/submodule--helper: store update_clone information in a struct\n>>   builtin/submodule--helper: factor out method to update a single\n>>     submodule\n>>   submodule--helper: introduce new update-module-mode helper\n>>\n>>  builtin/submodule--helper.c | 152 ++++++++++++++++++++++++++++--------\n>>  git-submodule.sh            |  22 +-----\n>>  2 files changed, 122 insertions(+), 52 deletions(-)\n>>\n>> -- \n\nA tangent.\n\nBecause this \"-- \" is a conventional signature separator, MUAs like\nEmacs message-mode seems to omit everything below it from the quote\nwhile responding, making it cumbersome to comment on the tbdiff.\n\nSomething to think about if somebody is contemplating on adding more\nto format-patch's cover letter.\n\n>> 2.18.0.203.gfac676dfb9-goog\n>>\n>> 1:  d4e1ec45740 ! 1:  bbc8697a8ca git-submodule.sh: align error reporting for update mode to use path\n>>     @@ -6,7 +6,6 @@\n>>          on its path, so let's do that for invalid update modes, too.\n>>      \n>>          Signed-off-by: Stefan Beller <sbeller@google.com>\n>>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>>      \n>>      diff --git a/git-submodule.sh b/git-submodule.sh\n>>      --- a/git-submodule.sh\n\nThis is quite unfortunate.  I wonder if it is easy to tell\nrange-diff that certain differences in the log message are to be\nignored so that we can show that the first patch is unchanged in a\ncase like this.  This series has 4 really changed ones with 2\notherwise unchanged ones shown all as changed, which is not too bad,\nbut for a series like sb/diff-colro-move-more reroll that has 9\npatches, out of only two have real updated patches, showing\notherwise unchanged 7 as changed like this hunk does would make the\ncover letter useless.  It is a shame that adding range-diff to the\ncover does have so much potential.\n\n"},{"id":"352816","messageId":"CAGZ79kZtrmjODGi1j-HRbchZYuALHnms22=wWBV1AF+zUBru_g@mail.gmail.com","threadId":"48893","inReplyTo":"xmqqmuupogei.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T18:53:29Z","receivedAt":"2018-07-17T18:53:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> A tangent.\n>\n> Because this \"-- \" is a conventional signature separator, MUAs like\n> Emacs message-mode seems to omit everything below it from the quote\n> while responding, making it cumbersome to comment on the tbdiff.\n>\n> Something to think about if somebody is contemplating on adding more\n> to format-patch's cover letter.\n\n+cc Eric who needs to think about this tangent, then.\nhttps://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/\n\n>\n> >> 2.18.0.203.gfac676dfb9-goog\n> >>\n> >> 1:  d4e1ec45740 ! 1:  bbc8697a8ca git-submodule.sh: align error reporting for update mode to use path\n> >>     @@ -6,7 +6,6 @@\n> >>          on its path, so let's do that for invalid update modes, too.\n> >>\n> >>          Signed-off-by: Stefan Beller <sbeller@google.com>\n> >>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> >>\n> >>      diff --git a/git-submodule.sh b/git-submodule.sh\n> >>      --- a/git-submodule.sh\n>\n> This is quite unfortunate.  I wonder if it is easy to tell\n> range-diff that certain differences in the log message are to be\n> ignored so that we can show that the first patch is unchanged in a\n> case like this.  This series has 4 really changed ones with 2\n> otherwise unchanged ones shown all as changed, which is not too bad,\n> but for a series like sb/diff-colro-move-more reroll that has 9\n> patches, out of only two have real updated patches, showing\n> otherwise unchanged 7 as changed like this hunk does would make the\n> cover letter useless.  It is a shame that adding range-diff to the\n> cover does have so much potential.\n\nActually I thought it was really cool, i.e. when using your queued branch\ninstead of my last sent branch, I can see any edits *you* did\n(including fixing up typos or applying at slightly different bases).\n\nThe sign offs are a bit unfortunate as they are repetitive.\nI have two conflicting points of view on that:\n\n(A) This sign off is inherent to the workflow. So we could\nchange the workflow, i.e. you pull series instead of applying them.\nI think this \"more in git, less in email\" workflow would find supporters,\nsuch as DScho (cc'd).\n\nThe downside is that (1) you'd have to change your\nworkflow, i.e. instead of applying the patches at the base you think is\nbest for maintenance you'd have to tell the author \"please rebase to $X\";\nbut that also has upsides, such as \"If you want to have your series integrated\nplease merge with $Y and $Z\" (looking at the object store stuff).\n\nThe other (2) downside is that everyone else (authors, reviewers) have\nto adapt as well. For authors this might be easy to adapt (push instead\nof sending email sounds like a win). For reviewers we'd need to have\nan easy way to review things \"stored in git\" and not exposed via email,\nwhich is not obvious how to do.\n\n(B) The other point of view that I can offer is that we teach range-diff\nto ignore certain patterns. Maybe in combination with interpret-trailers\nthis can be an easy configurable thing, or even a default to ignore\nall sign offs?\n\nThanks,\nStefan\n"},{"id":"352818","messageId":"CAPig+cRx5-2TYOm_8oayFfbKGpmTJf=M0cNR3L5UJGGC6vHPDQ@mail.gmail.com","threadId":"48893","inReplyTo":"CAGZ79kZtrmjODGi1j-HRbchZYuALHnms22=wWBV1AF+zUBru_g@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-17T18:59:39Z","receivedAt":"2018-07-17T18:59:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller <sbeller@google.com> wrote:\n> > A tangent.\n> >\n> > Because this \"-- \" is a conventional signature separator, MUAs like\n> > Emacs message-mode seems to omit everything below it from the quote\n> > while responding, making it cumbersome to comment on the tbdiff.\n> >\n> > Something to think about if somebody is contemplating on adding more\n> > to format-patch's cover letter.\n>\n> +cc Eric who needs to think about this tangent, then.\n> https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/\n\nThe \"git-format-patch --range-diff\" option implemented by that patch\nseries (and its upcoming re-roll) place the range-diff before the \"--\n\" signature line, so this isn't a problem.\n\nI think Junio's tangent was targeted more at humans blindly plopping\nthe copy/pasted range-diff at the end of the cover letter without\ntaking the \"-- \" signature line into account. (Indeed, Gmail hides\neverything below the \"-- \" line by default, as well.)\n"},{"id":"352823","messageId":"xmqqa7qpod29.fsf@gitster-ct.c.googlers.com","threadId":"48893","inReplyTo":"CAGZ79kZtrmjODGi1j-HRbchZYuALHnms22=wWBV1AF+zUBru_g@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T19:51:58Z","receivedAt":"2018-07-17T19:52:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Actually I thought it was really cool, i.e. when using your queued branch\n> instead of my last sent branch, I can see any edits *you* did\n> (including fixing up typos or applying at slightly different bases).\n\nAbsolutely.  I did not say that there needs a mode to ignore log\nmessage.\n\n> The sign offs are a bit unfortunate as they are repetitive.\n> I have two conflicting points of view on that:\n>\n> (A) This sign off is inherent to the workflow. So we could\n> change the workflow, i.e. you pull series instead of applying them.\n> I think this \"more in git, less in email\" workflow would find supporters,\n> such as DScho (cc'd).\n\nSign-off is inherent to the project, in the sense that we want to\nsee how the change flowed recorded in the commits.\n\nWith a pull-request based workflow, until Git is somehow improved so\nthat a \"pull\" becomes \"fetch and rebase what got fetched on top of\nmy stuff, and add my sign-off while at it\" (which is the opposite of\nthe usual \"pull --rebase\"), we would lose the ability to fully \"use\"\nGit to run this project, as we would lose most sign-offs, unlike the\ne-mail based workflow, which we can use Git fully to have it do its\njob of recording necessary information.\n\nAnd much more importantly, when \"pull-request\" based workflow is\nimproved enough so that your original without my sign-off (and you\nshouldn't, unless you are relaying my changes) becomes what I\npulled, which does have my sign-off, range-diff that compares both\nhistories does need to deal with a pair of commits with only one\nside having a sign-off.  So switching the tool is not a proper\nsolution to work around the \"sign-off noise\" we observed.  One side\nhaving a sign-off while the other side does not is inherent to what\nwe actively want, and you are letting your tail wag your dog by\nsuggesting to discard it, which is disappointing.\n\n> The other (2) downside is that everyone else (authors, reviewers) have\n> to adapt as well. For authors this might be easy to adapt (push instead\n> of sending email sounds like a win).\n\nAs I most often edit the log message and material below three-dash\nlines (long) _after_ format-patch produced files, I do not think it\nis a win to force me to push and ask to pull.\n\n> (B) The other point of view that I can offer is that we teach range-diff\n> to ignore certain patterns. Maybe in combination with interpret-trailers\n> this can be an easy configurable thing, or even a default to ignore\n> all sign offs?\n\nThat indicates the same direction as I alluded to in the message you\nare responding to, I guess, which is a good thing.\n\n"},{"id":"352831","messageId":"CAGZ79kbYtws-HhX_kQ5Gb4FeAfgRjbCfVbPS1Gcow4xfN-04XQ@mail.gmail.com","threadId":"48893","inReplyTo":"xmqqa7qpod29.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T20:56:48Z","receivedAt":"2018-07-17T20:57:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 17, 2018 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n\n> > (A) This sign off is inherent to the workflow. So we could\n> > change the workflow, i.e. you pull series instead of applying them.\n> > I think this \"more in git, less in email\" workflow would find supporters,\n> > such as DScho (cc'd).\n>\n> Sign-off is inherent to the project, in the sense that we want to\n> see how the change flowed recorded in the commits.\n>\n> With a pull-request based workflow, until Git is somehow improved so\n> that a \"pull\" becomes \"fetch and rebase what got fetched on top of\n> my stuff, and add my sign-off while at it\" (which is the opposite of\n> the usual \"pull --rebase\"),\n\nand here is where our thoughts did not align.\nI imagined a git-pull as of today (fetch + merge), where you in the role\nof the maintainer tells me in the role of an author to base my series\non top of $X, such that there is no need for rebase on your end,\n(you would also not sign off each commit, but only the resulting merge)\nEven merge conflicts could be handed off to the authors instead of\nburdening you.\n\n>  we would lose the ability to fully \"use\"\n> Git to run this project, as we would lose most sign-offs, unlike the\n> e-mail based workflow, which we can use Git fully to have it do its\n> job of recording necessary information.\n\nI think all needed information would still be there, but there would be an\nactual change as authors would be committers (as they commit locally\nand we keep it all in Git to get it to you and you keep it in Git to put it\ninto the blessed repository)\n\n> And much more importantly, when \"pull-request\" based workflow is\n> improved enough so that your original without my sign-off (and you\n> shouldn't, unless you are relaying my changes) becomes what I\n> pulled, which does have my sign-off, range-diff that compares both\n> histories does need to deal with a pair of commits with only one\n> side having a sign-off.  So switching the tool is not a proper\n> solution to work around the \"sign-off noise\" we observed.\n\nI do not view it as work around, but \"another proper workflow that\nhas advantages and disadvantages, one of the advantages is that it\nwould enable us to work with this tool\".\n\n>  One side\n> having a sign-off while the other side does not is inherent to what\n> we actively want,\n\n[in the current workflow that has proven good for 10 years]\n\n> and you are letting your tail wag your dog by\n> suggesting to discard it, which is disappointing.\n\nI am suggesting to continue thinking about workflows in general, as there\nare many; all of them having advantages and disadvantages.\nI am not sure if workflows can be adapted easily via improving the current\nworkflow continually or if sometimes a workflow has to be rethought to to\nchanges in the landscape of available tools.\n\nWhen the Git project started, an email based workflow was chosen,\nprecisely because Git was not available.\n\nNow that it has gained wide spread adoption (among its developers at least)\nthe workflow could adapt to that.\n\n> > The other (2) downside is that everyone else (authors, reviewers) have\n> > to adapt as well. For authors this might be easy to adapt (push instead\n> > of sending email sounds like a win).\n>\n> As I most often edit the log message and material below three-dash\n> lines (long) _after_ format-patch produced files, I do not think it\n> is a win to force me to push and ask to pull\n\nAh, that is an interesting workflow. Do you keep patch files/emails\naround locally, only to (long after) add a message and resend it?\nI try to keep any contribution of mine in Git as long as possible as that\nhelps me tracking and fixing errors in my code and log messages.\n\n> > (B) The other point of view that I can offer is that we teach range-diff\n> > to ignore certain patterns. Maybe in combination with interpret-trailers\n> > this can be an easy configurable thing, or even a default to ignore\n> > all sign offs?\n>\n> That indicates the same direction as I alluded to in the message you\n> are responding to, I guess, which is a good thing.\n\nYes, I imagined this is the approach we'll be taking.\nI think we would want to give %(trailers:none) or rather\n\"ignore-sign-offs\" to the inner diffs.\n\nThanks,\nStefan\n"},{"id":"352835","messageId":"xmqq36whmtit.fsf@gitster-ct.c.googlers.com","threadId":"48893","inReplyTo":"CAGZ79kbYtws-HhX_kQ5Gb4FeAfgRjbCfVbPS1Gcow4xfN-04XQ@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T21:39:22Z","receivedAt":"2018-07-17T21:39:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> As I most often edit the log message and material below three-dash\n>> lines (long) _after_ format-patch produced files, I do not think it\n>> is a win to force me to push and ask to pull\n>\n> Ah, that is an interesting workflow. Do you keep patch files/emails\n> around locally, only to (long after) add a message and resend it?\n\nThe time I \"finish\" working on a series and commit is typically much\ncloser to the time I format-patch the result, but it will take time\nfor me to actually mail out these files.  It will take time to\nre-review and re-think if the explanation in them makes sense to\nreaders, and sending half-edited patch is something I try to avoid\nas it would be a waste of time for everybody involved (including me\nwho would then need to respond to messages that helpfully point out\nsilly typoes, in addition to those who spots them).\n\nI am trained myself not to touch these files after sending them out,\nas comparing them with a newer iteration of the correspoinding files\nwas one way to see what has (and hasn't) changed between iterations\nbefore I learned \"git tbdiff\", and that habit persists.\n\n"},{"id":"352836","messageId":"xmqqy3e9leqd.fsf@gitster-ct.c.googlers.com","threadId":"48893","inReplyTo":"20180717075959.30594-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2 6/6] submodule--helper: introduce new update-module-mode helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T21:44:10Z","receivedAt":"2018-07-17T21:44:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> +\tfprintf(stdout, submodule_strategy_to_string(&update_strategy));\n>\n> Various compilers warn about the potential insecurity of the above\n> call:\n>\n>       CC builtin/submodule--helper.o\n>   builtin/submodule--helper.c: In function ‘module_update_module_mode’:\n>   builtin/submodule--helper.c:1502:2: error: format not a string literal and no format arguments [-Werror=format-security]\n>     fprintf(stdout, submodule_strategy_to_string(&update_strategy));\n>     ^\n>   cc1: all warnings being treated as errors\n>   Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed\n>   make: *** [builtin/submodule--helper.o] Error 1\n>\n> I think it should either use an explicit format string:\n>\n>   fprintf(stdout, \"%s\", submodule_strategy_to_string(&update_strategy));\n>\n> or, perhaps better yet, simply use fputs().\n\nSounds good.\n"},{"id":"352960","messageId":"CAGZ79kYCDR=m0C1Vmh5U2tsj5bTTpGUo1mKe0BMDKmO5riBLLw@mail.gmail.com","threadId":"48893","inReplyTo":"CAPig+cRx5-2TYOm_8oayFfbKGpmTJf=M0cNR3L5UJGGC6vHPDQ@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T19:34:34Z","receivedAt":"2018-07-18T19:34:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Eric,\nOn Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller <sbeller@google.com> wrote:\n> > > A tangent.\n> > >\n> > > Because this \"-- \" is a conventional signature separator, MUAs like\n> > > Emacs message-mode seems to omit everything below it from the quote\n> > > while responding, making it cumbersome to comment on the tbdiff.\n> > >\n> > > Something to think about if somebody is contemplating on adding more\n> > > to format-patch's cover letter.\n> >\n> > +cc Eric who needs to think about this tangent, then.\n> > https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/\n>\n> The \"git-format-patch --range-diff\" option implemented by that patch\n> series (and its upcoming re-roll) place the range-diff before the \"--\n> \" signature line, so this isn't a problem.\n>\n> I think Junio's tangent was targeted more at humans blindly plopping\n> the copy/pasted range-diff at the end of the cover letter without\n> taking the \"-- \" signature line into account. (Indeed, Gmail hides\n> everything below the \"-- \" line by default, as well.)\n\nAwesome thanks! (I missed that hint given by Junio)\n\nNow that I grow more accustomed to range-diffs, I wonder\nif we want to have them even *before* the short stat in the\ncover letter. (I usually scroll over the short stat just to take it\nas a FYI on what to expect, whereas the range-diff can already\nbe reviewed, hence seems more useful that the stats)\n\nThanks,\nStefan\n"},{"id":"352973","messageId":"CAPig+cT9zCcNxn1+DPMQWqJ-hfxb7gE7rKyfbqHjTC+FDNY_mw@mail.gmail.com","threadId":"48893","inReplyTo":"CAGZ79kYCDR=m0C1Vmh5U2tsj5bTTpGUo1mKe0BMDKmO5riBLLw@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-18T19:55:38Z","receivedAt":"2018-07-18T19:55:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 18, 2018 at 3:34 PM Stefan Beller <sbeller@google.com> wrote:\n> On Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > The \"git-format-patch --range-diff\" option implemented by that patch\n> > series (and its upcoming re-roll) place the range-diff before the \"--\n> > \" signature line, so this isn't a problem.\n>\n> Now that I grow more accustomed to range-diffs, I wonder\n> if we want to have them even *before* the short stat in the\n> cover letter. (I usually scroll over the short stat just to take it\n> as a FYI on what to expect, whereas the range-diff can already\n> be reviewed, hence seems more useful that the stats)\n\nI did consider placing the range-diff before the diffstat, however,\nwhat convinced me to position range-diff last was that the diffstat is\nusually short and easy to skip over both visually and via scrolling,\nwhereas the range-diff often is long and noisy, thus more difficult to\nskip for someone more interested in the diffstat.\n\nSo, the choice was deliberate. However, I also considered making it\nconfigurable, but that's probably something that can be done later, if\nneeded, once we get more experience with the feature. It's also worth\ntaking Duy's wish[1] for customization into account, as well, before\ndesigning such capability.\n\n[1]: https://public-inbox.org/git/CACsJy8D=6fAEpO5m4cc7KZyggAW1AosSkUWaunQBFH0nr-YrdA@mail.gmail.com/\n"},{"id":"352990","messageId":"xmqqy3e8i4wj.fsf@gitster-ct.c.googlers.com","threadId":"48893","inReplyTo":"CAPig+cT9zCcNxn1+DPMQWqJ-hfxb7gE7rKyfbqHjTC+FDNY_mw@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T21:57:00Z","receivedAt":"2018-07-18T21:57:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> I did consider placing the range-diff before the diffstat, however,\n> what convinced me to position range-diff last was that the diffstat is\n> usually short and easy to skip over both visually and via scrolling,\n> whereas the range-diff often is long and noisy, thus more difficult to\n> skip for someone more interested in the diffstat.\n\nI find that quite sensible, from a reader's point of view.\n\n"},{"id":"353633","messageId":"nycvar.QRO.7.76.6.1807261238160.71@tvgsbejvaqbjf.bet","threadId":"48893","inReplyTo":"CAGZ79kZtrmjODGi1j-HRbchZYuALHnms22=wWBV1AF+zUBru_g@mail.gmail.com","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-26T10:47:57Z","receivedAt":"2018-07-26T10:48:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Tue, 17 Jul 2018, Stefan Beller wrote:\n\n> > A tangent.\n> >\n> > Because this \"-- \" is a conventional signature separator, MUAs like\n> > Emacs message-mode seems to omit everything below it from the quote\n> > while responding, making it cumbersome to comment on the tbdiff.\n> >\n> > Something to think about if somebody is contemplating on adding more\n> > to format-patch's cover letter.\n> \n> +cc Eric who needs to think about this tangent, then.\n> https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/\n\nI think this is just a natural fall-out from the users' choice of mail\nprogram. Personally, I have no difficulty commenting on anything below the\n`--` separator.\n\nFWIW GitGitGadget follows the example of the `base-commit` footer and\nplaces this information *above* the `--` separator.\n\n> > >> 1:  d4e1ec45740 ! 1:  bbc8697a8ca git-submodule.sh: align error reporting for update mode to use path\n> > >>     @@ -6,7 +6,6 @@\n> > >>          on its path, so let's do that for invalid update modes, too.\n> > >>\n> > >>          Signed-off-by: Stefan Beller <sbeller@google.com>\n> > >>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> > >>\n> > >>      diff --git a/git-submodule.sh b/git-submodule.sh\n> > >>      --- a/git-submodule.sh\n> >\n> > This is quite unfortunate.  I wonder if it is easy to tell\n> > range-diff that certain differences in the log message are to be\n> > ignored so that we can show that the first patch is unchanged in a\n> > case like this.  This series has 4 really changed ones with 2\n> > otherwise unchanged ones shown all as changed, which is not too bad,\n> > but for a series like sb/diff-colro-move-more reroll that has 9\n> > patches, out of only two have real updated patches, showing\n> > otherwise unchanged 7 as changed like this hunk does would make the\n> > cover letter useless.  It is a shame that adding range-diff to the\n> > cover does have so much potential.\n> \n> Actually I thought it was really cool, i.e. when using your queued branch\n> instead of my last sent branch, I can see any edits *you* did\n> (including fixing up typos or applying at slightly different bases).\n\nThis is probably a good indicator that the practice on insisting on signing\noff on every patch, rather than just the merge commit, is something to\nre-think.\n\nThose are real changes relative to the original commit, after all, and if\nthey are not desired, they should not be made.\n\n> The sign offs are a bit unfortunate as they are repetitive.\n> I have two conflicting points of view on that:\n> \n> (A) This sign off is inherent to the workflow. So we could\n> change the workflow, i.e. you pull series instead of applying them.\n> I think this \"more in git, less in email\" workflow would find supporters,\n> such as DScho (cc'd).\n> \n> The downside is that (1) you'd have to change your\n> workflow, i.e. instead of applying the patches at the base you think is\n> best for maintenance you'd have to tell the author \"please rebase to $X\";\n> but that also has upsides, such as \"If you want to have your series integrated\n> please merge with $Y and $Z\" (looking at the object store stuff).\n> \n> The other (2) downside is that everyone else (authors, reviewers) have\n> to adapt as well. For authors this might be easy to adapt (push instead\n> of sending email sounds like a win). For reviewers we'd need to have\n> an easy way to review things \"stored in git\" and not exposed via email,\n> which is not obvious how to do.\n> \n> (B) The other point of view that I can offer is that we teach range-diff\n> to ignore certain patterns. Maybe in combination with interpret-trailers\n> this can be an easy configurable thing, or even a default to ignore\n> all sign offs?\n\nI thought about that myself.\n\nThe reason: I was surprised, a couple of times, when I realized long after\nthe fact, that some of my patches were changed without my knowledge nor\nblessing before being merged into `master`.\n\nTo allow me to protest in a timely manner, I wanted to teach GitGitGadget\n(which is the main reason I work on range-diff, as you undoubtedly suspect\nby now) to warn me about such instances.\n\nThe range-diff patch series has simmered too long at this stage, though,\nand I did not try to address such a \"ignore <regex>\" feature\n*specifically* so that the range-diff command could be available sooner\nthan later. I already missed one major version, please refrain from\nforcing me to miss another one.\n\nCiao,\nDscho\n"},{"id":"353667","messageId":"CAGZ79kbqLene9x2Pp27LxXbavP6hRLnBrsXJ49X4WQewJWxSzA@mail.gmail.com","threadId":"48893","inReplyTo":"nycvar.QRO.7.76.6.1807261238160.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-26T18:14:52Z","receivedAt":"2018-07-26T18:15:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">\n> To allow me to protest in a timely manner, I wanted to teach GitGitGadget\n> (which is the main reason I work on range-diff, as you undoubtedly suspect\n> by now) to warn me about such instances.\n\nI did not suspect that GGG is the prime motivation for range diff; as it proves\nuseful (a) on its own, e.g. looking at the updates in pu and (b) is helping the\ncurrent workflow very much.\n"}]}