{"thread":{"id":"46470","subject":"[PATCH 00/15] submodule-config cleanup","startedAt":"2017-07-25T21:39:49Z","lastAt":"2017-08-11T17:24:37Z","messageCount":61,"participants":["Brandon Williams","Stefan Beller","Junio C Hamano","Jens Lehmann","Heiko Voigt"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"325080","messageId":"20170725213928.125998-1-bmwill@google.com","threadId":"46470","inReplyTo":null,"subject":"[PATCH 00/15] submodule-config cleanup","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:13Z","receivedAt":"2017-07-25T21:39:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The aim of this series is to cleanup the submodule-config and make it simpler\nto use. The two main parts to this series are:\n(1) removing the ability to overlay the repository's config over the\n submodule-config. This makes the API clunky as you don't really know when\n you want to overlay and when you don't. So instead all the relevant\n sections (where you are interested in the repository's config) are patched\n to read the configuration directly from the repository's config.\n(2) Add the ability to lazy-load the gitmodules file from the working\n directory. Most callers are required to first populate the\n submodule-config by calling gitmodules_config. Instead let's just\n lazy-load it if needed. Only a couple callers will still require loading\n the gitmodules files by hand while the rest can have it lazy-loaded and no\n longer need to explicitly load it themselves. This falls more in line with\n how specific revisions are already lazy-loaded.\n\nAs a side note, instead of having unpack-trees read configuration for the\n'update' config (which is used by submodule update) we may just want to drop\nrespecting this all together as it doesn't make much sense in the context of a\ncheckout or reset. If that's the case then we can make the parts of the code\nwhich use 'update' even simpler.\n\nThis series is built on and requires the 'bw/grep-recurse-submodules' and\n'bc/object-id' branches.\n\nBrandon Williams (15):\n t7411: check configuration parsing errors\n submodule: don't use submodule_from_name\n add, reset: ensure submodules can be added or reset\n submodule--helper: don't overlay config in remote_submodule_branch\n submodule--helper: don't overlay config in update-clone\n fetch: don't overlay config with submodule-config\n submodule: don't rely on overlayed config when setting diffopts\n unpack-trees: don't rely on overlayed config\n submodule: remove submodule_config callback routine\n diff: stop allowing diff to have submodules configured in .git/config\n submodule-config: remove support for overlaying repository config\n submodule-config: move submodule-config functions to\n submodule-config.c\n submodule-config: lazy-load a repository's .gitmodules file\n unpack-trees: improve loading of .gitmodules\n submodule: remove gitmodules_config\n\n builtin/add.c | 1 +\n builtin/checkout.c | 3 +-\n builtin/commit.c | 1 -\n builtin/diff-files.c | 1 -\n builtin/diff-index.c | 1 -\n builtin/diff-tree.c | 1 -\n builtin/diff.c | 2 -\n builtin/fetch.c | 5 --\n builtin/grep.c | 4 --\n builtin/ls-files.c | 6 +-\n builtin/mv.c | 1 -\n builtin/read-tree.c | 2 -\n builtin/reset.c | 3 +-\n builtin/rm.c | 1 -\n builtin/submodule--helper.c | 42 ++++++------\n diff.c | 3 -\n submodule-config.c | 65 ++++++++++++++----\n submodule-config.h | 8 +--\n submodule.c | 140 ++++++++++++++++-----------------------\n submodule.h | 8 +--\n t/helper/test-submodule-config.c | 7 --\n t/t4027-diff-submodule.sh | 67 -------------------\n t/t7400-submodule-basic.sh | 10 ---\n t/t7411-submodule-config.sh | 87 +++++-------------------\n unpack-trees.c | 54 +++++++++------\n 25 files changed, 189 insertions(+), 334 deletions(-)\n\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325081","messageId":"20170725213928.125998-3-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:15Z","receivedAt":"2017-07-25T21:39:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The function 'submodule_from_name()' is being used incorrectly here as a\nsubmodule path is being used instead of a submodule name. Since the\ncorrect function to use with a path to a submodule is already being used\n('submodule_from_path()') let's remove the call to\n'submodule_from_name()'.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7e87e4698..fd391aea6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \n \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\n-\t\tif (!submodule)\n-\t\t\tsubmodule = submodule_from_name(&null_oid, ce->name);\n \n \t\tdefault_argv = \"yes\";\n \t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325082","messageId":"20170725213928.125998-5-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 04/15] submodule--helper: don't overlay config in remote_submodule_branch","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:17Z","receivedAt":"2017-07-25T21:39:54Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directly for the\nbranch field.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1e49ce580..f71f4270d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1066,17 +1066,24 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix\n static const char *remote_submodule_branch(const char *path)\n {\n \tconst struct submodule *sub;\n+\tconst char *branch = NULL;\n+\tchar *key;\n+\n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n \n \tsub = submodule_from_path(&null_oid, path);\n \tif (!sub)\n \t\treturn NULL;\n \n-\tif (!sub->branch)\n+\tkey = xstrfmt(\"submodule.%s.branch\", sub->name);\n+\tif (repo_config_get_string_const(the_repository, key, &branch))\n+\t\tbranch = sub->branch;\n+\tfree(key);\n+\n+\tif (!branch)\n \t\treturn \"master\";\n \n-\tif (!strcmp(sub->branch, \".\")) {\n+\tif (!strcmp(branch, \".\")) {\n \t\tunsigned char sha1[20];\n \t\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n \n@@ -1094,7 +1101,7 @@ static const char *remote_submodule_branch(const char *path)\n \t\treturn refname;\n \t}\n \n-\treturn sub->branch;\n+\treturn branch;\n }\n \n static int resolve_remote_submodule_branch(int argc, const char **argv,\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325083","messageId":"20170725213928.125998-6-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 05/15] submodule--helper: don't overlay config in update-clone","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:18Z","receivedAt":"2017-07-25T21:39:58Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directly for the\nurl and the update strategy configuration.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 14 ++++++++++----\n submodule.c | 30 ++++++++++++++++++++++++++++++\n submodule.h | 3 +++\n 3 files changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f71f4270d..25f471ba1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -780,6 +780,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t struct strbuf *out)\n {\n \tconst struct submodule *sub = NULL;\n+\tconst char *url = NULL;\n+\tstruct submodule_update_strategy update;\n \tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *displaypath = NULL;\n@@ -808,9 +810,10 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tgoto cleanup;\n \t}\n \n+\tupdate = submodule_strategy_with_config_overlayed(the_repository, sub);\n \tif (suc->update.type == SM_UPDATE_NONE\n \t || (suc->update.type == SM_UPDATE_UNSPECIFIED\n-\t\t&& sub->update_strategy.type == SM_UPDATE_NONE)) {\n+\t\t&& update.type == SM_UPDATE_NONE)) {\n \t\tstrbuf_addf(out, _(\"Skipping submodule '%s'\"), displaypath);\n \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n@@ -822,6 +825,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tgoto cleanup;\n \t}\n \n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\tif (repo_config_get_string_const(the_repository, sb.buf, &url))\n+\t\turl = sub->url;\n+\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n \tneeds_cloning = !file_exists(sb.buf);\n@@ -851,7 +859,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\targv_array_push(&child->args, \"--depth=1\");\n \targv_array_pushl(&child->args, \"--path\", sub->path, NULL);\n \targv_array_pushl(&child->args, \"--name\", sub->name, NULL);\n-\targv_array_pushl(&child->args, \"--url\", sub->url, NULL);\n+\targv_array_pushl(&child->args, \"--url\", url, NULL);\n \tif (suc->references.nr) {\n \t\tstruct string_list_item *item;\n \t\tfor_each_string_list_item(item, &suc->references)\n@@ -1025,9 +1033,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-\t/* Overlay the parsed .gitmodules file with .git/config */\n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n \n \trun_processes_parallel(max_jobs,\n \t\t\t update_clone_get_next_task,\ndiff --git a/submodule.c b/submodule.c\nindex fd391aea6..8b9e48a61 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -440,6 +440,36 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy\n \treturn NULL;\n }\n \n+struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,\n+\t\t\t\t\t\t\t\t\t const struct submodule *sub)\n+{\n+\tstruct submodule_update_strategy strat = sub->update_strategy;\n+\tconst char *update;\n+\tchar *key;\n+\n+\tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n+\tif (!repo_config_get_string_const(repo, key, &update)) {\n+\t\tstrat.command = NULL;\n+\t\tif (!strcmp(update, \"none\")) {\n+\t\t\tstrat.type = SM_UPDATE_NONE;\n+\t\t} else if (!strcmp(update, \"checkout\")) {\n+\t\t\tstrat.type = SM_UPDATE_CHECKOUT;\n+\t\t} else if (!strcmp(update, \"rebase\")) {\n+\t\t\tstrat.type = SM_UPDATE_REBASE;\n+\t\t} else if (!strcmp(update, \"merge\")) {\n+\t\t\tstrat.type = SM_UPDATE_MERGE;\n+\t\t} else if (skip_prefix(update, \"!\", &update)) {\n+\t\t\tstrat.type = SM_UPDATE_COMMAND;\n+\t\t\tstrat.command = update;\n+\t\t} else {\n+\t\t\tdie(\"invalid submodule update strategy '%s'\", update);\n+\t\t}\n+\t}\n+\tfree(key);\n+\n+\treturn strat;\n+}\n+\n void handle_ignore_submodules_arg(struct diff_options *diffopt,\n \t\t\t\t const char *arg)\n {\ndiff --git a/submodule.h b/submodule.h\nindex e402b004f..f17ca1e34 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -6,6 +6,7 @@ struct diff_options;\n struct argv_array;\n struct oid_array;\n struct remote;\n+struct submodule;\n \n enum {\n \tRECURSE_SUBMODULES_ONLY = -5,\n@@ -65,6 +66,8 @@ extern void die_path_inside_submodule(const struct index_state *istate,\n extern int parse_submodule_update_strategy(const char *value,\n \t\tstruct submodule_update_strategy *dst);\n extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n+extern struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,\n+\t\t\t\t\t\t\t\t\t\t const struct submodule *sub);\n extern void handle_ignore_submodules_arg(struct diff_options *, const char *);\n extern void show_submodule_summary(FILE *f, const char *path,\n \t\tconst char *line_prefix,\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325084","messageId":"20170725213928.125998-9-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 08/15] unpack-trees: don't rely on overlayed config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:21Z","receivedAt":"2017-07-25T21:39:59Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directory for\nthe submodule's update strategy.\n\nAlso remove the overlaying of the repository's config (via using\n'submodule_config()') from the commands which use the unpack-trees\nlogic (checkout, read-tree, reset).\n\nSigned-off-by: Brandon Williams \n---\n builtin/checkout.c | 2 +-\n submodule.c | 1 -\n unpack-trees.c | 12 +++++++++---\n 3 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9661e1bcb..246e0cd16 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -858,7 +858,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (starts_with(var, \"submodule.\"))\n-\t\treturn submodule_config(var, value, NULL);\n+\t\treturn git_default_submodule_config(var, value, NULL);\n \n \treturn git_xmerge_config(var, value, NULL);\n }\ndiff --git a/submodule.c b/submodule.c\nindex f86b82fbb..13380fed1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -235,7 +235,6 @@ void load_submodule_cache(void)\n \t\treturn;\n \n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n }\n \n static int gitmodules_cb(const char *var, const char *value, void *data)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex dd535bc84..dc66b880d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1,5 +1,6 @@\n #define NO_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n+#include \"repository.h\"\n #include \"config.h\"\n #include \"dir.h\"\n #include \"tree.h\"\n@@ -255,13 +256,16 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n {\n \tunsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n \tconst struct submodule *sub = submodule_from_ce(ce);\n+\tstruct submodule_update_strategy update;\n+\n \tif (!sub)\n \t\treturn 0;\n \n \tif (o->reset)\n \t\tflags |= SUBMODULE_MOVE_HEAD_FORCE;\n \n-\tswitch (sub->update_strategy.type) {\n+\tupdate = submodule_strategy_with_config_overlayed(the_repository, sub);\n+\tswitch (update.type) {\n \tcase SM_UPDATE_UNSPECIFIED:\n \tcase SM_UPDATE_CHECKOUT:\n \t\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n@@ -293,7 +297,6 @@ static void reload_gitmodules_file(struct index_state *index,\n \t\t\t\tsubmodule_free();\n \t\t\t\tcheckout_entry(ce, state, NULL);\n \t\t\t\tgitmodules_config();\n-\t\t\t\tgit_config(submodule_config, NULL);\n \t\t\t} else\n \t\t\t\tbreak;\n \t\t}\n@@ -308,7 +311,10 @@ static void unlink_entry(const struct cache_entry *ce)\n {\n \tconst struct submodule *sub = submodule_from_ce(ce);\n \tif (sub) {\n-\t\tswitch (sub->update_strategy.type) {\n+\t\tstruct submodule_update_strategy update =\n+\t\t\tsubmodule_strategy_with_config_overlayed(the_repository,\n+\t\t\t\t\t\t\t\t sub);\n+\t\tswitch (update.type) {\n \t\tcase SM_UPDATE_UNSPECIFIED:\n \t\tcase SM_UPDATE_CHECKOUT:\n \t\tcase SM_UPDATE_REBASE:\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325085","messageId":"20170725213928.125998-10-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 09/15] submodule: remove submodule_config callback routine","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:22Z","receivedAt":"2017-07-25T21:40:03Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Remove the last remaining caller of 'submodule_config()' as well as the\nfunction itself.\n\nWith 'submodule_config()' being removed the submodule-config API can be\na little simpler as callers don't need to worry about whether or not\nthey need to overlay the repository's config on top of the\nsubmodule-config. This also makes it more difficult to accidentally\nadd non-submodule specific configuration to the .gitmodules file.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 1 -\n submodule.c | 25 ++-----------------------\n submodule.h | 1 -\n 3 files changed, 2 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 25f471ba1..c16249e30 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1196,7 +1196,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t\t\t git_submodule_helper_usage, 0);\n \n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n \n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n \t\treturn 1;\ndiff --git a/submodule.c b/submodule.c\nindex 13380fed1..f63940347 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -180,27 +180,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t}\n }\n \n-/* For loading from the .gitmodules file. */\n-static int git_modules_config(const char *var, const char *value, void *cb)\n-{\n-\tif (starts_with(var, \"submodule.\"))\n-\t\treturn parse_submodule_config_option(var, value);\n-\treturn 0;\n-}\n-\n-/* Loads all submodule settings from the config. */\n-int submodule_config(const char *var, const char *value, void *cb)\n-{\n-\tif (!strcmp(var, \"submodule.recurse\")) {\n-\t\tint v = git_config_bool(var, value) ?\n-\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n-\t\tconfig_update_recurse_submodules = v;\n-\t\treturn 0;\n-\t} else {\n-\t\treturn git_modules_config(var, value, cb);\n-\t}\n-}\n-\n /* Cheap function that only determines if we're interested in submodules at all */\n int git_default_submodule_config(const char *var, const char *value, void *cb)\n {\n@@ -271,8 +250,8 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tstruct object_id oid;\n \n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n-\t\tgit_config_from_blob_oid(submodule_config, rev.buf,\n-\t\t\t\t\t &oid, NULL);\n+\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n+\t\t\t\t\t &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n }\ndiff --git a/submodule.h b/submodule.h\nindex f17ca1e34..1c6b2ab4e 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,7 +41,6 @@ extern int remove_path_from_gitmodules(const char *path);\n extern void stage_updated_gitmodules(void);\n extern void set_diffopt_flags_from_submodule_config(struct diff_options *,\n \t\tconst char *path);\n-extern int submodule_config(const char *var, const char *value, void *cb);\n extern int git_default_submodule_config(const char *var, const char *value, void *cb);\n \n struct option;\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325086","messageId":"20170725213928.125998-13-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 12/15] submodule-config: move submodule-config functions to submodule-config.c","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:25Z","receivedAt":"2017-07-25T21:40:07Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Migrate the functions used to initialize the submodule-config to\nsubmodule-config.c so that the callback routine used in the\ninitialization process can be static and prevent it from being used\noutside of initializing the submodule-config through the main API.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 1 +\n submodule-config.c | 38 +++++++++++++++++++++++++++++++-------\n submodule-config.h | 7 ++-----\n submodule.c | 35 -----------------------------------\n submodule.h | 2 --\n 5 files changed, 34 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b8514a002..d14612057 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -19,6 +19,7 @@\n #include \"pathspec.h\"\n #include \"run-command.h\"\n #include \"submodule.h\"\n+#include \"submodule-config.h\"\n \n static int abbrev;\n static int show_deleted;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 0b429e942..86636654b 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -449,9 +449,9 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n-int gitmodule_oid_from_commit(const struct object_id *treeish_name,\n-\t\t\t\t struct object_id *gitmodules_oid,\n-\t\t\t\t struct strbuf *rev)\n+static int gitmodule_oid_from_commit(const struct object_id *treeish_name,\n+\t\t\t\t struct object_id *gitmodules_oid,\n+\t\t\t\t struct strbuf *rev)\n {\n \tint ret = 0;\n \n@@ -552,9 +552,9 @@ static void submodule_cache_check_init(struct repository *repo)\n \tsubmodule_cache_init(repo->submodule_cache);\n }\n \n-int submodule_config_option(struct repository *repo,\n-\t\t\t const char *var, const char *value)\n+static int gitmodules_cb(const char *var, const char *value, void *data)\n {\n+\tstruct repository *repo = data;\n \tstruct parse_config_parameter parameter;\n \n \tsubmodule_cache_check_init(repo);\n@@ -567,9 +567,33 @@ int submodule_config_option(struct repository *repo,\n \treturn parse_config(var, value, ¶meter);\n }\n \n-int parse_submodule_config_option(const char *var, const char *value)\n+void repo_read_gitmodules(struct repository *repo)\n {\n-\treturn submodule_config_option(the_repository, var, value);\n+\tif (repo->worktree) {\n+\t\tchar *gitmodules;\n+\n+\t\tif (repo_read_index(repo) < 0)\n+\t\t\treturn;\n+\n+\t\tgitmodules = repo_worktree_path(repo, GITMODULES_FILE);\n+\n+\t\tif (!is_gitmodules_unmerged(repo->index))\n+\t\t\tgit_config_from_file(gitmodules_cb, gitmodules, repo);\n+\n+\t\tfree(gitmodules);\n+\t}\n+}\n+\n+void gitmodules_config_oid(const struct object_id *commit_oid)\n+{\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tstruct object_id oid;\n+\n+\tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n+\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n+\t\t\t\t\t &oid, the_repository);\n+\t}\n+\tstrbuf_release(&rev);\n }\n \n const struct submodule *submodule_from_name(const struct object_id *treeish_name,\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 84c2cf515..e3845831f 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -34,8 +34,8 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-extern int submodule_config_option(struct repository *repo,\n-\t\t\t\t const char *var, const char *value);\n+extern void repo_read_gitmodules(struct repository *repo);\n+extern void gitmodules_config_oid(const struct object_id *commit_oid);\n extern const struct submodule *submodule_from_name(\n \t\tconst struct object_id *commit_or_tree, const char *name);\n extern const struct submodule *submodule_from_path(\n@@ -43,9 +43,6 @@ extern const struct submodule *submodule_from_path(\n extern const struct submodule *submodule_from_cache(struct repository *repo,\n \t\t\t\t\t\t const struct object_id *treeish_name,\n \t\t\t\t\t\t const char *key);\n-extern int gitmodule_oid_from_commit(const struct object_id *commit_oid,\n-\t\t\t\t struct object_id *gitmodules_oid,\n-\t\t\t\t struct strbuf *rev);\n extern void submodule_free(void);\n \n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex f63940347..7ebd639f4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -216,46 +216,11 @@ void load_submodule_cache(void)\n \tgitmodules_config();\n }\n \n-static int gitmodules_cb(const char *var, const char *value, void *data)\n-{\n-\tstruct repository *repo = data;\n-\treturn submodule_config_option(repo, var, value);\n-}\n-\n-void repo_read_gitmodules(struct repository *repo)\n-{\n-\tif (repo->worktree) {\n-\t\tchar *gitmodules;\n-\n-\t\tif (repo_read_index(repo) < 0)\n-\t\t\treturn;\n-\n-\t\tgitmodules = repo_worktree_path(repo, GITMODULES_FILE);\n-\n-\t\tif (!is_gitmodules_unmerged(repo->index))\n-\t\t\tgit_config_from_file(gitmodules_cb, gitmodules, repo);\n-\n-\t\tfree(gitmodules);\n-\t}\n-}\n-\n void gitmodules_config(void)\n {\n \trepo_read_gitmodules(the_repository);\n }\n \n-void gitmodules_config_oid(const struct object_id *commit_oid)\n-{\n-\tstruct strbuf rev = STRBUF_INIT;\n-\tstruct object_id oid;\n-\n-\tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n-\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n-\t\t\t\t\t &oid, the_repository);\n-\t}\n-\tstrbuf_release(&rev);\n-}\n-\n /*\n * Determine if a submodule has been initialized at a given 'path'\n */\ndiff --git a/submodule.h b/submodule.h\nindex 1c6b2ab4e..36fc7f7cf 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -48,8 +48,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n void load_submodule_cache(void);\n extern void gitmodules_config(void);\n-extern void repo_read_gitmodules(struct repository *repo);\n-extern void gitmodules_config_oid(const struct object_id *commit_oid);\n extern int is_submodule_active(struct repository *repo, const char *path);\n /*\n * Determine if a submodule has been populated at a given 'path' by checking if\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325087","messageId":"20170725213928.125998-16-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 15/15] submodule: remove gitmodules_config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:28Z","receivedAt":"2017-07-25T21:40:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Now that the submodule-config subsystem can lazily read the gitmodules\nfile we no longer need to explicitly pre-read the gitmodules by calling\n'gitmodules_config()' so let's remove it.\n\nSigned-off-by: Brandon Williams \n---\n builtin/checkout.c | 1 -\n builtin/commit.c | 1 -\n builtin/diff-files.c | 1 -\n builtin/diff-index.c | 1 -\n builtin/diff-tree.c | 1 -\n builtin/diff.c | 2 --\n builtin/fetch.c | 4 ----\n builtin/grep.c | 4 ----\n builtin/mv.c | 1 -\n builtin/read-tree.c | 2 --\n builtin/reset.c | 2 --\n builtin/rm.c | 1 -\n builtin/submodule--helper.c | 14 --------------\n submodule.c | 15 ---------------\n submodule.h | 2 --\n t/helper/test-submodule-config.c | 1 -\n 16 files changed, 53 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 246e0cd16..63ae16afc 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1179,7 +1179,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \topts.prefix = prefix;\n \topts.show_progress = -1;\n \n-\tgitmodules_config();\n \tgit_config(git_checkout_config, &opts);\n \n \topts.track = BRANCH_TRACK_UNSPECIFIED;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4bbac014a..18ad714d9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -195,7 +195,6 @@ static void determine_whence(struct wt_status *s)\n static void status_init_config(struct wt_status *s, config_fn_t fn)\n {\n \twt_status_prepare(s);\n-\tgitmodules_config();\n \tgit_config(fn, s);\n \tdetermine_whence(s);\n \tinit_diff_ui_defaults();\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 17bf84d18..e88493ffe 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -26,7 +26,6 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n-\tgitmodules_config();\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 185e6f9b5..9d772f8f2 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -23,7 +23,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n-\tgitmodules_config();\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 31d2cb410..d66499909 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -110,7 +110,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n-\tgitmodules_config();\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7cde6abbc..7e3ebcea3 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -315,8 +315,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n-\tif (!no_index)\n-\t\tgitmodules_config();\n \tinit_diff_ui_defaults();\n \tgit_config(git_diff_ui_config, NULL);\n \tprecompose_argv(argc, argv);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3fe99073d..132e3224e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1360,10 +1360,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n-\t\tgitmodules_config();\n-\t}\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ac06d2d33..2d65f27d0 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1048,10 +1048,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t}\n #endif\n \n-\tif (recurse_submodules) {\n-\t\tgitmodules_config();\n-\t}\n-\n \tif (show_in_pager && (cached || list.nr))\n \t\tdie(_(\"--open-files-in-pager only works on the worktree\"));\n \ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 94fbaaa5d..ffdd5f01a 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -131,7 +131,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tstruct stat st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n \n-\tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_mv_options,\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex d5f618d08..bf87a2710 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -164,8 +164,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n \t\t\t read_tree_usage, 0);\n \n-\tload_submodule_cache();\n-\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tprefix_set = opts.prefix ? 1 : 0;\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 772d078b8..50488d273 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -309,8 +309,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n \tparse_args(&pathspec, argv, prefix, patch_mode, &rev);\n \n-\tload_submodule_cache();\n-\n \tunborn = !strcmp(rev, \"HEAD\") && get_oid(\"HEAD\", &oid);\n \tif (unborn) {\n \t\t/* reset on unborn branch: treat as reset to empty tree */\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4057e73fa..d91451fea 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -255,7 +255,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tstruct pathspec pathspec;\n \tchar *seen;\n \n-\tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_rm_options,\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c16249e30..d74855b2d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -275,8 +275,6 @@ static void module_list_active(struct module_list *list)\n \tint i;\n \tstruct module_list active_modules = MODULE_LIST_INIT;\n \n-\tgitmodules_config();\n-\n \tfor (i = 0; i < list->nr; i++) {\n \t\tconst struct cache_entry *ce = list->entries[i];\n \n@@ -337,9 +335,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* Only loads from .gitmodules, no overlay with .git/config */\n-\tgitmodules_config();\n-\n \tif (prefix && get_super_prefix())\n \t\tdie(\"BUG: cannot have prefix and superprefix\");\n \telse if (prefix)\n@@ -475,7 +470,6 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \tif (argc != 2)\n \t\tusage(_(\"git submodule--helper name \"));\n \n-\tgitmodules_config();\n \tsub = submodule_from_path(&null_oid, argv[1]);\n \n \tif (!sub)\n@@ -1033,8 +1027,6 @@ 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-\tgitmodules_config();\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@@ -1075,8 +1067,6 @@ static const char *remote_submodule_branch(const char *path)\n \tconst char *branch = NULL;\n \tchar *key;\n \n-\tgitmodules_config();\n-\n \tsub = submodule_from_path(&null_oid, path);\n \tif (!sub)\n \t\treturn NULL;\n@@ -1195,8 +1185,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, embed_gitdir_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n-\tgitmodules_config();\n-\n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n \t\treturn 1;\n \n@@ -1212,8 +1200,6 @@ static int is_active(int argc, const char **argv, const char *prefix)\n \tif (argc != 2)\n \t\tdie(\"submodule--helper is-active takes exactly 1 argument\");\n \n-\tgitmodules_config();\n-\n \treturn !is_submodule_active(the_repository, argv[1]);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 7ebd639f4..1e4ff4e51 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -208,19 +208,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \treturn 0;\n }\n \n-void load_submodule_cache(void)\n-{\n-\tif (config_update_recurse_submodules == RECURSE_SUBMODULES_OFF)\n-\t\treturn;\n-\n-\tgitmodules_config();\n-}\n-\n-void gitmodules_config(void)\n-{\n-\trepo_read_gitmodules(the_repository);\n-}\n-\n /*\n * Determine if a submodule has been initialized at a given 'path'\n */\n@@ -1109,7 +1096,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \tstruct argv_array args = ARGV_ARRAY_INIT;\n \tint ret;\n \n-\tgitmodules_config();\n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(NULL, NULL))\n \t\treturn 0;\n@@ -2016,7 +2002,6 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \t\tstrbuf_addstr(buf, git_dir);\n \t}\n \tif (!is_git_directory(buf->buf)) {\n-\t\tgitmodules_config();\n \t\tsub = submodule_from_path(&null_oid, submodule);\n \t\tif (!sub) {\n \t\t\tret = -1;\ndiff --git a/submodule.h b/submodule.h\nindex 36fc7f7cf..7d0b5aa43 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -46,8 +46,6 @@ extern int git_default_submodule_config(const char *var, const char *value, void\n struct option;\n int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n-void load_submodule_cache(void);\n-extern void gitmodules_config(void);\n extern int is_submodule_active(struct repository *repo, const char *path);\n /*\n * Determine if a submodule has been populated at a given 'path' by checking if\ndiff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c\nindex f4a7c431c..f23db3b19 100644\n--- a/t/helper/test-submodule-config.c\n+++ b/t/helper/test-submodule-config.c\n@@ -32,7 +32,6 @@ int cmd_main(int argc, const char **argv)\n \t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n \n \tsetup_git_directory();\n-\tgitmodules_config();\n \n \twhile (*arg) {\n \t\tstruct object_id commit_oid;\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325088","messageId":"20170725213928.125998-15-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 14/15] unpack-trees: improve loading of .gitmodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:27Z","receivedAt":"2017-07-25T21:40:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When recursing submodules 'check_updates()' needs to have strict control\nover the submodule-config subsystem to ensure that the gitmodules file\nhas been read before checking cache entries which are marked for\nremoval as well ensuring the proper gitmodules file is read before\nupdating cache entries.\n\nBecause of this let's not rely on callers of 'check_updates()' to read\nthe gitmodules file before calling 'check_updates()' and handle the\nreading explicitly.\n\nSigned-off-by: Brandon Williams \n---\n unpack-trees.c | 42 ++++++++++++++++++++++++++----------------\n 1 file changed, 26 insertions(+), 16 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex dc66b880d..144c556c8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -283,22 +283,28 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n \t}\n }\n \n-static void reload_gitmodules_file(struct index_state *index,\n-\t\t\t\t struct checkout *state)\n+/*\n+ * Preform the loading of the repository's gitmodules file. This function is\n+ * used by 'check_update()' to perform loading of the gitmodules file in two\n+ * differnt situations:\n+ * (1) before removing entries from the working tree if the gitmodules file has\n+ * been marked for removal. This situation is specified by 'state' == NULL.\n+ * (2) before checking out entries to the working tree if the gitmodules file\n+ * has been marked for update. This situation is specified by 'state' != NULL.\n+ */\n+static void load_gitmodules_file(struct index_state *index,\n+\t\t\t\t struct checkout *state)\n {\n-\tint i;\n-\tfor (i = 0; i < index->cache_nr; i++) {\n-\t\tstruct cache_entry *ce = index->cache[i];\n-\t\tif (ce->ce_flags & CE_UPDATE) {\n-\t\t\tint r = strcmp(ce->name, \".gitmodules\");\n-\t\t\tif (r < 0)\n-\t\t\t\tcontinue;\n-\t\t\telse if (r == 0) {\n-\t\t\t\tsubmodule_free();\n-\t\t\t\tcheckout_entry(ce, state, NULL);\n-\t\t\t\tgitmodules_config();\n-\t\t\t} else\n-\t\t\t\tbreak;\n+\tint pos = index_name_pos(index, GITMODULES_FILE, strlen(GITMODULES_FILE));\n+\n+\tif (pos >= 0) {\n+\t\tstruct cache_entry *ce = index->cache[pos];\n+\t\tif (!state && ce->ce_flags & CE_WT_REMOVE) {\n+\t\t\trepo_read_gitmodules(the_repository);\n+\t\t} else if (state && (ce->ce_flags & CE_UPDATE)) {\n+\t\t\tsubmodule_free();\n+\t\t\tcheckout_entry(ce, state, NULL);\n+\t\t\trepo_read_gitmodules(the_repository);\n \t\t}\n \t}\n }\n@@ -371,6 +377,10 @@ static int check_updates(struct unpack_trees_options *o)\n \n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, index);\n+\n+\tif (should_update_submodules() && o->update && !o->dry_run)\n+\t\tload_gitmodules_file(index, NULL);\n+\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = index->cache[i];\n \n@@ -384,7 +394,7 @@ static int check_updates(struct unpack_trees_options *o)\n \tremove_scheduled_dirs();\n \n \tif (should_update_submodules() && o->update && !o->dry_run)\n-\t\treload_gitmodules_file(index, &state);\n+\t\tload_gitmodules_file(index, &state);\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325089","messageId":"20170725213928.125998-14-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 13/15] submodule-config: lazy-load a repository's .gitmodules file","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:26Z","receivedAt":"2017-07-25T21:40:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In order to use the submodule-config subsystem, callers first need to\ninitialize it by calling 'repo_read_gitmodules()' or\n'gitmodules_config()' (which just redirects to\n'repo_read_gitmodules()'). There are a couple of callers who need to\nload an explicit revision of the repository's .gitmodules file (grep) or\nneed to modify the .gitmodules file so they would need to load it before\nmodify the file (checkout), but the majority of callers are simply\nreading the .gitmodules file present in the working tree. For the\ncommon case it would be nice to avoid the boilerplate of initializing\nthe submodule-config system before using it, so instead let's perform\nlazy-loading of the submodule-config system.\n\nRemove the calls to reading the gitmodules file from ls-files to show\nthat lazy-loading the .gitmodules file works.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 5 -----\n submodule-config.c | 27 ++++++++++++++++++++++-----\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d14612057..bd74ee07d 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -211,8 +211,6 @@ static void show_submodule(struct repository *superproject,\n \tif (repo_read_index(&submodule) < 0)\n \t\tdie(\"index file corrupt\");\n \n-\trepo_read_gitmodules(&submodule);\n-\n \tshow_files(&submodule, dir);\n \n \trepo_clear(&submodule);\n@@ -611,9 +609,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n-\tif (recurse_submodules)\n-\t\trepo_read_gitmodules(the_repository);\n-\n \tif (recurse_submodules &&\n \t (show_stage || show_deleted || show_others || show_unmerged ||\n \t show_killed || show_modified || show_resolve_undo || with_tree))\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 86636654b..56d9d76d4 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -18,6 +18,7 @@ struct submodule_cache {\n \tstruct hashmap for_path;\n \tstruct hashmap for_name;\n \tunsigned initialized:1;\n+\tunsigned gitmodules_read:1;\n };\n \n /*\n@@ -93,6 +94,7 @@ static void submodule_cache_clear(struct submodule_cache *cache)\n \thashmap_free(&cache->for_path, 1);\n \thashmap_free(&cache->for_name, 1);\n \tcache->initialized = 0;\n+\tcache->gitmodules_read = 0;\n }\n \n void submodule_cache_free(struct submodule_cache *cache)\n@@ -557,8 +559,6 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \tstruct repository *repo = data;\n \tstruct parse_config_parameter parameter;\n \n-\tsubmodule_cache_check_init(repo);\n-\n \tparameter.cache = repo->submodule_cache;\n \tparameter.treeish_name = NULL;\n \tparameter.gitmodules_sha1 = null_sha1;\n@@ -569,6 +569,8 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \n void repo_read_gitmodules(struct repository *repo)\n {\n+\tsubmodule_cache_check_init(repo);\n+\n \tif (repo->worktree) {\n \t\tchar *gitmodules;\n \n@@ -582,6 +584,8 @@ void repo_read_gitmodules(struct repository *repo)\n \n \t\tfree(gitmodules);\n \t}\n+\n+\trepo->submodule_cache->gitmodules_read = 1;\n }\n \n void gitmodules_config_oid(const struct object_id *commit_oid)\n@@ -589,24 +593,37 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tstruct strbuf rev = STRBUF_INIT;\n \tstruct object_id oid;\n \n+\tsubmodule_cache_check_init(the_repository);\n+\n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n \t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n \t\t\t\t\t &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n+\n+\tthe_repository->submodule_cache->gitmodules_read = 1;\n+}\n+\n+static void gitmodules_read_check(struct repository *repo)\n+{\n+\tsubmodule_cache_check_init(repo);\n+\n+\t/* read the repo's .gitmodules file if it hasn't been already */\n+\tif (!repo->submodule_cache->gitmodules_read)\n+\t\trepo_read_gitmodules(repo);\n }\n \n const struct submodule *submodule_from_name(const struct object_id *treeish_name,\n \t\tconst char *name)\n {\n-\tsubmodule_cache_check_init(the_repository);\n+\tgitmodules_read_check(the_repository);\n \treturn config_from(the_repository->submodule_cache, treeish_name, name, lookup_name);\n }\n \n const struct submodule *submodule_from_path(const struct object_id *treeish_name,\n \t\tconst char *path)\n {\n-\tsubmodule_cache_check_init(the_repository);\n+\tgitmodules_read_check(the_repository);\n \treturn config_from(the_repository->submodule_cache, treeish_name, path, lookup_path);\n }\n \n@@ -614,7 +631,7 @@ const struct submodule *submodule_from_cache(struct repository *repo,\n \t\t\t\t\t const struct object_id *treeish_name,\n \t\t\t\t\t const char *key)\n {\n-\tsubmodule_cache_check_init(repo);\n+\tgitmodules_read_check(repo);\n \treturn config_from(repo->submodule_cache, treeish_name,\n \t\t\t key, lookup_path);\n }\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325090","messageId":"20170725213928.125998-12-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 11/15] submodule-config: remove support for overlaying repository config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:24Z","receivedAt":"2017-07-25T21:40:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"All callers have been migrated to explicitly read any configuration they\nneed. The support for handling it automatically in submodule-config is\nno longer needed.\n\nSigned-off-by: Brandon Williams \n---\n submodule-config.h | 1 -\n t/helper/test-submodule-config.c | 6 ----\n t/t7411-submodule-config.sh | 72 ----------------------------------------\n 3 files changed, 79 deletions(-)\n\ndiff --git a/submodule-config.h b/submodule-config.h\nindex cccd34b92..84c2cf515 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -34,7 +34,6 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-extern int parse_submodule_config_option(const char *var, const char *value);\n extern int submodule_config_option(struct repository *repo,\n \t\t\t\t const char *var, const char *value);\n extern const struct submodule *submodule_from_name(\ndiff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c\nindex e13fbcc1b..f4a7c431c 100644\n--- a/t/helper/test-submodule-config.c\n+++ b/t/helper/test-submodule-config.c\n@@ -10,11 +10,6 @@ static void die_usage(int argc, const char **argv, const char *msg)\n \texit(1);\n }\n \n-static int git_test_config(const char *var, const char *value, void *cb)\n-{\n-\treturn parse_submodule_config_option(var, value);\n-}\n-\n int cmd_main(int argc, const char **argv)\n {\n \tconst char **arg = argv;\n@@ -38,7 +33,6 @@ int cmd_main(int argc, const char **argv)\n \n \tsetup_git_directory();\n \tgitmodules_config();\n-\tgit_config(git_test_config, NULL);\n \n \twhile (*arg) {\n \t\tstruct object_id commit_oid;\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 7d6b25ba2..46c09c776 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -122,78 +122,6 @@ test_expect_success 'using different treeishs works' '\n \t)\n '\n \n-cat >super/expect_url <super/expect_local_path <actual &&\n-\t\ttest_cmp expect_url actual &&\n-\t\tgit config submodule.a.path c &&\n-\t\ttest-submodule-config \\\n-\t\t\t\"\" c \\\n-\t\t\t\"\" submodule \\\n-\t\t\t\t>actual &&\n-\t\ttest_cmp expect_local_path actual &&\n-\t\tgit config submodule.a.url \"$old_a\" &&\n-\t\tgit config submodule.submodule.url \"$old_submodule\" &&\n-\t\tgit config --unset submodule.a.path c\n-\t)\n-'\n-\n-cat >super/expect_url <actual &&\n-\t\ttest_cmp expect_url actual &&\n-\t\tgit config submodule.submodule.url \"$old_submodule\" &&\n-\t\tgit submodule init b\n-\t)\n-'\n-\n-cat >super/expect_fetchrecurse_die.err <actual.out 2>actual.err &&\n-\t\ttouch expect_fetchrecurse_die.out &&\n-\t\ttest_cmp expect_fetchrecurse_die.out actual.out &&\n-\t\ttest_cmp expect_fetchrecurse_die.err actual.err &&\n-\t\tgit config --unset submodule.submodule.fetchrecursesubmodules\n-\t)\n-'\n-\n test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n \t(cd super &&\n \t\tgit config -f .gitmodules \\\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325091","messageId":"20170725213928.125998-11-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 10/15] diff: stop allowing diff to have submodules configured in .git/config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:23Z","receivedAt":"2017-07-25T21:40:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Traditionally a submodule is comprised of a gitlink as well as a\ncorresponding entry in the .gitmodules file. Diff doesn't follow this\nparadigm as its config callback routine falls back to populating the\nsubmodule-config if a config entry starts with 'submodule.'.\n\nRemove this behavior in order to be consistent with how the\nsubmodule-config is populated, via calling 'gitmodules_config()' or\n'repo_read_gitmodules()'.\n\nSigned-off-by: Brandon Williams \n---\n diff.c | 3 ---\n t/t4027-diff-submodule.sh | 67 -----------------------------------------------\n 2 files changed, 70 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 85e714f6c..e43519b88 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (starts_with(var, \"submodule.\"))\n-\t\treturn parse_submodule_config_option(var, value);\n-\n \tif (git_diff_heuristic_config(var, value, cb) < 0)\n \t\treturn -1;\n \ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex 518bf9524..2ffd11a14 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'\n \t! test -s actual4\n '\n \n-test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '\n-\tgit config diff.ignoreSubmodules all &&\n-\tgit diff HEAD >actual &&\n-\t! test -s actual &&\n-\tgit config submodule.subname.ignore none &&\n-\tgit config submodule.subname.path sub &&\n-\tgit diff HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config submodule.subname.ignore all &&\n-\tgit diff HEAD >actual2 &&\n-\t! test -s actual2 &&\n-\tgit config submodule.subname.ignore untracked &&\n-\tgit diff HEAD >actual3 &&\n-\tsed -e \"1,/^@@/d\" actual3 >actual3.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual3.body &&\n-\tgit config submodule.subname.ignore dirty &&\n-\tgit diff HEAD >actual4 &&\n-\t! test -s actual4 &&\n-\tgit diff HEAD --ignore-submodules=none >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config --remove-section submodule.subname &&\n-\tgit config --unset diff.ignoreSubmodules\n-'\n-\n test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '\n \tgit config diff.ignoreSubmodules dirty &&\n \tgit diff HEAD >actual &&\n@@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'\n \t! test -s actual4\n '\n \n-test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '\n-\tgit config submodule.subname.ignore all &&\n-\tgit config submodule.subname.path sub &&\n-\tgit diff HEAD >actual2 &&\n-\t! test -s actual2 &&\n-\tgit config submodule.subname.ignore untracked &&\n-\tgit diff HEAD >actual3 &&\n-\t! test -s actual3 &&\n-\tgit config submodule.subname.ignore dirty &&\n-\tgit diff HEAD >actual4 &&\n-\t! test -s actual4 &&\n-\tgit diff --ignore-submodules=none HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config --remove-section submodule.subname\n-'\n-\n test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '\n \tgit config --add -f .gitmodules submodule.subname.ignore all &&\n \tgit config --add -f .gitmodules submodule.subname.path sub &&\n@@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '\n \t! test -s actual\n '\n \n-test_expect_success 'git diff between submodule commits [.git/config]' '\n-\tgit diff HEAD^..HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subtip $subprev &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config submodule.subname.ignore dirty &&\n-\tgit config submodule.subname.path sub &&\n-\tgit diff HEAD^..HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subtip $subprev &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config submodule.subname.ignore all &&\n-\tgit diff HEAD^..HEAD >actual &&\n-\t! test -s actual &&\n-\tgit diff --ignore-submodules=dirty HEAD^..HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subtip $subprev &&\n-\tgit config --remove-section submodule.subname\n-'\n-\n test_expect_success 'git diff between submodule commits [.gitmodules]' '\n \tgit diff HEAD^..HEAD >actual &&\n \tsed -e \"1,/^@@/d\" actual >actual.body &&\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325092","messageId":"20170725213928.125998-7-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 06/15] fetch: don't overlay config with submodule-config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:19Z","receivedAt":"2017-07-25T21:40:41Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directly for the\nfetch_recurse field.\n\nSigned-off-by: Brandon Williams \n---\n builtin/fetch.c | 1 -\n submodule.c | 24 +++++++++++++++++-------\n 2 files changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d84c26391..3fe99073d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tgitmodules_config();\n-\t\tgit_config(submodule_config, NULL);\n \t}\n \n \tif (all) {\ndiff --git a/submodule.c b/submodule.c\nindex 8b9e48a61..c5058a4b8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1210,14 +1210,24 @@ static int get_next_submodule(struct child_process *cp,\n \n \t\tdefault_argv = \"yes\";\n \t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-\t\t\tif (submodule &&\n-\t\t\t submodule->fetch_recurse !=\n-\t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n-\t\t\t\tif (submodule->fetch_recurse ==\n-\t\t\t\t\t\tRECURSE_SUBMODULES_OFF)\n+\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n+\n+\t\t\tif (submodule) {\n+\t\t\t\tchar *key;\n+\t\t\t\tconst char *value;\n+\n+\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n+\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n+\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n+\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n+\t\t\t\t}\n+\t\t\t\tfree(key);\n+\t\t\t}\n+\n+\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (submodule->fetch_recurse ==\n-\t\t\t\t\t\tRECURSE_SUBMODULES_ON_DEMAND) {\n+\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\tdefault_argv = \"on-demand\";\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325093","messageId":"20170725213928.125998-8-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 07/15] submodule: don't rely on overlayed config when setting diffopts","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:20Z","receivedAt":"2017-07-25T21:40:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directory for\nthe ignore field.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c5058a4b8..f86b82fbb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -165,8 +165,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n {\n \tconst struct submodule *submodule = submodule_from_path(&null_oid, path);\n \tif (submodule) {\n-\t\tif (submodule->ignore)\n-\t\t\thandle_ignore_submodules_arg(diffopt, submodule->ignore);\n+\t\tconst char *ignore;\n+\t\tchar *key;\n+\n+\t\tkey = xstrfmt(\"submodule.%s.ignore\", submodule->name);\n+\t\tif (repo_config_get_string_const(the_repository, key, &ignore))\n+\t\t\tignore = submodule->ignore;\n+\t\tfree(key);\n+\n+\t\tif (ignore)\n+\t\t\thandle_ignore_submodules_arg(diffopt, ignore);\n \t\telse if (is_gitmodules_unmerged(&the_index))\n \t\t\tDIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);\n \t}\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325094","messageId":"20170725213928.125998-2-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH 01/15] t7411: check configuration parsing errors","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T21:39:14Z","receivedAt":"2017-07-25T21:40:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Check for configuration parsing errors in '.gitmodules' in t7411, which\nis explicitly testing the submodule-config subsystem, instead of in\nt7400. Also explicitly use the test helper instead of relying on the\ngitmodules file from being read in status.\n\nSigned-off-by: Brandon Williams \n---\n t/t7400-submodule-basic.sh | 10 ----------\n t/t7411-submodule-config.sh | 15 +++++++++++++++\n 2 files changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex dcac364c5..717447526 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -46,16 +46,6 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '\n \ttest_must_fail git submodule init\n '\n \n-test_expect_success 'configuration parsing' '\n-\ttest_when_finished \"rm -f .gitmodules\" &&\n-\tcat >.gitmodules <<-\\EOF &&\n-\t[submodule \"s\"]\n-\t\tpath\n-\t\tignore\n-\tEOF\n-\ttest_must_fail git status\n-'\n-\n test_expect_success 'setup - repository in init subdirectory' '\n \tmkdir init &&\n \t(\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex eea36f1db..7d6b25ba2 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -31,6 +31,21 @@ test_expect_success 'submodule config cache setup' '\n \t)\n '\n \n+test_expect_success 'configuration parsing with error' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\ttest_create_repo repo &&\n+\tcat >repo/.gitmodules <<-\\EOF &&\n+\t[submodule \"s\"]\n+\t\tpath\n+\t\tignore\n+\tEOF\n+\t(\n+\t\tcd repo &&\n+\t\ttest_must_fail test-submodule-config \"\" s 2>actual &&\n+\t\ttest_i18ngrep \"bad config\" actual\n+\t)\n+'\n+\n cat >super/expect <\n---\n builtin/add.c | 1 +\n builtin/reset.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex e888fb8c5..6f271512f 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\n+\trev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \trev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n \treturn !!data.add_errors;\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 046403ed6..772d078b8 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.output_format = DIFF_FORMAT_CALLBACK;\n \topt.format_callback = update_index_from_diff;\n \topt.format_callback_data = &intent_to_add;\n+\topt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \n \tif (do_diff_cache(tree_oid, &opt))\n \t\treturn 1;\n-- \n2.14.0.rc0.400.g1c36432dff-goog\n\n"},{"id":"325099","messageId":"CAGZ79kY9Pdk5C8=k-AQpCPwo3q9Jzfg9A93UQxGyyf_OyrMS_Q@mail.gmail.com","threadId":"46470","inReplyTo":"20170725213928.125998-3-bmwill@google.com","subject":"Re: [PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T23:17:42Z","receivedAt":"2017-07-25T23:17:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> The function 'submodule_from_name()' is being used incorrectly here as a\n> submodule path is being used instead of a submodule name. Since the\n> correct function to use with a path to a submodule is already being used\n> ('submodule_from_path()') let's remove the call to\n> 'submodule_from_name()'.\n\nThis blames to 851e18c385 (submodule: use new config API for worktree\nconfigurations, 2015-08-17), but that is a refactoring. The issue of using\nthe path instead of a name was there before that. The actual issue\nwas introduced in 7dce19d374 (fetch/pull: Add the\n--recurse-submodules option, 2010-11-12).\n\n+ name = ce->name;\n+ name_for_path =\nunsorted_string_list_lookup(&config_name_for_path, ce->name);\n+ if (name_for_path)\n+ name = name_for_path->util;\n\nRereading the archives, there was quite some discussion on the design\nof these patches, but these lines of code did not get any attention\n\n https://public-inbox.org/git/4CDB3063.5010801@web.de/\n\nI cc'd Jens in the hope of him having a good memory why he\nwrote the code that way. :)\n\nNote that this is the last caller of submodule_from_name being\nremoved, so I would expect removal of submodule_from_name from\nthe t/helper/test-submodule-config.c as well as\nDocumentation/technical/api-submodule-config.txt\nin a later part of this series. (Well technically it could go outside\nof the series, but in the mean time we'd document and test\ndead code)\n\n> Signed-off-by: Brandon Williams \n> ---\n> submodule.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 7e87e4698..fd391aea6 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n> continue;\n>\n> submodule = submodule_from_path(&null_oid, ce->name);\n> - if (!submodule)\n> - submodule = submodule_from_name(&null_oid, ce->name);\n>\n> default_argv = \"yes\";\n> if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> --\n> 2.14.0.rc0.400.g1c36432dff-goog\n>\n"},{"id":"325100","messageId":"CAGZ79kZByzPbyLUNJ8ViVa2TDk-L+TfnF+wVWRj2d92_MhXPbg@mail.gmail.com","threadId":"46470","inReplyTo":"20170725213928.125998-4-bmwill@google.com","subject":"Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T23:33:10Z","receivedAt":"2017-07-25T23:33:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> Commit aee9c7d65 (Submodules: Add the new \"ignore\" config option for\n> diff and status) ...\n\nintroduced in 2010, so quite widely spread.\n\n> ... introduced the ignore configuration option for\n> submodules so that configured submodules could be omitted from the\n> status and diff commands. Because this flag is respected in the diff\n> machinery it has the unintended consequence of potentially prohibiting\n> users from adding or resetting a submodule, even when a path to the\n> submodule is explicitly given.\n>\n> Ensure that submodules can be added or set, even if they are configured\n> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff\n> flag.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> builtin/add.c | 1 +\n> builtin/reset.c | 1 +\n> 2 files changed, 2 insertions(+)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index e888fb8c5..6f271512f 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,\n> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n> rev.diffopt.format_callback = update_callback;\n> rev.diffopt.format_callback_data = &data;\n> + rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n\n\nThis flag occurs once in the code base, with the comment:\n /*\n * Unless the user did explicitly request a submodule\n * ignore mode by passing a command line option we do\n * not ignore any changed submodule SHA-1s when\n * comparing index and parent, no matter what is\n * configured. Otherwise we won't commit any\n * submodules which were manually staged, which would\n * be really confusing.\n */\n int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n\nin prepare_commit, so commit ignores the .gitmodules file.\n\nThis allows git-add to add ignored submodules, currently ignored submodules\nwould have to be added using the plumbing\n git update-index --add --cacheinfo 160000,$SHA1,\n\nThis makes sense, though a test demonstrating the change in behavior\nwould be nice, but git-add doesn't seem to change as it doesn't even load\nthe git modules config?\n\n> rev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n> run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n> return !!data.add_errors;\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 046403ed6..772d078b8 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n> opt.output_format = DIFF_FORMAT_CALLBACK;\n> opt.format_callback = update_index_from_diff;\n> opt.format_callback_data = &intent_to_add;\n> + opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n\nsame here? Also as this is not failing any test, it may be worth adding one\nto document the behavior of the \"submodule..ignore\" flag in tests?\n\n>\n> if (do_diff_cache(tree_oid, &opt))\n> return 1;\n> --\n> 2.14.0.rc0.400.g1c36432dff-goog\n>\n"},{"id":"325101","messageId":"CAGZ79kaof5O2EwFjg6-NpGMYdVYWdSxL+5STfEm8WF5gV1qbGQ@mail.gmail.com","threadId":"46470","inReplyTo":"20170725213928.125998-5-bmwill@google.com","subject":"Re: [PATCH 04/15] submodule--helper: don't overlay config in remote_submodule_branch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T23:35:13Z","receivedAt":"2017-07-25T23:35:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> Don't rely on overlaying the repository's config on top of the\n> submodule-config, instead query the repository's config directly for the\n> branch field.\n>\n> Signed-off-by: Brandon Williams \n\nReviewed-by: Stefan Beller \n"},{"id":"325102","messageId":"CAGZ79kacdTFVJknTx+ceT8epytXSJDRVAwZO4HyzpsmVbK5VTQ@mail.gmail.com","threadId":"46470","inReplyTo":"20170725213928.125998-6-bmwill@google.com","subject":"Re: [PATCH 05/15] submodule--helper: don't overlay config in update-clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T23:37:34Z","receivedAt":"2017-07-25T23:37:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> Don't rely on overlaying the repository's config on top of the\n> submodule-config, instead query the repository's config directly for the\n> url and the update strategy configuration.\n>\n> Signed-off-by: Brandon Williams \n> ---\n...\n\n> +struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,\n> + const struct submodule *sub)\n> +{\n> + struct submodule_update_strategy strat = sub->update_strategy;\n> + const char *update;\n> + char *key;\n> +\n> + key = xstrfmt(\"submodule.%s.update\", sub->name);\n> + if (!repo_config_get_string_const(repo, key, &update)) {\n> + strat.command = NULL;\n> + if (!strcmp(update, \"none\")) {\n> + strat.type = SM_UPDATE_NONE;\n> + } else if (!strcmp(update, \"checkout\")) {\n> + strat.type = SM_UPDATE_CHECKOUT;\n> + } else if (!strcmp(update, \"rebase\")) {\n> + strat.type = SM_UPDATE_REBASE;\n> + } else if (!strcmp(update, \"merge\")) {\n> + strat.type = SM_UPDATE_MERGE;\n> + } else if (skip_prefix(update, \"!\", &update)) {\n> + strat.type = SM_UPDATE_COMMAND;\n> + strat.command = update;\n> + } else {\n> + die(\"invalid submodule update strategy '%s'\", update);\n> + }\n> + }\n\nCan this be simplified by reusing\n parse_submodule_update_strategy(value, dest)\n?\n"},{"id":"325103","messageId":"20170725233736.GA71799@google.com","threadId":"46470","inReplyTo":"CAGZ79kZByzPbyLUNJ8ViVa2TDk-L+TfnF+wVWRj2d92_MhXPbg@mail.gmail.com","subject":"Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T23:37:36Z","receivedAt":"2017-07-25T23:37:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/25, Stefan Beller wrote:\n> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> > Commit aee9c7d65 (Submodules: Add the new \"ignore\" config option for\n> > diff and status) ...\n> \n> introduced in 2010, so quite widely spread.\n> \n> > ... introduced the ignore configuration option for\n> > submodules so that configured submodules could be omitted from the\n> > status and diff commands. Because this flag is respected in the diff\n> > machinery it has the unintended consequence of potentially prohibiting\n> > users from adding or resetting a submodule, even when a path to the\n> > submodule is explicitly given.\n> >\n> > Ensure that submodules can be added or set, even if they are configured\n> > to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff\n> > flag.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > builtin/add.c | 1 +\n> > builtin/reset.c | 1 +\n> > 2 files changed, 2 insertions(+)\n> >\n> > diff --git a/builtin/add.c b/builtin/add.c\n> > index e888fb8c5..6f271512f 100644\n> > --- a/builtin/add.c\n> > +++ b/builtin/add.c\n> > @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,\n> > rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n> > rev.diffopt.format_callback = update_callback;\n> > rev.diffopt.format_callback_data = &data;\n> > + rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n> \n> \n> This flag occurs once in the code base, with the comment:\n> /*\n> * Unless the user did explicitly request a submodule\n> * ignore mode by passing a command line option we do\n> * not ignore any changed submodule SHA-1s when\n> * comparing index and parent, no matter what is\n> * configured. Otherwise we won't commit any\n> * submodules which were manually staged, which would\n> * be really confusing.\n> */\n> int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n> \n> in prepare_commit, so commit ignores the .gitmodules file.\n> \n> This allows git-add to add ignored submodules, currently ignored submodules\n> would have to be added using the plumbing\n> git update-index --add --cacheinfo 160000,$SHA1,\n> \n> This makes sense, though a test demonstrating the change in behavior\n> would be nice, but git-add doesn't seem to change as it doesn't even load\n> the git modules config?\n\nI can add a comment to the code but its already being tested in the\nsubmodule test suite. The only reason this doesn't cause any changes\nnow is that the gitmodules config is never loaded, but that may\nchange if we decide to allow lazy-loading of the gitmodules file (like\nthe last couple patches in this series do).\n\n-- \nBrandon Williams\n"},{"id":"325104","messageId":"20170725233903.GB71799@google.com","threadId":"46470","inReplyTo":"CAGZ79kacdTFVJknTx+ceT8epytXSJDRVAwZO4HyzpsmVbK5VTQ@mail.gmail.com","subject":"Re: [PATCH 05/15] submodule--helper: don't overlay config in update-clone","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T23:39:03Z","receivedAt":"2017-07-25T23:39:09Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/25, Stefan Beller wrote:\n> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> > Don't rely on overlaying the repository's config on top of the\n> > submodule-config, instead query the repository's config directly for the\n> > url and the update strategy configuration.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> ...\n> \n> > +struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,\n> > + const struct submodule *sub)\n> > +{\n> > + struct submodule_update_strategy strat = sub->update_strategy;\n> > + const char *update;\n> > + char *key;\n> > +\n> > + key = xstrfmt(\"submodule.%s.update\", sub->name);\n> > + if (!repo_config_get_string_const(repo, key, &update)) {\n> > + strat.command = NULL;\n> > + if (!strcmp(update, \"none\")) {\n> > + strat.type = SM_UPDATE_NONE;\n> > + } else if (!strcmp(update, \"checkout\")) {\n> > + strat.type = SM_UPDATE_CHECKOUT;\n> > + } else if (!strcmp(update, \"rebase\")) {\n> > + strat.type = SM_UPDATE_REBASE;\n> > + } else if (!strcmp(update, \"merge\")) {\n> > + strat.type = SM_UPDATE_MERGE;\n> > + } else if (skip_prefix(update, \"!\", &update)) {\n> > + strat.type = SM_UPDATE_COMMAND;\n> > + strat.command = update;\n> > + } else {\n> > + die(\"invalid submodule update strategy '%s'\", update);\n> > + }\n> > + }\n> \n> Can this be simplified by reusing\n> parse_submodule_update_strategy(value, dest)\n> ?\n\nIt would result in a memory leak if we did. Really I'd like to just\nremove this entirely. The only reason this needs to be done is for\ncheckout, which if we don't have respect the update config it can be\nremoved.\n\n-- \nBrandon Williams\n"},{"id":"325105","messageId":"CAGZ79kZGFhiNAYqJ9hZqDLEZt-9jYQ=o0ej2VmO0E=pZg85Fsg@mail.gmail.com","threadId":"46470","inReplyTo":"20170725213928.125998-7-bmwill@google.com","subject":"Re: [PATCH 06/15] fetch: don't overlay config with submodule-config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T23:44:04Z","receivedAt":"2017-07-25T23:44:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> Don't rely on overlaying the repository's config on top of the\n> submodule-config, instead query the repository's config directly for the\n> fetch_recurse field.\n>\n> Signed-off-by: Brandon Williams \n\nReviewed-by: Stefan Beller \n\n> ---\n> builtin/fetch.c | 1 -\n> submodule.c | 24 +++++++++++++++++-------\n> 2 files changed, 17 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index d84c26391..3fe99073d 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>\n> if (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n> gitmodules_config();\n> - git_config(submodule_config, NULL);\n> }\n>\n> if (all) {\n> diff --git a/submodule.c b/submodule.c\n> index 8b9e48a61..c5058a4b8 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1210,14 +1210,24 @@ static int get_next_submodule(struct child_process *cp,\n>\n> default_argv = \"yes\";\n> if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> - if (submodule &&\n> - submodule->fetch_recurse !=\n> - RECURSE_SUBMODULES_NONE) {\n> - if (submodule->fetch_recurse ==\n> - RECURSE_SUBMODULES_OFF)\n> + int fetch_recurse = RECURSE_SUBMODULES_NONE;\n> +\n> + if (submodule) {\n> + char *key;\n> + const char *value;\n> +\n> + fetch_recurse = submodule->fetch_recurse;\n> + key = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> + if (!repo_config_get_string_const(the_repository, key, &value)) {\n> + fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> + }\n> + free(key);\n> + }\n\nI wonder if it would be better to parse this in builtin/fetch.c#git_fetch_config\nand then pass it in here as a parameter, instead of looking it up directly here?\nThat way it is easier to keep track of what a builtin pays attention to.\n\n\n> +\n> + if (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n> + if (fetch_recurse == RECURSE_SUBMODULES_OFF)\n> continue;\n> - if (submodule->fetch_recurse ==\n> - RECURSE_SUBMODULES_ON_DEMAND) {\n> + if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n> if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n> continue;\n> default_argv = \"on-demand\";\n> --\n> 2.14.0.rc0.400.g1c36432dff-goog\n>\n"},{"id":"325106","messageId":"CAGZ79kZ24eT6kyLE6W8PTQwCy3gJWUaexFCOdQrQbS5tPEAmsg@mail.gmail.com","threadId":"46470","inReplyTo":"20170725213928.125998-8-bmwill@google.com","subject":"Re: [PATCH 07/15] submodule: don't rely on overlayed config when setting diffopts","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T23:46:44Z","receivedAt":"2017-07-25T23:46:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> Don't rely on overlaying the repository's config on top of the\n> submodule-config, instead query the repository's config directory for\n> the ignore field.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> submodule.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index c5058a4b8..f86b82fbb 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -165,8 +165,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n> {\n> const struct submodule *submodule = submodule_from_path(&null_oid, path);\n> if (submodule) {\n> - if (submodule->ignore)\n> - handle_ignore_submodules_arg(diffopt, submodule->ignore);\n> + const char *ignore;\n> + char *key;\n> +\n> + key = xstrfmt(\"submodule.%s.ignore\", submodule->name);\n> + if (repo_config_get_string_const(the_repository, key, &ignore))\n> + ignore = submodule->ignore;\n\nUnlike the last patch, we have to use a direct lookup here as\nthe alternative is hugely painful.\n\n\n> + free(key);\n> +\n> + if (ignore)\n> + handle_ignore_submodules_arg(diffopt, ignore);\n> else if (is_gitmodules_unmerged(&the_index))\n> DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);\n> }\n> --\n> 2.14.0.rc0.400.g1c36432dff-goog\n>\n"},{"id":"325107","messageId":"20170725234825.GC71799@google.com","threadId":"46470","inReplyTo":"CAGZ79kZGFhiNAYqJ9hZqDLEZt-9jYQ=o0ej2VmO0E=pZg85Fsg@mail.gmail.com","subject":"Re: [PATCH 06/15] fetch: don't overlay config with submodule-config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-25T23:48:25Z","receivedAt":"2017-07-25T23:48:32Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/25, Stefan Beller wrote:\n> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> > Don't rely on overlaying the repository's config on top of the\n> > submodule-config, instead query the repository's config directly for the\n> > fetch_recurse field.\n> >\n> > Signed-off-by: Brandon Williams \n> \n> Reviewed-by: Stefan Beller \n> \n> > ---\n> > builtin/fetch.c | 1 -\n> > submodule.c | 24 +++++++++++++++++-------\n> > 2 files changed, 17 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index d84c26391..3fe99073d 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> >\n> > if (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n> > gitmodules_config();\n> > - git_config(submodule_config, NULL);\n> > }\n> >\n> > if (all) {\n> > diff --git a/submodule.c b/submodule.c\n> > index 8b9e48a61..c5058a4b8 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -1210,14 +1210,24 @@ static int get_next_submodule(struct child_process *cp,\n> >\n> > default_argv = \"yes\";\n> > if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> > - if (submodule &&\n> > - submodule->fetch_recurse !=\n> > - RECURSE_SUBMODULES_NONE) {\n> > - if (submodule->fetch_recurse ==\n> > - RECURSE_SUBMODULES_OFF)\n> > + int fetch_recurse = RECURSE_SUBMODULES_NONE;\n> > +\n> > + if (submodule) {\n> > + char *key;\n> > + const char *value;\n> > +\n> > + fetch_recurse = submodule->fetch_recurse;\n> > + key = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> > + if (!repo_config_get_string_const(the_repository, key, &value)) {\n> > + fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> > + }\n> > + free(key);\n> > + }\n> \n> I wonder if it would be better to parse this in builtin/fetch.c#git_fetch_config\n> and then pass it in here as a parameter, instead of looking it up directly here?\n> That way it is easier to keep track of what a builtin pays attention to.\n\nReally the fact that you can configure individual submodules in\n.gitmodules to be fetched recursively or not is a terrible design IMO.\nAlso this is a per-submodule configuration so having it in\nbuiltin/fetch.c would be incredibly annoying to handle.\n\n> \n> \n> > +\n> > + if (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n> > + if (fetch_recurse == RECURSE_SUBMODULES_OFF)\n> > continue;\n> > - if (submodule->fetch_recurse ==\n> > - RECURSE_SUBMODULES_ON_DEMAND) {\n> > + if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n> > if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n> > continue;\n> > default_argv = \"on-demand\";\n> > --\n> > 2.14.0.rc0.400.g1c36432dff-goog\n> >\n\n-- \nBrandon Williams\n"},{"id":"325152","messageId":"xmqq1sp2rk6w.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"20170725213928.125998-2-bmwill@google.com","subject":"Re: [PATCH 01/15] t7411: check configuration parsing errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T20:56:39Z","receivedAt":"2017-07-26T20:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Check for configuration parsing errors in '.gitmodules' in t7411, which\n> is explicitly testing the submodule-config subsystem, instead of in\n> t7400. Also explicitly use the test helper instead of relying on the\n> gitmodules file from being read in status.\n\nMakes sense.\n\n> ...\n> -\ttest_must_fail git status\n> -'\n> -...\n> +test_expect_success 'configuration parsing with error' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\ttest_create_repo repo &&\n> +\tcat >repo/.gitmodules <<-\\EOF &&\n> +\t[submodule \"s\"]\n> +\t\tpath\n> +\t\tignore\n> +\tEOF\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_must_fail test-submodule-config \"\" s 2>actual &&\n> +\t\ttest_i18ngrep \"bad config\" actual\n> +\t)\n> +'\n> +\n> cat >super/expect < Submodule name: 'a' for path 'a'\n> Submodule name: 'a' for path 'b'\n"},{"id":"325153","messageId":"xmqqwp6uq56s.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"CAGZ79kY9Pdk5C8=k-AQpCPwo3q9Jzfg9A93UQxGyyf_OyrMS_Q@mail.gmail.com","subject":"Re: [PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T21:06:03Z","receivedAt":"2017-07-26T21:06:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Rereading the archives, there was quite some discussion on the design\n> of these patches, but these lines of code did not get any attention\n>\n> https://public-inbox.org/git/4CDB3063.5010801@web.de/\n>\n> I cc'd Jens in the hope of him having a good memory why he\n> wrote the code that way. :)\n\nThanks for digging. I wouldn't be surprised if this were a fallback\nto help a broken entry in .gitmodules that lack .path variable, but\nwe shouldn't be sweeping the problem under the rug like that. \n\nI wonder if we should barf loudly if there shouldn't be a submodule\nat that path, i.e.\n\n\tif (!submodule)\n\t\tdie(\"there is no submodule defined for path '%s'\"...);\n\nthough.\n\n> Note that this is the last caller of submodule_from_name being\n> removed, so I would expect removal of submodule_from_name from\n> the t/helper/test-submodule-config.c as well as\n> Documentation/technical/api-submodule-config.txt\n> in a later part of this series. (Well technically it could go outside\n> of the series, but in the mean time we'd document and test\n> dead code)\n\nGood thinking. As this is \"cleanup\" series, I think it is within\nits scope to remove an API function that becomes unused.\n\n>\n>> Signed-off-by: Brandon Williams \n>> ---\n>> submodule.c | 2 --\n>> 1 file changed, 2 deletions(-)\n>>\n>> diff --git a/submodule.c b/submodule.c\n>> index 7e87e4698..fd391aea6 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n>> continue;\n>>\n>> submodule = submodule_from_path(&null_oid, ce->name);\n>> - if (!submodule)\n>> - submodule = submodule_from_name(&null_oid, ce->name);\n>>\n>> default_argv = \"yes\";\n>> if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n>> --\n>> 2.14.0.rc0.400.g1c36432dff-goog\n>>\n"},{"id":"325155","messageId":"xmqqinieq49v.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"CAGZ79kZByzPbyLUNJ8ViVa2TDk-L+TfnF+wVWRj2d92_MhXPbg@mail.gmail.com","subject":"Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T21:25:48Z","receivedAt":"2017-07-26T21:25:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n>> Commit aee9c7d65 (Submodules: Add the new \"ignore\" config option for\n>> diff and status) ...\n>\n> introduced in 2010, so quite widely spread.\n>\n>> ... introduced the ignore configuration option for\n>> submodules so that configured submodules could be omitted from the\n>> status and diff commands. Because this flag is respected in the diff\n>> machinery it has the unintended consequence of potentially prohibiting\n>> users from adding or resetting a submodule, even when a path to the\n>> submodule is explicitly given.\n>>\n>> Ensure that submodules can be added or set, even if they are configured\n>> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff\n>> flag.\n>>\n>> Signed-off-by: Brandon Williams \n>> ---\n>> builtin/add.c | 1 +\n>> builtin/reset.c | 1 +\n>> 2 files changed, 2 insertions(+)\n>>\n>> diff --git a/builtin/add.c b/builtin/add.c\n>> index e888fb8c5..6f271512f 100644\n>> --- a/builtin/add.c\n>> +++ b/builtin/add.c\n>> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,\n>> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n>> rev.diffopt.format_callback = update_callback;\n>> rev.diffopt.format_callback_data = &data;\n>> + rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n>\n>\n> This flag occurs once in the code base, with the comment:\n> /*\n> * Unless the user did explicitly request a submodule\n> * ignore mode by passing a command line option we do\n> * not ignore any changed submodule SHA-1s when\n> * comparing index and parent, no matter what is\n> * configured. Otherwise we won't commit any\n> * submodules which were manually staged, which would\n> * be really confusing.\n> */\n> int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n>\n> in prepare_commit, so commit ignores the .gitmodules file.\n>\n> This allows git-add to add ignored submodules, currently ignored submodules\n> would have to be added using the plumbing\n> git update-index --add --cacheinfo 160000,$SHA1,\n\nLet me play devil's advocate (as I have this suspicion that .ignore\nthing specific for submodule is probably misdesigned and certainly\nits implementation is backwards). Is the primary use case for this\n.ignore thing to be able to do\n\n\tgit add .\n\nwithout having to worry about adding the submodule marked as such? \nAnd if so, wouldn't it surprise these users who do use .ignore if\n\"git add\" suddenly started adding them?\n\nI think the right tool to use these days for excluding some paths\nwhen adding all others is the negative pathspec; perhaps back when\nthe .ignore thing was added, it didn't exist or not widely known? \n\nI suspect that it may result in a better system overall if we can\ndeprecate and remove the submodule-specific .ignore thing. At\nleast, I think the DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG is backwards\nin that .ignore causes a submodule to be excluded from the diff by\ndefault and forces paths that care about differences to opt into the\n\"override\" thing, which is wrong---the specific UI thing that wants\nnot to show them should instead opt into ignoring, while keeping the\ndefault not to special case such a flag that can only be set to a\nsubmodule path.\n\n> This makes sense, though a test demonstrating the change in behavior\n> would be nice, but git-add doesn't seem to change as it doesn't even load\n> the git modules config?\n>\n>> rev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n>> run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n>> return !!data.add_errors;\n>> diff --git a/builtin/reset.c b/builtin/reset.c\n>> index 046403ed6..772d078b8 100644\n>> --- a/builtin/reset.c\n>> +++ b/builtin/reset.c\n>> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n>> opt.output_format = DIFF_FORMAT_CALLBACK;\n>> opt.format_callback = update_index_from_diff;\n>> opt.format_callback_data = &intent_to_add;\n>> + opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n>\n> same here? Also as this is not failing any test, it may be worth adding one\n> to document the behavior of the \"submodule..ignore\" flag in tests?\n>\n>>\n>> if (do_diff_cache(tree_oid, &opt))\n>> return 1;\n>> --\n>> 2.14.0.rc0.400.g1c36432dff-goog\n>>\n"},{"id":"325156","messageId":"xmqqeft2q3zu.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"20170725213928.125998-10-bmwill@google.com","subject":"Re: [PATCH 09/15] submodule: remove submodule_config callback routine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T21:31:49Z","receivedAt":"2017-07-26T21:31:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Remove the last remaining caller of 'submodule_config()' as well as the\n> function itself.\n>\n> With 'submodule_config()' being removed the submodule-config API can be\n> a little simpler as callers don't need to worry about whether or not\n> they need to overlay the repository's config on top of the\n> submodule-config. This also makes it more difficult to accidentally\n> add non-submodule specific configuration to the .gitmodules file.\n\nNice.\n"},{"id":"325275","messageId":"a3650c9a-fa42-09e6-efcd-f912d5ffc042@web.de","threadId":"46470","inReplyTo":"xmqqwp6uq56s.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2017-07-30T13:43:25Z","receivedAt":"2017-07-30T13:43:53Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 26.07.2017 um 23:06 schrieb Junio C Hamano:\n> Stefan Beller writes:\n> \n>> Rereading the archives, there was quite some discussion on the design\n>> of these patches, but these lines of code did not get any attention\n>>\n>> https://public-inbox.org/git/4CDB3063.5010801@web.de/\n>>\n>> I cc'd Jens in the hope of him having a good memory why he\n>> wrote the code that way. :)\n> \n> Thanks for digging. I wouldn't be surprised if this were a fallback\n> to help a broken entry in .gitmodules that lack .path variable, but\n> we shouldn't be sweeping the problem under the rug like that.\n\nSorry to disappoint you ;-) I added this in 7dce19d374 because\nsubmodule by path lookup back then only parsed the checked out\n.gitmodules file. So looking for it by name was a good guess to\nfetch a new submodule that wasn't present in the current HEAD's\n.gitmodules, as the path is used as the default name in \"git\nsubmodule add\".\n\nThe refactoring in 851e18c385 could and should have removed that\nbecause since then we use the .gitmodules path to name mapping\nof the fetched commit.\n\n> I wonder if we should barf loudly if there shouldn't be a submodule\n> at that path, i.e.\n> \n> \tif (!submodule)\n> \t\tdie(\"there is no submodule defined for path '%s'\"...);\n> \n> though.\n\nNot sure if you want to die() or just issue a warning(), but yes.\n"},{"id":"325284","messageId":"xmqqefsxk469.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"a3650c9a-fa42-09e6-efcd-f912d5ffc042@web.de","subject":"Re: [PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-30T21:25:50Z","receivedAt":"2017-07-30T21:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n>> I wonder if we should barf loudly if there shouldn't be a submodule\n>> at that path, i.e.\n>>\n>> \tif (!submodule)\n>> \t\tdie(\"there is no submodule defined for path '%s'\"...);\n>>\n>> though.\n>\n> Not sure if you want to die() or just issue a warning(), but yes.\n\nAs long as the code after that point is prepared to see a NULL\nsubmodule and still behaves sensibly, then I would of course prefer\nnot dying. Continuing with just a warning() may not be a safe thing\nto do if we are not prepared to see a NULL submodule after that\npoint, though.\n"},{"id":"325319","messageId":"CAGZ79kZxprtLGOzURHaxc5YzviSj_2Kx23v=gjr2uFb+tbNfjw@mail.gmail.com","threadId":"46470","inReplyTo":"a3650c9a-fa42-09e6-efcd-f912d5ffc042@web.de","subject":"Re: [PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-31T20:43:04Z","receivedAt":"2017-07-31T20:43:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann wrote:\n> Am 26.07.2017 um 23:06 schrieb Junio C Hamano:\n>>\n>> Stefan Beller writes:\n>>\n>>> Rereading the archives, there was quite some discussion on the design\n>>> of these patches, but these lines of code did not get any attention\n>>>\n>>> https://public-inbox.org/git/4CDB3063.5010801@web.de/\n>>>\n>>> I cc'd Jens in the hope of him having a good memory why he\n>>> wrote the code that way. :)\n>>\n>>\n>> Thanks for digging. I wouldn't be surprised if this were a fallback\n>> to help a broken entry in .gitmodules that lack .path variable, but\n>> we shouldn't be sweeping the problem under the rug like that.\n>\n>\n> Sorry to disappoint you ;-) I added this in 7dce19d374 because\n> submodule by path lookup back then only parsed the checked out\n> .gitmodules file.\n\nThis is still the case AFAICT, as we never ask for a specific .gitmodules\nfile identified by sha1 of the commit.\n\n> So looking for it by name was a good guess to\n> fetch a new submodule that wasn't present in the current HEAD's\n> .gitmodules, as the path is used as the default name in \"git\n> submodule add\".\n\n3 things:\na) I think it is not as much a feature ('fallback to still make it work'),\n but rather a bug as when there is no (or wrong) entry in the .gitmodules\n file, reporting it is better than trying something.\nb) in the case of moved submodules (2 submodules swapped their path)\n this may be harmful as we'd get a wrong submodule potentially.\n\nc) I wonder if we want to use a different default for submodule names\n as I have seen people get confused by path and name being the same,\n e.g. to move a submodule they would have not just adapted the path,\n but any occurrence of the string that reads like the path.\n (i.e. also change the name, defeating the purpose of name/path\n separation).\n\n For a new name default, I would wager for some non-legible gibberish\n such as \"hash( path/time )\", as that sends a clear message to not mess\n with the value of the name.\n\n>\n> The refactoring in 851e18c385 could and should have removed that\n> because since then we use the .gitmodules path to name mapping\n> of the fetched commit.\n>\n>> I wonder if we should barf loudly if there shouldn't be a submodule\n>> at that path, i.e.\n>>\n>> if (!submodule)\n>> die(\"there is no submodule defined for path '%s'\"...);\n>>\n>> though.\n>\n>\n> Not sure if you want to die() or just issue a warning(), but yes.\n\nEither die() or \"warning && return 0\" is fine with me.\n"},{"id":"325320","messageId":"20170731205003.GB181489@google.com","threadId":"46470","inReplyTo":"xmqqinieq49v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-31T20:50:03Z","receivedAt":"2017-07-31T20:50:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/26, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n> > On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams wrote:\n> >> Commit aee9c7d65 (Submodules: Add the new \"ignore\" config option for\n> >> diff and status) ...\n> >\n> > introduced in 2010, so quite widely spread.\n> >\n> >> ... introduced the ignore configuration option for\n> >> submodules so that configured submodules could be omitted from the\n> >> status and diff commands. Because this flag is respected in the diff\n> >> machinery it has the unintended consequence of potentially prohibiting\n> >> users from adding or resetting a submodule, even when a path to the\n> >> submodule is explicitly given.\n> >>\n> >> Ensure that submodules can be added or set, even if they are configured\n> >> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff\n> >> flag.\n> >>\n> >> Signed-off-by: Brandon Williams \n> >> ---\n> >> builtin/add.c | 1 +\n> >> builtin/reset.c | 1 +\n> >> 2 files changed, 2 insertions(+)\n> >>\n> >> diff --git a/builtin/add.c b/builtin/add.c\n> >> index e888fb8c5..6f271512f 100644\n> >> --- a/builtin/add.c\n> >> +++ b/builtin/add.c\n> >> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,\n> >> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n> >> rev.diffopt.format_callback = update_callback;\n> >> rev.diffopt.format_callback_data = &data;\n> >> + rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n> >\n> >\n> > This flag occurs once in the code base, with the comment:\n> > /*\n> > * Unless the user did explicitly request a submodule\n> > * ignore mode by passing a command line option we do\n> > * not ignore any changed submodule SHA-1s when\n> > * comparing index and parent, no matter what is\n> > * configured. Otherwise we won't commit any\n> > * submodules which were manually staged, which would\n> > * be really confusing.\n> > */\n> > int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n> >\n> > in prepare_commit, so commit ignores the .gitmodules file.\n> >\n> > This allows git-add to add ignored submodules, currently ignored submodules\n> > would have to be added using the plumbing\n> > git update-index --add --cacheinfo 160000,$SHA1,\n> \n> Let me play devil's advocate (as I have this suspicion that .ignore\n> thing specific for submodule is probably misdesigned and certainly\n> its implementation is backwards). Is the primary use case for this\n> .ignore thing to be able to do\n> \n> \tgit add .\n> \n> without having to worry about adding the submodule marked as such? \n> And if so, wouldn't it surprise these users who do use .ignore if\n> \"git add\" suddenly started adding them?\n> \n> I think the right tool to use these days for excluding some paths\n> when adding all others is the negative pathspec; perhaps back when\n> the .ignore thing was added, it didn't exist or not widely known? \n> \n> I suspect that it may result in a better system overall if we can\n> deprecate and remove the submodule-specific .ignore thing. At\n> least, I think the DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG is backwards\n> in that .ignore causes a submodule to be excluded from the diff by\n> default and forces paths that care about differences to opt into the\n> \"override\" thing, which is wrong---the specific UI thing that wants\n> not to show them should instead opt into ignoring, while keeping the\n> default not to special case such a flag that can only be set to a\n> submodule path.\n\nIt looks like .ignore was added with aee9c7d65 (Submodules: Add the new\n\"ignore\" config option for diff and status, 2010-08-06) in order to\nignore particular submodules with 'status' and 'diff' commands. I don't\nthink it was intended to ignore submodules with commands like add and\nreset. Either way I agree that some of the things with most of the\nsubmodules config seem a bit backwards and we may want to migrate away\nfrom them completely as we begin to add more support for submodules into\nthe builtin commands.\n\n> \n> > This makes sense, though a test demonstrating the change in behavior\n> > would be nice, but git-add doesn't seem to change as it doesn't even load\n> > the git modules config?\n> >\n> >> rev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n> >> run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n> >> return !!data.add_errors;\n> >> diff --git a/builtin/reset.c b/builtin/reset.c\n> >> index 046403ed6..772d078b8 100644\n> >> --- a/builtin/reset.c\n> >> +++ b/builtin/reset.c\n> >> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n> >> opt.output_format = DIFF_FORMAT_CALLBACK;\n> >> opt.format_callback = update_index_from_diff;\n> >> opt.format_callback_data = &intent_to_add;\n> >> + opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n> >\n> > same here? Also as this is not failing any test, it may be worth adding one\n> > to document the behavior of the \"submodule..ignore\" flag in tests?\n> >\n> >>\n> >> if (do_diff_cache(tree_oid, &opt))\n> >> return 1;\n> >> --\n> >> 2.14.0.rc0.400.g1c36432dff-goog\n> >>\n\n-- \nBrandon Williams\n"},{"id":"325554","messageId":"20170803182000.179328-4-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 03/15] add, reset: ensure submodules can be added or reset","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:48Z","receivedAt":"2017-08-03T18:20:21Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Commit aee9c7d65 (Submodules: Add the new \"ignore\" config option for\ndiff and status) introduced the ignore configuration option for\nsubmodules so that configured submodules could be omitted from the\nstatus and diff commands. Because this flag is respected in the diff\nmachinery it has the unintended consequence of potentially prohibiting\nusers from adding or resetting a submodule, even when a path to the\nsubmodule is explicitly given.\n\nEnsure that submodules can be added or set, even if they are configured\nto be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff\nflag.\n\nSigned-off-by: Brandon Williams \n---\n builtin/add.c | 1 +\n builtin/reset.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex e888fb8c5..6f271512f 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\n+\trev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \trev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n \treturn !!data.add_errors;\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 046403ed6..772d078b8 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.output_format = DIFF_FORMAT_CALLBACK;\n \topt.format_callback = update_index_from_diff;\n \topt.format_callback_data = &intent_to_add;\n+\topt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \n \tif (do_diff_cache(tree_oid, &opt))\n \t\treturn 1;\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325555","messageId":"20170803182000.179328-6-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 05/15] submodule--helper: don't overlay config in update-clone","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:50Z","receivedAt":"2017-08-03T18:20:22Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directly for the\nurl and the update strategy configuration.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 23 +++++++++++++++++++----\n submodule.c | 38 ++++++++++++++++++++++++++------------\n submodule.h | 1 +\n 3 files changed, 46 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f71f4270d..36df7ab78 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -780,6 +780,10 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t struct strbuf *out)\n {\n \tconst struct submodule *sub = NULL;\n+\tconst char *url = NULL;\n+\tconst char *update_string;\n+\tenum submodule_update_type update_type;\n+\tchar *key;\n \tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *displaypath = NULL;\n@@ -808,9 +812,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tgoto cleanup;\n \t}\n \n+\tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n+\tif (!repo_config_get_string_const(the_repository, key, &update_string)) {\n+\t\tupdate_type = parse_submodule_update_type(update_string);\n+\t} else {\n+\t\tupdate_type = sub->update_strategy.type;\n+\t}\n+\tfree(key);\n+\n \tif (suc->update.type == SM_UPDATE_NONE\n \t || (suc->update.type == SM_UPDATE_UNSPECIFIED\n-\t\t&& sub->update_strategy.type == SM_UPDATE_NONE)) {\n+\t\t&& update_type == SM_UPDATE_NONE)) {\n \t\tstrbuf_addf(out, _(\"Skipping submodule '%s'\"), displaypath);\n \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n@@ -822,6 +834,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tgoto cleanup;\n \t}\n \n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\tif (repo_config_get_string_const(the_repository, sb.buf, &url))\n+\t\turl = sub->url;\n+\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n \tneeds_cloning = !file_exists(sb.buf);\n@@ -851,7 +868,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\targv_array_push(&child->args, \"--depth=1\");\n \targv_array_pushl(&child->args, \"--path\", sub->path, NULL);\n \targv_array_pushl(&child->args, \"--name\", sub->name, NULL);\n-\targv_array_pushl(&child->args, \"--url\", sub->url, NULL);\n+\targv_array_pushl(&child->args, \"--url\", url, NULL);\n \tif (suc->references.nr) {\n \t\tstruct string_list_item *item;\n \t\tfor_each_string_list_item(item, &suc->references)\n@@ -1025,9 +1042,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-\t/* Overlay the parsed .gitmodules file with .git/config */\n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n \n \trun_processes_parallel(max_jobs,\n \t\t\t update_clone_get_next_task,\ndiff --git a/submodule.c b/submodule.c\nindex 19bd13bb2..8a9b964ce 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -398,24 +398,38 @@ void die_path_inside_submodule(const struct index_state *istate,\n \t}\n }\n \n-int parse_submodule_update_strategy(const char *value,\n-\t\tstruct submodule_update_strategy *dst)\n+enum submodule_update_type parse_submodule_update_type(const char *value)\n {\n-\tfree((void*)dst->command);\n-\tdst->command = NULL;\n \tif (!strcmp(value, \"none\"))\n-\t\tdst->type = SM_UPDATE_NONE;\n+\t\treturn SM_UPDATE_NONE;\n \telse if (!strcmp(value, \"checkout\"))\n-\t\tdst->type = SM_UPDATE_CHECKOUT;\n+\t\treturn SM_UPDATE_CHECKOUT;\n \telse if (!strcmp(value, \"rebase\"))\n-\t\tdst->type = SM_UPDATE_REBASE;\n+\t\treturn SM_UPDATE_REBASE;\n \telse if (!strcmp(value, \"merge\"))\n-\t\tdst->type = SM_UPDATE_MERGE;\n-\telse if (skip_prefix(value, \"!\", &value)) {\n-\t\tdst->type = SM_UPDATE_COMMAND;\n-\t\tdst->command = xstrdup(value);\n-\t} else\n+\t\treturn SM_UPDATE_MERGE;\n+\telse if (*value == '!')\n+\t\treturn SM_UPDATE_COMMAND;\n+\telse\n+\t\treturn SM_UPDATE_UNSPECIFIED;\n+}\n+\n+int parse_submodule_update_strategy(const char *value,\n+\t\tstruct submodule_update_strategy *dst)\n+{\n+\tenum submodule_update_type type;\n+\n+\tfree((void*)dst->command);\n+\tdst->command = NULL;\n+\n+\ttype = parse_submodule_update_type(value);\n+\tif (type == SM_UPDATE_UNSPECIFIED)\n \t\treturn -1;\n+\n+\tdst->type = type;\n+\tif (type == SM_UPDATE_COMMAND)\n+\t\tdst->command = xstrdup(value + 1);\n+\n \treturn 0;\n }\n \ndiff --git a/submodule.h b/submodule.h\nindex e402b004f..48586efe7 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -62,6 +62,7 @@ extern void die_in_unpopulated_submodule(const struct index_state *istate,\n \t\t\t\t\t const char *prefix);\n extern void die_path_inside_submodule(const struct index_state *istate,\n \t\t\t\t const struct pathspec *ps);\n+extern enum submodule_update_type parse_submodule_update_type(const char *value);\n extern int parse_submodule_update_strategy(const char *value,\n \t\tstruct submodule_update_strategy *dst);\n extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325556","messageId":"20170803182000.179328-9-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 08/15] unpack-trees: don't respect submodule.update","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:53Z","receivedAt":"2017-08-03T18:20:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The 'submodule.update' config was historically used and respected by the\n'submodule update' command because update handled a variety of different\nways it updated a submodule. As we begin teaching other commands about\nsubmodules it makes more sense for the different settings of\n'submodule.update' to be handled by the individual commands themselves\n(checkout, rebase, merge, etc) so it shouldn't be respected by the\nnative checkout command.\n\nAlso remove the overlaying of the repository's config (via using\n'submodule_config()') from the commands which use the unpack-trees\nlogic (checkout, read-tree, reset).\n\nSigned-off-by: Brandon Williams \n---\n builtin/checkout.c | 2 +-\n submodule.c | 1 -\n unpack-trees.c | 38 ++++++++------------------------------\n 3 files changed, 9 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9661e1bcb..246e0cd16 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -858,7 +858,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (starts_with(var, \"submodule.\"))\n-\t\treturn submodule_config(var, value, NULL);\n+\t\treturn git_default_submodule_config(var, value, NULL);\n \n \treturn git_xmerge_config(var, value, NULL);\n }\ndiff --git a/submodule.c b/submodule.c\nindex a32043893..f913c2341 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -235,7 +235,6 @@ void load_submodule_cache(void)\n \t\treturn;\n \n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n }\n \n static int gitmodules_cb(const char *var, const char *value, void *data)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 05335fe5b..5dce7ff7d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -255,28 +255,17 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n {\n \tunsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n \tconst struct submodule *sub = submodule_from_ce(ce);\n+\n \tif (!sub)\n \t\treturn 0;\n \n \tif (o->reset)\n \t\tflags |= SUBMODULE_MOVE_HEAD_FORCE;\n \n-\tswitch (sub->update_strategy.type) {\n-\tcase SM_UPDATE_UNSPECIFIED:\n-\tcase SM_UPDATE_CHECKOUT:\n-\t\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n-\t\t\treturn o->gently ? -1 :\n-\t\t\t\tadd_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n-\t\treturn 0;\n-\tcase SM_UPDATE_NONE:\n-\t\treturn 0;\n-\tcase SM_UPDATE_REBASE:\n-\tcase SM_UPDATE_MERGE:\n-\tcase SM_UPDATE_COMMAND:\n-\tdefault:\n-\t\twarning(_(\"submodule update strategy not supported for submodule '%s'\"), ce->name);\n-\t\treturn -1;\n-\t}\n+\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n+\t\treturn o->gently ? -1 :\n+\t\t\t\t add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n+\treturn 0;\n }\n \n static void reload_gitmodules_file(struct index_state *index,\n@@ -293,7 +282,6 @@ static void reload_gitmodules_file(struct index_state *index,\n \t\t\t\tsubmodule_free();\n \t\t\t\tcheckout_entry(ce, state, NULL);\n \t\t\t\tgitmodules_config();\n-\t\t\t\tgit_config(submodule_config, NULL);\n \t\t\t} else\n \t\t\t\tbreak;\n \t\t}\n@@ -308,19 +296,9 @@ static void unlink_entry(const struct cache_entry *ce)\n {\n \tconst struct submodule *sub = submodule_from_ce(ce);\n \tif (sub) {\n-\t\tswitch (sub->update_strategy.type) {\n-\t\tcase SM_UPDATE_UNSPECIFIED:\n-\t\tcase SM_UPDATE_CHECKOUT:\n-\t\tcase SM_UPDATE_REBASE:\n-\t\tcase SM_UPDATE_MERGE:\n-\t\t\t/* state.force is set at the caller. */\n-\t\t\tsubmodule_move_head(ce->name, \"HEAD\", NULL,\n-\t\t\t\t\t SUBMODULE_MOVE_HEAD_FORCE);\n-\t\t\tbreak;\n-\t\tcase SM_UPDATE_NONE:\n-\t\tcase SM_UPDATE_COMMAND:\n-\t\t\treturn; /* Do not touch the submodule. */\n-\t\t}\n+\t\t/* state.force is set at the caller. */\n+\t\tsubmodule_move_head(ce->name, \"HEAD\", NULL,\n+\t\t\t\t SUBMODULE_MOVE_HEAD_FORCE);\n \t}\n \tif (!check_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn;\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325557","messageId":"20170803182000.179328-8-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 07/15] submodule: don't rely on overlayed config when setting diffopts","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:52Z","receivedAt":"2017-08-03T18:20:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directory for\nthe ignore field.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 59e3d0828..a32043893 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -165,8 +165,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n {\n \tconst struct submodule *submodule = submodule_from_path(&null_oid, path);\n \tif (submodule) {\n-\t\tif (submodule->ignore)\n-\t\t\thandle_ignore_submodules_arg(diffopt, submodule->ignore);\n+\t\tconst char *ignore;\n+\t\tchar *key;\n+\n+\t\tkey = xstrfmt(\"submodule.%s.ignore\", submodule->name);\n+\t\tif (repo_config_get_string_const(the_repository, key, &ignore))\n+\t\t\tignore = submodule->ignore;\n+\t\tfree(key);\n+\n+\t\tif (ignore)\n+\t\t\thandle_ignore_submodules_arg(diffopt, ignore);\n \t\telse if (is_gitmodules_unmerged(&the_index))\n \t\t\tDIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);\n \t}\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325558","messageId":"20170803182000.179328-10-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 09/15] submodule: remove submodule_config callback routine","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:54Z","receivedAt":"2017-08-03T18:20:31Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Remove the last remaining caller of 'submodule_config()' as well as the\nfunction itself.\n\nWith 'submodule_config()' being removed the submodule-config API can be\na little simpler as callers don't need to worry about whether or not\nthey need to overlay the repository's config on top of the\nsubmodule-config. This also makes it more difficult to accidentally\nadd non-submodule specific configuration to the .gitmodules file.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 1 -\n submodule.c | 25 ++-----------------------\n submodule.h | 1 -\n 3 files changed, 2 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 36df7ab78..ba767c704 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1205,7 +1205,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t\t\t git_submodule_helper_usage, 0);\n \n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n \n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n \t\treturn 1;\ndiff --git a/submodule.c b/submodule.c\nindex f913c2341..3b383d8c4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -180,27 +180,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t}\n }\n \n-/* For loading from the .gitmodules file. */\n-static int git_modules_config(const char *var, const char *value, void *cb)\n-{\n-\tif (starts_with(var, \"submodule.\"))\n-\t\treturn parse_submodule_config_option(var, value);\n-\treturn 0;\n-}\n-\n-/* Loads all submodule settings from the config. */\n-int submodule_config(const char *var, const char *value, void *cb)\n-{\n-\tif (!strcmp(var, \"submodule.recurse\")) {\n-\t\tint v = git_config_bool(var, value) ?\n-\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n-\t\tconfig_update_recurse_submodules = v;\n-\t\treturn 0;\n-\t} else {\n-\t\treturn git_modules_config(var, value, cb);\n-\t}\n-}\n-\n /* Cheap function that only determines if we're interested in submodules at all */\n int git_default_submodule_config(const char *var, const char *value, void *cb)\n {\n@@ -271,8 +250,8 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tstruct object_id oid;\n \n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n-\t\tgit_config_from_blob_oid(submodule_config, rev.buf,\n-\t\t\t\t\t &oid, NULL);\n+\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n+\t\t\t\t\t &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n }\ndiff --git a/submodule.h b/submodule.h\nindex 48586efe7..4f70c4944 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -40,7 +40,6 @@ extern int remove_path_from_gitmodules(const char *path);\n extern void stage_updated_gitmodules(void);\n extern void set_diffopt_flags_from_submodule_config(struct diff_options *,\n \t\tconst char *path);\n-extern int submodule_config(const char *var, const char *value, void *cb);\n extern int git_default_submodule_config(const char *var, const char *value, void *cb);\n \n struct option;\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325559","messageId":"20170803182000.179328-13-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 12/15] submodule-config: move submodule-config functions to submodule-config.c","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:57Z","receivedAt":"2017-08-03T18:20:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Migrate the functions used to initialize the submodule-config to\nsubmodule-config.c so that the callback routine used in the\ninitialization process can be static and prevent it from being used\noutside of initializing the submodule-config through the main API.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 1 +\n submodule-config.c | 38 +++++++++++++++++++++++++++++++-------\n submodule-config.h | 7 ++-----\n submodule.c | 35 -----------------------------------\n submodule.h | 2 --\n 5 files changed, 34 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b8514a002..d14612057 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -19,6 +19,7 @@\n #include \"pathspec.h\"\n #include \"run-command.h\"\n #include \"submodule.h\"\n+#include \"submodule-config.h\"\n \n static int abbrev;\n static int show_deleted;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 0b429e942..86636654b 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -449,9 +449,9 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n-int gitmodule_oid_from_commit(const struct object_id *treeish_name,\n-\t\t\t\t struct object_id *gitmodules_oid,\n-\t\t\t\t struct strbuf *rev)\n+static int gitmodule_oid_from_commit(const struct object_id *treeish_name,\n+\t\t\t\t struct object_id *gitmodules_oid,\n+\t\t\t\t struct strbuf *rev)\n {\n \tint ret = 0;\n \n@@ -552,9 +552,9 @@ static void submodule_cache_check_init(struct repository *repo)\n \tsubmodule_cache_init(repo->submodule_cache);\n }\n \n-int submodule_config_option(struct repository *repo,\n-\t\t\t const char *var, const char *value)\n+static int gitmodules_cb(const char *var, const char *value, void *data)\n {\n+\tstruct repository *repo = data;\n \tstruct parse_config_parameter parameter;\n \n \tsubmodule_cache_check_init(repo);\n@@ -567,9 +567,33 @@ int submodule_config_option(struct repository *repo,\n \treturn parse_config(var, value, ¶meter);\n }\n \n-int parse_submodule_config_option(const char *var, const char *value)\n+void repo_read_gitmodules(struct repository *repo)\n {\n-\treturn submodule_config_option(the_repository, var, value);\n+\tif (repo->worktree) {\n+\t\tchar *gitmodules;\n+\n+\t\tif (repo_read_index(repo) < 0)\n+\t\t\treturn;\n+\n+\t\tgitmodules = repo_worktree_path(repo, GITMODULES_FILE);\n+\n+\t\tif (!is_gitmodules_unmerged(repo->index))\n+\t\t\tgit_config_from_file(gitmodules_cb, gitmodules, repo);\n+\n+\t\tfree(gitmodules);\n+\t}\n+}\n+\n+void gitmodules_config_oid(const struct object_id *commit_oid)\n+{\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tstruct object_id oid;\n+\n+\tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n+\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n+\t\t\t\t\t &oid, the_repository);\n+\t}\n+\tstrbuf_release(&rev);\n }\n \n const struct submodule *submodule_from_name(const struct object_id *treeish_name,\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 84c2cf515..e3845831f 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -34,8 +34,8 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-extern int submodule_config_option(struct repository *repo,\n-\t\t\t\t const char *var, const char *value);\n+extern void repo_read_gitmodules(struct repository *repo);\n+extern void gitmodules_config_oid(const struct object_id *commit_oid);\n extern const struct submodule *submodule_from_name(\n \t\tconst struct object_id *commit_or_tree, const char *name);\n extern const struct submodule *submodule_from_path(\n@@ -43,9 +43,6 @@ extern const struct submodule *submodule_from_path(\n extern const struct submodule *submodule_from_cache(struct repository *repo,\n \t\t\t\t\t\t const struct object_id *treeish_name,\n \t\t\t\t\t\t const char *key);\n-extern int gitmodule_oid_from_commit(const struct object_id *commit_oid,\n-\t\t\t\t struct object_id *gitmodules_oid,\n-\t\t\t\t struct strbuf *rev);\n extern void submodule_free(void);\n \n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex 3b383d8c4..c1cef1c37 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -216,46 +216,11 @@ void load_submodule_cache(void)\n \tgitmodules_config();\n }\n \n-static int gitmodules_cb(const char *var, const char *value, void *data)\n-{\n-\tstruct repository *repo = data;\n-\treturn submodule_config_option(repo, var, value);\n-}\n-\n-void repo_read_gitmodules(struct repository *repo)\n-{\n-\tif (repo->worktree) {\n-\t\tchar *gitmodules;\n-\n-\t\tif (repo_read_index(repo) < 0)\n-\t\t\treturn;\n-\n-\t\tgitmodules = repo_worktree_path(repo, GITMODULES_FILE);\n-\n-\t\tif (!is_gitmodules_unmerged(repo->index))\n-\t\t\tgit_config_from_file(gitmodules_cb, gitmodules, repo);\n-\n-\t\tfree(gitmodules);\n-\t}\n-}\n-\n void gitmodules_config(void)\n {\n \trepo_read_gitmodules(the_repository);\n }\n \n-void gitmodules_config_oid(const struct object_id *commit_oid)\n-{\n-\tstruct strbuf rev = STRBUF_INIT;\n-\tstruct object_id oid;\n-\n-\tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n-\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n-\t\t\t\t\t &oid, the_repository);\n-\t}\n-\tstrbuf_release(&rev);\n-}\n-\n /*\n * Determine if a submodule has been initialized at a given 'path'\n */\ndiff --git a/submodule.h b/submodule.h\nindex 4f70c4944..02195c24f 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -47,8 +47,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n void load_submodule_cache(void);\n extern void gitmodules_config(void);\n-extern void repo_read_gitmodules(struct repository *repo);\n-extern void gitmodules_config_oid(const struct object_id *commit_oid);\n extern int is_submodule_active(struct repository *repo, const char *path);\n /*\n * Determine if a submodule has been populated at a given 'path' by checking if\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325560","messageId":"20170803182000.179328-15-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 14/15] unpack-trees: improve loading of .gitmodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:59Z","receivedAt":"2017-08-03T18:20:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When recursing submodules 'check_updates()' needs to have strict control\nover the submodule-config subsystem to ensure that the gitmodules file\nhas been read before checking cache entries which are marked for\nremoval as well ensuring the proper gitmodules file is read before\nupdating cache entries.\n\nBecause of this let's not rely on callers of 'check_updates()' to read\nthe gitmodules file before calling 'check_updates()' and handle the\nreading explicitly.\n\nSigned-off-by: Brandon Williams \n---\n unpack-trees.c | 43 +++++++++++++++++++++++++++----------------\n 1 file changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 5dce7ff7d..3c7f464fa 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1,5 +1,6 @@\n #define NO_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n+#include \"repository.h\"\n #include \"config.h\"\n #include \"dir.h\"\n #include \"tree.h\"\n@@ -268,22 +269,28 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n \treturn 0;\n }\n \n-static void reload_gitmodules_file(struct index_state *index,\n-\t\t\t\t struct checkout *state)\n+/*\n+ * Preform the loading of the repository's gitmodules file. This function is\n+ * used by 'check_update()' to perform loading of the gitmodules file in two\n+ * differnt situations:\n+ * (1) before removing entries from the working tree if the gitmodules file has\n+ * been marked for removal. This situation is specified by 'state' == NULL.\n+ * (2) before checking out entries to the working tree if the gitmodules file\n+ * has been marked for update. This situation is specified by 'state' != NULL.\n+ */\n+static void load_gitmodules_file(struct index_state *index,\n+\t\t\t\t struct checkout *state)\n {\n-\tint i;\n-\tfor (i = 0; i < index->cache_nr; i++) {\n-\t\tstruct cache_entry *ce = index->cache[i];\n-\t\tif (ce->ce_flags & CE_UPDATE) {\n-\t\t\tint r = strcmp(ce->name, GITMODULES_FILE);\n-\t\t\tif (r < 0)\n-\t\t\t\tcontinue;\n-\t\t\telse if (r == 0) {\n-\t\t\t\tsubmodule_free();\n-\t\t\t\tcheckout_entry(ce, state, NULL);\n-\t\t\t\tgitmodules_config();\n-\t\t\t} else\n-\t\t\t\tbreak;\n+\tint pos = index_name_pos(index, GITMODULES_FILE, strlen(GITMODULES_FILE));\n+\n+\tif (pos >= 0) {\n+\t\tstruct cache_entry *ce = index->cache[pos];\n+\t\tif (!state && ce->ce_flags & CE_WT_REMOVE) {\n+\t\t\trepo_read_gitmodules(the_repository);\n+\t\t} else if (state && (ce->ce_flags & CE_UPDATE)) {\n+\t\t\tsubmodule_free();\n+\t\t\tcheckout_entry(ce, state, NULL);\n+\t\t\trepo_read_gitmodules(the_repository);\n \t\t}\n \t}\n }\n@@ -343,6 +350,10 @@ static int check_updates(struct unpack_trees_options *o)\n \n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, index);\n+\n+\tif (should_update_submodules() && o->update && !o->dry_run)\n+\t\tload_gitmodules_file(index, NULL);\n+\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = index->cache[i];\n \n@@ -356,7 +367,7 @@ static int check_updates(struct unpack_trees_options *o)\n \tremove_scheduled_dirs();\n \n \tif (should_update_submodules() && o->update && !o->dry_run)\n-\t\treload_gitmodules_file(index, &state);\n+\t\tload_gitmodules_file(index, &state);\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325561","messageId":"20170803182000.179328-16-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 15/15] submodule: remove gitmodules_config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:20:00Z","receivedAt":"2017-08-03T18:20:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Now that the submodule-config subsystem can lazily read the gitmodules\nfile we no longer need to explicitly pre-read the gitmodules by calling\n'gitmodules_config()' so let's remove it.\n\nSigned-off-by: Brandon Williams \n---\n builtin/checkout.c | 1 -\n builtin/commit.c | 1 -\n builtin/diff-files.c | 1 -\n builtin/diff-index.c | 1 -\n builtin/diff-tree.c | 1 -\n builtin/diff.c | 2 --\n builtin/fetch.c | 4 ----\n builtin/grep.c | 4 ----\n builtin/mv.c | 1 -\n builtin/read-tree.c | 2 --\n builtin/reset.c | 2 --\n builtin/rm.c | 1 -\n builtin/submodule--helper.c | 14 --------------\n submodule.c | 15 ---------------\n submodule.h | 2 --\n t/helper/test-submodule-config.c | 1 -\n 16 files changed, 53 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 246e0cd16..63ae16afc 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1179,7 +1179,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \topts.prefix = prefix;\n \topts.show_progress = -1;\n \n-\tgitmodules_config();\n \tgit_config(git_checkout_config, &opts);\n \n \topts.track = BRANCH_TRACK_UNSPECIFIED;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4bbac014a..18ad714d9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -195,7 +195,6 @@ static void determine_whence(struct wt_status *s)\n static void status_init_config(struct wt_status *s, config_fn_t fn)\n {\n \twt_status_prepare(s);\n-\tgitmodules_config();\n \tgit_config(fn, s);\n \tdetermine_whence(s);\n \tinit_diff_ui_defaults();\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 17bf84d18..e88493ffe 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -26,7 +26,6 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n-\tgitmodules_config();\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 185e6f9b5..9d772f8f2 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -23,7 +23,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n-\tgitmodules_config();\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 31d2cb410..d66499909 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -110,7 +110,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n-\tgitmodules_config();\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7cde6abbc..7e3ebcea3 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -315,8 +315,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n-\tif (!no_index)\n-\t\tgitmodules_config();\n \tinit_diff_ui_defaults();\n \tgit_config(git_diff_ui_config, NULL);\n \tprecompose_argv(argc, argv);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3fe99073d..132e3224e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1360,10 +1360,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n-\t\tgitmodules_config();\n-\t}\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ac06d2d33..2d65f27d0 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1048,10 +1048,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t}\n #endif\n \n-\tif (recurse_submodules) {\n-\t\tgitmodules_config();\n-\t}\n-\n \tif (show_in_pager && (cached || list.nr))\n \t\tdie(_(\"--open-files-in-pager only works on the worktree\"));\n \ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 94fbaaa5d..ffdd5f01a 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -131,7 +131,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tstruct stat st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n \n-\tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_mv_options,\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex d5f618d08..bf87a2710 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -164,8 +164,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n \t\t\t read_tree_usage, 0);\n \n-\tload_submodule_cache();\n-\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tprefix_set = opts.prefix ? 1 : 0;\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 772d078b8..50488d273 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -309,8 +309,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n \tparse_args(&pathspec, argv, prefix, patch_mode, &rev);\n \n-\tload_submodule_cache();\n-\n \tunborn = !strcmp(rev, \"HEAD\") && get_oid(\"HEAD\", &oid);\n \tif (unborn) {\n \t\t/* reset on unborn branch: treat as reset to empty tree */\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4057e73fa..d91451fea 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -255,7 +255,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tstruct pathspec pathspec;\n \tchar *seen;\n \n-\tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_rm_options,\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ba767c704..c97fde439 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -275,8 +275,6 @@ static void module_list_active(struct module_list *list)\n \tint i;\n \tstruct module_list active_modules = MODULE_LIST_INIT;\n \n-\tgitmodules_config();\n-\n \tfor (i = 0; i < list->nr; i++) {\n \t\tconst struct cache_entry *ce = list->entries[i];\n \n@@ -337,9 +335,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* Only loads from .gitmodules, no overlay with .git/config */\n-\tgitmodules_config();\n-\n \tif (prefix && get_super_prefix())\n \t\tdie(\"BUG: cannot have prefix and superprefix\");\n \telse if (prefix)\n@@ -475,7 +470,6 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \tif (argc != 2)\n \t\tusage(_(\"git submodule--helper name \"));\n \n-\tgitmodules_config();\n \tsub = submodule_from_path(&null_oid, argv[1]);\n \n \tif (!sub)\n@@ -1042,8 +1036,6 @@ 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-\tgitmodules_config();\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@@ -1084,8 +1076,6 @@ static const char *remote_submodule_branch(const char *path)\n \tconst char *branch = NULL;\n \tchar *key;\n \n-\tgitmodules_config();\n-\n \tsub = submodule_from_path(&null_oid, path);\n \tif (!sub)\n \t\treturn NULL;\n@@ -1204,8 +1194,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, embed_gitdir_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n-\tgitmodules_config();\n-\n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n \t\treturn 1;\n \n@@ -1221,8 +1209,6 @@ static int is_active(int argc, const char **argv, const char *prefix)\n \tif (argc != 2)\n \t\tdie(\"submodule--helper is-active takes exactly 1 argument\");\n \n-\tgitmodules_config();\n-\n \treturn !is_submodule_active(the_repository, argv[1]);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex c1cef1c37..77346da88 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -208,19 +208,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \treturn 0;\n }\n \n-void load_submodule_cache(void)\n-{\n-\tif (config_update_recurse_submodules == RECURSE_SUBMODULES_OFF)\n-\t\treturn;\n-\n-\tgitmodules_config();\n-}\n-\n-void gitmodules_config(void)\n-{\n-\trepo_read_gitmodules(the_repository);\n-}\n-\n /*\n * Determine if a submodule has been initialized at a given 'path'\n */\n@@ -1093,7 +1080,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \tstruct argv_array args = ARGV_ARRAY_INIT;\n \tint ret;\n \n-\tgitmodules_config();\n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(NULL, NULL))\n \t\treturn 0;\n@@ -2000,7 +1986,6 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \t\tstrbuf_addstr(buf, git_dir);\n \t}\n \tif (!is_git_directory(buf->buf)) {\n-\t\tgitmodules_config();\n \t\tsub = submodule_from_path(&null_oid, submodule);\n \t\tif (!sub) {\n \t\t\tret = -1;\ndiff --git a/submodule.h b/submodule.h\nindex 02195c24f..be103ad9d 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -45,8 +45,6 @@ extern int git_default_submodule_config(const char *var, const char *value, void\n struct option;\n int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n-void load_submodule_cache(void);\n-extern void gitmodules_config(void);\n extern int is_submodule_active(struct repository *repo, const char *path);\n /*\n * Determine if a submodule has been populated at a given 'path' by checking if\ndiff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c\nindex f4a7c431c..f23db3b19 100644\n--- a/t/helper/test-submodule-config.c\n+++ b/t/helper/test-submodule-config.c\n@@ -32,7 +32,6 @@ int cmd_main(int argc, const char **argv)\n \t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n \n \tsetup_git_directory();\n-\tgitmodules_config();\n \n \twhile (*arg) {\n \t\tstruct object_id commit_oid;\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325562","messageId":"20170803182000.179328-14-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 13/15] submodule-config: lazy-load a repository's .gitmodules file","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:58Z","receivedAt":"2017-08-03T18:20:42Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In order to use the submodule-config subsystem, callers first need to\ninitialize it by calling 'repo_read_gitmodules()' or\n'gitmodules_config()' (which just redirects to\n'repo_read_gitmodules()'). There are a couple of callers who need to\nload an explicit revision of the repository's .gitmodules file (grep) or\nneed to modify the .gitmodules file so they would need to load it before\nmodify the file (checkout), but the majority of callers are simply\nreading the .gitmodules file present in the working tree. For the\ncommon case it would be nice to avoid the boilerplate of initializing\nthe submodule-config system before using it, so instead let's perform\nlazy-loading of the submodule-config system.\n\nRemove the calls to reading the gitmodules file from ls-files to show\nthat lazy-loading the .gitmodules file works.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 5 -----\n submodule-config.c | 27 ++++++++++++++++++++++-----\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d14612057..bd74ee07d 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -211,8 +211,6 @@ static void show_submodule(struct repository *superproject,\n \tif (repo_read_index(&submodule) < 0)\n \t\tdie(\"index file corrupt\");\n \n-\trepo_read_gitmodules(&submodule);\n-\n \tshow_files(&submodule, dir);\n \n \trepo_clear(&submodule);\n@@ -611,9 +609,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n-\tif (recurse_submodules)\n-\t\trepo_read_gitmodules(the_repository);\n-\n \tif (recurse_submodules &&\n \t (show_stage || show_deleted || show_others || show_unmerged ||\n \t show_killed || show_modified || show_resolve_undo || with_tree))\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 86636654b..56d9d76d4 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -18,6 +18,7 @@ struct submodule_cache {\n \tstruct hashmap for_path;\n \tstruct hashmap for_name;\n \tunsigned initialized:1;\n+\tunsigned gitmodules_read:1;\n };\n \n /*\n@@ -93,6 +94,7 @@ static void submodule_cache_clear(struct submodule_cache *cache)\n \thashmap_free(&cache->for_path, 1);\n \thashmap_free(&cache->for_name, 1);\n \tcache->initialized = 0;\n+\tcache->gitmodules_read = 0;\n }\n \n void submodule_cache_free(struct submodule_cache *cache)\n@@ -557,8 +559,6 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \tstruct repository *repo = data;\n \tstruct parse_config_parameter parameter;\n \n-\tsubmodule_cache_check_init(repo);\n-\n \tparameter.cache = repo->submodule_cache;\n \tparameter.treeish_name = NULL;\n \tparameter.gitmodules_sha1 = null_sha1;\n@@ -569,6 +569,8 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \n void repo_read_gitmodules(struct repository *repo)\n {\n+\tsubmodule_cache_check_init(repo);\n+\n \tif (repo->worktree) {\n \t\tchar *gitmodules;\n \n@@ -582,6 +584,8 @@ void repo_read_gitmodules(struct repository *repo)\n \n \t\tfree(gitmodules);\n \t}\n+\n+\trepo->submodule_cache->gitmodules_read = 1;\n }\n \n void gitmodules_config_oid(const struct object_id *commit_oid)\n@@ -589,24 +593,37 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tstruct strbuf rev = STRBUF_INIT;\n \tstruct object_id oid;\n \n+\tsubmodule_cache_check_init(the_repository);\n+\n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n \t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n \t\t\t\t\t &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n+\n+\tthe_repository->submodule_cache->gitmodules_read = 1;\n+}\n+\n+static void gitmodules_read_check(struct repository *repo)\n+{\n+\tsubmodule_cache_check_init(repo);\n+\n+\t/* read the repo's .gitmodules file if it hasn't been already */\n+\tif (!repo->submodule_cache->gitmodules_read)\n+\t\trepo_read_gitmodules(repo);\n }\n \n const struct submodule *submodule_from_name(const struct object_id *treeish_name,\n \t\tconst char *name)\n {\n-\tsubmodule_cache_check_init(the_repository);\n+\tgitmodules_read_check(the_repository);\n \treturn config_from(the_repository->submodule_cache, treeish_name, name, lookup_name);\n }\n \n const struct submodule *submodule_from_path(const struct object_id *treeish_name,\n \t\tconst char *path)\n {\n-\tsubmodule_cache_check_init(the_repository);\n+\tgitmodules_read_check(the_repository);\n \treturn config_from(the_repository->submodule_cache, treeish_name, path, lookup_path);\n }\n \n@@ -614,7 +631,7 @@ const struct submodule *submodule_from_cache(struct repository *repo,\n \t\t\t\t\t const struct object_id *treeish_name,\n \t\t\t\t\t const char *key)\n {\n-\tsubmodule_cache_check_init(repo);\n+\tgitmodules_read_check(repo);\n \treturn config_from(repo->submodule_cache, treeish_name,\n \t\t\t key, lookup_path);\n }\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325563","messageId":"20170803182000.179328-11-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:55Z","receivedAt":"2017-08-03T18:20:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Traditionally a submodule is comprised of a gitlink as well as a\ncorresponding entry in the .gitmodules file. Diff doesn't follow this\nparadigm as its config callback routine falls back to populating the\nsubmodule-config if a config entry starts with 'submodule.'.\n\nRemove this behavior in order to be consistent with how the\nsubmodule-config is populated, via calling 'gitmodules_config()' or\n'repo_read_gitmodules()'.\n\nSigned-off-by: Brandon Williams \n---\n diff.c | 3 ---\n t/t4027-diff-submodule.sh | 67 -----------------------------------------------\n 2 files changed, 70 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 85e714f6c..e43519b88 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (starts_with(var, \"submodule.\"))\n-\t\treturn parse_submodule_config_option(var, value);\n-\n \tif (git_diff_heuristic_config(var, value, cb) < 0)\n \t\treturn -1;\n \ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex 518bf9524..2ffd11a14 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'\n \t! test -s actual4\n '\n \n-test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '\n-\tgit config diff.ignoreSubmodules all &&\n-\tgit diff HEAD >actual &&\n-\t! test -s actual &&\n-\tgit config submodule.subname.ignore none &&\n-\tgit config submodule.subname.path sub &&\n-\tgit diff HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config submodule.subname.ignore all &&\n-\tgit diff HEAD >actual2 &&\n-\t! test -s actual2 &&\n-\tgit config submodule.subname.ignore untracked &&\n-\tgit diff HEAD >actual3 &&\n-\tsed -e \"1,/^@@/d\" actual3 >actual3.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual3.body &&\n-\tgit config submodule.subname.ignore dirty &&\n-\tgit diff HEAD >actual4 &&\n-\t! test -s actual4 &&\n-\tgit diff HEAD --ignore-submodules=none >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config --remove-section submodule.subname &&\n-\tgit config --unset diff.ignoreSubmodules\n-'\n-\n test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '\n \tgit config diff.ignoreSubmodules dirty &&\n \tgit diff HEAD >actual &&\n@@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'\n \t! test -s actual4\n '\n \n-test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '\n-\tgit config submodule.subname.ignore all &&\n-\tgit config submodule.subname.path sub &&\n-\tgit diff HEAD >actual2 &&\n-\t! test -s actual2 &&\n-\tgit config submodule.subname.ignore untracked &&\n-\tgit diff HEAD >actual3 &&\n-\t! test -s actual3 &&\n-\tgit config submodule.subname.ignore dirty &&\n-\tgit diff HEAD >actual4 &&\n-\t! test -s actual4 &&\n-\tgit diff --ignore-submodules=none HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subprev $subprev-dirty &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config --remove-section submodule.subname\n-'\n-\n test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '\n \tgit config --add -f .gitmodules submodule.subname.ignore all &&\n \tgit config --add -f .gitmodules submodule.subname.path sub &&\n@@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '\n \t! test -s actual\n '\n \n-test_expect_success 'git diff between submodule commits [.git/config]' '\n-\tgit diff HEAD^..HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subtip $subprev &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config submodule.subname.ignore dirty &&\n-\tgit config submodule.subname.path sub &&\n-\tgit diff HEAD^..HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subtip $subprev &&\n-\ttest_cmp expect.body actual.body &&\n-\tgit config submodule.subname.ignore all &&\n-\tgit diff HEAD^..HEAD >actual &&\n-\t! test -s actual &&\n-\tgit diff --ignore-submodules=dirty HEAD^..HEAD >actual &&\n-\tsed -e \"1,/^@@/d\" actual >actual.body &&\n-\texpect_from_to >expect.body $subtip $subprev &&\n-\tgit config --remove-section submodule.subname\n-'\n-\n test_expect_success 'git diff between submodule commits [.gitmodules]' '\n \tgit diff HEAD^..HEAD >actual &&\n \tsed -e \"1,/^@@/d\" actual >actual.body &&\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325564","messageId":"20170803182000.179328-12-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 11/15] submodule-config: remove support for overlaying repository config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:56Z","receivedAt":"2017-08-03T18:20:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"All callers have been migrated to explicitly read any configuration they\nneed. The support for handling it automatically in submodule-config is\nno longer needed.\n\nSigned-off-by: Brandon Williams \n---\n submodule-config.h | 1 -\n t/helper/test-submodule-config.c | 6 ----\n t/t7411-submodule-config.sh | 72 ----------------------------------------\n 3 files changed, 79 deletions(-)\n\ndiff --git a/submodule-config.h b/submodule-config.h\nindex cccd34b92..84c2cf515 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -34,7 +34,6 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-extern int parse_submodule_config_option(const char *var, const char *value);\n extern int submodule_config_option(struct repository *repo,\n \t\t\t\t const char *var, const char *value);\n extern const struct submodule *submodule_from_name(\ndiff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c\nindex e13fbcc1b..f4a7c431c 100644\n--- a/t/helper/test-submodule-config.c\n+++ b/t/helper/test-submodule-config.c\n@@ -10,11 +10,6 @@ static void die_usage(int argc, const char **argv, const char *msg)\n \texit(1);\n }\n \n-static int git_test_config(const char *var, const char *value, void *cb)\n-{\n-\treturn parse_submodule_config_option(var, value);\n-}\n-\n int cmd_main(int argc, const char **argv)\n {\n \tconst char **arg = argv;\n@@ -38,7 +33,6 @@ int cmd_main(int argc, const char **argv)\n \n \tsetup_git_directory();\n \tgitmodules_config();\n-\tgit_config(git_test_config, NULL);\n \n \twhile (*arg) {\n \t\tstruct object_id commit_oid;\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 7d6b25ba2..46c09c776 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -122,78 +122,6 @@ test_expect_success 'using different treeishs works' '\n \t)\n '\n \n-cat >super/expect_url <super/expect_local_path <actual &&\n-\t\ttest_cmp expect_url actual &&\n-\t\tgit config submodule.a.path c &&\n-\t\ttest-submodule-config \\\n-\t\t\t\"\" c \\\n-\t\t\t\"\" submodule \\\n-\t\t\t\t>actual &&\n-\t\ttest_cmp expect_local_path actual &&\n-\t\tgit config submodule.a.url \"$old_a\" &&\n-\t\tgit config submodule.submodule.url \"$old_submodule\" &&\n-\t\tgit config --unset submodule.a.path c\n-\t)\n-'\n-\n-cat >super/expect_url <actual &&\n-\t\ttest_cmp expect_url actual &&\n-\t\tgit config submodule.submodule.url \"$old_submodule\" &&\n-\t\tgit submodule init b\n-\t)\n-'\n-\n-cat >super/expect_fetchrecurse_die.err <actual.out 2>actual.err &&\n-\t\ttouch expect_fetchrecurse_die.out &&\n-\t\ttest_cmp expect_fetchrecurse_die.out actual.out &&\n-\t\ttest_cmp expect_fetchrecurse_die.err actual.err &&\n-\t\tgit config --unset submodule.submodule.fetchrecursesubmodules\n-\t)\n-'\n-\n test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n \t(cd super &&\n \t\tgit config -f .gitmodules \\\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325565","messageId":"20170803182000.179328-7-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 06/15] fetch: don't overlay config with submodule-config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:51Z","receivedAt":"2017-08-03T18:20:53Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directly for the\nfetch_recurse field.\n\nSigned-off-by: Brandon Williams \n---\n builtin/fetch.c | 1 -\n submodule.c | 24 +++++++++++++++++-------\n 2 files changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d84c26391..3fe99073d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tgitmodules_config();\n-\t\tgit_config(submodule_config, NULL);\n \t}\n \n \tif (all) {\ndiff --git a/submodule.c b/submodule.c\nindex 8a9b964ce..59e3d0828 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1194,14 +1194,24 @@ static int get_next_submodule(struct child_process *cp,\n \n \t\tdefault_argv = \"yes\";\n \t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-\t\t\tif (submodule &&\n-\t\t\t submodule->fetch_recurse !=\n-\t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n-\t\t\t\tif (submodule->fetch_recurse ==\n-\t\t\t\t\t\tRECURSE_SUBMODULES_OFF)\n+\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n+\n+\t\t\tif (submodule) {\n+\t\t\t\tchar *key;\n+\t\t\t\tconst char *value;\n+\n+\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n+\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n+\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n+\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n+\t\t\t\t}\n+\t\t\t\tfree(key);\n+\t\t\t}\n+\n+\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (submodule->fetch_recurse ==\n-\t\t\t\t\t\tRECURSE_SUBMODULES_ON_DEMAND) {\n+\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\tdefault_argv = \"on-demand\";\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325566","messageId":"20170803182000.179328-5-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 04/15] submodule--helper: don't overlay config in remote_submodule_branch","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:49Z","receivedAt":"2017-08-03T18:20:55Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Don't rely on overlaying the repository's config on top of the\nsubmodule-config, instead query the repository's config directly for the\nbranch field.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1e49ce580..f71f4270d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1066,17 +1066,24 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix\n static const char *remote_submodule_branch(const char *path)\n {\n \tconst struct submodule *sub;\n+\tconst char *branch = NULL;\n+\tchar *key;\n+\n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n \n \tsub = submodule_from_path(&null_oid, path);\n \tif (!sub)\n \t\treturn NULL;\n \n-\tif (!sub->branch)\n+\tkey = xstrfmt(\"submodule.%s.branch\", sub->name);\n+\tif (repo_config_get_string_const(the_repository, key, &branch))\n+\t\tbranch = sub->branch;\n+\tfree(key);\n+\n+\tif (!branch)\n \t\treturn \"master\";\n \n-\tif (!strcmp(sub->branch, \".\")) {\n+\tif (!strcmp(branch, \".\")) {\n \t\tunsigned char sha1[20];\n \t\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n \n@@ -1094,7 +1101,7 @@ static const char *remote_submodule_branch(const char *path)\n \t\treturn refname;\n \t}\n \n-\treturn sub->branch;\n+\treturn branch;\n }\n \n static int resolve_remote_submodule_branch(int argc, const char **argv,\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325568","messageId":"20170803182000.179328-1-bmwill@google.com","threadId":"46470","inReplyTo":"20170725213928.125998-1-bmwill@google.com","subject":"[PATCH v2 00/15] submodule-config cleanup","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:45Z","receivedAt":"2017-08-03T18:21:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Changes in v2:\n * Rebased on latest 'bw/grep-recurse-submodules' branch (Still also requires\n the 'bc/object-id' series).\n * Changed unpack-trees.c (checkout command) so that it no longer respects the\n 'submodule..update' config since it really didn't make much sense for\n it to respect it.\n * The above point also enabled me to fix some issues that coverity found with\n how I was overlaying the repo config with the submodule update strategy.\n Instead the update strategy parsing logic is separated into two functions so\n that just the enum can be determined from a string (which is all\n update-clone needed).\n\nBrandon Williams (15):\n t7411: check configuration parsing errors\n submodule: don't use submodule_from_name\n add, reset: ensure submodules can be added or reset\n submodule--helper: don't overlay config in remote_submodule_branch\n submodule--helper: don't overlay config in update-clone\n fetch: don't overlay config with submodule-config\n submodule: don't rely on overlayed config when setting diffopts\n unpack-trees: don't respect submodule.update\n submodule: remove submodule_config callback routine\n diff: stop allowing diff to have submodules configured in .git/config\n submodule-config: remove support for overlaying repository config\n submodule-config: move submodule-config functions to\n submodule-config.c\n submodule-config: lazy-load a repository's .gitmodules file\n unpack-trees: improve loading of .gitmodules\n submodule: remove gitmodules_config\n\n builtin/add.c | 1 +\n builtin/checkout.c | 3 +-\n builtin/commit.c | 1 -\n builtin/diff-files.c | 1 -\n builtin/diff-index.c | 1 -\n builtin/diff-tree.c | 1 -\n builtin/diff.c | 2 -\n builtin/fetch.c | 5 --\n builtin/grep.c | 4 --\n builtin/ls-files.c | 6 +-\n builtin/mv.c | 1 -\n builtin/read-tree.c | 2 -\n builtin/reset.c | 3 +-\n builtin/rm.c | 1 -\n builtin/submodule--helper.c | 51 ++++++++------\n diff.c | 3 -\n submodule-config.c | 65 +++++++++++++----\n submodule-config.h | 8 +--\n submodule.c | 148 ++++++++++++++-------------------------\n submodule.h | 6 +-\n t/helper/test-submodule-config.c | 7 --\n t/t4027-diff-submodule.sh | 67 ------------------\n t/t7400-submodule-basic.sh | 10 ---\n t/t7411-submodule-config.sh | 87 ++++-------------------\n unpack-trees.c | 81 +++++++++------------\n 25 files changed, 192 insertions(+), 373 deletions(-)\n\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325569","messageId":"20170803182000.179328-3-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 02/15] submodule: don't use submodule_from_name","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:47Z","receivedAt":"2017-08-03T18:21:51Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The function 'submodule_from_name()' is being used incorrectly here as a\nsubmodule path is being used instead of a submodule name. Since the\ncorrect function to use with a path to a submodule is already being used\n('submodule_from_path()') let's remove the call to\n'submodule_from_name()'.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5139b9256..19bd13bb2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \n \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\n-\t\tif (!submodule)\n-\t\t\tsubmodule = submodule_from_name(&null_oid, ce->name);\n \n \t\tdefault_argv = \"yes\";\n \t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-- \n2.14.0.rc1.383.gd1ce394fe2-goog\n\n"},{"id":"325570","messageId":"20170803182000.179328-2-bmwill@google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"[PATCH v2 01/15] t7411: check configuration parsing errors","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-03T18:19:46Z","receivedAt":"2017-08-03T18:22:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Check for configuration parsing errors in '.gitmodules' in t7411, which\nis explicitly testing the submodule-config subsystem, instead of in\nt7400. Also explicitly use the test helper instead of relying on the\ngitmodules file from being read in status.\n\nSigned-off-by: Brandon Williams \n---\n t/t7400-submodule-basic.sh | 10 ----------\n t/t7411-submodule-config.sh | 15 +++++++++++++++\n 2 files changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex dcac364c5..717447526 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -46,16 +46,6 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '\n \ttest_must_fail git submodule init\n '\n \n-test_expect_success 'configuration parsing' '\n-\ttest_when_finished \"rm -f .gitmodules\" &&\n-\tcat >.gitmodules <<-\\EOF &&\n-\t[submodule \"s\"]\n-\t\tpath\n-\t\tignore\n-\tEOF\n-\ttest_must_fail git status\n-'\n-\n test_expect_success 'setup - repository in init subdirectory' '\n \tmkdir init &&\n \t(\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex eea36f1db..7d6b25ba2 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -31,6 +31,21 @@ test_expect_success 'submodule config cache setup' '\n \t)\n '\n \n+test_expect_success 'configuration parsing with error' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\ttest_create_repo repo &&\n+\tcat >repo/.gitmodules <<-\\EOF &&\n+\t[submodule \"s\"]\n+\t\tpath\n+\t\tignore\n+\tEOF\n+\t(\n+\t\tcd repo &&\n+\t\ttest_must_fail test-submodule-config \"\" s 2>actual &&\n+\t\ttest_i18ngrep \"bad config\" actual\n+\t)\n+'\n+\n cat >super/expect < wrote:\n> The function 'submodule_from_name()' is being used incorrectly here as a\n> submodule path is being used instead of a submodule name. Since the\n> correct function to use with a path to a submodule is already being used\n> ('submodule_from_path()') let's remove the call to\n> 'submodule_from_name()'.\n>\n> Signed-off-by: Brandon Williams \n\nIn case a reroll is needed, you could incorperate Jens feedback\nstating that 851e18c385 should have done it.\n\n> ---\n> submodule.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 5139b9256..19bd13bb2 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n> continue;\n>\n> submodule = submodule_from_path(&null_oid, ce->name);\n> - if (!submodule)\n> - submodule = submodule_from_name(&null_oid, ce->name);\n>\n> default_argv = \"yes\";\n> if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> --\n> 2.14.0.rc1.383.gd1ce394fe2-goog\n>\n"},{"id":"325583","messageId":"xmqqy3r0wgzz.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"20170803182000.179328-1-bmwill@google.com","subject":"Re: [PATCH v2 00/15] submodule-config cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T20:09:20Z","receivedAt":"2017-08-03T20:09:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Changes in v2:\n> * Rebased on latest 'bw/grep-recurse-submodules' branch (Still also requires\n> the 'bc/object-id' series).\n> * Changed unpack-trees.c (checkout command) so that it no longer respects the\n> 'submodule..update' config since it really didn't make much sense for\n> it to respect it.\n> * The above point also enabled me to fix some issues that coverity found with\n> how I was overlaying the repo config with the submodule update strategy.\n> Instead the update strategy parsing logic is separated into two functions so\n> that just the enum can be determined from a string (which is all\n> update-clone needed).\n\nThanks. I was wondering what the status of this series was when I\naccepted the updated \"grep --recurse-submodules\" the other day.\n"},{"id":"325584","messageId":"xmqqtw1owgn7.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"20170803182000.179328-3-bmwill@google.com","subject":"Re: [PATCH v2 02/15] submodule: don't use submodule_from_name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T20:17:00Z","receivedAt":"2017-08-03T20:17:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> The function 'submodule_from_name()' is being used incorrectly here as a\n> submodule path is being used instead of a submodule name. Since the\n> correct function to use with a path to a submodule is already being used\n> ('submodule_from_path()') let's remove the call to\n> 'submodule_from_name()'.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> submodule.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 5139b9256..19bd13bb2 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\tcontinue;\n> \n> \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\n> -\t\tif (!submodule)\n> -\t\t\tsubmodule = submodule_from_name(&null_oid, ce->name);\n> \n> \t\tdefault_argv = \"yes\";\n> \t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n\nIt appears to me that the scope of the variable \"submodule\" in this\nfunction can be narrowed to be limited to the block inside this \"if\"\nstatement we see in the post-context of this hunk. That would make\nit even easier to see why leaving submodule to NULL is a safe thing\nto do.\n\nThis comment applies to the state of this function before or after\nthis patch. It can be left outside the scope of this immediate\nseries, and instead be done as a follow-up (or preparatory) cleanup.\n\nThanks.\n"},{"id":"325588","messageId":"CAGZ79kZOLkJEJE-Rtid5LfmwgQ_AVnC0Mm-GwQJFOL+1SWB-nw@mail.gmail.com","threadId":"46470","inReplyTo":"20170803182000.179328-9-bmwill@google.com","subject":"Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-03T20:26:03Z","receivedAt":"2017-08-03T20:26:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams wrote:\n> The 'submodule.update' config was historically used and respected by the\n> 'submodule update' command because update handled a variety of different\n> ways it updated a submodule. As we begin teaching other commands about\n> submodules it makes more sense for the different settings of\n> 'submodule.update' to be handled by the individual commands themselves\n> (checkout, rebase, merge, etc) so it shouldn't be respected by the\n> native checkout command.\n>\n> Also remove the overlaying of the repository's config (via using\n> 'submodule_config()') from the commands which use the unpack-trees\n> logic (checkout, read-tree, reset).\n\nThat was a mistake that I introduced with the checkout series.\n\nThanks for fixing it.\n"},{"id":"325590","messageId":"xmqqpoccwfpl.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"20170803182000.179328-9-bmwill@google.com","subject":"Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T20:37:10Z","receivedAt":"2017-08-03T20:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> The 'submodule.update' config was historically used and respected by the\n> 'submodule update' command because update handled a variety of different\n> ways it updated a submodule. As we begin teaching other commands about\n> submodules it makes more sense for the different settings of\n> 'submodule.update' to be handled by the individual commands themselves\n> (checkout, rebase, merge, etc) so it shouldn't be respected by the\n> native checkout command.\n\nSoooo... what's the externally observable effect of this change? Is\nit something that can be illustrated in a set of new tests?\n\nIOW does this commit by itself want to change the behaviour of\n\"submodule update\" and existing (indirect) users of unpack-trees?\nOr does it want to keep the documented behaviour of \"submodule\nupdate\" while correcting unintended triggering in other (indirect)\nusers of unpack-trees of the same machinery that is being removed in\nthis patch?\n\n> -\tswitch (sub->update_strategy.type) {\n> -\tcase SM_UPDATE_UNSPECIFIED:\n> -\tcase SM_UPDATE_CHECKOUT:\n> -\t\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n> -\t\t\treturn o->gently ? -1 :\n> -\t\t\t\tadd_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n> -\t\treturn 0;\n> -\tcase SM_UPDATE_NONE:\n> -\t\treturn 0;\n> -\tcase SM_UPDATE_REBASE:\n> -\tcase SM_UPDATE_MERGE:\n> -\tcase SM_UPDATE_COMMAND:\n> -\tdefault:\n> -\t\twarning(_(\"submodule update strategy not supported for submodule '%s'\"), ce->name);\n> -\t\treturn -1;\n> -\t}\n> +\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n> +\t\treturn o->gently ? -1 :\n> +\t\t\t\t add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n> +\treturn 0;\n\nWith this update, we always behave as if update_strategy.type were\neither left unspecified or explicitly set to checkout. Other arms\nin this switch (and the other switch too), especially \"none\", were\nnot expecting a call to submodule_move_head() to be made, but now\nthe call is unconditional.\n\n\n\n> }\n> \n> static void reload_gitmodules_file(struct index_state *index,\n> @@ -293,7 +282,6 @@ static void reload_gitmodules_file(struct index_state *index,\n> \t\t\t\tsubmodule_free();\n> \t\t\t\tcheckout_entry(ce, state, NULL);\n> \t\t\t\tgitmodules_config();\n> -\t\t\t\tgit_config(submodule_config, NULL);\n> \t\t\t} else\n> \t\t\t\tbreak;\n> \t\t}\n> @@ -308,19 +296,9 @@ static void unlink_entry(const struct cache_entry *ce)\n> {\n> \tconst struct submodule *sub = submodule_from_ce(ce);\n> \tif (sub) {\n> -\t\tswitch (sub->update_strategy.type) {\n> -\t\tcase SM_UPDATE_UNSPECIFIED:\n> -\t\tcase SM_UPDATE_CHECKOUT:\n> -\t\tcase SM_UPDATE_REBASE:\n> -\t\tcase SM_UPDATE_MERGE:\n> -\t\t\t/* state.force is set at the caller. */\n> -\t\t\tsubmodule_move_head(ce->name, \"HEAD\", NULL,\n> -\t\t\t\t\t SUBMODULE_MOVE_HEAD_FORCE);\n> -\t\t\tbreak;\n> -\t\tcase SM_UPDATE_NONE:\n> -\t\tcase SM_UPDATE_COMMAND:\n> -\t\t\treturn; /* Do not touch the submodule. */\n> -\t\t}\n> +\t\t/* state.force is set at the caller. */\n> +\t\tsubmodule_move_head(ce->name, \"HEAD\", NULL,\n> +\t\t\t\t SUBMODULE_MOVE_HEAD_FORCE);\n> \t}\n> \tif (!check_leading_path(ce->name, ce_namelen(ce)))\n> \t\treturn;\n"},{"id":"325591","messageId":"xmqqlgn0wfjt.fsf@gitster.mtv.corp.google.com","threadId":"46470","inReplyTo":"20170803182000.179328-11-bmwill@google.com","subject":"Re: [PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T20:40:38Z","receivedAt":"2017-08-03T20:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Traditionally a submodule is comprised of a gitlink as well as a\n> corresponding entry in the .gitmodules file. Diff doesn't follow this\n> paradigm as its config callback routine falls back to populating the\n> submodule-config if a config entry starts with 'submodule.'.\n>\n> Remove this behavior in order to be consistent with how the\n> submodule-config is populated, via calling 'gitmodules_config()' or\n> 'repo_read_gitmodules()'.\n\nI am all for dropping special cases deep in the diff machinery, even\nthough there may be submodule users who care about submodule.*.ignore\n\nDoes this change mean we can eventually get rid of the ugly\nDIFF_OPT_OVERRIDE_SUBMODULE_CONFIG hack and also need for a patch\nlike 03/15?\n\n>\n> Signed-off-by: Brandon Williams \n> ---\n> diff.c | 3 ---\n> t/t4027-diff-submodule.sh | 67 -----------------------------------------------\n> 2 files changed, 70 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 85e714f6c..e43519b88 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> -\tif (starts_with(var, \"submodule.\"))\n> -\t\treturn parse_submodule_config_option(var, value);\n> -\n> \tif (git_diff_heuristic_config(var, value, cb) < 0)\n> \t\treturn -1;\n> \n> diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\n> index 518bf9524..2ffd11a14 100755\n> --- a/t/t4027-diff-submodule.sh\n> +++ b/t/t4027-diff-submodule.sh\n> @@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'\n> \t! test -s actual4\n> '\n> \n> -test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '\n> -\tgit config diff.ignoreSubmodules all &&\n> -\tgit diff HEAD >actual &&\n> -\t! test -s actual &&\n> -\tgit config submodule.subname.ignore none &&\n> -\tgit config submodule.subname.path sub &&\n> -\tgit diff HEAD >actual &&\n> -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> -\ttest_cmp expect.body actual.body &&\n> -\tgit config submodule.subname.ignore all &&\n> -\tgit diff HEAD >actual2 &&\n> -\t! test -s actual2 &&\n> -\tgit config submodule.subname.ignore untracked &&\n> -\tgit diff HEAD >actual3 &&\n> -\tsed -e \"1,/^@@/d\" actual3 >actual3.body &&\n> -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> -\ttest_cmp expect.body actual3.body &&\n> -\tgit config submodule.subname.ignore dirty &&\n> -\tgit diff HEAD >actual4 &&\n> -\t! test -s actual4 &&\n> -\tgit diff HEAD --ignore-submodules=none >actual &&\n> -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> -\ttest_cmp expect.body actual.body &&\n> -\tgit config --remove-section submodule.subname &&\n> -\tgit config --unset diff.ignoreSubmodules\n> -'\n> -\n> test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '\n> \tgit config diff.ignoreSubmodules dirty &&\n> \tgit diff HEAD >actual &&\n> @@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'\n> \t! test -s actual4\n> '\n> \n> -test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '\n> -\tgit config submodule.subname.ignore all &&\n> -\tgit config submodule.subname.path sub &&\n> -\tgit diff HEAD >actual2 &&\n> -\t! test -s actual2 &&\n> -\tgit config submodule.subname.ignore untracked &&\n> -\tgit diff HEAD >actual3 &&\n> -\t! test -s actual3 &&\n> -\tgit config submodule.subname.ignore dirty &&\n> -\tgit diff HEAD >actual4 &&\n> -\t! test -s actual4 &&\n> -\tgit diff --ignore-submodules=none HEAD >actual &&\n> -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> -\ttest_cmp expect.body actual.body &&\n> -\tgit config --remove-section submodule.subname\n> -'\n> -\n> test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '\n> \tgit config --add -f .gitmodules submodule.subname.ignore all &&\n> \tgit config --add -f .gitmodules submodule.subname.path sub &&\n> @@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '\n> \t! test -s actual\n> '\n> \n> -test_expect_success 'git diff between submodule commits [.git/config]' '\n> -\tgit diff HEAD^..HEAD >actual &&\n> -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> -\texpect_from_to >expect.body $subtip $subprev &&\n> -\ttest_cmp expect.body actual.body &&\n> -\tgit config submodule.subname.ignore dirty &&\n> -\tgit config submodule.subname.path sub &&\n> -\tgit diff HEAD^..HEAD >actual &&\n> -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> -\texpect_from_to >expect.body $subtip $subprev &&\n> -\ttest_cmp expect.body actual.body &&\n> -\tgit config submodule.subname.ignore all &&\n> -\tgit diff HEAD^..HEAD >actual &&\n> -\t! test -s actual &&\n> -\tgit diff --ignore-submodules=dirty HEAD^..HEAD >actual &&\n> -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> -\texpect_from_to >expect.body $subtip $subprev &&\n> -\tgit config --remove-section submodule.subname\n> -'\n> -\n> test_expect_success 'git diff between submodule commits [.gitmodules]' '\n> \tgit diff HEAD^..HEAD >actual &&\n> \tsed -e \"1,/^@@/d\" actual >actual.body &&\n"},{"id":"325592","messageId":"CAGZ79ka8YeW0ChP3Z3xjfV0r0aqg2sGpLDU5m5LGWyG9QED0Uw@mail.gmail.com","threadId":"46470","inReplyTo":"xmqqpoccwfpl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-03T20:43:50Z","receivedAt":"2017-08-03T20:43:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 3, 2017 at 1:37 PM, Junio C Hamano wrote:\n> Brandon Williams writes:\n>\n>> The 'submodule.update' config was historically used and respected by the\n>> 'submodule update' command because update handled a variety of different\n>> ways it updated a submodule. As we begin teaching other commands about\n>> submodules it makes more sense for the different settings of\n>> 'submodule.update' to be handled by the individual commands themselves\n>> (checkout, rebase, merge, etc) so it shouldn't be respected by the\n>> native checkout command.\n>\n> Soooo... what's the externally observable effect of this change? Is\n> it something that can be illustrated in a set of new tests?\n\nThe illustration can be as follows\n\n git config submodule.NAME.update none\n git checkout -f --recurse-submodules HEAD\n git status\n # observe dirty submodule, which is\n # not what checkout -f promises\n\n> IOW does this commit by itself want to change the behaviour of\n> \"submodule update\" and existing (indirect) users of unpack-trees?\n> Or does it want to keep the documented behaviour of \"submodule\n> update\" while correcting unintended triggering in other (indirect)\n> users of unpack-trees of the same machinery that is being removed in\n> this patch?\n\n\"submodule update\" is unaffected, only the recently introduced submodule\nawareness of checkout/reset/read-tree are changed.\n\nThis option is documented as\n submodule..update\n The default update procedure for a submodule. This variable is\n populated by git submodule init from the gitmodules(5) file. See\n description of update command in git-submodule(1).\n\nwhich doesn't indicate that any other command apart from\n\"submodule update\" should respect it.\n\n>\n>> - switch (sub->update_strategy.type) {\n>> - case SM_UPDATE_UNSPECIFIED:\n>> - case SM_UPDATE_CHECKOUT:\n>> - if (submodule_move_head(ce->name, old_id, new_id, flags))\n>> - return o->gently ? -1 :\n>> - add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n>> - return 0;\n>> - case SM_UPDATE_NONE:\n>> - return 0;\n>> - case SM_UPDATE_REBASE:\n>> - case SM_UPDATE_MERGE:\n>> - case SM_UPDATE_COMMAND:\n>> - default:\n>> - warning(_(\"submodule update strategy not supported for submodule '%s'\"), ce->name);\n>> - return -1;\n>> - }\n>> + if (submodule_move_head(ce->name, old_id, new_id, flags))\n>> + return o->gently ? -1 :\n>> + add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n>> + return 0;\n>\n> With this update, we always behave as if update_strategy.type were\n> either left unspecified or explicitly set to checkout. Other arms\n> in this switch (and the other switch too), especially \"none\", were\n> not expecting a call to submodule_move_head() to be made, but now\n> the call is unconditional.\n>\n\nYes. This is because each command (reset/checkout) should provide\none expected behavior. It is not that we can configure reset to omit certain\n(tracked) files from being reset?\n"},{"id":"325631","messageId":"20170804215311.GB126093@google.com","threadId":"46470","inReplyTo":"CAGZ79kaZcpZ-6+=19CbW1v+h-njguXZH9z9GMYA3Ci=acfreKQ@mail.gmail.com","subject":"Re: [PATCH v2 02/15] submodule: don't use submodule_from_name","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-04T21:53:11Z","receivedAt":"2017-08-04T21:53:18Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/03, Stefan Beller wrote:\n> On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams wrote:\n> > The function 'submodule_from_name()' is being used incorrectly here as a\n> > submodule path is being used instead of a submodule name. Since the\n> > correct function to use with a path to a submodule is already being used\n> > ('submodule_from_path()') let's remove the call to\n> > 'submodule_from_name()'.\n> >\n> > Signed-off-by: Brandon Williams \n> \n> In case a reroll is needed, you could incorperate Jens feedback\n> stating that 851e18c385 should have done it.\n\nK I'll add that into the commit message.\n\n> \n> > ---\n> > submodule.c | 2 --\n> > 1 file changed, 2 deletions(-)\n> >\n> > diff --git a/submodule.c b/submodule.c\n> > index 5139b9256..19bd13bb2 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,\n> > continue;\n> >\n> > submodule = submodule_from_path(&null_oid, ce->name);\n> > - if (!submodule)\n> > - submodule = submodule_from_name(&null_oid, ce->name);\n> >\n> > default_argv = \"yes\";\n> > if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> > --\n> > 2.14.0.rc1.383.gd1ce394fe2-goog\n> >\n\n-- \nBrandon Williams\n"},{"id":"325632","messageId":"20170804215956.GC126093@google.com","threadId":"46470","inReplyTo":"xmqqlgn0wfjt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-04T21:59:56Z","receivedAt":"2017-08-04T22:00:05Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/03, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > Traditionally a submodule is comprised of a gitlink as well as a\n> > corresponding entry in the .gitmodules file. Diff doesn't follow this\n> > paradigm as its config callback routine falls back to populating the\n> > submodule-config if a config entry starts with 'submodule.'.\n> >\n> > Remove this behavior in order to be consistent with how the\n> > submodule-config is populated, via calling 'gitmodules_config()' or\n> > 'repo_read_gitmodules()'.\n> \n> I am all for dropping special cases deep in the diff machinery, even\n> though there may be submodule users who care about submodule.*.ignore\n> \n> Does this change mean we can eventually get rid of the ugly\n> DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG hack and also need for a patch\n> like 03/15?\n\nI think that this is a step toward getting rid of that. We can either\ndo two things: 1) deprecate submodule.*.ignore and don't respect it\nanymore or 2) flip the polarity of that flag so that by default we\ndon't respect the submodule.*.ignore config and instead callers must opt\nin instead of the current opt out behavior.\n\n> \n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > diff.c | 3 ---\n> > t/t4027-diff-submodule.sh | 67 -----------------------------------------------\n> > 2 files changed, 70 deletions(-)\n> >\n> > diff --git a/diff.c b/diff.c\n> > index 85e714f6c..e43519b88 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> > \t\treturn 0;\n> > \t}\n> > \n> > -\tif (starts_with(var, \"submodule.\"))\n> > -\t\treturn parse_submodule_config_option(var, value);\n> > -\n> > \tif (git_diff_heuristic_config(var, value, cb) < 0)\n> > \t\treturn -1;\n> > \n> > diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\n> > index 518bf9524..2ffd11a14 100755\n> > --- a/t/t4027-diff-submodule.sh\n> > +++ b/t/t4027-diff-submodule.sh\n> > @@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'\n> > \t! test -s actual4\n> > '\n> > \n> > -test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '\n> > -\tgit config diff.ignoreSubmodules all &&\n> > -\tgit diff HEAD >actual &&\n> > -\t! test -s actual &&\n> > -\tgit config submodule.subname.ignore none &&\n> > -\tgit config submodule.subname.path sub &&\n> > -\tgit diff HEAD >actual &&\n> > -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> > -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> > -\ttest_cmp expect.body actual.body &&\n> > -\tgit config submodule.subname.ignore all &&\n> > -\tgit diff HEAD >actual2 &&\n> > -\t! test -s actual2 &&\n> > -\tgit config submodule.subname.ignore untracked &&\n> > -\tgit diff HEAD >actual3 &&\n> > -\tsed -e \"1,/^@@/d\" actual3 >actual3.body &&\n> > -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> > -\ttest_cmp expect.body actual3.body &&\n> > -\tgit config submodule.subname.ignore dirty &&\n> > -\tgit diff HEAD >actual4 &&\n> > -\t! test -s actual4 &&\n> > -\tgit diff HEAD --ignore-submodules=none >actual &&\n> > -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> > -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> > -\ttest_cmp expect.body actual.body &&\n> > -\tgit config --remove-section submodule.subname &&\n> > -\tgit config --unset diff.ignoreSubmodules\n> > -'\n> > -\n> > test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '\n> > \tgit config diff.ignoreSubmodules dirty &&\n> > \tgit diff HEAD >actual &&\n> > @@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'\n> > \t! test -s actual4\n> > '\n> > \n> > -test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '\n> > -\tgit config submodule.subname.ignore all &&\n> > -\tgit config submodule.subname.path sub &&\n> > -\tgit diff HEAD >actual2 &&\n> > -\t! test -s actual2 &&\n> > -\tgit config submodule.subname.ignore untracked &&\n> > -\tgit diff HEAD >actual3 &&\n> > -\t! test -s actual3 &&\n> > -\tgit config submodule.subname.ignore dirty &&\n> > -\tgit diff HEAD >actual4 &&\n> > -\t! test -s actual4 &&\n> > -\tgit diff --ignore-submodules=none HEAD >actual &&\n> > -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> > -\texpect_from_to >expect.body $subprev $subprev-dirty &&\n> > -\ttest_cmp expect.body actual.body &&\n> > -\tgit config --remove-section submodule.subname\n> > -'\n> > -\n> > test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '\n> > \tgit config --add -f .gitmodules submodule.subname.ignore all &&\n> > \tgit config --add -f .gitmodules submodule.subname.path sub &&\n> > @@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '\n> > \t! test -s actual\n> > '\n> > \n> > -test_expect_success 'git diff between submodule commits [.git/config]' '\n> > -\tgit diff HEAD^..HEAD >actual &&\n> > -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> > -\texpect_from_to >expect.body $subtip $subprev &&\n> > -\ttest_cmp expect.body actual.body &&\n> > -\tgit config submodule.subname.ignore dirty &&\n> > -\tgit config submodule.subname.path sub &&\n> > -\tgit diff HEAD^..HEAD >actual &&\n> > -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> > -\texpect_from_to >expect.body $subtip $subprev &&\n> > -\ttest_cmp expect.body actual.body &&\n> > -\tgit config submodule.subname.ignore all &&\n> > -\tgit diff HEAD^..HEAD >actual &&\n> > -\t! test -s actual &&\n> > -\tgit diff --ignore-submodules=dirty HEAD^..HEAD >actual &&\n> > -\tsed -e \"1,/^@@/d\" actual >actual.body &&\n> > -\texpect_from_to >expect.body $subtip $subprev &&\n> > -\tgit config --remove-section submodule.subname\n> > -'\n> > -\n> > test_expect_success 'git diff between submodule commits [.gitmodules]' '\n> > \tgit diff HEAD^..HEAD >actual &&\n> > \tsed -e \"1,/^@@/d\" actual >actual.body &&\n\n-- \nBrandon Williams\n"},{"id":"326147","messageId":"20170811165354.GA1472@book.hvoigt.net","threadId":"46470","inReplyTo":"CAGZ79kZxprtLGOzURHaxc5YzviSj_2Kx23v=gjr2uFb+tbNfjw@mail.gmail.com","subject":"Re: [PATCH 02/15] submodule: don't use submodule_from_name","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-11T16:53:54Z","receivedAt":"2017-08-11T17:14:03Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nsorry for the late reply, just stumpled upon this.\n\nOn Mon, Jul 31, 2017 at 01:43:04PM -0700, Stefan Beller wrote:\n> On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann wrote:\n> > Am 26.07.2017 um 23:06 schrieb Junio C Hamano:\n> >>\n> >> Stefan Beller writes:\n> >>\n> >>> Rereading the archives, there was quite some discussion on the design\n> >>> of these patches, but these lines of code did not get any attention\n> >>>\n> >>> https://public-inbox.org/git/4CDB3063.5010801@web.de/\n> >>>\n> >>> I cc'd Jens in the hope of him having a good memory why he\n> >>> wrote the code that way. :)\n> >>\n> >>\n> >> Thanks for digging. I wouldn't be surprised if this were a fallback\n> >> to help a broken entry in .gitmodules that lack .path variable, but\n> >> we shouldn't be sweeping the problem under the rug like that.\n> >\n> >\n> > Sorry to disappoint you ;-) I added this in 7dce19d374 because\n> > submodule by path lookup back then only parsed the checked out\n> > .gitmodules file.\n> \n> This is still the case AFAICT, as we never ask for a specific .gitmodules\n> file identified by sha1 of the commit.\n\nThis was actually part of my original approach[1] but it seems I never got\naround to implement that last part for which I originally started the\nsubmodule config API: Proper recursive fetch.\n\nI still have a patch for moved submodules lying around which pass a commit id\nfor a gitmodules file. That particular patch is quite simple and finished but\nI was planning to include that in the finished fetch series. So I can have a\nlook if I can quickly update that to the current state, so we can at least have\none proper user of the submodule config API.\n\n> > So looking for it by name was a good guess to\n> > fetch a new submodule that wasn't present in the current HEAD's\n> > .gitmodules, as the path is used as the default name in \"git\n> > submodule add\".\n\nI will have a look whether we can easily replace this hack with the proper\nlookup now. Lets see how many low hanging fruits we have lying around\nfor recursive fetch. The full blown implementation including cloning of\nnew submodules might still take some time...\n\nCheers Heiko\n\n[1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net/\n"},{"id":"326149","messageId":"20170811171811.GC1472@book.hvoigt.net","threadId":"46470","inReplyTo":"20170803182000.179328-15-bmwill@google.com","subject":"Re: [PATCH v2 14/15] unpack-trees: improve loading of .gitmodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-11T17:18:11Z","receivedAt":"2017-08-11T17:18:19Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Aug 03, 2017 at 11:19:59AM -0700, Brandon Williams wrote:\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 5dce7ff7d..3c7f464fa 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1,5 +1,6 @@\n> #define NO_THE_INDEX_COMPATIBILITY_MACROS\n> #include \"cache.h\"\n> +#include \"repository.h\"\n> #include \"config.h\"\n> #include \"dir.h\"\n> #include \"tree.h\"\n> @@ -268,22 +269,28 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n> \treturn 0;\n> }\n> \n> -static void reload_gitmodules_file(struct index_state *index,\n> -\t\t\t\t struct checkout *state)\n> +/*\n> + * Preform the loading of the repository's gitmodules file. This function is\n\ns/Preform/Perform/\n\nand a nit: There is some extra space after the end of this sentence.\n\nCheers Heiko\n"},{"id":"326150","messageId":"20170811165940.GB1472@book.hvoigt.net","threadId":"46470","inReplyTo":"20170804215311.GB126093@google.com","subject":"Re: [PATCH v2 02/15] submodule: don't use submodule_from_name","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-11T16:59:40Z","receivedAt":"2017-08-11T17:24:37Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Aug 04, 2017 at 02:53:11PM -0700, Brandon Williams wrote:\n> On 08/03, Stefan Beller wrote:\n> > On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams wrote:\n> > > The function 'submodule_from_name()' is being used incorrectly here as a\n> > > submodule path is being used instead of a submodule name. Since the\n> > > correct function to use with a path to a submodule is already being used\n> > > ('submodule_from_path()') let's remove the call to\n> > > 'submodule_from_name()'.\n> > >\n> > > Signed-off-by: Brandon Williams \n> > \n> > In case a reroll is needed, you could incorperate Jens feedback\n> > stating that 851e18c385 should have done it.\n> \n> K I'll add that into the commit message.\n\nWell, thats not 100% correct... IMO, it should have been a follow up patch\nwhich I never got to implement. See my other reply to the v1 of this\npatch I just sent out.\n\nAs stated there I will have a look into where it makes sense to pass a\ncommit id and behave more correctly.\n\nCheers Heiko\n"}]}