{"thread":{"id":"49121","subject":"[PATCH v3 6/7] t7506: clean up .gitmodules properly before setting up new scenario","startedAt":"2018-08-14T11:05:51Z","lastAt":"2018-08-23T11:48:31Z","messageCount":20,"participants":["Antonio Ospite","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":7},"messages":[{"id":"355527","messageId":"20180814110525.17801-7-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 6/7] t7506: clean up .gitmodules properly before setting up new scenario","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:24Z","receivedAt":"2018-08-14T11:05:51Z","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 <ao2@ao2.it>\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.18.0\n\n"},{"id":"355528","messageId":"20180814110525.17801-1-ao2@ao2.it","threadId":"49121","inReplyTo":null,"subject":"[PATCH v3 0/7] Make submodules work if .gitmodules is not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:18Z","receivedAt":"2018-08-14T11:05:51Z","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\ncurrent branch (HEAD:.gitmodules) when it's not readily available in the\nworking tree.\n\nThis can be used, along with sparse checkouts, to enable submodule usage\nwith programs like vcsh[1] which manage multiple repositories with their\nworking trees sharing the same path.\n\n[1] https://github.com/RichiH/vcsh\n\n\nThis is v3 of the series from:\nhttps://public-inbox.org/git/20180802134634.10300-1-ao2@ao2.it/\n\nThe cover letter of the first proposal contains more background:\nhttps://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n\nChanges since v2:\n\n  * Removed the extern keyword from the public declaration of\n    print_config_from_gitmodules() and\n    config_set_in_gitmodules_file_gently()\n\n  * Used test_when_finished in t/t7411-submodule-config.sh and remove\n    the problematic commits as soon as they are not needed anymore.\n\n  * Restructured the code in module_config to avoid an unreachable\n    section, the code now dies as a fallback if the arguments are not\n    supported, as suggested by Jeff.\n\n  * Dropped patches and tests about \"submodule--helper config --stage\"\n    as they are not strictly needed for now and there is no immediate\n    benefit from them.\n\n  * Added a check to git-submodule.sh::cdm_add to make it fail earlier\n    if the .gitmodules file is not \"safely writeable\". This also fixes\n    one of the new tests which was previously marked as\n    \"test_expect_failure\".\n\n  * Fixed a broken &&-chain in a subshell in one of the new tests, the\n    issue was exposed by a recent change in master.\n\n  * Dropped a note about \"git rm\" and \"git mv\", it was intended as\n    a personal reminder and not for the general public.\n\n  * Squashed t7416-submodule-sparse-gitmodules.sh in the same commit of\n    the code it exercises.\n\n  * Dropped the two unrelated patches from v2:\n    \n      - dir: move is_empty_file() from builtin/am.c to dir.c and make it\n        public\n      - submodule: remove the .gitmodules file when it is empty\n\n    as they are orthogonal to this series. I will send them as\n    a standalone series.\n\n  * Minor wording fixes here and there.\n\n\nThe series looks a lot cleaner and more to the point, thanks for the\nreview.\n\nCiao,\n   Antonio\n\nAntonio Ospite (7):\n  submodule: add a print_config_from_gitmodules() helper\n  submodule: factor out a config_set_in_gitmodules_file_gently function\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: support reading .gitmodules even when it's not checked out\n\n builtin/submodule--helper.c            | 29 +++++++++\n cache.h                                |  1 +\n git-submodule.sh                       | 15 +++--\n new                                    |  0\n submodule-config.c                     | 53 ++++++++++++++-\n submodule-config.h                     |  3 +\n submodule.c                            | 10 +--\n t/t7411-submodule-config.sh            | 33 +++++++++-\n t/t7416-submodule-sparse-gitmodules.sh | 90 ++++++++++++++++++++++++++\n t/t7506-status-submodule.sh            |  3 +-\n 10 files changed, 221 insertions(+), 16 deletions(-)\n create mode 100644 new\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":"355529","messageId":"20180814110525.17801-6-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 5/7] submodule: use the 'submodule--helper config' command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:23Z","receivedAt":"2018-08-14T11:05:51Z","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 <ao2@ao2.it>\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 8b5ad59bde..ff258e2e8c 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.18.0\n\n"},{"id":"355530","messageId":"20180814110525.17801-8-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:25Z","receivedAt":"2018-08-14T11:05:53Z","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 HEAD:.gitmodules from the current branch. This covers the case\nwhen the file is part of the repository but for some reason it is not\nchecked 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 <ao2@ao2.it>\n---\n\nMaybe the check in config_set_in_gitmodules_file_gently and\ngit-submodule.sh::cmd_add() can share some code:\n\n  - add an is_gitmodules_safely_writeable() helper\n  - expose a \"submodule--helper config --is-safely-writeable\" subcommand\n\nBut for now I preferred to keep the changes with v2 to a minimum to avoid\nblocking the series.\n\nIf adding a new helper is preferred I can do a v4 or send a follow-up patch.\n\nThank you,\n   Antonio\n\n\n builtin/submodule--helper.c            | 17 ++++-\n cache.h                                |  1 +\n git-submodule.sh                       |  7 ++\n submodule-config.c                     | 16 ++++-\n t/t7416-submodule-sparse-gitmodules.sh | 90 ++++++++++++++++++++++++++\n 5 files changed, 128 insertions(+), 3 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 7481d03b63..c0370a756b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2036,8 +2036,23 @@ 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\tstruct object_id oid;\n+\n+\t\t/*\n+\t\t * If the .gitmodules file is not in the working tree but it\n+\t\t * is in the current branch, stop, as writing new values (and\n+\t\t * staging them) would blindly overwrite ALL the old content.\n+\t\t *\n+\t\t * This check still makes it possible to create a brand new\n+\t\t * .gitmodules when it is safe to do so: when neither\n+\t\t * GITMODULES_FILE nor GITMODULES_HEAD exist.\n+\t\t */\n+\t\tif (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_HEAD, &oid) >= 0)\n+\t\t\tdie(_(\"please make sure that the .gitmodules file in the current branch is checked out\"));\n+\n \t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n+\t}\n \n \tdie(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n }\ndiff --git a/cache.h b/cache.h\nindex 8dc7134f00..900f9e09e5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -486,6 +486,7 @@ 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_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/git-submodule.sh b/git-submodule.sh\nindex ff258e2e8c..b1cb187227 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -159,6 +159,13 @@ cmd_add()\n \t\tshift\n \tdone\n \n+\t# For more details about this check, see\n+\t# builtin/submodule--helper.c::module_config()\n+\tif test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1\n+\tthen\n+\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file in the current branch is checked out\")\"\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 b7ef055c63..088dabb56f 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,19 @@ 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_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/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh\nnew file mode 100755\nindex 0000000000..5341e9b012\n--- /dev/null\n+++ b/t/t7416-submodule-sparse-gitmodules.sh\n@@ -0,0 +1,90 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>\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+/*\n+!/.gitmodules\n+EOF\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\" >expected &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n+test_expect_success 'not writing gitmodules config file when it is not checked out' '\n+\t(cd super &&\n+\t\ttest_must_fail git submodule--helper config submodule.submodule.url newurl\n+\t)\n+'\n+\n+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '\n+\t(cd super &&\n+\t\tgit submodule init\n+\t)\n+'\n+\n+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '\n+\t(cd super &&\n+\t\tgit submodule summary\n+\t)\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+\t(cd super &&\n+\t\tgit submodule update\n+\t)\n+'\n+\n+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '\n+\t(cd super &&\n+\t\ttest_must_fail git submodule add ../new_submodule\n+\t)\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+\t(cd super &&\n+\t\tgit submodule init\n+\t)\n+'\n+\n+test_done\n-- \n2.18.0\n\n"},{"id":"355531","messageId":"20180814110525.17801-4-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 3/7] t7411: be nicer to future tests and really clean things up","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:21Z","receivedAt":"2018-08-14T11:05:54Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Tests 5 and 8 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\nThe error introduced in test 5 is also required by test 6, so the two\ncommits from above are removed respectively in tests 6 and 8.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\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 0bde5850ac..c6b6cf6fae 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '\n '\n \n test_expect_success 'error message contains blob reference' '\n+\t# Remove the error introduced in the previous test.\n+\t# It is not needed in the following tests.\n+\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n \t(cd super &&\n \t\tsha1=$(git rev-parse HEAD) &&\n \t\ttest-tool submodule-config \\\n@@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '\n '\n \n test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n+\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n \t(cd super &&\n \t\tgit config -f .gitmodules \\\n \t\t\tsubmodule.submodule.fetchrecursesubmodules blabla &&\n@@ -134,8 +138,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.18.0\n\n"},{"id":"355532","messageId":"20180814110525.17801-2-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 1/7] submodule: add a print_config_from_gitmodules() helper","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:19Z","receivedAt":"2018-08-14T11:05:55Z","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 <ao2@ao2.it>\n---\n submodule-config.c | 25 +++++++++++++++++++++++++\n submodule-config.h |  2 ++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex fc2c41b947..eef96c4198 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 *key_, const char *value_, void *cb_data)\n+{\n+\tchar *key = cb_data;\n+\n+\tif (!strcmp(key, key_))\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..ed40e9a478 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -56,6 +56,8 @@ void submodule_free(struct repository *r);\n  */\n int check_submodule_name(const char *name);\n \n+int print_config_from_gitmodules(const char *key);\n+\n /*\n  * Note: these helper functions exist solely to maintain backward\n  * compatibility with 'fetch' and 'update_clone' storing configuration in\n-- \n2.18.0\n\n"},{"id":"355533","messageId":"20180814110525.17801-3-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 2/7] submodule: factor out a config_set_in_gitmodules_file_gently function","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:20Z","receivedAt":"2018-08-14T11:05:56Z","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 <ao2@ao2.it>\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 eef96c4198..b7ef055c63 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 ed40e9a478..9957bcbbfa 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -57,6 +57,7 @@ void submodule_free(struct repository *r);\n int check_submodule_name(const char *name);\n \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  * Note: these helper functions exist solely to maintain backward\ndiff --git a/submodule.c b/submodule.c\nindex 6e14547e9e..fd95cb76b3 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.18.0\n\n"},{"id":"355534","messageId":"20180814110525.17801-5-ao2@ao2.it","threadId":"49121","inReplyTo":"20180814110525.17801-1-ao2@ao2.it","subject":"[PATCH v3 4/7] submodule--helper: add a new 'config' subcommand","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-14T11:05:22Z","receivedAt":"2018-08-14T11:05:57Z","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 <ao2@ao2.it>\n---\n builtin/submodule--helper.c | 14 ++++++++++++++\n new                         |  0\n t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 40 insertions(+)\n create mode 100644 new\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a3c4564c6c..7481d03b63 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2029,6 +2029,19 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\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@@ -2057,6 +2070,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/new b/new\nnew file mode 100644\nindex 0000000000..e69de29bb2\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex c6b6cf6fae..4afb6f152e 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -142,4 +142,30 @@ 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\" >expected &&\n+\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest_cmp expected 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\" >expected &&\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 expected actual\n+\t)\n+'\n+\n+test_expect_success 'overwriting unstaged submodules config with \"submodule--helper config\"' '\n+\t(cd super &&\n+\t\techo \"newer_url\" >expected &&\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 expected actual\n+\t)\n+'\n+\n test_done\n-- \n2.18.0\n\n"},{"id":"355559","messageId":"20180814170619.GE240194@google.com","threadId":"49121","inReplyTo":"20180814110525.17801-4-ao2@ao2.it","subject":"Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-14T17:06:19Z","receivedAt":"2018-08-14T17:06:24Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Antonio Ospite wrote:\n> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with\n> invalid lines in .gitmodules but then only the second commit is removed.\n> \n> This may affect future subsequent tests if they assume that the\n> .gitmodules file has no errors.\n> \n> Remove both the commits as soon as they are not needed anymore.\n> \n> The error introduced in test 5 is also required by test 6, so the two\n> commits from above are removed respectively in tests 6 and 8.\n\nThanks for cleaning this up.  We seem to have a habit for leaving\ntesting state around for longer than is necessary which makes it a bit\nmore difficult to read and understand when looking at it later.  What\nwould really be nice is if each test was self-contained...course that\nwould take a herculean effort to realize in our testsuite so I'm not\nsuggesting you do that :)\n\n> \n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n> ---\n>  t/t7411-submodule-config.sh | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> index 0bde5850ac..c6b6cf6fae 100755\n> --- a/t/t7411-submodule-config.sh\n> +++ b/t/t7411-submodule-config.sh\n> @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '\n>  '\n>  \n>  test_expect_success 'error message contains blob reference' '\n> +\t# Remove the error introduced in the previous test.\n> +\t# It is not needed in the following tests.\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n>  \t(cd super &&\n>  \t\tsha1=$(git rev-parse HEAD) &&\n>  \t\ttest-tool submodule-config \\\n> @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '\n>  '\n>  \n>  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n>  \t(cd super &&\n>  \t\tgit config -f .gitmodules \\\n>  \t\t\tsubmodule.submodule.fetchrecursesubmodules blabla &&\n> @@ -134,8 +138,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> -- \n> 2.18.0\n> \n\n-- \nBrandon Williams\n"},{"id":"355560","messageId":"20180814171058.GF240194@google.com","threadId":"49121","inReplyTo":"20180814110525.17801-5-ao2@ao2.it","subject":"Re: [PATCH v3 4/7] submodule--helper: add a new 'config' subcommand","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-14T17:10:58Z","receivedAt":"2018-08-14T17:11:02Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Antonio Ospite wrote:\n> Add a new 'config' subcommand to 'submodule--helper', this extra level\n> of indirection makes it possible to add some flexibility to how the\n> submodules configuration is handled.\n> \n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n> ---\n>  builtin/submodule--helper.c | 14 ++++++++++++++\n\n>  new                         |  0\n\nLooks like you may have accidentally left in an empty file \"new\" it should\nprobably be removed from this commit before it gets merged.\n\nAside from that this patch looks good.  I've recently run into issues\nwhere we don't have a good enough abstraction layer around how we\ninteract with submodules so I'm glad we're moving towards better\nabstractions :)\n\n>  t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++\n>  3 files changed, 40 insertions(+)\n>  create mode 100644 new\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index a3c4564c6c..7481d03b63 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2029,6 +2029,19 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\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> @@ -2057,6 +2070,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)\n> diff --git a/new b/new\n> new file mode 100644\n> index 0000000000..e69de29bb2\n> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> index c6b6cf6fae..4afb6f152e 100755\n> --- a/t/t7411-submodule-config.sh\n> +++ b/t/t7411-submodule-config.sh\n> @@ -142,4 +142,30 @@ 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\" >expected &&\n> +\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest_cmp expected 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\" >expected &&\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 expected actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'overwriting unstaged submodules config with \"submodule--helper config\"' '\n> +\t(cd super &&\n> +\t\techo \"newer_url\" >expected &&\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 expected actual\n> +\t)\n> +'\n> +\n>  test_done\n> -- \n> 2.18.0\n> \n\n-- \nBrandon Williams\n"},{"id":"355561","messageId":"20180814171241.GA233973@google.com","threadId":"49121","inReplyTo":"20180814110525.17801-6-ao2@ao2.it","subject":"Re: [PATCH v3 5/7] submodule: use the 'submodule--helper config' command","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-14T17:12:41Z","receivedAt":"2018-08-14T17:12:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Antonio Ospite wrote:\n> Use the 'submodule--helper config' command in git-submodules.sh to avoid\n> referring explicitly to .gitmodules by the hardcoded file path.\n> \n> This makes it possible to access the submodules configuration in a more\n> controlled way.\n> \n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n\nLooks great.  I also like you're approach of introducing the new API and\ntesting it in one commit, and then using it in the next.  Makes the\npatch set very easy to follow.\n\n> ---\n>  git-submodule.sh | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 8b5ad59bde..ff258e2e8c 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> -- \n> 2.18.0\n> \n\n-- \nBrandon Williams\n"},{"id":"355562","messageId":"20180814172258.GB233973@google.com","threadId":"49121","inReplyTo":"20180814110525.17801-8-ao2@ao2.it","subject":"Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-14T17:22:58Z","receivedAt":"2018-08-14T17:23:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Antonio Ospite wrote:\n> When the .gitmodules file is not available in the working tree, try\n> using HEAD:.gitmodules from the current branch. This covers the case\n> when the file is part of the repository but for some reason it is not\n> 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 <ao2@ao2.it>\n> ---\n> \n> Maybe the check in config_set_in_gitmodules_file_gently and\n> git-submodule.sh::cmd_add() can share some code:\n> \n>   - add an is_gitmodules_safely_writeable() helper\n>   - expose a \"submodule--helper config --is-safely-writeable\" subcommand\n> \n> But for now I preferred to keep the changes with v2 to a minimum to avoid\n> blocking the series.\n> \n> If adding a new helper is preferred I can do a v4 or send a follow-up patch.\n\nI see how it would be nice to have the addition of a helper like this.\nI think maybe at some point we'd want it but its definitely not needed\nnow and can easily be added at a later point (maybe we can avoid needing\nit until we can convert all of the git-submodule.sh code to C!).\n\nGreat work, thanks for working on this.\n\n> \n> Thank you,\n>    Antonio\n> \n> \n>  builtin/submodule--helper.c            | 17 ++++-\n>  cache.h                                |  1 +\n>  git-submodule.sh                       |  7 ++\n>  submodule-config.c                     | 16 ++++-\n>  t/t7416-submodule-sparse-gitmodules.sh | 90 ++++++++++++++++++++++++++\n>  5 files changed, 128 insertions(+), 3 deletions(-)\n>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 7481d03b63..c0370a756b 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2036,8 +2036,23 @@ 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\tstruct object_id oid;\n> +\n> +\t\t/*\n> +\t\t * If the .gitmodules file is not in the working tree but it\n> +\t\t * is in the current branch, stop, as writing new values (and\n> +\t\t * staging them) would blindly overwrite ALL the old content.\n> +\t\t *\n> +\t\t * This check still makes it possible to create a brand new\n> +\t\t * .gitmodules when it is safe to do so: when neither\n> +\t\t * GITMODULES_FILE nor GITMODULES_HEAD exist.\n> +\t\t */\n> +\t\tif (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_HEAD, &oid) >= 0)\n> +\t\t\tdie(_(\"please make sure that the .gitmodules file in the current branch is checked out\"));\n> +\n>  \t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n> +\t}\n>  \n>  \tdie(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n>  }\n> diff --git a/cache.h b/cache.h\n> index 8dc7134f00..900f9e09e5 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -486,6 +486,7 @@ 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_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\"\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index ff258e2e8c..b1cb187227 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -159,6 +159,13 @@ cmd_add()\n>  \t\tshift\n>  \tdone\n>  \n> +\t# For more details about this check, see\n> +\t# builtin/submodule--helper.c::module_config()\n> +\tif test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1\n> +\tthen\n> +\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file in the current branch is checked out\")\"\n> +\tfi\n> +\n>  \tif test -n \"$reference_path\"\n>  \tthen\n>  \t\tis_absolute_path \"$reference_path\" ||\n> diff --git a/submodule-config.c b/submodule-config.c\n> index b7ef055c63..088dabb56f 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,19 @@ 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_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> diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh\n> new file mode 100755\n> index 0000000000..5341e9b012\n> --- /dev/null\n> +++ b/t/t7416-submodule-sparse-gitmodules.sh\n> @@ -0,0 +1,90 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>\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> +/*\n> +!/.gitmodules\n> +EOF\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\" >expected &&\n> +\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest_cmp expected actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'not writing gitmodules config file when it is not checked out' '\n> +\t(cd super &&\n> +\t\ttest_must_fail git submodule--helper config submodule.submodule.url newurl\n> +\t)\n> +'\n> +\n> +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '\n> +\t(cd super &&\n> +\t\tgit submodule init\n> +\t)\n> +'\n> +\n> +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '\n> +\t(cd super &&\n> +\t\tgit submodule summary\n> +\t)\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> +\t(cd super &&\n> +\t\tgit submodule update\n> +\t)\n> +'\n> +\n> +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '\n> +\t(cd super &&\n> +\t\ttest_must_fail git submodule add ../new_submodule\n> +\t)\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> +\t(cd super &&\n> +\t\tgit submodule init\n> +\t)\n> +'\n> +\n> +test_done\n> -- \n> 2.18.0\n> \n\n-- \nBrandon Williams\n"},{"id":"355611","messageId":"xmqq7eks1z6h.fsf@gitster-ct.c.googlers.com","threadId":"49121","inReplyTo":"20180814110525.17801-4-ao2@ao2.it","subject":"Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-14T20:16:38Z","receivedAt":"2018-08-14T20:16:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite <ao2@ao2.it> writes:\n\n>  test_expect_success 'error message contains blob reference' '\n> +\t# Remove the error introduced in the previous test.\n> +\t# It is not needed in the following tests.\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n>  \t(cd super &&\n>  \t\tsha1=$(git rev-parse HEAD) &&\n>  \t\ttest-tool submodule-config \\\n\nAntonio Ospite <ao2@ao2.it> writes:\n\n> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with\n> invalid lines in .gitmodules but then only the second commit is removed.\n>\n> This may affect future subsequent tests if they assume that the\n> .gitmodules file has no errors.\n>\n> Remove both the commits as soon as they are not needed anymore.\n>\n> The error introduced in test 5 is also required by test 6, so the two\n> commits from above are removed respectively in tests 6 and 8.\n>\n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n> ---\n>  t/t7411-submodule-config.sh | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> index 0bde5850ac..c6b6cf6fae 100755\n> --- a/t/t7411-submodule-config.sh\n> +++ b/t/t7411-submodule-config.sh\n> @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '\n>  '\n>  \n>  test_expect_success 'error message contains blob reference' '\n> +\t# Remove the error introduced in the previous test.\n> +\t# It is not needed in the following tests.\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n\nHmm, that is ugly.  Depending on where in the subshell the previous\ntest failed, you'd still be taking us to an unexpected place.\nImagine if \"git commit -m 'add error'\" failed, for example, in the\ntest before this one.\n\nI am wondering if the proper fix is to merge the previous one and\nthis one into a single test.  The combined test would\n\n    - remember where the HEAD in super is and arrange to come back\n      to it when test is done\n    - break .gitmodules and commit it\n    - run test-tool and check its output\n    - also check its error output\n\nin a single test_expect_success.\n\n> @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '\n>  '\n>  \n>  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n>  \t(cd super &&\n>  \t\tgit config -f .gitmodules \\\n>  \t\t\tsubmodule.submodule.fetchrecursesubmodules blabla &&\n> @@ -134,8 +138,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\nIf we want to be more robust, you'd probably need to find a better\nanchoring point than HEAD, which can be pointing different commit\ndepending on where in the subshell the process is hit with ^C,\ni.e.\n\n\tORIG=$(git -C super rev-parse HEAD) &&\n\ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n\t(\n\t\tcd super &&\n\t\t...\n\nThe patch is still an improvement compared to the current code,\nwhere a broken test-tool that does not produce expected output in\nthe file 'actual' is guaranteed to leave us at a commit that we do\nnot expect to be at, but not entirely satisfactory.\n"},{"id":"355613","messageId":"xmqqmutoznwe.fsf@gitster-ct.c.googlers.com","threadId":"49121","inReplyTo":"20180814110525.17801-8-ao2@ao2.it","subject":"Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-14T20:36:17Z","receivedAt":"2018-08-14T20:36:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite <ao2@ao2.it> writes:\n\n>  \t/* Equivalent to ACTION_SET in builtin/config.c */\n> -\tif (argc == 3)\n> +\tif (argc == 3) {\n> +\t\tstruct object_id oid;\n> +\n> +\t\t/*\n> +\t\t * If the .gitmodules file is not in the working tree but it\n> +\t\t * is in the current branch, stop, as writing new values (and\n> +\t\t * staging them) would blindly overwrite ALL the old content.\n\nHmph, \"the file is missing\" certainly is a condition we would want\nto notice, but wouldn't we in general want to prevent us from\noverwriting any local modification, where \"missing\" is merely a very\nspecial case of local modification?  I am wondering if we would want\nto stop if .gitmodules file exists both in the working tree and in\nthe index, and the contents of them differ, or something like that.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index ff258e2e8c..b1cb187227 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -159,6 +159,13 @@ cmd_add()\n>  \t\tshift\n>  \tdone\n>  \n> +\t# For more details about this check, see\n> +\t# builtin/submodule--helper.c::module_config()\n> +\tif test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1\n\nNo SP between redirection '>' and its target '/dev/null'.\n\nMore importantly, I think it is better to add a submodule--helper\nsubcommand that exposes the check in question, as the code is\nalready written ;-) That approach will guarantee that the logic and\nthe message stay the same between here and in the C code.  Then you\ndo not even need these two line comment.\n\n> +\tthen\n> +\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file in the current branch is checked out\")\"\n> +\tfi\n> +\n\n> diff --git a/submodule-config.c b/submodule-config.c\n> index b7ef055c63..088dabb56f 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,19 @@ 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_HEAD, &oid) >= 0)\n> +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n\nWhat is the reason why we fall back directly to HEAD when working\ntree file does not exist?  I thought that our usual fallback was to\nthe version in the index for other things like .gitignore/attribute\nand this codepath look like an oddball.  Are you trying to handle\nthe case where we are in a bare repository without any file checked\nout (and there is not even the index)?\n\n> diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh\n> new file mode 100755\n> index 0000000000..5341e9b012\n> --- /dev/null\n> +++ b/t/t7416-submodule-sparse-gitmodules.sh\n> @@ -0,0 +1,90 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>\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\nNo SP between redirection '>' and its target 'file'.\n\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> +/*\n> +!/.gitmodules\n> +EOF\n\nYou can use <<-\\EOF and indent the body of the here-doc, which makes\nthe result easier to read, i.e.\n\n\t\tcat >target <<-\\EOF &&\n\t\tline 1\n\t\tline 2\n\t\tEOF\n\n> +\t\tgit config core.sparsecheckout true &&\n> +\t\tgit read-tree -m -u HEAD &&\n\nThat's old fashioned---I am curious if this has to be one-way merge\nor can just be a usual \"git checkout\" (I am merely curious; not\nsuggesting to change anything).\n\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\" >expected &&\n> +\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest_cmp expected actual\n\nA minor style thing, but I thought that it was more common in our\ntests to call the expected output 'expect' (which has the same\nnumber of letters as 'actual') than 'expected'.\n\nMore importantly, do we want a subshell, or is something like this\nsufficient?\n\n\techo \"../submodule\" >expected &&\n\tgit -C super submodule--helper config ... >actual &&\n\ttest_cmp expect actual\n\nThe same comment applies to many tests I see below (omitted).\n\n"},{"id":"356093","messageId":"20180820184653.1ad1d5bc72effe4e995cff18@ao2.it","threadId":"49121","inReplyTo":"xmqq7eks1z6h.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-20T16:46:53Z","receivedAt":"2018-08-20T16:46:58Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 14 Aug 2018 13:16:38 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Antonio Ospite <ao2@ao2.it> writes:\n> \n[...]\n> >  test_expect_success 'error message contains blob reference' '\n> > +\t# Remove the error introduced in the previous test.\n> > +\t# It is not needed in the following tests.\n> > +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> \n> Hmm, that is ugly.  Depending on where in the subshell the previous\n> test failed, you'd still be taking us to an unexpected place.\n> Imagine if \"git commit -m 'add error'\" failed, for example, in the\n> test before this one.\n> \n> I am wondering if the proper fix is to merge the previous one and\n> this one into a single test.  The combined test would\n> \n>     - remember where the HEAD in super is and arrange to come back\n>       to it when test is done\n>     - break .gitmodules and commit it\n>     - run test-tool and check its output\n>     - also check its error output\n> \n> in a single test_expect_success.\n>\n\nI will try that.\n\n> > @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '\n> >  '\n> >  \n> >  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> > +\ttest_when_finished \"git -C super reset --hard HEAD^\" &&\n> >  \t(cd super &&\n> >  \t\tgit config -f .gitmodules \\\n> >  \t\t\tsubmodule.submodule.fetchrecursesubmodules blabla &&\n> > @@ -134,8 +138,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> If we want to be more robust, you'd probably need to find a better\n> anchoring point than HEAD, which can be pointing different commit\n> depending on where in the subshell the process is hit with ^C,\n> i.e.\n> \n> \tORIG=$(git -C super rev-parse HEAD) &&\n> \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n> \t(\n> \t\tcd super &&\n> \t\t...\n>\n\nI see, ORIG is set and evaluated immediately but the value will be\nused only at a later time.\n\nI remember that you raised concerns also in the previous review round\nbut I didn't quite get what you meant, now I think I do.\n\n> The patch is still an improvement compared to the current code,\n> where a broken test-tool that does not produce expected output in\n> the file 'actual' is guaranteed to leave us at a commit that we do\n> not expect to be at, but not entirely satisfactory.\n\nI can do a v4 with these fixes since there are also some comments about\nother patches.\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":"356094","messageId":"20180820185029.ba1f20d79db5479255f0ff68@ao2.it","threadId":"49121","inReplyTo":"20180814171058.GF240194@google.com","subject":"Re: [PATCH v3 4/7] submodule--helper: add a new 'config' subcommand","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-20T16:50:29Z","receivedAt":"2018-08-20T16:50:32Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 14 Aug 2018 10:10:58 -0700\nBrandon Williams <bmwill@google.com> wrote:\n\n> On 08/14, Antonio Ospite wrote:\n> > Add a new 'config' subcommand to 'submodule--helper', this extra level\n> > of indirection makes it possible to add some flexibility to how the\n> > submodules configuration is handled.\n> > \n> > Signed-off-by: Antonio Ospite <ao2@ao2.it>\n> > ---\n> >  builtin/submodule--helper.c | 14 ++++++++++++++\n> \n> >  new                         |  0\n> \n> Looks like you may have accidentally left in an empty file \"new\" it should\n> probably be removed from this commit before it gets merged.\n> \n\nYeah, I had added it to test \"git cat-file -e new\" for a later patch and\nthen I must have messed up some rebase. Thanks for pointing it out.\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":"356140","messageId":"20180820233755.dc7b6a6927faccc37b25075f@ao2.it","threadId":"49121","inReplyTo":"xmqqmutoznwe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-20T21:37:55Z","receivedAt":"2018-08-20T21:38:03Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 14 Aug 2018 13:36:17 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Antonio Ospite <ao2@ao2.it> writes:\n> \n> >  \t/* Equivalent to ACTION_SET in builtin/config.c */\n> > -\tif (argc == 3)\n> > +\tif (argc == 3) {\n> > +\t\tstruct object_id oid;\n> > +\n> > +\t\t/*\n> > +\t\t * If the .gitmodules file is not in the working tree but it\n> > +\t\t * is in the current branch, stop, as writing new values (and\n> > +\t\t * staging them) would blindly overwrite ALL the old content.\n> \n> Hmph, \"the file is missing\" certainly is a condition we would want\n> to notice, but wouldn't we in general want to prevent us from\n> overwriting any local modification, where \"missing\" is merely a very\n> special case of local modification?  I am wondering if we would want\n> to stop if .gitmodules file exists both in the working tree and in\n> the index, and the contents of them differ, or something like that.\n>\n\nTTBOMK checking the index status (with something like\nis_staging_gitmodules_ok()) when the .gitmodules file *exists* in the\nworking tree would break calling \"git submodule add\" multiple times\nbefore committing the changes.\n\n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index ff258e2e8c..b1cb187227 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -159,6 +159,13 @@ cmd_add()\n> >  \t\tshift\n> >  \tdone\n> >  \n> > +\t# For more details about this check, see\n> > +\t# builtin/submodule--helper.c::module_config()\n> > +\tif test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1\n> \n> No SP between redirection '>' and its target '/dev/null'.\n>\n> More importantly, I think it is better to add a submodule--helper\n> subcommand that exposes the check in question, as the code is\n> already written ;-) That approach will guarantee that the logic and\n> the message stay the same between here and in the C code.  Then you\n> do not even need these two line comment.\n>\n\nYeah I anticipated this concern in the patch annotation, but I was\nhoping that it would be OK to have this as a followup change.\n\nI guess I can do it for v4 instead.\n\nDoes the interface suggested in the patch annotation sound acceptable?\n\nTo recap:\n\n  - add an is_gitmodules_safely_writeable() helper;\n  - expose a \"submodule--helper config --is-safely-writeable\"\n    subcommand for git-submodule.sh to use.\n\n[...]\n> > @@ -603,8 +604,19 @@ 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_HEAD, &oid) >= 0)\n> > +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n> \n> What is the reason why we fall back directly to HEAD when working\n> tree file does not exist?  I thought that our usual fallback was to\n> the version in the index for other things like .gitignore/attribute\n> and this codepath look like an oddball.  Are you trying to handle\n> the case where we are in a bare repository without any file checked\n> out (and there is not even the index)?\n>\n\nMy use case is about *reading* .gitmodules when it's ignored in a sparse\ncheckout, in this scenario there are usually no staged changes\nto .gitmodules, so I basically just didn't care about the index.\n\nWould using \":.gitmodules\" instead of \"HEAD:.gitmodules\" be enough?\n\nBy reading man  gitrevisions(7) and after a quick test with \"git\ncat-file blob :.gitmodules\" it looks like this would be more in line\nwith your suggestion, still covering my use case.\n\nIf so, what name should I use instead of GITMODULES_HEAD?\nGITMODULES_BLOB is already taken for something different, maybe\nGITMODULES_REF or GITMODULES_OBJECT?\n\n> > diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh\n> > new file mode 100755\n> > index 0000000000..5341e9b012\n> > --- /dev/null\n> > +++ b/t/t7416-submodule-sparse-gitmodules.sh\n> > @@ -0,0 +1,90 @@\n> > +#!/bin/sh\n> > +#\n> > +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>\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> \n> No SP between redirection '>' and its target 'file'.\n> \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> > +/*\n> > +!/.gitmodules\n> > +EOF\n> \n> You can use <<-\\EOF and indent the body of the here-doc, which makes\n> the result easier to read, i.e.\n> \n> \t\tcat >target <<-\\EOF &&\n> \t\tline 1\n> \t\tline 2\n> \t\tEOF\n> \n> > +\t\tgit config core.sparsecheckout true &&\n> > +\t\tgit read-tree -m -u HEAD &&\n> \n> That's old fashioned---I am curious if this has to be one-way merge\n> or can just be a usual \"git checkout\" (I am merely curious; not\n> suggesting to change anything).\n>\n\nIt was just how I learned to set up a sparse checkout, if there is a\nbetter way I'd be happy to use it.\n\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\" >expected &&\n> > +\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> > +\t\ttest_cmp expected actual\n> \n> A minor style thing, but I thought that it was more common in our\n> tests to call the expected output 'expect' (which has the same\n> number of letters as 'actual') than 'expected'.\n> \n\nWill fix.\n\n> More importantly, do we want a subshell, or is something like this\n> sufficient?\n> \n> \techo \"../submodule\" >expected &&\n> \tgit -C super submodule--helper config ... >actual &&\n> \ttest_cmp expect actual\n> \n> The same comment applies to many tests I see below (omitted).\n> \n\nI'll keep the subshell when there are multiple git commands ran in the\nsame sub directory, and remove it in tests which have only one git\ncommand per test (which is most of them), how does that sound?\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":"356242","messageId":"20180822135152.1d40cd05d0b0cadb5eefb31f@ao2.it","threadId":"49121","inReplyTo":"20180820233755.dc7b6a6927faccc37b25075f@ao2.it","subject":"Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-22T11:51:52Z","receivedAt":"2018-08-22T11:51:58Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Mon, 20 Aug 2018 23:37:55 +0200\nAntonio Ospite <ao2@ao2.it> wrote:\n\n> On Tue, 14 Aug 2018 13:36:17 -0700\n> Junio C Hamano <gitster@pobox.com> wrote:\n> \n> > Antonio Ospite <ao2@ao2.it> writes:\n[...]\n> > >  \n> > > +\t# For more details about this check, see\n> > > +\t# builtin/submodule--helper.c::module_config()\n> > > +\tif test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1\n> > \n[...]\n> > More importantly, I think it is better to add a submodule--helper\n> > subcommand that exposes the check in question, as the code is\n> > already written ;-) That approach will guarantee that the logic and\n> > the message stay the same between here and in the C code.  Then you\n> > do not even need these two line comment.\n> >\n[...]\n> Does the interface suggested in the patch annotation sound acceptable?\n> \n> To recap:\n> \n>   - add an is_gitmodules_safely_writeable() helper;\n>   - expose a \"submodule--helper config --is-safely-writeable\"\n>     subcommand for git-submodule.sh to use.\n>\n\nMaybe \"submodule--helper config --check-writeable\" could be a better\nname to avoid confusion between the boolean return value of the C\nfunction (0: false, 1: true) and the exit status returned to the shell\n(0: safe to write, !0: unsafe).\n\nI'll use the following to map the returned value, as I saw that in\nother places in the code base:\n\n\tif (argc == 1 && command == CHECK_WRITEABLE)\n\t\treturn is_gitmodules_safely_writeable() ? 0 : -1;\n\nI am assuming a command flag to the \"config\" subcommand is OK instead\nof a brand new subcommand.\n\n> [...]\n> > > @@ -603,8 +604,19 @@ 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_HEAD, &oid) >= 0)\n> > > +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n> > \n> > What is the reason why we fall back directly to HEAD when working\n> > tree file does not exist?  I thought that our usual fallback was to\n> > the version in the index for other things like .gitignore/attribute\n> > and this codepath look like an oddball.  Are you trying to handle\n> > the case where we are in a bare repository without any file checked\n> > out (and there is not even the index)?\n> >\n> \n> My use case is about *reading* .gitmodules when it's ignored in a sparse\n> checkout, in this scenario there are usually no staged changes\n> to .gitmodules, so I basically just didn't care about the index.\n> \n> Would using \":.gitmodules\" instead of \"HEAD:.gitmodules\" be enough?\n> \n[...]\n> \n> If so, what name should I use instead of GITMODULES_HEAD?\n> GITMODULES_BLOB is already taken for something different, maybe\n> GITMODULES_REF or GITMODULES_OBJECT?\n>\n\nIf using \":.gitmodules\" is good enough I could rename the current use\nof GITMODULES_BLOB in fsck.c to GITMODULES_NONBLOB and use\nGITMODULES_BLOB for \":.gitmodules\" after all.\n\nThis is to avoid preprocessor clashes with the symbolic constant\nGITMODULES_BLOB currently used in in fsck.c.\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":"356255","messageId":"xmqqd0ua773e.fsf@gitster-ct.c.googlers.com","threadId":"49121","inReplyTo":"20180822135152.1d40cd05d0b0cadb5eefb31f@ao2.it","subject":"Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-22T15:29:25Z","receivedAt":"2018-08-22T15:29:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite <ao2@ao2.it> writes:\n\n> Maybe \"submodule--helper config --check-writeable\" could be a better\n> name to avoid confusion between the boolean return value of the C\n> function (0: false, 1: true) and the exit status returned to the shell\n> (0: safe to write, !0: unsafe).\n\nPerhaps.  The main point was to replace the comment that tells the\ndevelopers to keep two things stay in sync with an actually shared\nimplementation; as long as that is done, I am not too much worried\nabout the details.\n\n>> > > +\t\telse if (get_oid(GITMODULES_HEAD, &oid) >= 0)\n>> > > +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n>> > \n>> Would using \":.gitmodules\" instead of \"HEAD:.gitmodules\" be enough?\n\nYeah, either \"instead of\", or \"in addition\" (i.e. \"try the index\nversion in addition, before falling further back to the HEAD\nversion\"), would be more consistent with the remainder of the system\n(or, at least where the remainder of the system wants to go).\n\n>> If so, what name should I use instead of GITMODULES_HEAD?\n>> GITMODULES_BLOB is already taken for something different, maybe\n>> GITMODULES_REF or GITMODULES_OBJECT?\n\nI do not know why you want to refrain from spelling them out as\n\"HEAD:.gitmodules\" and \":.gitmodules\"; at least to me the extra\nlayer of names do not look like they are making the code easier\nto understand that much.\n\n"},{"id":"356357","messageId":"20180823134824.db90021628baef8c6706fa38@ao2.it","threadId":"49121","inReplyTo":"xmqqd0ua773e.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-23T11:48:24Z","receivedAt":"2018-08-23T11:48:31Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Wed, 22 Aug 2018 08:29:25 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Antonio Ospite <ao2@ao2.it> writes:\n> \n[...]\n> >> > > +\t\telse if (get_oid(GITMODULES_HEAD, &oid) >= 0)\n> >> > > +\t\t\tconfig_source.blob = GITMODULES_HEAD;\n> >> > \n> >> Would using \":.gitmodules\" instead of \"HEAD:.gitmodules\" be enough?\n> \n> Yeah, either \"instead of\", or \"in addition\" (i.e. \"try the index\n> version in addition, before falling further back to the HEAD\n> version\"), would be more consistent with the remainder of the system\n> (or, at least where the remainder of the system wants to go).\n>\n\nOK, I now tested with both \"rm .gitmodules\" and \"git rm .gitmodules\"\nand I see why one would want to try _both_ \":.gitmodules\" and\n\"HEAD:.gitmodules\".\n\nI'll go with \"in addition\" then, adding tests for both the scenarios.\n\n> >> If so, what name should I use instead of GITMODULES_HEAD?\n> >> GITMODULES_BLOB is already taken for something different, maybe\n> >> GITMODULES_REF or GITMODULES_OBJECT?\n> \n> I do not know why you want to refrain from spelling them out as\n> \"HEAD:.gitmodules\" and \":.gitmodules\"; at least to me the extra\n> layer of names do not look like they are making the code easier\n> to understand that much.\n> \n\nThis is in the spirit of commit 4c0eeafe47 (cache.h: add\nGITMODULES_FILE macro, 2017-08-02), IIRC this was done mainly to get\nhelp from the preprocessor to spot typos: I caught myself writing\n\".gitmdoules\" several times; GITMDOULES_FILE would not compile.\n\nIf this makes sense I'll use GITMODULES_INDEX and GITMODULES_HEAD.\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"}]}