{"thread":{"id":"49359","subject":"[PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","startedAt":"2018-09-17T14:09:50Z","lastAt":"2018-10-01T19:43:00Z","messageCount":23,"participants":["Antonio Ospite","SZEDER Gábor","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":5,"patchTotal":9},"messages":[{"id":"358260","messageId":"20180917140940.3839-10-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:40Z","receivedAt":"2018-09-17T14:09:50Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"When the .gitmodules file is not available in the working tree, try\nusing the content from the index and from the current branch. This\ncovers the case when the file is part of the repository but for some\nreason it is not checked out, for example because of a sparse checkout.\n\nThis makes it possible to use at least the 'git submodule' commands\nwhich *read* the gitmodules configuration file without fully populating\nthe working tree.\n\nWriting to .gitmodules will still require that the file is checked out,\nso check for that before calling config_set_in_gitmodules_file_gently.\n\nAdd a similar check also in git-submodule.sh::cmd_add() to anticipate\nthe eventual failure of the \"git submodule add\" command when .gitmodules\nis not safely writeable; this prevents the command from leaving the\nrepository in a spurious state (e.g. the submodule repository was cloned\nbut .gitmodules was not updated because\nconfig_set_in_gitmodules_file_gently failed).\n\nFinally, add t7416-submodule-sparse-gitmodules.sh to verify that reading\nfrom .gitmodules succeeds and that writing to it fails when the file is\nnot checked out.\n\nSigned-off-by: Antonio Ospite \n---\n builtin/submodule--helper.c | 6 +-\n git-submodule.sh | 5 ++\n submodule-config.c | 18 +++++-\n t/t7411-submodule-config.sh | 26 ++++++++-\n t/t7416-submodule-sparse-gitmodules.sh | 78 ++++++++++++++++++++++++++\n 5 files changed, 129 insertions(+), 4 deletions(-)\n create mode 100755 t/t7416-submodule-sparse-gitmodules.sh\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex bd14f57d00..f72f6ee58a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2032,8 +2032,12 @@ static int module_config(int argc, const char **argv, const char *prefix)\n \t\treturn print_config_from_gitmodules(argv[1]);\n \n \t/* Equivalent to ACTION_SET in builtin/config.c */\n-\tif (argc == 3)\n+\tif (argc == 3) {\n+\t\tif (!is_writing_gitmodules_ok())\n+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\n \t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n+\t}\n \n \tusage_with_options(git_submodule_helper_usage, module_config_options);\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 25b9bc58cb..bff855f54a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -159,6 +159,11 @@ cmd_add()\n \t\tshift\n \tdone\n \n+\tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n+\tthen\n+\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n+\tfi\n+\n \tif test -n \"$reference_path\"\n \tthen\n \t\tis_absolute_path \"$reference_path\" ||\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 61a555e920..bdb1d0e2c9 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"dir.h\"\n #include \"repository.h\"\n #include \"config.h\"\n #include \"submodule-config.h\"\n@@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct repository *repo)\n static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n {\n \tif (repo->worktree) {\n-\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n-\t\tgit_config_from_file(fn, file, data);\n+\t\tstruct git_config_source config_source = { 0 };\n+\t\tconst struct config_options opts = { 0 };\n+\t\tstruct object_id oid;\n+\t\tchar *file;\n+\n+\t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n+\t\tif (file_exists(file))\n+\t\t\tconfig_source.file = file;\n+\t\telse if (get_oid(GITMODULES_INDEX, &oid) >= 0)\n+\t\t\tconfig_source.blob = GITMODULES_INDEX;\n+\t\telse if (get_oid(GITMODULES_HEAD, &oid) >= 0)\n+\t\t\tconfig_source.blob = GITMODULES_HEAD;\n+\n+\t\tconfig_with_options(fn, data, &config_source, &opts);\n+\n \t\tfree(file);\n \t}\n }\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 45953f9300..2cfabb18bc 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -134,7 +134,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n \t)\n '\n \n-test_expect_success 'reading submodules config with \"submodule--helper config\"' '\n+test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n \t(cd super &&\n \t\techo \"../submodule\" >expect &&\n \t\tgit submodule--helper config submodule.submodule.url >actual &&\n@@ -192,4 +192,28 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch\n \t)\n '\n \n+test_expect_success 'reading submodules config from the index when .gitmodules is not in the working tree' '\n+\tORIG=$(git -C super rev-parse HEAD) &&\n+\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n+\t(cd super &&\n+\t\tgit submodule--helper config submodule.submodule.url \"staged_url\" &&\n+\t\tgit add .gitmodules &&\n+\t\trm -f .gitmodules &&\n+\t\techo \"staged_url\" >expect &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reading submodules config from the current branch when .gitmodules is not in the index' '\n+\tORIG=$(git -C super rev-parse HEAD) &&\n+\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n+\t(cd super &&\n+\t\tgit rm .gitmodules &&\n+\t\techo \"../submodule\" >expect &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh\nnew file mode 100755\nindex 0000000000..908a4e6958\n--- /dev/null\n+++ b/t/t7416-submodule-sparse-gitmodules.sh\n@@ -0,0 +1,78 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2018 Antonio Ospite \n+#\n+\n+test_description='Test reading/writing .gitmodules when not in the working tree\n+\n+This test verifies that, when .gitmodules is in the current branch but is not\n+in the working tree reading from it still works but writing to it does not.\n+\n+The test setup uses a sparse checkout, however the same scenario can be set up\n+also by committing .gitmodules and then just removing it from the filesystem.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'sparse checkout setup which hides .gitmodules' '\n+\techo file >file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m upstream &&\n+\tgit clone . super &&\n+\tgit clone super submodule &&\n+\tgit clone super new_submodule &&\n+\t(cd super &&\n+\t\tgit submodule add ../submodule &&\n+\t\ttest_tick &&\n+\t\tgit commit -m submodule &&\n+\t\tcat >.git/info/sparse-checkout <<-\\EOF &&\n+\t\t/*\n+\t\t!/.gitmodules\n+\t\tEOF\n+\t\tgit config core.sparsecheckout true &&\n+\t\tgit read-tree -m -u HEAD &&\n+\t\ttest_path_is_missing .gitmodules\n+\t)\n+'\n+\n+test_expect_success 'reading gitmodules config file when it is not checked out' '\n+\t(cd super &&\n+\t\techo \"../submodule\" >expect &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'not writing gitmodules config file when it is not checked out' '\n+\t test_must_fail git -C super submodule--helper config submodule.submodule.url newurl\n+'\n+\n+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '\n+\tgit -C super submodule init\n+'\n+\n+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '\n+\tgit -C super submodule summary\n+'\n+\n+test_expect_success 'updating submodule when the gitmodules config is not checked out' '\n+\t(cd submodule &&\n+\t\techo file2 >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m \"add file2 to submodule\"\n+\t) &&\n+\tgit -C super submodule update\n+'\n+\n+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '\n+\ttest_must_fail git -C super submodule add ../new_submodule\n+'\n+\n+# This test checks that the previous \"git submodule add\" did not leave the\n+# repository in a spurious state when it failed.\n+test_expect_success 'init submodule still works even after the previous add failed' '\n+\tgit -C super submodule init\n+'\n+\n+test_done\n-- \n2.19.0\n\n"},{"id":"358261","messageId":"20180917140940.3839-7-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 6/9] submodule: use the 'submodule--helper config' command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:37Z","receivedAt":"2018-09-17T14:09:50Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Use the 'submodule--helper config' command in git-submodules.sh to avoid\nreferring explicitly to .gitmodules by the hardcoded file path.\n\nThis makes it possible to access the submodules configuration in a more\ncontrolled way.\n\nSigned-off-by: Antonio Ospite \n---\n git-submodule.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1cb2c0a31b..25b9bc58cb 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -72,7 +72,7 @@ get_submodule_config () {\n \tvalue=$(git config submodule.\"$name\".\"$option\")\n \tif test -z \"$value\"\n \tthen\n-\t\tvalue=$(git config -f .gitmodules submodule.\"$name\".\"$option\")\n+\t\tvalue=$(git submodule--helper config submodule.\"$name\".\"$option\")\n \tfi\n \tprintf '%s' \"${value:-$default}\"\n }\n@@ -283,11 +283,11 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n \tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n \n-\tgit config -f .gitmodules submodule.\"$sm_name\".path \"$sm_path\" &&\n-\tgit config -f .gitmodules submodule.\"$sm_name\".url \"$repo\" &&\n+\tgit submodule--helper config submodule.\"$sm_name\".path \"$sm_path\" &&\n+\tgit submodule--helper config submodule.\"$sm_name\".url \"$repo\" &&\n \tif test -n \"$branch\"\n \tthen\n-\t\tgit config -f .gitmodules submodule.\"$sm_name\".branch \"$branch\"\n+\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n \tfi &&\n \tgit add --force .gitmodules ||\n \tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n-- \n2.19.0\n\n"},{"id":"358262","messageId":"20180917140940.3839-4-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 3/9] t7411: merge tests 5 and 6","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:34Z","receivedAt":"2018-09-17T14:09:51Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Tests 5 and 6 check for the effects of the same commit, merge the two\ntests to make it more straightforward to clean things up after the test\nhas finished.\n\nThe cleanup will be added in a future commit.\n\nSigned-off-by: Antonio Ospite \n---\n t/t7411-submodule-config.sh | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 0bde5850ac..f2cd1f4a2c 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b'\n Submodule name: 'submodule' for path 'submodule'\n EOF\n \n-test_expect_success 'error in one submodule config lets continue' '\n+test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' '\n \t(cd super &&\n \t\tcp .gitmodules .gitmodules.bak &&\n \t\techo \"\tvalue = \\\"\" >>.gitmodules &&\n \t\tgit add .gitmodules &&\n \t\tmv .gitmodules.bak .gitmodules &&\n \t\tgit commit -m \"add error\" &&\n-\t\ttest-tool submodule-config \\\n-\t\t\tHEAD b \\\n-\t\t\tHEAD submodule \\\n-\t\t\t\t>actual &&\n-\t\ttest_cmp expect_error actual\n-\t)\n-'\n-\n-test_expect_success 'error message contains blob reference' '\n-\t(cd super &&\n \t\tsha1=$(git rev-parse HEAD) &&\n \t\ttest-tool submodule-config \\\n \t\t\tHEAD b \\\n \t\t\tHEAD submodule \\\n-\t\t\t\t2>actual_err &&\n-\t\ttest_i18ngrep \"submodule-blob $sha1:.gitmodules\" actual_err >/dev/null\n+\t\t\t\t>actual \\\n+\t\t\t\t2>actual_stderr &&\n+\t\ttest_cmp expect_error actual &&\n+\t\ttest_i18ngrep \"submodule-blob $sha1:.gitmodules\" actual_stderr >/dev/null\n \t)\n '\n \n-- \n2.19.0\n\n"},{"id":"358263","messageId":"20180917140940.3839-6-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 5/9] submodule--helper: add a new 'config' subcommand","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:36Z","receivedAt":"2018-09-17T14:09:52Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Add a new 'config' subcommand to 'submodule--helper', this extra level\nof indirection makes it possible to add some flexibility to how the\nsubmodules configuration is handled.\n\nSigned-off-by: Antonio Ospite \n---\n builtin/submodule--helper.c | 14 ++++++++++++++\n t/t7411-submodule-config.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f6fb8991f3..80f939cd9e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2003,6 +2003,19 @@ static int check_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_config(int argc, const char **argv, const char *prefix)\n+{\n+\t/* Equivalent to ACTION_GET in builtin/config.c */\n+\tif (argc == 2)\n+\t\treturn print_config_from_gitmodules(argv[1]);\n+\n+\t/* Equivalent to ACTION_SET in builtin/config.c */\n+\tif (argc == 3)\n+\t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n+\n+\tdie(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2030,6 +2043,7 @@ static struct cmd_struct commands[] = {\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\n \t{\"check-name\", check_name, 0},\n+\t{\"config\", module_config, 0},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex b1f3c6489b..791245f18d 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -134,4 +134,31 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n \t)\n '\n \n+test_expect_success 'reading submodules config with \"submodule--helper config\"' '\n+\t(cd super &&\n+\t\techo \"../submodule\" >expect &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n+\t(cd super &&\n+\t\techo \"new_url\" >expect &&\n+\t\tgit submodule--helper config submodule.submodule.url \"new_url\" &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'overwriting unstaged submodules config with \"submodule--helper config\"' '\n+\ttest_when_finished \"git -C super checkout .gitmodules\" &&\n+\t(cd super &&\n+\t\techo \"newer_url\" >expect &&\n+\t\tgit submodule--helper config submodule.submodule.url \"newer_url\" &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.19.0\n\n"},{"id":"358266","messageId":"20180917140940.3839-8-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 7/9] t7506: clean up .gitmodules properly before setting up new scenario","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:38Z","receivedAt":"2018-09-17T14:09:54Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"In t/t7506-status-submodule.sh at some point a new scenario is set up to\ntest different things, in particular new submodules are added which are\nmeant to completely replace the previous ones.\n\nHowever before calling the \"git submodule add\" commands for the new\nlayout, the .gitmodules file is removed only from the working tree still\nleaving the previous content in current branch.\n\nThis can break if, in the future, \"git submodule add\" starts\ndifferentiating between the following two cases:\n\n - .gitmodules is not in the working tree but it is in the current\n branch (it may not be safe to add new submodules in this case);\n\n - .gitmodules is neither in the working tree nor anywhere in the\n current branch (it is safe to add new submodules).\n\nSince the test intends to get rid of .gitmodules anyways, let's\ncompletely remove it from the current branch, to actually start afresh\nin the new scenario.\n\nThis is more future-proof and does not break current tests.\n\nSigned-off-by: Antonio Ospite \n---\n t/t7506-status-submodule.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 943708fb04..08629a6e70 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file in nested submodule'\n \t(\n \t\tcd super &&\n \t\tgit clean -dfx &&\n-\t\trm .gitmodules &&\n+\t\tgit rm .gitmodules &&\n+\t\tgit commit -m \"remove .gitmodules\" &&\n \t\tgit submodule add -f ./sub1 &&\n \t\tgit submodule add -f ./sub2 &&\n \t\tgit submodule add -f ./sub1 sub3 &&\n-- \n2.19.0\n\n"},{"id":"358265","messageId":"20180917140940.3839-3-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 2/9] submodule: factor out a config_set_in_gitmodules_file_gently function","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:33Z","receivedAt":"2018-09-17T14:09:55Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Introduce a new config_set_in_gitmodules_file_gently() function to write\nconfig values to the .gitmodules file.\n\nThis is in preparation for a future change which will use the function\nto write to the .gitmodules file in a more controlled way instead of\nusing \"git config -f .gitmodules\".\n\nThe purpose of the change is mainly to centralize the code that writes\nto the .gitmodules file to avoid some duplication.\n\nThe naming follows git_config_set_in_file_gently() but the git_ prefix\nis removed to communicate that this is not a generic git-config API.\n\nSigned-off-by: Antonio Ospite \n---\n submodule-config.c | 12 ++++++++++++\n submodule-config.h | 1 +\n submodule.c | 10 +++-------\n 3 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex f70b7f1baf..61a555e920 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -707,6 +707,18 @@ int print_config_from_gitmodules(const char *key)\n \treturn 0;\n }\n \n+int config_set_in_gitmodules_file_gently(const char *key, const char *value)\n+{\n+\tint ret;\n+\n+\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);\n+\tif (ret < 0)\n+\t\t/* Maybe the user already did that, don't error out here */\n+\t\twarning(_(\"Could not update .gitmodules entry %s\"), key);\n+\n+\treturn ret;\n+}\n+\n struct fetch_config {\n \tint *max_children;\n \tint *recurse_submodules;\ndiff --git a/submodule-config.h b/submodule-config.h\nindex dd7f1b9a46..3921927aa1 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -49,6 +49,7 @@ const struct submodule *submodule_from_path(struct repository *r,\n \t\t\t\t\t const char *path);\n void submodule_free(struct repository *r);\n int print_config_from_gitmodules(const char *key);\n+int config_set_in_gitmodules_file_gently(const char *key, const char *value);\n \n /*\n * Returns 0 if the name is syntactically acceptable as a submodule \"name\"\ndiff --git a/submodule.c b/submodule.c\nindex a2b266fbfa..2e97032f86 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -89,6 +89,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n {\n \tstruct strbuf entry = STRBUF_INIT;\n \tconst struct submodule *submodule;\n+\tint ret;\n \n \tif (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */\n \t\treturn -1;\n@@ -104,14 +105,9 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \tstrbuf_addstr(&entry, \"submodule.\");\n \tstrbuf_addstr(&entry, submodule->name);\n \tstrbuf_addstr(&entry, \".path\");\n-\tif (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath) < 0) {\n-\t\t/* Maybe the user already did that, don't error out here */\n-\t\twarning(_(\"Could not update .gitmodules entry %s\"), entry.buf);\n-\t\tstrbuf_release(&entry);\n-\t\treturn -1;\n-\t}\n+\tret = config_set_in_gitmodules_file_gently(entry.buf, newpath);\n \tstrbuf_release(&entry);\n-\treturn 0;\n+\treturn ret;\n }\n \n /*\n-- \n2.19.0\n\n"},{"id":"358264","messageId":"20180917140940.3839-5-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 4/9] t7411: be nicer to future tests and really clean things up","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:35Z","receivedAt":"2018-09-17T14:09:57Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Tests 5 and 7 in t/t7411-submodule-config.sh add two commits with\ninvalid lines in .gitmodules but then only the second commit is removed.\n\nThis may affect future subsequent tests if they assume that the\n.gitmodules file has no errors.\n\nRemove both the commits as soon as they are not needed anymore.\n\nSigned-off-by: Antonio Ospite \n---\n t/t7411-submodule-config.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex f2cd1f4a2c..b1f3c6489b 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule'\n EOF\n \n test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' '\n+\tORIG=$(git -C super rev-parse HEAD) &&\n+\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n \t(cd super &&\n \t\tcp .gitmodules .gitmodules.bak &&\n \t\techo \"\tvalue = \\\"\" >>.gitmodules &&\n@@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' '\n '\n \n test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n+\tORIG=$(git -C super rev-parse HEAD) &&\n+\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n \t(cd super &&\n \t\tgit config -f .gitmodules \\\n \t\t\tsubmodule.submodule.fetchrecursesubmodules blabla &&\n@@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n \t\t\tHEAD b \\\n \t\t\tHEAD submodule \\\n \t\t\t\t>actual &&\n-\t\ttest_cmp expect_error actual &&\n-\t\tgit reset --hard HEAD^\n+\t\ttest_cmp expect_error actual\n \t)\n '\n \n-- \n2.19.0\n\n"},{"id":"358267","messageId":"20180917140940.3839-9-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 8/9] submodule: add a helper to check if it is safe to write to .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:39Z","receivedAt":"2018-09-17T14:09:58Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Introduce a helper function named is_writing_gitmodules_ok() to verify\nthat the .gitmodules file is safe to write.\n\nThe function name follows the scheme of is_staging_gitmodules_ok().\n\nThe two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used\nto get help from the C preprocessor in preventing typos, especially for\nfuture users.\n\nThis is in preparation for a future change which teaches git how to read\n.gitmodules from the index or from the current branch if the file is not\navailable in the working tree.\n\nThe rationale behind the check is that writing to .gitmodules requires\nthe file to be present in the working tree, unless a brand new\n.gitmodules is being created (in which case the .gitmodules file would\nnot exist at all: neither in the working tree nor in the index or in the\ncurrent branch).\n\nExpose the functionality also via a \"submodule-helper config\n--check-writeable\" command, as git scripts may want to perform the check\nbefore modifying submodules configuration.\n\nSigned-off-by: Antonio Ospite \n---\n builtin/submodule--helper.c | 24 +++++++++++++++++++++++-\n cache.h | 2 ++\n submodule.c | 18 ++++++++++++++++++\n submodule.h | 1 +\n t/t7411-submodule-config.sh | 31 +++++++++++++++++++++++++++++++\n 5 files changed, 75 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 80f939cd9e..bd14f57d00 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2005,6 +2005,28 @@ static int check_name(int argc, const char **argv, const char *prefix)\n \n static int module_config(int argc, const char **argv, const char *prefix)\n {\n+\tenum {\n+\t\tCHECK_WRITEABLE = 1\n+\t} command = 0;\n+\n+\tstruct option module_config_options[] = {\n+\t\tOPT_CMDMODE(0, \"check-writeable\", &command,\n+\t\t\t N_(\"check if it is safe to write to the .gitmodules file\"),\n+\t\t\t CHECK_WRITEABLE),\n+\t\tOPT_END()\n+\t};\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper config name [value]\"),\n+\t\tN_(\"git submodule--helper config --check-writeable\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_config_options,\n+\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);\n+\n+\tif (argc == 1 && command == CHECK_WRITEABLE)\n+\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n+\n \t/* Equivalent to ACTION_GET in builtin/config.c */\n \tif (argc == 2)\n \t\treturn print_config_from_gitmodules(argv[1]);\n@@ -2013,7 +2035,7 @@ static int module_config(int argc, const char **argv, const char *prefix)\n \tif (argc == 3)\n \t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n \n-\tdie(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n+\tusage_with_options(git_submodule_helper_usage, module_config_options);\n }\n \n #define SUPPORT_SUPER_PREFIX (1<<0)\ndiff --git a/cache.h b/cache.h\nindex 4d014541ab..33723d2a32 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -486,6 +486,8 @@ static inline enum object_type object_type(unsigned int mode)\n #define INFOATTRIBUTES_FILE \"info/attributes\"\n #define ATTRIBUTE_MACRO_PREFIX \"[attr]\"\n #define GITMODULES_FILE \".gitmodules\"\n+#define GITMODULES_INDEX \":.gitmodules\"\n+#define GITMODULES_HEAD \"HEAD:.gitmodules\"\n #define GIT_NOTES_REF_ENVIRONMENT \"GIT_NOTES_REF\"\n #define GIT_NOTES_DEFAULT_REF \"refs/notes/commits\"\n #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT \"GIT_NOTES_DISPLAY_REF\"\ndiff --git a/submodule.c b/submodule.c\nindex 2e97032f86..2b7082b2db 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -50,6 +50,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)\n \treturn 0;\n }\n \n+/*\n+ * Check if the .gitmodules file is safe to write.\n+ *\n+ * Writing to the .gitmodules file requires that the file exists in the\n+ * working tree or, if it doesn't, that a brand new .gitmodules file is going\n+ * to be created (i.e. it's neither in the index nor in the current branch).\n+ *\n+ * It is not safe to write to .gitmodules if it's not in the working tree but\n+ * it is in the index or in the current branch, because writing new values\n+ * (and staging them) would blindly overwrite ALL the old content.\n+ */\n+int is_writing_gitmodules_ok(void)\n+{\n+\tstruct object_id oid;\n+\treturn file_exists(GITMODULES_FILE) ||\n+\t\t(get_oid(GITMODULES_INDEX, &oid) < 0 && get_oid(GITMODULES_HEAD, &oid) < 0);\n+}\n+\n /*\n * Check if the .gitmodules file has unstaged modifications. This must be\n * checked before allowing modifications to the .gitmodules file with the\ndiff --git a/submodule.h b/submodule.h\nindex e452919aa4..7a22f71cb9 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -40,6 +40,7 @@ struct submodule_update_strategy {\n #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n int is_gitmodules_unmerged(const struct index_state *istate);\n+int is_writing_gitmodules_ok(void);\n int is_staging_gitmodules_ok(struct index_state *istate);\n int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n int remove_path_from_gitmodules(const char *path);\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 791245f18d..45953f9300 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -161,4 +161,35 @@ test_expect_success 'overwriting unstaged submodules config with \"submodule--hel\n \t)\n '\n \n+test_expect_success 'writeable .gitmodules when it is in the working tree' '\n+\tgit -C super submodule--helper config --check-writeable\n+'\n+\n+test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '\n+\tORIG=$(git -C super rev-parse HEAD) &&\n+\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n+\t(cd super &&\n+\t\tgit rm .gitmodules &&\n+\t\tgit commit -m \"remove .gitmodules from the current branch\" &&\n+\t\tgit submodule--helper config --check-writeable\n+\t)\n+'\n+\n+test_expect_success 'non-writeable .gitmodules when it is in the index but not in the working tree' '\n+\ttest_when_finished \"git -C super checkout .gitmodules\" &&\n+\t(cd super &&\n+\t\trm -f .gitmodules &&\n+\t\ttest_must_fail git submodule--helper config --check-writeable\n+\t)\n+'\n+\n+test_expect_success 'non-writeable .gitmodules when it is in the current branch but not in the index' '\n+\tORIG=$(git -C super rev-parse HEAD) &&\n+\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n+\t(cd super &&\n+\t\tgit rm .gitmodules &&\n+\t\ttest_must_fail git submodule--helper config --check-writeable\n+\t)\n+'\n+\n test_done\n-- \n2.19.0\n\n"},{"id":"358268","messageId":"20180917140940.3839-1-ao2@ao2.it","threadId":"49359","inReplyTo":null,"subject":"[PATCH v5 0/9] Make submodules work if .gitmodules is not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:31Z","receivedAt":"2018-09-17T14:09:59Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Hi,\n\nthis series teaches git to try and read the .gitmodules file from the\nindex (:.gitmodules) or from the current branch (HEAD:.gitmodules) when\nthe file is not readily available in the working tree.\n\nThis can be used, together with sparse checkouts, to enable submodule\nusage with programs like vcsh[1] which manage multiple repositories with\ntheir working trees sharing the same path.\n\n[1] https://github.com/RichiH/vcsh\n\n\nv4 of the series is here:\nhttps://public-inbox.org/git/20180824132951.8000-1-ao2@ao2.it/\n\nv3 of the series is here:\nhttps://public-inbox.org/git/20180814110525.17801-1-ao2@ao2.it/\n\nv2 of the series is here:\nhttps://public-inbox.org/git/20180802134634.10300-1-ao2@ao2.it/\n\nv1 of the series, with some background info, is here:\nhttps://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n\nv5 only contains some small cosmetic fixes suggested by Ævar Arnfjörð\nBjarmason, I ignored the comment about the memory leak in patch\n1 because I think there is no leak in how git_config_parse_key is used:\nhttps://public-inbox.org/git/87wosfesxl.fsf@evledraar.gmail.com/\n\nChanges since v4:\n\n * Improve names of local variables in patch 1.\n\n * Move new functions definitions in patches 1 and 2 before\n a pre-existing comment instead of after it, as the comment does not\n apply to the new functions.\n\nI am repeating the previous changelog below for your convenience, as v3\nis what is currently in pu/ao/submodule-wo-gitmodules-checked-out.\n\nChanges between v3 and v4:\n\n * Improve robustness of current tests in t7411-submodule-config.sh:\n - merge two tests that check for effects of the same commit.\n - reset to a well defined point in history when exiting the tests.\n\n * Fix style issues in new tests added to t7411-submodule-config.sh:\n - use test_when_finished in new tests.\n - name the output file 'expect' instead of 'expected'.\n\n * Add a new \"submodule--helper config --check-writeable\" command.\n\n * Use \"s--h config --check-wrteable\" in git-submodule.sh to share the\n code with the C implementation instead of duplicating the safety\n check in shell script.\n\n * Add the ability to read .gitmodules from the index and then\n fall-back to the current branch if the file is not in the index.\n Add also more tests to validate all the possible scenarios.\n\n * Fix style issues in t7416-submodule-sparse-gitmodules.sh:\n - name the output file 'expect' instead of 'expected'.\n - remove white space after the redirection operator.\n - indent the HEREDOC block.\n - use \"git -C super\" instead of a subshell when there is only one\n command in the test.\n\n * Remove a stale file named 'new' which erroneously slipped in\n a commit.\n\n * Update some comments and commit messages.\n\n\nThank you,\n Antonio\n\n\nAntonio Ospite (9):\n submodule: add a print_config_from_gitmodules() helper\n submodule: factor out a config_set_in_gitmodules_file_gently function\n t7411: merge tests 5 and 6\n t7411: be nicer to future tests and really clean things up\n submodule--helper: add a new 'config' subcommand\n submodule: use the 'submodule--helper config' command\n t7506: clean up .gitmodules properly before setting up new scenario\n submodule: add a helper to check if it is safe to write to .gitmodules\n submodule: support reading .gitmodules when it's not in the working\n tree\n\n builtin/submodule--helper.c | 40 +++++++++\n cache.h | 2 +\n git-submodule.sh | 13 ++-\n submodule-config.c | 55 ++++++++++++-\n submodule-config.h | 2 +\n submodule.c | 28 +++++--\n submodule.h | 1 +\n t/t7411-submodule-config.sh | 107 +++++++++++++++++++++----\n t/t7416-submodule-sparse-gitmodules.sh | 78 ++++++++++++++++++\n t/t7506-status-submodule.sh | 3 +-\n 10 files changed, 300 insertions(+), 29 deletions(-)\n create mode 100755 t/t7416-submodule-sparse-gitmodules.sh\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"358269","messageId":"20180917140940.3839-2-ao2@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-1-ao2@ao2.it","subject":"[PATCH v5 1/9] submodule: add a print_config_from_gitmodules() helper","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-17T14:09:32Z","receivedAt":"2018-09-17T14:10:00Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Add a new print_config_from_gitmodules() helper function to print values\nfrom .gitmodules just like \"git config -f .gitmodules\" would.\n\nThis will be used by a new submodule--helper subcommand to be able to\naccess the .gitmodules file in a more controlled way.\n\nSigned-off-by: Antonio Ospite \n---\n submodule-config.c | 25 +++++++++++++++++++++++++\n submodule-config.h | 1 +\n 2 files changed, 26 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex fc2c41b947..f70b7f1baf 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -682,6 +682,31 @@ void submodule_free(struct repository *r)\n \t\tsubmodule_cache_clear(r->submodule_cache);\n }\n \n+static int config_print_callback(const char *var, const char *value, void *cb_data)\n+{\n+\tchar *wanted_key = cb_data;\n+\n+\tif (!strcmp(wanted_key, var))\n+\t\tprintf(\"%s\\n\", value);\n+\n+\treturn 0;\n+}\n+\n+int print_config_from_gitmodules(const char *key)\n+{\n+\tint ret;\n+\tchar *store_key;\n+\n+\tret = git_config_parse_key(key, &store_key, NULL);\n+\tif (ret < 0)\n+\t\treturn CONFIG_INVALID_KEY;\n+\n+\tconfig_from_gitmodules(config_print_callback, the_repository, store_key);\n+\n+\tfree(store_key);\n+\treturn 0;\n+}\n+\n struct fetch_config {\n \tint *max_children;\n \tint *recurse_submodules;\ndiff --git a/submodule-config.h b/submodule-config.h\nindex dc7278eea4..dd7f1b9a46 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -48,6 +48,7 @@ const struct submodule *submodule_from_path(struct repository *r,\n \t\t\t\t\t const struct object_id *commit_or_tree,\n \t\t\t\t\t const char *path);\n void submodule_free(struct repository *r);\n+int print_config_from_gitmodules(const char *key);\n \n /*\n * Returns 0 if the name is syntactically acceptable as a submodule \"name\"\n-- \n2.19.0\n\n"},{"id":"358399","messageId":"20180918171257.GC27036@localhost","threadId":"49359","inReplyTo":"20180917140940.3839-10-ao2@ao2.it","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-18T17:12:57Z","receivedAt":"2018-09-18T17:13:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi Antonio,\n\nit appears that this patch (and its previous versions as well) is\nresponsible for triggering occasional test failures in\n't7814-grep-recurse-submodules.sh', more frequently, about once in\nevery ten runs, on macOS on Travis CI, less frequently, about once in\na couple of hundred runs on Linux on my machine.\n\nThe reason for the failure is memory corruption manifesting in various\nways: segfault, malloc() or use after free() errors from libc, corrupt\nloose object, invalid ref, bogus output, etc.\n\nApplying the following patch makes t7814 fail almost every time,\nthough sometimes that loop has to iterate over 1000 times until that\n'git grep' finally fails... so good luck with debugging ;)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 7184113b9b..93ae2e8e7c 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should pass the pattern type alon\n \ttest_must_fail git -c grep.patternType=fixed grep --recurse-submodules -e \"(.|.)[\\d]\" &&\n \n \t# Basic\n+\tfor i in $(seq 0 2000)\n+\tdo\n+\t\tgit grep --recurse-submodules 1 >/dev/null || return 1\n+\tdone &&\n \tgit grep -G --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n \tcat >expect <<-\\EOF &&\n \ta:(1|2)d(3|4)\n\n\nOn first look I didn't notice anything that is obviously wrong in this\npatch and could be responsible for the memory corruption, but there is\none thing I found strange, though:\n\n\nOn Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:\n> When the .gitmodules file is not available in the working tree, try\n> using the content from the index and from the current branch.\n\n\"from the index and from the current branch\" of which repository?\n\n> This\n> covers the case when the file is part of the repository but for some\n> reason it is not checked out, for example because of a sparse checkout.\n> \n> This makes it possible to use at least the 'git submodule' commands\n> which *read* the gitmodules configuration file without fully populating\n> the working tree.\n> \n> Writing to .gitmodules will still require that the file is checked out,\n> so check for that before calling config_set_in_gitmodules_file_gently.\n> \n> Add a similar check also in git-submodule.sh::cmd_add() to anticipate\n> the eventual failure of the \"git submodule add\" command when .gitmodules\n> is not safely writeable; this prevents the command from leaving the\n> repository in a spurious state (e.g. the submodule repository was cloned\n> but .gitmodules was not updated because\n> config_set_in_gitmodules_file_gently failed).\n> \n> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading\n> from .gitmodules succeeds and that writing to it fails when the file is\n> not checked out.\n> \n> Signed-off-by: Antonio Ospite \n> ---\n\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 61a555e920..bdb1d0e2c9 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n\n> @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct repository *repo)\n> static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n> {\n> \tif (repo->worktree) {\n> -\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n> -\t\tgit_config_from_file(fn, file, data);\n> +\t\tstruct git_config_source config_source = { 0 };\n> +\t\tconst struct config_options opts = { 0 };\n> +\t\tstruct object_id oid;\n> +\t\tchar *file;\n> +\n> +\t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n> +\t\tif (file_exists(file))\n> +\t\t\tconfig_source.file = file;\n> +\t\telse if (get_oid(GITMODULES_INDEX, &oid) >= 0)\n> +\t\t\tconfig_source.blob = GITMODULES_INDEX;\n\nThe repository used in t7814 contains nested submodules, which means\nthat config_from_gitmodules() is invoked three times.\n\nNow, the first two of those calls look at the superproject and at\n'submodule', and find the existing files '.../trash\ndirectory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash\ndirectory.t7814-grep-recurse-submodules/submodule/.gitmodules',\nrespectively. So far so good.\n\nThe third call, however, looks at the nested submodule at\n'submodule/sub', which doesn't contain a '.gitmodules' file. So this\nfunction goes on with the second condition and calls\nget_oid(GITMODULES_INDEX, &oid), which then appears to find the blob\nin the _superproject's_ index.\n\nI'm no expert on submodules, but my gut feeling says that this can't\nbe right. But if it _is_ right, then I would say that the commit\nmessage should explain in detail, why it is right.\n\nAnyway, even if it is indeed wrong, I'm not sure whether this is the\nroot cause of the memory corruption.\n\n\n> +\t\telse if (get_oid(GITMODULES_HEAD, &oid) >= 0)\n> +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n> +\n> +\t\tconfig_with_options(fn, data, &config_source, &opts);\n> +\n> \t\tfree(file);\n> \t}\n> }\n"},{"id":"358479","messageId":"xmqqfty547f5.fsf@gitster-ct.c.googlers.com","threadId":"49359","inReplyTo":"20180918171257.GC27036@localhost","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-19T19:24:30Z","receivedAt":"2018-09-19T19:24:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> it appears that this patch (and its previous versions as well) is\n> responsible for triggering occasional test failures in\n> 't7814-grep-recurse-submodules.sh', more frequently, about once in\n> every ten runs, on macOS on Travis CI, less frequently, about once in\n> a couple of hundred runs on Linux on my machine.\n\nI see that among Cc'ed are people who are more familiar with the\nsubmodule code and where it wants to go. Thanks for a report and\nanalysis.\n\n> The reason for the failure is memory corruption manifesting in various\n> ways: segfault, malloc() or use after free() errors from libc, corrupt\n> loose object, invalid ref, bogus output, etc.\n> \n> Applying the following patch makes t7814 fail almost every time,\n> though sometimes that loop has to iterate over 1000 times until that\n> 'git grep' finally fails... so good luck with debugging ;)\n>\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 7184113b9b..93ae2e8e7c 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should pass the pattern type alon\n> \ttest_must_fail git -c grep.patternType=fixed grep --recurse-submodules -e \"(.|.)[\\d]\" &&\n> \n> \t# Basic\n> +\tfor i in $(seq 0 2000)\n> +\tdo\n> +\t\tgit grep --recurse-submodules 1 >/dev/null || return 1\n> +\tdone &&\n> \tgit grep -G --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n> \tcat >expect <<-\\EOF &&\n> \ta:(1|2)d(3|4)\n>\n> On first look I didn't notice anything that is obviously wrong in this\n> patch and could be responsible for the memory corruption, but there is\n> one thing I found strange, though:\n>\n>\n> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:\n>> When the .gitmodules file is not available in the working tree, try\n>> using the content from the index and from the current branch.\n>\n> \"from the index and from the current branch\" of which repository?\n>\n>> This\n>> covers the case when the file is part of the repository but for some\n>> reason it is not checked out, for example because of a sparse checkout.\n>> \n>> This makes it possible to use at least the 'git submodule' commands\n>> which *read* the gitmodules configuration file without fully populating\n>> the working tree.\n>> \n>> Writing to .gitmodules will still require that the file is checked out,\n>> so check for that before calling config_set_in_gitmodules_file_gently.\n>> \n>> Add a similar check also in git-submodule.sh::cmd_add() to anticipate\n>> the eventual failure of the \"git submodule add\" command when .gitmodules\n>> is not safely writeable; this prevents the command from leaving the\n>> repository in a spurious state (e.g. the submodule repository was cloned\n>> but .gitmodules was not updated because\n>> config_set_in_gitmodules_file_gently failed).\n>> \n>> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading\n>> from .gitmodules succeeds and that writing to it fails when the file is\n>> not checked out.\n>> \n>> Signed-off-by: Antonio Ospite \n>> ---\n>\n>> diff --git a/submodule-config.c b/submodule-config.c\n>> index 61a555e920..bdb1d0e2c9 100644\n>> --- a/submodule-config.c\n>> +++ b/submodule-config.c\n>\n>> @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct repository *repo)\n>> static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n>> {\n>> \tif (repo->worktree) {\n>> -\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n>> -\t\tgit_config_from_file(fn, file, data);\n>> +\t\tstruct git_config_source config_source = { 0 };\n>> +\t\tconst struct config_options opts = { 0 };\n>> +\t\tstruct object_id oid;\n>> +\t\tchar *file;\n>> +\n>> +\t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n>> +\t\tif (file_exists(file))\n>> +\t\t\tconfig_source.file = file;\n>> +\t\telse if (get_oid(GITMODULES_INDEX, &oid) >= 0)\n>> +\t\t\tconfig_source.blob = GITMODULES_INDEX;\n>\n> The repository used in t7814 contains nested submodules, which means\n> that config_from_gitmodules() is invoked three times.\n>\n> Now, the first two of those calls look at the superproject and at\n> 'submodule', and find the existing files '.../trash\n> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash\n> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',\n> respectively. So far so good.\n>\n> The third call, however, looks at the nested submodule at\n> 'submodule/sub', which doesn't contain a '.gitmodules' file. So this\n> function goes on with the second condition and calls\n> get_oid(GITMODULES_INDEX, &oid), which then appears to find the blob\n> in the _superproject's_ index.\n>\n> I'm no expert on submodules, but my gut feeling says that this can't\n> be right. But if it _is_ right, then I would say that the commit\n> message should explain in detail, why it is right.\n>\n> Anyway, even if it is indeed wrong, I'm not sure whether this is the\n> root cause of the memory corruption.\n>\n>\n>> +\t\telse if (get_oid(GITMODULES_HEAD, &oid) >= 0)\n>> +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n>> +\n>> +\t\tconfig_with_options(fn, data, &config_source, &opts);\n>> +\n>> \t\tfree(file);\n>> \t}\n>> }\n"},{"id":"358515","messageId":"20180920173552.6109014827a062dcf3821632@ao2.it","threadId":"49359","inReplyTo":"20180918171257.GC27036@localhost","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-20T15:35:52Z","receivedAt":"2018-09-20T15:35:58Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 18 Sep 2018 19:12:57 +0200\nSZEDER Gábor wrote:\n\n> Hi Antonio,\n> \n> it appears that this patch (and its previous versions as well) is\n> responsible for triggering occasional test failures in\n> 't7814-grep-recurse-submodules.sh', more frequently, about once in\n> every ten runs, on macOS on Travis CI, less frequently, about once in\n> a couple of hundred runs on Linux on my machine.\n>\n\nThanks a lot for testing Gábor, it's really appreciated.\n\n> The reason for the failure is memory corruption manifesting in various\n> ways: segfault, malloc() or use after free() errors from libc, corrupt\n> loose object, invalid ref, bogus output, etc.\n> \n> Applying the following patch makes t7814 fail almost every time,\n> though sometimes that loop has to iterate over 1000 times until that\n> 'git grep' finally fails... so good luck with debugging ;)\n[...]\n\nI managed to capture some traces of the segfaults using this variation:\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 7184113b9b..56e87c3f8a 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should pass the pattern type alon\n test_must_fail git -c grep.patternType=fixed grep --recurse-submodules -e \"(.|.)[\\d]\" &&\n\n # Basic\n+ for i in $(test_seq 0 2000)\n+ do\n+ debug --debugger=\"gdb --silent -ex run -ex quit --return-child-result --args\" git grep --recurse-submodules 1 >/dev/null || return 1\n+ done &&\n git grep -G --recurse-submodules -e \"(.|.)[\\d]\" >actual &&\n cat >expect <<-\\EOF &&\n a:(1|2)d(3|4)\n\n\nRunning t7814 with --run=\"1,6,22\" is enough to observe the issue.\n\nFWICS these corruptions are caused by concurrent accesses to the object\nstore.\n\nThe issue is caused by these facts:\n 1. git grep uses threads;\n 2. git grep reads submodules config with repo_read_gitmodules;\n 3. repo_read_gitmodules calls config_from_gitmodules\n 4. the changes in patch 9 in this series make config_from_gitmodules\n use the object store, which apparently is not mt-safe, while the\n previous use of git_config_from_file() was.\n\n> On first look I didn't notice anything that is obviously wrong in this\n> patch and could be responsible for the memory corruption, but there is\n> one thing I found strange, though:\n> \n> \n> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:\n> > When the .gitmodules file is not available in the working tree, try\n> > using the content from the index and from the current branch.\n> \n> \"from the index and from the current branch\" of which repository?\n> \n[...]\n\n> > diff --git a/submodule-config.c b/submodule-config.c\n> > index 61a555e920..bdb1d0e2c9 100644\n> > --- a/submodule-config.c\n> > +++ b/submodule-config.c\n> \n> > @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct repository *repo)\n> > static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n> > {\n> > \tif (repo->worktree) {\n> > -\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n> > -\t\tgit_config_from_file(fn, file, data);\n> > +\t\tstruct git_config_source config_source = { 0 };\n> > +\t\tconst struct config_options opts = { 0 };\n> > +\t\tstruct object_id oid;\n> > +\t\tchar *file;\n> > +\n> > +\t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n> > +\t\tif (file_exists(file))\n> > +\t\t\tconfig_source.file = file;\n> > +\t\telse if (get_oid(GITMODULES_INDEX, &oid) >= 0)\n> > +\t\t\tconfig_source.blob = GITMODULES_INDEX;\n> \n> The repository used in t7814 contains nested submodules, which means\n> that config_from_gitmodules() is invoked three times.\n> \n> Now, the first two of those calls look at the superproject and at\n> 'submodule', and find the existing files '.../trash\n> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash\n> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',\n> respectively. So far so good.\n> \n> The third call, however, looks at the nested submodule at\n> 'submodule/sub', which doesn't contain a '.gitmodules' file. So this\n> function goes on with the second condition and calls\n> get_oid(GITMODULES_INDEX, &oid), which then appears to find the blob\n> in the _superproject's_ index.\n> \n> I'm no expert on submodules, but my gut feeling says that this can't\n> be right. But if it _is_ right, then I would say that the commit\n> message should explain in detail, why it is right.\n>\n\nI'll think about that too.\n\n> Anyway, even if it is indeed wrong, I'm not sure whether this is the\n> root cause of the memory corruption.\n> \n\nI think the immediate cause of the corruptions is multi-threading in\ngrep, I can prevent the issue from happening by using \"git grep\n--threads 1 ...\".\n\nProtecting the problematic submodules function could work for now, but\nI'd like to have more comments, my proposal is:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 601f801158..52b45de749 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -427,6 +427,11 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n if (repo_submodule_init(&submodule, superproject, path))\n return 0;\n\n+ grep_read_lock();\n+ /*\n+ * NEEDSWORK: repo_read_gitmodules accesses the object store which is\n+ * global, thus it needs to be protected.\n+ */\n repo_read_gitmodules(&submodule);\n\n /*\n@@ -439,7 +444,6 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n * store is no longer global and instead is a member of the repository\n * object.\n */\n- grep_read_lock();\n add_to_alternates_memory(submodule.objects->objectdir);\n grep_read_unlock();\n\n\nThe pre-existing NEEDSWORK comment there also suggests that these\nproblems with the object store are known. I was not aware of them.\n\nWith the change from above I could not reproduce the problem anymore,\nthis should be the only location where\nrepo_read_gitmodules/config_from_gitmodules is called in a thread.\n\nThanks you,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"358605","messageId":"xmqq5zyyyg9q.fsf@gitster-ct.c.googlers.com","threadId":"49359","inReplyTo":"20180920173552.6109014827a062dcf3821632@ao2.it","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-21T16:19:45Z","receivedAt":"2018-09-21T16:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite writes:\n\n> Protecting the problematic submodules function could work for now, but\n> I'd like to have more comments, my proposal is:\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 601f801158..52b45de749 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -427,6 +427,11 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n> if (repo_submodule_init(&submodule, superproject, path))\n> return 0;\n>\n> + grep_read_lock();\n> + /*\n> + * NEEDSWORK: repo_read_gitmodules accesses the object store which is\n> + * global, thus it needs to be protected.\n> + */\n> repo_read_gitmodules(&submodule);\n>\n> /*\n> @@ -439,7 +444,6 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n> * store is no longer global and instead is a member of the repository\n> * object.\n> */\n> - grep_read_lock();\n> add_to_alternates_memory(submodule.objects->objectdir);\n> grep_read_unlock();\n\nI think this is in line with how the grep codepath protects itself\nwhen doing anything that accesses the object store.\n\nThanks.\n"},{"id":"358749","messageId":"20180924122031.9dbec6b4c2e2a8c1bff3365b@ao2.it","threadId":"49359","inReplyTo":"20180918171257.GC27036@localhost","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-24T10:20:31Z","receivedAt":"2018-09-24T10:20:37Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 18 Sep 2018 19:12:57 +0200\nSZEDER Gábor wrote:\n\n[...]\n> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:\n> > When the .gitmodules file is not available in the working tree, try\n> > using the content from the index and from the current branch.\n> \n> \"from the index and from the current branch\" of which repository?\n>\n\nI took a look, some comments below.\n\n> > diff --git a/submodule-config.c b/submodule-config.c\n> > index 61a555e920..bdb1d0e2c9 100644\n> > --- a/submodule-config.c\n> > +++ b/submodule-config.c\n> \n> > @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct repository *repo)\n> > static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n> > {\n[...]\n> > +\t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n> > +\t\tif (file_exists(file))\n> > +\t\t\tconfig_source.file = file;\n> > +\t\telse if (get_oid(GITMODULES_INDEX, &oid) >= 0)\n> > +\t\t\tconfig_source.blob = GITMODULES_INDEX;\n> > +\t\telse if (get_oid(GITMODULES_HEAD, &oid) >= 0)\n> > +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n> > +\n> \n> The repository used in t7814 contains nested submodules, which means\n> that config_from_gitmodules() is invoked three times.\n> \n> Now, the first two of those calls look at the superproject and at\n> 'submodule', and find the existing files '.../trash\n> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash\n> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',\n> respectively. So far so good.\n> \n> The third call, however, looks at the nested submodule at\n> 'submodule/sub', which doesn't contain a '.gitmodules' file. So this\n> function goes on with the second condition and calls\n> get_oid(GITMODULES_INDEX, &oid), which then appears to find the blob\n> in the _superproject's_ index.\n>\n\nYou are correct.\n\nThis is a limitation of the object store in git, there is no equivalent\nof get_oid() to get the oid from a specific repository and this affects\nconfig_with_options too when the config source is a blob.\n\nThis does not affect commands called via \"git -C submodule_dir cmd\"\nbecause in that case the chdir happens before the_repository is set up,\nfor instance \"git-submodule $SOMETHING --recursive\" commands seem to\nchange the working directory before the recursion.\n\n> > +\t\tconfig_with_options(fn, data, &config_source, &opts);\n> > +\n> > \t\tfree(file);\n> > \t}\n> > }\n> I'm no expert on submodules, but my gut feeling says that this can't\n> be right. But if it _is_ right, then I would say that the commit\n> message should explain in detail, why it is right.\n> \n\nI agree it isn't right, I didn't consider the case of nested\nsubmodules, but even if I did the current git design does not\nallow to correctly solve the problem: \"read config from blob of an\narbitrary repository\".\n\nSo what to do for the time being?\n\nThe issue is there but in a \"normal\" scenario it is not causing any real\nharm because:\n\n 1. currently it is exposed \"only\" by git grep and nested submodules.\n\n 2. the new mechanism works fine when reading the submodule config for\n the root repository.\n\n 3. the new mechanism does not \"usually\" impact non-leaf nested\n submodules, because the .gitmodules file is \"normally\" there.\n\n 4. git grep never *uses* the GITMODULES_INDEX erroneously read\n from the root project when scanning the _leaf_ nested submodule\n because there are no further submodules down the line, the\n following check fails in builtin/grep.c:\n\n if (recurse_submodules && S_ISGITLINK(entry.mode) ...\n ...\n\nIn fact, because of 4. the test suite passes even if the gitmodule\nconfig is not correct for the leaf submodule.\n\nActually 4. makes me think that the repo_read_gitmodules() call in\nbuiltin/grep.c might not be strictly necessary, its purpose seems to be\nto *anticipate* reading the config of *child* submodules while still\nprocessing the *current* submodule, the config for the *current*\nsubmodule was already read from the superproject by the immediately\npreceding repo_submodule_init(), via:\n\n repo_submodule_init()\n submodule_from_path()\n gitmodules_read_check()\n repo_read_gitmodules()\n\nAnd this would happen anyway also for child submodules down the\nrecursion path if we removed repo_read_gitmodules() in builtin/grep.c,\nthe operation would be not protected by grep_read_lock() tho.\n\nThe test suite passes even after removing repo_read_gitmodules()\nentirely from builtin/grep.c, but I am still not confident that I get\nall the implication of why that call was originally added in commit\nf9ee2fcdfa (grep: recurse in-process using 'struct repository',\n2017-08-02).\n\nAnyways, even if we removed the call we would prevent the problem from\nhappening in the test suite, but not in the real world, in case non-leaf\nsubmodules without .gitmodules in their working tree.\n\nTo recap:\n - patch 9/9 exposes a problem with the object store but for now it's\n only a potential problem in the future case that someone wanted to\n use *nested* submodules without .gitmodules in the working tree.\n - the new code should not affect current users which\n assume .gitmodules to be in the working tree for nested submodules;\n - even if we removed the call to repo_read_gitmodules() call in\n builtin/grep.c we would not avoid the problem entirely, just avoid\n it for the the _leaf_ submodules in case of nested submodules.\n\nSelfishly, I'd propose to still merge the changes (I can send a v6 with\nthe locking fix in) and I'll write a test_expect_failure snippet to\ndocument the problem Gábor spotted so we remember about it and fix it\nwhen the object store can be accessed per-repository.\n\nI am afraid I cannot look into the core issue about the object store in\nmy free time, however if someone wanted to sponsor some time I might\nconsider taking a stab at it.\n\nCiao,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"358750","messageId":"20180924122502.f932da9d6b71c1f81341040a@ao2.it","threadId":"49359","inReplyTo":"20180917140940.3839-2-ao2@ao2.it","subject":"Re: [PATCH v5 1/9] submodule: add a print_config_from_gitmodules() helper","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-24T10:25:02Z","receivedAt":"2018-09-24T10:25:07Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Mon, 17 Sep 2018 16:09:32 +0200\nAntonio Ospite wrote:\n\n> Add a new print_config_from_gitmodules() helper function to print values\n> from .gitmodules just like \"git config -f .gitmodules\" would.\n> \n[...]\n\n> +int print_config_from_gitmodules(const char *key)\n\nI am thinking about adding a \"struct repository\" argument to this\nfunction\n\n> +{\n> +\tint ret;\n> +\tchar *store_key;\n> +\n> +\tret = git_config_parse_key(key, &store_key, NULL);\n> +\tif (ret < 0)\n> +\t\treturn CONFIG_INVALID_KEY;\n> +\n> +\tconfig_from_gitmodules(config_print_callback, the_repository, store_key);\n\nAnd use it here, to avoid another usage of \"the_repository\" when it's\nnot strictly necessary.\n\nCiao,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"358785","messageId":"CAGZ79kZaomuE3p1puznM1x+hu-w4O+ZqeGUODBDj=-R3Z1hDzg@mail.gmail.com","threadId":"49359","inReplyTo":"20180924122031.9dbec6b4c2e2a8c1bff3365b@ao2.it","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-24T21:00:50Z","receivedAt":"2018-09-24T21:01:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 24, 2018 at 3:20 AM Antonio Ospite wrote:\n\n> > The third call, however, looks at the nested submodule at\n> > 'submodule/sub', which doesn't contain a '.gitmodules' file. So this\n> > function goes on with the second condition and calls\n> > get_oid(GITMODULES_INDEX, &oid), which then appears to find the blob\n> > in the _superproject's_ index.\n> >\n>\n> You are correct.\n>\n> This is a limitation of the object store in git, there is no equivalent\n> of get_oid() to get the oid from a specific repository and this affects\n> config_with_options too when the config source is a blob.\n\nNot yet, as there is a big push to pass-through an object-store object\nor similar recently and rely less on global variables.\nI am not sure I get to this code, though.\n\n> This does not affect commands called via \"git -C submodule_dir cmd\"\n> because in that case the chdir happens before the_repository is set up,\n> for instance \"git-submodule $SOMETHING --recursive\" commands seem to\n> change the working directory before the recursion.\n\nFor this it may be worth looking into the option\n --super-prefix=\n Currently for internal use only. Set a prefix which gives a\n path from above a repository down to its root. One use is\n to give submodules context about the superproject that\n invoked it.\n\nthe whole motion of moving to in-process deprecates this clunky\nAPI to pass around strings to subprocesses.\n\n> The test suite passes even after removing repo_read_gitmodules()\n> entirely from builtin/grep.c, but I am still not confident that I get\n> all the implication of why that call was originally added in commit\n> f9ee2fcdfa (grep: recurse in-process using 'struct repository',\n> 2017-08-02).\n\nIf you checkout that commit and remove the call to repo_read_gitmodules\nand then call git-grep in a superproject with nested submodules, you\nget a segfault.\n\nOn master (and deleting out that line) you do not get the segfault,\nI think praise goes to ff6f1f564c4 (submodule-config: lazy-load a\nrepository's .gitmodules file, 2017-08-03) which happened shortly\nafter f9ee2fcdfa.\n\nIt showcased that it worked by converting ls-files, but left out grep.\n\nSo I think based on ff6f1f564c4 it is safe to remove all calls to\nrepo_read_gitmodules.\n\n> Anyways, even if we removed the call we would prevent the problem from\n> happening in the test suite, but not in the real world, in case non-leaf\n> submodules without .gitmodules in their working tree.\n\nQuite frankly I think grep was just overlooked in review of\nhttps://public-inbox.org/git/20170803182000.179328-14-bmwill@google.com/\n\nStefan\n"},{"id":"358805","messageId":"CAGZ79kaKSZeZwbcaRd810mk5wW6C0ewZWv8EX_KUB82=R1MYaQ@mail.gmail.com","threadId":"49359","inReplyTo":"20180924122502.f932da9d6b71c1f81341040a@ao2.it","subject":"Re: [PATCH v5 1/9] submodule: add a print_config_from_gitmodules() helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-24T23:06:06Z","receivedAt":"2018-09-24T23:06:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> > +int print_config_from_gitmodules(const char *key)\n>\n> I am thinking about adding a \"struct repository\" argument to this\n> function\n\nSounds like a good idea.\n"},{"id":"359051","messageId":"20180927164415.44b1d00ee5f8e582afdaa933@ao2.it","threadId":"49359","inReplyTo":"CAGZ79kZaomuE3p1puznM1x+hu-w4O+ZqeGUODBDj=-R3Z1hDzg@mail.gmail.com","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-27T14:44:15Z","receivedAt":"2018-09-27T14:44:20Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Hi Stefan,\n\nOn Mon, 24 Sep 2018 14:00:50 -0700\nStefan Beller wrote:\n\n> On Mon, Sep 24, 2018 at 3:20 AM Antonio Ospite wrote:\n> \n[...]\n> > This is a limitation of the object store in git, there is no equivalent\n> > of get_oid() to get the oid from a specific repository and this affects\n> > config_with_options too when the config source is a blob.\n> \n> Not yet, as there is a big push to pass-through an object-store object\n> or similar recently and rely less on global variables.\n> I am not sure I get to this code, though.\n>\n\nIf you end up touching get_oid() please CC me.\n\n> > This does not affect commands called via \"git -C submodule_dir cmd\"\n> > because in that case the chdir happens before the_repository is set up,\n> > for instance \"git-submodule $SOMETHING --recursive\" commands seem to\n> > change the working directory before the recursion.\n> \n> For this it may be worth looking into the option\n> --super-prefix=\n> Currently for internal use only. Set a prefix which gives a\n> path from above a repository down to its root. One use is\n> to give submodules context about the superproject that\n> invoked it.\n>\n\nMy comment wanted to highlight that there are NO problems in the\nmentioned cases:\n\n - git -C submodule_dir cmd\n - git submodule cmd --recursive\n\nAre you suggesting to look into super-prefix for any reason in\nparticular?\n\n[...]\n> > The test suite passes even after removing repo_read_gitmodules()\n> > entirely from builtin/grep.c, but I am still not confident that I get\n> > all the implication of why that call was originally added in commit\n> > f9ee2fcdfa (grep: recurse in-process using 'struct repository',\n> > 2017-08-02).\n> \n> If you checkout that commit and remove the call to repo_read_gitmodules\n> and then call git-grep in a superproject with nested submodules, you\n> get a segfault.\n> \n> On master (and deleting out that line) you do not get the segfault,\n> I think praise goes to ff6f1f564c4 (submodule-config: lazy-load a\n> repository's .gitmodules file, 2017-08-03) which happened shortly\n> after f9ee2fcdfa.\n> \n> It showcased that it worked by converting ls-files, but left out grep.\n> \n> So I think based on ff6f1f564c4 it is safe to remove all calls to\n> repo_read_gitmodules.\n>\n\nThanks for confirming.\n\n> > Anyways, even if we removed the call we would prevent the problem from\n> > happening in the test suite, but not in the real world, in case non-leaf\n> > submodules without .gitmodules in their working tree.\n> \n> Quite frankly I think grep was just overlooked in review of\n> https://public-inbox.org/git/20170803182000.179328-14-bmwill@google.com/\n> \n\nOK, so the plan for v6 is:\n\n - avoid the corruption issues spotted by Gábor by removing the call\n to repo_read_gitmodules in builtin/grep.c (this still does not fix\n the potential problem with nested submodules).\n\n - add a new test-tool which better exercises the new\n config_from_gitmodules code,\n\n - add also a test_expect_failure test to document the use case that\n cannot be supported yet: nested submodules without .gitmodules in\n their working tree.\n\nThanks,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"359052","messageId":"20180927164906.d3bd6ae0aeb4210d4fcf92ea@ao2.it","threadId":"49359","inReplyTo":"xmqq5zyyyg9q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-09-27T14:49:06Z","receivedAt":"2018-09-27T14:49:12Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Fri, 21 Sep 2018 09:19:45 -0700\nJunio C Hamano wrote:\n\n> Antonio Ospite writes:\n> \n> > Protecting the problematic submodules function could work for now, but\n> > I'd like to have more comments, my proposal is:\n> >\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 601f801158..52b45de749 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -427,6 +427,11 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n> > if (repo_submodule_init(&submodule, superproject, path))\n> > return 0;\n> >\n> > + grep_read_lock();\n> > + /*\n> > + * NEEDSWORK: repo_read_gitmodules accesses the object store which is\n> > + * global, thus it needs to be protected.\n> > + */\n> > repo_read_gitmodules(&submodule);\n> >\n> > /*\n> > @@ -439,7 +444,6 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n> > * store is no longer global and instead is a member of the repository\n> > * object.\n> > */\n> > - grep_read_lock();\n> > add_to_alternates_memory(submodule.objects->objectdir);\n> > grep_read_unlock();\n> \n> I think this is in line with how the grep codepath protects itself\n> when doing anything that accesses the object store.\n> \n\nThanks for the comment.\n\nHowever, after confirming with Stefan Beller, I think we are going to\nsolve the corruption issue by removing this call to\nrepo_read_gitmodules(), which is not strictly necessary:\n\nhttps://public-inbox.org/git/CAGZ79kZaomuE3p1puznM1x+hu-w4O+ZqeGUODBDj=-R3Z1hDzg@mail.gmail.com/\n\nThanks,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"359079","messageId":"CAGZ79kYHLF0TVfVuVfKfe_A4D2QGziRCsrYpyh7wuHjdpPEkDA@mail.gmail.com","threadId":"49359","inReplyTo":"20180927164415.44b1d00ee5f8e582afdaa933@ao2.it","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T18:00:52Z","receivedAt":"2018-09-27T18:01:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 27, 2018 at 7:44 AM Antonio Ospite wrote:\n>\n> If you end up touching get_oid() please CC me.\n\nnoted. I am not sure I'll touch it anytime soon, though.\n\n>\n> Are you suggesting to look into super-prefix for any reason in\n> particular?\n\nNo, I misread the intent of that part of your message\n\n> >\n> > So I think based on ff6f1f564c4 it is safe to remove all calls to\n> > repo_read_gitmodules.\n> >\n>\n> Thanks for confirming.\n>\n\n> OK, so the plan for v6 is:\n>\n> - avoid the corruption issues spotted by Gábor by removing the call\n> to repo_read_gitmodules in builtin/grep.c (this still does not fix\n> the potential problem with nested submodules).\n>\n> - add a new test-tool which better exercises the new\n> config_from_gitmodules code,\n\nSounds good.\n\n>\n> - add also a test_expect_failure test to document the use case that\n> cannot be supported yet: nested submodules without .gitmodules in\n> their working tree.\n\nPersonally I would want to live in a world where we don't *have* to nor\n*want* to support submodules without .gitmodules in the respective\nsuperproject.\n\nWe did support some use cases historically that I would make sure to\ncontinue to support, but I am not sure how much effort we want to spend\non supporting further use cases of incomplete submodules.\n\nFeel free to do so, as such tests help to document the boundaries.\n\nStefan\n"},{"id":"359374","messageId":"20181001174504.684457e627ed76abee5e19b8@ao2.it","threadId":"49359","inReplyTo":"CAGZ79kYHLF0TVfVuVfKfe_A4D2QGziRCsrYpyh7wuHjdpPEkDA@mail.gmail.com","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-10-01T15:45:04Z","receivedAt":"2018-10-01T15:45:10Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 27 Sep 2018 11:00:52 -0700\nStefan Beller wrote:\n\n> On Thu, Sep 27, 2018 at 7:44 AM Antonio Ospite wrote:\n[...]\n> > OK, so the plan for v6 is:\n> >\n> > - avoid the corruption issues spotted by Gábor by removing the call\n> > to repo_read_gitmodules in builtin/grep.c (this still does not fix\n> > the potential problem with nested submodules).\n> >\n\nActually that is not enough to fix the inconsistent access to the\nobject store: the functions is_submodule_active() and\nrepo_submodule_init() too end up calling config_from_gitmodules() and\nneed protecting as well, so I am going to put them under the git read\nlock and leave repo_read_gitmodules() in place for now.\n\nRemoving unneeded code can go in a possible stand-alone patch.\n\n> > - add a new test-tool which better exercises the new\n> > config_from_gitmodules code,\n> \n> Sounds good.\n> \n> >\n> > - add also a test_expect_failure test to document the use case that\n> > cannot be supported yet: nested submodules without .gitmodules in\n> > their working tree.\n> \n> Personally I would want to live in a world where we don't *have* to nor\n> *want* to support submodules without .gitmodules in the respective\n> superproject.\n>\n\nJust to double check: are you referring to *nested* submodules in the\nsentence above?\n\nI am asking because the whole point of this patchset is to *enable* the\nuse of submodules without .gitmodules in the working tree of the\nsuperproject. :)\n\nIt's just that current limitations in git do not allow to support this\nfor *nested* submodules yet.\n\n> We did support some use cases historically that I would make sure to\n> continue to support, but I am not sure how much effort we want to spend\n> on supporting further use cases of incomplete submodules.\n>\n> Feel free to do so, as such tests help to document the boundaries.\n> \n\nLet's see how v6 turns out.\n\nThanks,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"359387","messageId":"CAGZ79kanvDJcCQom6-w-LBUW7ST9Nmfs9ysAjBYBy2GTAzgx7A@mail.gmail.com","threadId":"49359","inReplyTo":"20181001174504.684457e627ed76abee5e19b8@ao2.it","subject":"Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-01T19:42:45Z","receivedAt":"2018-10-01T19:43:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 1, 2018 at 8:45 AM Antonio Ospite wrote:\n>\n> On Thu, 27 Sep 2018 11:00:52 -0700\n> Stefan Beller wrote:\n>\n> > On Thu, Sep 27, 2018 at 7:44 AM Antonio Ospite wrote:\n> [...]\n> > > OK, so the plan for v6 is:\n> > >\n> > > - avoid the corruption issues spotted by Gábor by removing the call\n> > > to repo_read_gitmodules in builtin/grep.c (this still does not fix\n> > > the potential problem with nested submodules).\n> > >\n>\n> Actually that is not enough to fix the inconsistent access to the\n> object store: the functions is_submodule_active() and\n> repo_submodule_init() too end up calling config_from_gitmodules() and\n> need protecting as well, so I am going to put them under the git read\n> lock and leave repo_read_gitmodules() in place for now.\n>\n\n>\n> I am asking because the whole point of this patchset is to *enable* the\n> use of submodules without .gitmodules in the working tree of the\n> superproject. :)\n\nI was imprecise and meant to\ns/.gitmodules/mechanism to configure the name <-> path mapping/\n\nIn this series, the .gitmodules may not be present in the working tree,\nbut it is still there in the repo. Later we may want to rename that file\nor put it into a magic branch, and I'd still find it a good idea.\nWhat I find a bad idea is to have only a gitlink and a repo put\ninto that path and expect that it magically works as then it is not\na submodule, but some \"halfway there thing\". We need to have\nan explicit \"yes this is a submodule\" statement, (which currently\ncomes from the .gitmodules file in the working tree), and I am not\nattached to where it comes from, but that it exists.\n"}]}