{"thread":{"id":"49010","subject":"[RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper","startedAt":"2018-08-02T14:27:30Z","lastAt":"2018-08-09T13:59:45Z","messageCount":38,"participants":["Antonio Ospite","SZEDER Gábor","Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":12},"messages":[{"id":"354248","messageId":"20180802134634.10300-2-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:23Z","receivedAt":"2018-08-02T14:27:30Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"This will be used to print values just like \"git config -f .gitmodules\"\nwould.\n\nSigned-off-by: Antonio Ospite \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 2a7259ba8b..6f6f5f9960 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..6fec3caadd 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+extern 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":"354249","messageId":"20180802134634.10300-11-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:32Z","receivedAt":"2018-08-02T14:27:33Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"git submodule commands can now access .gitmodules from the current\nbranch even when it's not in the working tree, add some tests for that\nscenario.\n\nSigned-off-by: Antonio Ospite \n---\n\nFor the test files I used the most used style in other tests, Stefan suggested\nto avoid subshells and use \"git -C\" but subshells make the test look cleaner\nIMHO.\n\n t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++\n 1 file changed, 112 insertions(+)\n create mode 100755 t/t7416-submodule-sparse-gitmodules.sh\n\ndiff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh\nnew file mode 100755\nindex 0000000000..3c7a53316b\n--- /dev/null\n+++ b/t/t7416-submodule-sparse-gitmodules.sh\n@@ -0,0 +1,112 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2018 Antonio Ospite \n+#\n+\n+test_description=' Test reading/writing .gitmodules is 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, but the same scenario can be set up\n+also by committing .gitmodules and then just removing it from the filesystem.\n+\n+NOTE: \"git mv\" and \"git rm\" are still supposed to work even without\n+a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.\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 ! -e .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 'not staging gitmodules config when it is not checked out' '\n+\t(cd super &&\n+\t\ttest_must_fail git submodule--helper config --stage\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+# \"git add\" in the test above fails as expected, however it still leaves the\n+# cloned tree in there and adds a config entry to .git/config. This is because\n+# no cleanup is done by cmd_add in git-submodule.sh when \"git\n+# submodule--helper config\" fails to add a new config setting.\n+#\n+# If we added the following commands to the test above:\n+#\n+# rm -rf .git/modules/new_submodule &&\n+# git reset HEAD new_submodule &&\n+# rm -rf new_submodule\n+#\n+# then the repository would be in a clean state and the test below would pass.\n+#\n+# Maybe cmd_add should do the cleanup from above itself when failing to add\n+# a submodule.\n+test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '\n+\t(cd super &&\n+\t\tgit submodule init\n+\t)\n+'\n+\n+test_done\n-- \n2.18.0\n\n"},{"id":"354250","messageId":"20180802134634.10300-8-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 07/12] submodule: use 'submodule--helper config --stage' to stage .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:29Z","receivedAt":"2018-08-02T14:27:34Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Use 'git submodule--helper config --stage' in git-submodule.sh to remove\nthe last place where the .gitmodules file is mentioned explicitly by its\nfile path.\n\nSigned-off-by: Antonio Ospite \n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ff258e2e8c..f20c37d92d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -289,7 +289,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \tthen\n \t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n \tfi &&\n-\tgit add --force .gitmodules ||\n+\tgit submodule--helper config --stage ||\n \tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n \n \t# NEEDSWORK: In a multi-working-tree world, this needs to be\n-- \n2.18.0\n\n"},{"id":"354251","messageId":"20180802134634.10300-9-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:30Z","receivedAt":"2018-08-02T14:27:36Z","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 means to get rid of .gitmodules anyways, let's completely\nremove it from the current branch, to actually start afresh in the new\nscenario.\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 b4b74dbe29..af91ba92ff 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":"354252","messageId":"20180802134634.10300-13-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:34Z","receivedAt":"2018-08-02T14:27:41Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"In particular this makes it possible to really clean things up when\nremoving the last submodule with \"git rm\".\n\nThe rationale is that if git creates .gitmodules when adding the first\nsubmodule it should also remove it when removing the last submodule.\n\nSigned-off-by: Antonio Ospite \n---\n submodule.c | 10 ++++++++--\n t/t3600-rm.sh | 32 ++++++++++++++++----------------\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 3 files changed, 35 insertions(+), 18 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2af09068d7..8cfa82231d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -145,8 +145,14 @@ int remove_path_from_gitmodules(const char *path)\n \n void stage_updated_gitmodules(struct index_state *istate)\n {\n-\tif (add_file_to_index(istate, GITMODULES_FILE, 0))\n-\t\tdie(_(\"staging updated .gitmodules failed\"));\n+\tif (is_empty_file(GITMODULES_FILE)) {\n+\t\tif (remove_path(GITMODULES_FILE) < 0)\n+\t\t\tdie(_(\"removing .empty gitmodules failed\"));\n+\t\tremove_file_from_index(&the_index, GITMODULES_FILE);\n+\t} else {\n+\t\tif (add_file_to_index(istate, GITMODULES_FILE, 0))\n+\t\t\tdie(_(\"staging updated .gitmodules failed\"));\n+\t}\n }\n \n /* TODO: remove this function, use repo_submodule_init instead. */\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex b8fbdefcdc..bac2054f51 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -296,7 +296,7 @@ test_expect_success 'rm removes empty submodules from work tree' '\n \tgit rm submod &&\n \ttest ! -e submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect.both_deleted actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n@@ -307,7 +307,7 @@ test_expect_success 'rm removes removed submodule from index and .gitmodules' '\n \trm -rf submod &&\n \tgit rm submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect.both_deleted actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n@@ -318,7 +318,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit rm submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect.both_deleted actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n@@ -329,7 +329,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '\n \tgit rm submod/ &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm fails when given a file with a trailing /' '\n@@ -352,7 +352,7 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect.both_deleted actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n@@ -433,7 +433,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a populated submodule with untracked files fails unless forced' '\n@@ -448,7 +448,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'setup submodule conflict' '\n@@ -485,7 +485,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '\n \tgit rm submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a conflicted populated submodule with different HEAD fails unless forced' '\n@@ -502,7 +502,7 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect.both_deleted actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n@@ -521,7 +521,7 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect.both_deleted actual &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n@@ -540,7 +540,7 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a conflicted populated submodule with a .git directory fails even when forced' '\n@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \tgit rm submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '\n@@ -618,7 +618,7 @@ test_expect_success 'rm recursively removes work tree of unmodified submodules'\n \tgit rm submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a populated nested submodule with different nested HEAD fails unless forced' '\n@@ -633,7 +633,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a populated nested submodule with nested modifications fails unless forced' '\n@@ -648,7 +648,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success 'rm of a populated nested submodule with nested untracked files fails unless forced' '\n@@ -663,7 +663,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect.both_deleted actual\n '\n \n test_expect_success \"rm absorbs submodule's nested .git directory\" '\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 48fd14fae6..2bb42a4c8f 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -99,6 +99,17 @@ inspect() {\n \t)\n }\n \n+test_expect_success 'removal of last submodule also removes empty .gitmodules' '\n+\t(\n+\t\tcd addtest &&\n+\t\ttest ! -d .git/modules &&\n+\t\tgit submodule add -q \"$submodurl\" first_submod &&\n+\t\ttest -e .gitmodules &&\n+\t\tgit rm -f first_submod &&\n+\t\ttest ! -e .gitmodules\n+\t)\n+'\n+\n test_expect_success 'submodule add' '\n \techo \"refs/heads/master\" >expect &&\n \t>empty &&\n-- \n2.18.0\n\n"},{"id":"354253","messageId":"20180802134634.10300-12-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:33Z","receivedAt":"2018-08-02T14:27:42Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"The is_empty_file() function can be generally useful, move it to dir.c\nand make it public.\n\nSigned-off-by: Antonio Ospite \n---\n builtin/am.c | 15 ---------------\n dir.c | 16 ++++++++++++++++\n dir.h | 1 +\n 3 files changed, 17 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6273ea5195..0c04312a50 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -33,21 +33,6 @@\n #include \"string-list.h\"\n #include \"packfile.h\"\n \n-/**\n- * Returns 1 if the file is empty or does not exist, 0 otherwise.\n- */\n-static int is_empty_file(const char *filename)\n-{\n-\tstruct stat st;\n-\n-\tif (stat(filename, &st) < 0) {\n-\t\tif (errno == ENOENT)\n-\t\t\treturn 1;\n-\t\tdie_errno(_(\"could not stat %s\"), filename);\n-\t}\n-\n-\treturn !st.st_size;\n-}\n \n /**\n * Returns the length of the first line of msg.\ndiff --git a/dir.c b/dir.c\nindex 21e6f2520a..c1f7b90256 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)\n \treturn ret;\n }\n \n+/**\n+ * Returns 1 if the file is empty or does not exist, 0 otherwise.\n+ */\n+int is_empty_file(const char *filename)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(filename, &st) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 1;\n+\t\tdie_errno(_(\"could not stat %s\"), filename);\n+\t}\n+\n+\treturn !st.st_size;\n+}\n+\n static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n {\n \tDIR *dir;\ndiff --git a/dir.h b/dir.h\nindex f5fdedbab2..e45aa3f459 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -281,6 +281,7 @@ static inline int is_dot_or_dotdot(const char *name)\n }\n \n extern int is_empty_dir(const char *dir);\n+extern int is_empty_file(const char *filename);\n \n extern void setup_standard_excludes(struct dir_struct *dir);\n \n-- \n2.18.0\n\n"},{"id":"354254","messageId":"20180802134634.10300-4-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:25Z","receivedAt":"2018-08-02T14:27:43Z","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\nSince those commits are not needed anymore remove both of them.\n\nThe modified line is in the last test of the file, so this does not\nchange the current behavior, it only affects future tests.\n\nSigned-off-by: Antonio Ospite \n---\n\nNote that test_when_finished is not used here, both to keep the current style\nand also because it does not work in sub-shells.\n\n t/t7411-submodule-config.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 0bde5850ac..248da0bc4f 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\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\t# Remove both the commits which add errors to .gitmodules,\n+\t\t# the one from this test and the one from a previous test.\n+\t\tgit reset --hard HEAD~2\n \t)\n '\n \n-- \n2.18.0\n\n"},{"id":"354255","messageId":"20180802134634.10300-10-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 09/12] submodule: support reading .gitmodules even when it's not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:31Z","receivedAt":"2018-08-02T14:27:45Z","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\nSigned-off-by: Antonio Ospite \n---\n builtin/submodule--helper.c | 17 ++++++++++++++++-\n cache.h | 1 +\n submodule-config.c | 16 ++++++++++++++--\n 3 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c388f4ee6f..616d5de0a9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2089,8 +2089,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 still makes it possible to create a brand new\n+\t\t * .gitmodules when neither GITMODULES_FILE nor\n+\t\t * 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 \treturn 0;\n }\ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..8f75cafbb6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -424,6 +424,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/submodule-config.c b/submodule-config.c\nindex 702d40dd6b..cf08264220 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-- \n2.18.0\n\n"},{"id":"354256","messageId":"20180802134634.10300-3-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 02/12] submodule: factor out a config_set_in_gitmodules_file_gently function","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:24Z","receivedAt":"2018-08-02T14:27:49Z","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 6f6f5f9960..702d40dd6b 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 6fec3caadd..074e74a01c 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 extern int print_config_from_gitmodules(const char *key);\n+extern 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 2a6381864e..2af09068d7 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":"354257","messageId":"20180802134634.10300-1-ao2@ao2.it","threadId":"49010","inReplyTo":null,"subject":"[RFC PATCH v2 00/12] Make submodules work if .gitmodules is not checked out","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:22Z","receivedAt":"2018-08-02T14:27:51Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Hi,\n\nthis is a second version of the RFC from\nhttps://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n\nPlease refer to the cover letter of the previous series for the\nmotivation and background of the changes.\n\nPatches from 01 to 08 should be rather uncontroversial, as they do not\nintroduce any change of behavior but just prepare the ground for patches\n09 and 10.\n\nThe last two patches, 11 and 12 are not strictly related to the topic,\nit's just an idea I came up with while reading the code for the series;\nI could send them separately if you think they are valid, or drop them.\n\nThe behavior of accessing HEAD:.gitmodules have been analyzed with the\nfollowing test matrix:\n\nPredicates:\n G = .gitmodules exists\n H = HEAD:.gitmodules exists\n\nOperations:\n read = git submodule--helper config name\n write = git submodule--helper config name value\n stage = git submodule--helper config --stage\n\n!G and !H:\n - read: nop\n - write: set value to .gitmodules (we are creating a brand new\n .gitmodules)\n - stage: exit with error, staging requires .gitmodules (like git\n add/rm)\n\nG and !H:\n - read: get value from from .gitmodules\n - write: set value to .gitmodules\n - stage: stage .gitmodules\n\n!G and H:\n - read: get the value from HEAD:.gitmodules\n - write: exit with error, setting a value requires .gitmodules\n - stage: exit with error, staging requires .gitmodules\n \nG and H:\n - read: get value from from .gitmodules\n - write: set value to .gitmodules\n - stage: stage .gitmodules\n\n\nNote that \"git rm\" and \"git mv\" will keep working when !G just as\nt3600-rm.sh and t7001-mv.sh mandate, I am not sure if they should fail\nif \"!G and H\".\n\nIn my previous attempt I also tried to check for the skip-worktree bit,\nbut I am not sure if this is needed.\n\nMy intuition suggested the following behavior to cover the case of\na manually crafted .gitmodules which could be committed and override the\nhidden one:\n\nS = .gitmodules has the skip-worktree bit set\n\nS:\n - read: get value from .gitmodules if G or from HEAD:.gitmodules if H\n - write: exit with error, .gitmodules is not supposed to be changed\n - stage: exit with error, .gitmodules is not supposed to be changed\n\nHowever it looks like git gives the user quite some freedom to add,\nstage, commit, remove files even when they have the skip-worktree bit\nset (which was a little surprising to me TBH) so I am leaving this out\nfor now.\n\n\nChanges since v1:\n\n * Added a helper to print config values from the gitmodules\n configuration, the code is now in submoudle-config.c instead of\n buildtin/submodule--helper.c\n\n * Renamed config_gitmodules_set to\n config_set_in_gitmodules_file_gently and put it in\n submodule-config.c instead of submodule.c\n\n The naming follows the style of git config functions, and also takes\n into account Stefan Beller's comment about the final purpose of the\n function: we only write gitmodules config to the actual .gitmodules\n file, so we may as well reflect that in the function name,\n\n * Started using \"working tree\" instead of \"work tree\" as the former is\n the official term for the set of checked out files.\n\n * Start referring to HEAD as the \"current branch\" instead of \"the\n index\", the former should be more accurate.\n\n * Renamed GITMODULES_BLOB to GITMODULES_HEAD because GITMODULES_BLOB\n was added in commit 59e7b080 (\"fsck: detect gitmodules files\",\n 2018-05-02)\n\n * Do not check for the skip-worktree bit on .gitmodules anymore, just\n check for file existence when appropriate.\n\n * Renamed t7415-submodule-sparse-gitmodules.sh to\n t7416-submodule-sparse-gitmodules.sh because t7415 was taken by\n t7415-submodule-names.sh\n\n * Made \"git mv\" and \"git rm\" work again when .gitmodules does not\n exist.\n\n * Removed checks about the skip-worktree bit.\n\n\nThanks,\n Antonio\n\nAntonio Ospite (12):\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 submodule--helper: add a '--stage' option to the 'config' sub command\n submodule: use 'submodule--helper config --stage' to stage .gitmodules\n t7506: cleanup .gitmodules properly before setting up new scenario\n submodule: support reading .gitmodules even when it's not checked out\n t7416: add new test about HEAD:.gitmodules and not existing\n .gitmodules\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 builtin/am.c | 15 ----\n builtin/submodule--helper.c | 82 ++++++++++++++++++\n cache.h | 1 +\n dir.c | 16 ++++\n dir.h | 1 +\n git-submodule.sh | 10 +--\n submodule-config.c | 53 +++++++++++-\n submodule-config.h | 3 +\n submodule.c | 20 +++--\n t/t3600-rm.sh | 32 +++----\n t/t7400-submodule-basic.sh | 11 +++\n t/t7411-submodule-config.sh | 65 +++++++++++++-\n t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++\n t/t7506-status-submodule.sh | 3 +-\n 14 files changed, 375 insertions(+), 49 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":"354258","messageId":"20180802134634.10300-6-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 05/12] submodule: use the 'submodule--helper config' command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:27Z","receivedAt":"2018-08-02T14:27:53Z","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\nNote that there are other instances of \"git config -f .gitmodules\" in test\nfiles, but I am not touching those for now.\n\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":"354259","messageId":"20180802134634.10300-5-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:26Z","receivedAt":"2018-08-02T14:27:55Z","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\nNote that the tests follow the predominant style in the file: subshell and cd\nright at the start of the sub-shell opening.\n\n builtin/submodule--helper.c | 17 +++++++++++++++++\n t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a3c4564c6c..14f0845d30 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2029,6 +2029,22 @@ 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+\tif (argc < 2 || argc > 3)\n+\t\tdie(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\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+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2057,6 +2073,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 248da0bc4f..bbca4b421d 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -141,4 +141,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":"354260","messageId":"20180802134634.10300-7-ao2@ao2.it","threadId":"49010","inReplyTo":"20180802134634.10300-1-ao2@ao2.it","subject":"[RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-02T13:46:28Z","receivedAt":"2018-08-02T14:27:57Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Add a --stage option to the 'submodule--helper config' command so that\nthe .gitmodules file can be staged without referring to it explicitly by\nits file path.\n\nIn practice the config will still be written to .gitmodules, there are\nno plans to change the file path, but having this level of indirection\nmakes it possible to perform additional checks before staging the file.\n\nNote that \"git submodule--helper config --stage\" will fail when\n.gitmodules does not exist, this is to be consistent with how \"git add\"\nand \"git rm\" work: if the file to be staged does not exist they will\ncomplain.\n\nSigned-off-by: Antonio Ospite \n---\n builtin/submodule--helper.c | 54 +++++++++++++++++++++++++++++++++++--\n t/t7411-submodule-config.sh | 35 ++++++++++++++++++++++++\n 2 files changed, 87 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 14f0845d30..c388f4ee6f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -5,6 +5,7 @@\n #include \"parse-options.h\"\n #include \"quote.h\"\n #include \"pathspec.h\"\n+#include \"lockfile.h\"\n #include \"dir.h\"\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n@@ -2031,8 +2032,57 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\n \n static int module_config(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc < 2 || argc > 3)\n-\t\tdie(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n+\tint stage_gitmodules = 0;\n+\n+\tstruct option module_config_options[] = {\n+\t\tOPT_BOOL(0, \"stage\", &stage_gitmodules,\n+\t\t\t N_(\"stage the .gitmodules file content\")),\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 --stage\"),\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 ((stage_gitmodules && argc > 1) ||\n+\t (!stage_gitmodules && (argc < 2 || argc > 3)))\n+\t\tusage_with_options(git_submodule_helper_usage, module_config_options);\n+\n+\t/*\n+\t * Equivalent to \"git add --force .gitmodules\".\n+\t *\n+\t * Silently override already staged changes, to support consecutive\n+\t * invocations of \"git submodule add\".\n+\t */\n+\tif (stage_gitmodules) {\n+\t\tstatic struct lock_file lock_file;\n+\n+\t\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n+\n+\t\tif (read_cache() < 0)\n+\t\t\tdie(_(\"index file corrupt\"));\n+\n+\t\t/*\n+\t\t * Do not use is_staging_gitmodules_ok() here to make it\n+\t\t * possible to stage multiple changes before committing them,\n+\t\t * in particular this covers the case of consecutive calls of\n+\t\t * \"git submodule add\".\n+\t\t */\n+\t\tif (!file_exists(GITMODULES_FILE))\n+\t\t\tdie(_(\"%s does not exists\"), GITMODULES_FILE);\n+\n+\t\tstage_updated_gitmodules(&the_index);\n+\n+\t\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\t\tdie(_(\"Unable to write new index file\"));\n+\n+\t\treturn 0;\n+\t}\n \n \t/* Equivalent to ACTION_GET in builtin/config.c */\n \tif (argc == 2)\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex bbca4b421d..8ff6ed444a 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -167,4 +167,39 @@ test_expect_success 'overwriting unstaged submodules config with \"submodule--hel\n \t)\n '\n \n+test_expect_success 'staging submodules config with \"submodule--helper config\"' '\n+\t(cd super &&\n+\t\t: >expected &&\n+\t\tgit diff --name-only --cached >actual &&\n+\t\ttest_cmp expected actual &&\n+\t\tgit submodule--helper config --stage &&\n+\t\techo \".gitmodules\" >expected &&\n+\t\tgit diff --name-only --cached >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n+test_expect_success 'overwriting staged submodules config with \"submodule--helper config\"' '\n+\t(cd super &&\n+\t\techo \"even_newer_url\" >expected &&\n+\t\tgit submodule--helper config submodule.submodule.url \"even_newer_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 're-staging overridden submodule config with \"submodule--helper config\"' '\n+\t(cd super &&\n+\t\techo \".gitmodules\" >expected &&\n+\t\tgit diff --name-only --cached >actual &&\n+\t\ttest_cmp expected actual &&\n+\t\techo \"bogus config\" >.gitmodules &&\n+\t\tgit submodule--helper config --stage &&\n+\t\tgit diff --name-only --cached >actual &&\n+\t\ttest_cmp expected actual &&\n+\t\tgit reset HEAD .gitmodules &&\n+\t\tgit checkout .gitmodules\n+\t)\n+'\n+\n test_done\n-- \n2.18.0\n\n"},{"id":"354274","messageId":"20180802164042.28026-1-szeder.dev@gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-4-ao2@ao2.it","subject":"Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-02T16:40:42Z","receivedAt":"2018-08-02T16:41:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> 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> Since those commits are not needed anymore remove both of them.\n> \n> The modified line is in the last test of the file, so this does not\n> change the current behavior, it only affects future tests.\n> \n> Signed-off-by: Antonio Ospite \n> ---\n> \n> Note that test_when_finished is not used here, both to keep the current style\n> and also because it does not work in sub-shells.\n\nThat's true, but I think that this:\n\n test_when_finished git -C super reset --hard HEAD~2\n\nat the very beginning of the test should work.\n\n> t/t7411-submodule-config.sh | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> index 0bde5850ac..248da0bc4f 100755\n> --- a/t/t7411-submodule-config.sh\n> +++ b/t/t7411-submodule-config.sh\n> @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\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\t# Remove both the commits which add errors to .gitmodules,\n> +\t\t# the one from this test and the one from a previous test.\n> +\t\tgit reset --hard HEAD~2\n> \t)\n> '\n> \n> -- \n> 2.18.0\n> \n> \n"},{"id":"354287","messageId":"CAGZ79ka-rbOiwWrwbW2vJW9ZOgSas23LkfmzBRLfK=g_cW=MhA@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-2-ao2@ao2.it","subject":"Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T18:05:02Z","receivedAt":"2018-08-02T18:05:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\n> This will be used to print values just like \"git config -f .gitmodules\"\n> would.\n>\n> Signed-off-by: Antonio Ospite \n> ---\n> submodule-config.c | 25 +++++++++++++++++++++++++\n> submodule-config.h | 2 ++\n> 2 files changed, 27 insertions(+)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 2a7259ba8b..6f6f5f9960 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -682,6 +682,31 @@ void submodule_free(struct repository *r)\n> submodule_cache_clear(r->submodule_cache);\n> }\n>\n> +static int config_print_callback(const char *key_, const char *value_, void *cb_data)\n> +{\n> + char *key = cb_data;\n> +\n> + if (!strcmp(key, key_))\n> + printf(\"%s\\n\", value_);\n> +\n> + return 0;\n> +}\n> +\n> +int print_config_from_gitmodules(const char *key)\n> +{\n> + int ret;\n> + char *store_key;\n> +\n> + ret = git_config_parse_key(key, &store_key, NULL);\n> + if (ret < 0)\n> + return CONFIG_INVALID_KEY;\n> +\n> + config_from_gitmodules(config_print_callback, the_repository, store_key);\n> +\n> + free(store_key);\n> + return 0;\n> +}\n> +\n> struct fetch_config {\n> int *max_children;\n> int *recurse_submodules;\n> diff --git a/submodule-config.h b/submodule-config.h\n> index dc7278eea4..6fec3caadd 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> +extern int print_config_from_gitmodules(const char *key);\n\nThe only real pushback for this patch I'd have is lack of documentation\nin public functions, though this is pretty self explanatory; so I'd be fine\nfor lacking the docs here.\n\nIn case a resend is needed, please drop the extern keyword here.\n\nThanks,\nStefan\n"},{"id":"354288","messageId":"CAGZ79kYn8hCV3z7wLhG+-nOooie-PwHN7n1SrEJdka8bSHc+3w@mail.gmail.com","threadId":"49010","inReplyTo":"20180802164042.28026-1-szeder.dev@gmail.com","subject":"Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T18:15:03Z","receivedAt":"2018-08-02T18:15:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor wrote:\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> > Since those commits are not needed anymore remove both of them.\n> >\n> > The modified line is in the last test of the file, so this does not\n> > change the current behavior, it only affects future tests.\n> >\n> > Signed-off-by: Antonio Ospite \n> > ---\n> >\n> > Note that test_when_finished is not used here, both to keep the current style\n> > and also because it does not work in sub-shells.\n>\n> That's true, but I think that this:\n>\n> test_when_finished git -C super reset --hard HEAD~2\n>\n> at the very beginning of the test should work.\n\nYeah that is a better way to do it.\nEven better would be to have 2 of these for both tests 5 and 8,\nsuch that each of them could be skipped individually and any following\ntests still work fine.\n\n> > t/t7411-submodule-config.sh | 4 +++-\n> > 1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> > index 0bde5850ac..248da0bc4f 100755\n> > --- a/t/t7411-submodule-config.sh\n> > +++ b/t/t7411-submodule-config.sh\n> > @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> > HEAD submodule \\\n> > >actual &&\n> > test_cmp expect_error actual &&\n> > - git reset --hard HEAD^\n> > + # Remove both the commits which add errors to .gitmodules,\n> > + # the one from this test and the one from a previous test.\n> > + git reset --hard HEAD~2\n\nI am a bit hesitant to removing the commits though, as it is expected to have\npotentially broken history and submodules still working.\n\nThe config --unset already fixes the gitmodules file,\nso I think we can rather do\n\n git commit -a -m 'now the .gitmodules file is fixed at HEAD \\\n but has a messy history'\n\nBut as I have only read up to here, not knowing what the future tests will\nbring this is all speculation at this point.\n"},{"id":"354293","messageId":"xmqqh8kc1ucd.fsf@gitster-ct.c.googlers.com","threadId":"49010","inReplyTo":"20180802164042.28026-1-szeder.dev@gmail.com","subject":"Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T18:44:50Z","receivedAt":"2018-08-02T18:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor 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>> Since those commits are not needed anymore remove both of them.\n>> \n>> The modified line is in the last test of the file, so this does not\n>> change the current behavior, it only affects future tests.\n>> \n>> Signed-off-by: Antonio Ospite \n>> ---\n>> \n>> Note that test_when_finished is not used here, both to keep the current style\n>> and also because it does not work in sub-shells.\n>\n> That's true, but I think that this:\n>\n> test_when_finished git -C super reset --hard HEAD~2\n>\n> at the very beginning of the test should work.\n\nAssuming that the operations to create these two extra commits\nalways succeed, yes, that would be a more robust and preferrable\noption.\n\nI don't know if that assumption hold true offhand, though. Don't we\nhave a more stable point to anchor that going-back-to commit to?\n"},{"id":"354296","messageId":"CAGZ79kY78rgb8VpRn-A2moucy16z1TsDGdz2cjJzVOxsiKiiYw@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-5-ao2@ao2.it","subject":"Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T18:47:30Z","receivedAt":"2018-08-02T18:47:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\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 \n> ---\n>\n> Note that the tests follow the predominant style in the file: subshell and cd\n> right at the start of the sub-shell opening.\n>\n> builtin/submodule--helper.c | 17 +++++++++++++++++\n> t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 43 insertions(+)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index a3c4564c6c..14f0845d30 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2029,6 +2029,22 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\n> return 0;\n> }\n>\n> +static int module_config(int argc, const char **argv, const char *prefix)\n> +{\n> + if (argc < 2 || argc > 3)\n> + die(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n> +\n> + /* Equivalent to ACTION_GET in builtin/config.c */\n> + if (argc == 2)\n> + return print_config_from_gitmodules(argv[1]);\n> +\n> + /* Equivalent to ACTION_SET in builtin/config.c */\n> + if (argc == 3)\n> + return config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n> +\n> + return 0;\n\nTechnically we cannot reach this point here?\nMaybe it would be more defensive to\n\n BUG(\"How did we get here?\");\n\nor at least return something !=0 ?\n\n>\n> +test_expect_success 'reading submodules config with \"submodule--helper config\"' '\n\nI shortly wondered if it would make sense to put these tests at the\nbeginning of either\nthis or a new file, as the functionality is rather fundamental. (I\nnever thought about\nit, but actually it is better to go from common basic to more exotic\ntests as you scroll\ndown the file), but this place is ok, if you choose to do so.\n\nThanks,\nStefan\n"},{"id":"354297","messageId":"xmqqd0v01trp.fsf@gitster-ct.c.googlers.com","threadId":"49010","inReplyTo":"20180802134634.10300-7-ao2@ao2.it","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T18:57:14Z","receivedAt":"2018-08-02T18:57:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite writes:\n\n> Add a --stage option to the 'submodule--helper config' command so that\n> the .gitmodules file can be staged without referring to it explicitly by\n> its file path.\n\nSorry, but I have no clue what the above is trying to say.\n\nThe original 's--h config []' is quite understandable. It\nis like \"git config []\", i.e. either get the current value\nfor the name, or set a new value to the name.\n\nWhat does this 's--h config --stage' that does not take any option\ndo? \"git add .gitmodules\"? Something else? In what meaning is the\nword \"stage\" used? Is it used as a verb \"to stage\"?\n\nIn a series that lets us use the data in the .gitmodules file without\nmaterializing the file in the working tree, I would have expected\nthat you would want an option to specify which .gitmodules among\n(1) the one in the working tree (i.e. the only option we currently\nhave), (2) the one in the index, and (3) the one in the HEAD, and\nwhen I saw the title, I would have expected that\n\n\tgit submodule--helper config --stage name\n\nmay be a way to read from the .gitmodules in the index to find the\nvalue for name (however, we we follow the option naming convention\nin gitcli.txt, it should be called --cached, I would think).\n\n> In practice the config will still be written to .gitmodules, there are\n> no plans to change the file path, but having this level of indirection\n> makes it possible to perform additional checks before staging the file.\n\nAgain, a claim without explanation or justification.\n\nIf you are planning to something like\n\n - prepare trial contents in .gitmodules.new file\n - do whatever \"additional checks\" on .gitmodules.new\n - add the contents to it to the index as a new .gitmodules blob\n\nThen you do not need such an option. \"submodule--helper\" is purely\na helper for scripts, and not for human consumption, so scripts can\njust hash-object the blob contents out and update-index --cacheinfo\nto register the blob at any location of choice.\n\nBut as I said, this step is way under-explained, so my guess above\nmay not match what you really wanted to do.\n"},{"id":"354299","messageId":"CAGZ79kbwK1Kf2Oov9W9+UBBJpiOfDgZjZakmGP3iqxPPAhAUPQ@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-6-ao2@ao2.it","subject":"Re: [RFC PATCH v2 05/12] submodule: use the 'submodule--helper config' command","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T18:59:09Z","receivedAt":"2018-08-02T18:59:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\n> Use the 'submodule--helper config' command in git-submodules.sh to avoid\n> referring explicitly to .gitmodules by the hardcoded file path.\n\nccol! This is the corner stone to the work of the previous patches. Nicely done!\n\n> This makes it possible to access the submodules configuration in a more\n> controlled way.\n>\n> Signed-off-by: Antonio Ospite \n> ---\n>\n> Note that there are other instances of \"git config -f .gitmodules\" in test\n> files, but I am not touching those for now.\n\nI just checked and this converts all of git-submodule.sh which would\nhave been my expectation as that is the real product.\n\nThe tests are fine to use \"git config -f .gitmodules\" as there we want to\nsetup a specific environment to be tested? So I would think even future\npatches in this series will not touch test files for such a conversion\nas in here.\n\nStefan\n"},{"id":"354301","messageId":"CAGZ79kb=KgSSVXPCpa0wSyohpgywBk8c9P7Lq5BNJcdZ1exUyw@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-9-ao2@ao2.it","subject":"Re: [RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T19:11:15Z","receivedAt":"2018-08-02T19:11:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\n> In t/t7506-status-submodule.sh at some point a new scenario is set up to\n> test different things, in particular new submodules are added which are\n> meant to completely replace the previous ones.\n>\n> However before calling the \"git submodule add\" commands for the new\n> layout, the .gitmodules file is removed only from the working tree still\n> leaving the previous content in current branch.\n>\n> This can break if, in the future, \"git submodule add\" starts\n> differentiating 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>\n> Since the test means to get rid of .gitmodules anyways, let's completely\n> remove it from the current branch, to actually start afresh in the new\n> scenario.\n>\n> This is more future-proof and does not break current tests.\n\nMakes sense.\n\nThanks,\nStefan\n\n>\n> Signed-off-by: Antonio Ospite \n> ---\n> t/t7506-status-submodule.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\n> index b4b74dbe29..af91ba92ff 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> (\n> cd super &&\n> git clean -dfx &&\n> - rm .gitmodules &&\n> + git rm .gitmodules &&\n> + git commit -m \"remove .gitmodules\" &&\n> git submodule add -f ./sub1 &&\n> git submodule add -f ./sub2 &&\n> git submodule add -f ./sub1 sub3 &&\n> --\n> 2.18.0\n>\n"},{"id":"354308","messageId":"20180802192032.GA28962@sigill.intra.peff.net","threadId":"49010","inReplyTo":"CAGZ79kY78rgb8VpRn-A2moucy16z1TsDGdz2cjJzVOxsiKiiYw@mail.gmail.com","subject":"Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T19:20:33Z","receivedAt":"2018-08-02T19:20:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:\n\n> > +static int module_config(int argc, const char **argv, const char *prefix)\n> > +{\n> > + if (argc < 2 || argc > 3)\n> > + die(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n> > +\n> > + /* Equivalent to ACTION_GET in builtin/config.c */\n> > + if (argc == 2)\n> > + return print_config_from_gitmodules(argv[1]);\n> > +\n> > + /* Equivalent to ACTION_SET in builtin/config.c */\n> > + if (argc == 3)\n> > + return config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n> > +\n> > + return 0;\n> \n> Technically we cannot reach this point here?\n> Maybe it would be more defensive to\n> \n> BUG(\"How did we get here?\");\n> \n> or at least return something !=0 ?\n\nWhen I find myself reaching for a BUG(), it is often a good time to see\nif we can restructure the code so that the logic more naturally falls\nout. Here the issue is that the first if conditional repeats the \"else\"\nfor the other two. So I think we could just write:\n\n if (argc == 2)\n\treturn ...\n if (argc == 3)\n\treturn ...\n\n die(\"need 1 or 2 arguments\");\n\n-Peff\n"},{"id":"354317","messageId":"CAGZ79kZABbh6bP1TD8gkx_051yo-=bLJq1XvJz9xUKdBKAPssg@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-10-ao2@ao2.it","subject":"Re: [RFC PATCH v2 09/12] submodule: support reading .gitmodules even when it's not checked out","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T20:27:00Z","receivedAt":"2018-08-02T20:27:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\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\nThe reading part shows how nice our internal config system is, once\neverything is put in place.\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"},{"id":"354321","messageId":"CAGZ79kbQ0DsAXZrvpp3_2CrMU6Jburf6UdjTxNSd72JqQCczWQ@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-11-ao2@ao2.it","subject":"Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T20:43:05Z","receivedAt":"2018-08-02T20:43:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\n> git submodule commands can now access .gitmodules from the current\n> branch even when it's not in the working tree, add some tests for that\n> scenario.\n>\n> Signed-off-by: Antonio Ospite \n> ---\n>\n> For the test files I used the most used style in other tests, Stefan suggested\n> to avoid subshells and use \"git -C\" but subshells make the test look cleaner\n> IMHO.\n\nOh well. Let's not dive into a style argument, so let me focus on the tests.\n\nIMHO it would be nice if (at least partially) these tests are in the same patch\nthat added the reading from HEAD:.gitmodules (although I like short\npatches, too).\n\n>\n> t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++\n> 1 file changed, 112 insertions(+)\n> create mode 100755 t/t7416-submodule-sparse-gitmodules.sh\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..3c7a53316b\n> --- /dev/null\n> +++ b/t/t7416-submodule-sparse-gitmodules.sh\n> @@ -0,0 +1,112 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (C) 2018 Antonio Ospite \n> +#\n> +\n> +test_description=' Test reading/writing .gitmodules is 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, but the same scenario can be set up\n> +also by committing .gitmodules and then just removing it from the filesystem.\n> +\n> +NOTE: \"git mv\" and \"git rm\" are still supposed to work even without\n> +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.\n\n\"supposed to work\" != \"tested that it works\" ?\nI am not sure what the NOTE wants to tell me? (Should I review those\ntests to double check them now? or do we just want to tell future readers\nof this test there are other tangent tests to this?)\n\n\n> +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '\n> + (cd super &&\n> + git submodule init\n> + )\n> +'\n> +\n> +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '\n> + (cd super &&\n> + git submodule summary\n> + )\n> +'\n> +\n> +test_expect_success 'updating submodule when the gitmodules config is not checked out' '\n> + (cd submodule &&\n> + echo file2 >file2 &&\n> + git add file2 &&\n> + git commit -m \"add file2 to submodule\"\n> + ) &&\n> + (cd super &&\n> + git submodule update\n> + )\n> +'\n> +\n> +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '\n> + (cd super &&\n> + test_must_fail git submodule add ../new_submodule\n> + )\n> +'\n> +\n> +# \"git add\" in the test above fails as expected, however it still leaves the\n> +# cloned tree in there and adds a config entry to .git/config. This is because\n> +# no cleanup is done by cmd_add in git-submodule.sh when \"git\n> +# submodule--helper config\" fails to add a new config setting.\n> +#\n> +# If we added the following commands to the test above:\n> +#\n> +# rm -rf .git/modules/new_submodule &&\n> +# git reset HEAD new_submodule &&\n> +# rm -rf new_submodule\n\nAlternatively we could check for the existence of .gitmodules\nbefore starting all these things?\n\nI think it is okay to not clean up if we check all \"regular\" or rather expected\nthings such as a non-writable .gitmodules file before actually doing it.\n(This is similar to 'checkout' that walks the whole tree and checks if the\ncheckout is possible given the dirtyness of the tree, to either abort early\nor pull through completely. In catastrophic problems such as a full disk\nwe'd still die in the middle of work)\n\n> +#\n> +# then the repository would be in a clean state and the test below would pass.\n> +#\n> +# Maybe cmd_add should do the cleanup from above itself when failing to add\n> +# a submodule.\n> +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '\n\nSo this comment and test is about explaining why we can fail mid way through,\nwhich we could not before unless we had the catastrophic event.\n\nI think we should check for a \"writable\" .gitmodules file at the beginning,\nwhich is if (G || (!G && !H)) [using the notation from the cover letter]?\n\n> + (cd super &&\n> + git submodule init\n> + )\n> +'\n> +\n> +test_done\n> --\n> 2.18.0\n>\n"},{"id":"354323","messageId":"CAGZ79kYpsYbg8nkE+8hTPM=Bg_3-p3EzoRfJ1y7f+Dz-3hT-HA@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-12-ao2@ao2.it","subject":"Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T20:50:55Z","receivedAt":"2018-08-02T20:51:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\n> The is_empty_file() function can be generally useful, move it to dir.c\n> and make it public.\n>\n> Signed-off-by: Antonio Ospite \n\nMakes sense,\n\nThanks,\nStefan\n\n> +++ b/dir.c\n> @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)\n> return ret;\n> }\n>\n> +/**\n> + * Returns 1 if the file is empty or does not exist, 0 otherwise.\n> + */\n\nPlease move the comment to the header instead.\n/* possibly as a oneliner ? */\n"},{"id":"354327","messageId":"CAGZ79kaDK9nuUm_hqAZKMHydiaBDhDkM_y1UFwpRHoGjCQoq4w@mail.gmail.com","threadId":"49010","inReplyTo":"20180802134634.10300-13-ao2@ao2.it","subject":"Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-02T21:15:54Z","receivedAt":"2018-08-02T21:16:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n>\n> In particular this makes it possible to really clean things up when\n> removing the last submodule with \"git rm\".\n\nThis sentence is a continuation of the subject line, and I had to reread\nit to follow along.\n\n>\n> The rationale is that if git creates .gitmodules when adding the first\n> submodule it should also remove it when removing the last submodule.\n\nI agree with this sentiment. It seems slightly odd to me to have this tied\nin the same patch series that changes .gitmodules reading behavior\nas I could think of this feature as orthogonal to what this series achieved\nup to patch 10.\n\n> - test_cmp expect actual &&\n> + test_cmp expect.both_deleted actual &&\n\nThis seems to be the re-occuring pattern in t3600, and given that\nwe have\n\n cat >expect <expect.both_deleted< test_expect_success \"rm absorbs submodule's nested .git directory\" '\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 48fd14fae6..2bb42a4c8f 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -99,6 +99,17 @@ inspect() {\n> )\n> }\n>\n> +test_expect_success 'removal of last submodule also removes empty .gitmodules' '\n> + (\n> + cd addtest &&\n> + test ! -d .git/modules &&\n\ntest_path_is_missing ?\n\n> + git submodule add -q \"$submodurl\" first_submod &&\n> + test -e .gitmodules &&\n\ntest_path_is_file\n\n> + git rm -f first_submod &&\n\nDo we need to force it here?\n\n> + test ! -e .gitmodules\n\ntest_path_is_missing\n\nThanks for this series!\nOverall it was a pleasant read, though I had some comments.\n\nThanks,\nStefan\n"},{"id":"354364","messageId":"20180803104932.e22c45fdc007e5884944696b@ao2.it","threadId":"49010","inReplyTo":"CAGZ79kYpsYbg8nkE+8hTPM=Bg_3-p3EzoRfJ1y7f+Dz-3hT-HA@mail.gmail.com","subject":"Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-03T08:49:32Z","receivedAt":"2018-08-03T08:49:35Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 2 Aug 2018 13:50:55 -0700\nStefan Beller wrote:\n\n> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n> >\n> > The is_empty_file() function can be generally useful, move it to dir.c\n> > and make it public.\n> >\n> > Signed-off-by: Antonio Ospite \n> \n> Makes sense,\n> \n> Thanks,\n> Stefan\n> \n> > +++ b/dir.c\n> > @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)\n> > return ret;\n> > }\n> >\n> > +/**\n> > + * Returns 1 if the file is empty or does not exist, 0 otherwise.\n> > + */\n> \n> Please move the comment to the header instead.\n> /* possibly as a oneliner ? */\n\nWill do.\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":"354365","messageId":"20180803105717.7a023507f96651a4769c1fd7@ao2.it","threadId":"49010","inReplyTo":"CAGZ79kaDK9nuUm_hqAZKMHydiaBDhDkM_y1UFwpRHoGjCQoq4w@mail.gmail.com","subject":"Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-03T08:57:17Z","receivedAt":"2018-08-03T08:57:22Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 2 Aug 2018 14:15:54 -0700\nStefan Beller wrote:\n\n> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n> >\n> > In particular this makes it possible to really clean things up when\n> > removing the last submodule with \"git rm\".\n> \n> This sentence is a continuation of the subject line, and I had to reread\n> it to follow along.\n>\n\nOK, I'll fix that.\n\n> >\n> > The rationale is that if git creates .gitmodules when adding the first\n> > submodule it should also remove it when removing the last submodule.\n> \n> I agree with this sentiment. It seems slightly odd to me to have this tied\n> in the same patch series that changes .gitmodules reading behavior\n> as I could think of this feature as orthogonal to what this series achieved\n> up to patch 10.\n>\n\nI will send this as a separate series, I briefly mentioned this\npossibility in the cover letter.\n\n> > - test_cmp expect actual &&\n> > + test_cmp expect.both_deleted actual &&\n> \n> This seems to be the re-occuring pattern in t3600, and given that\n> we have\n> \n> cat >expect < M .gitmodules\n> D submod\n> EOF\n> cat >expect.both_deleted< D .gitmodules\n> D submod\n> EOF\n> \n> with no other writing of expect in the range, this seems to be correct.\n> Maybe worth testing that we do not delete a .gitmodules file if we have\n> more than one submodule? (But I would expect this to be covered implicitly\n> somewhere in the test suite. If so that would be worth mentioning in the\n> commit message instead of writing a test -- just looking quickly we\n> do have \" git rm --cached submodule2\" in t7406 which might be sufficient?)\n>\n\nI think I will remove the new test in t7400 as the changes to t3600\nshould cover that case already.\n\nI will check about the case of more than one submodule and I'll add a\ncomment to the commit message.\n\n[...]\n> Thanks for this series!\n> Overall it was a pleasant read, though I had some comments.\n> \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":"354370","messageId":"20180803122138.ca4be250bf1ad3b3730866e3@ao2.it","threadId":"49010","inReplyTo":"20180802192032.GA28962@sigill.intra.peff.net","subject":"Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-03T10:21:38Z","receivedAt":"2018-08-03T10:21:42Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 2 Aug 2018 15:20:33 -0400\nJeff King wrote:\n\n> On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:\n> \n> > > +static int module_config(int argc, const char **argv, const char *prefix)\n> > > +{\n> > > + if (argc < 2 || argc > 3)\n> > > + die(\"submodule--helper config takes 1 or 2 arguments: name [value]\");\n> > > +\n> > > + /* Equivalent to ACTION_GET in builtin/config.c */\n> > > + if (argc == 2)\n> > > + return print_config_from_gitmodules(argv[1]);\n> > > +\n> > > + /* Equivalent to ACTION_SET in builtin/config.c */\n> > > + if (argc == 3)\n> > > + return config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n> > > +\n> > > + return 0;\n> > \n> > Technically we cannot reach this point here?\n> > Maybe it would be more defensive to\n> > \n> > BUG(\"How did we get here?\");\n> > \n> > or at least return something !=0 ?\n> \n> When I find myself reaching for a BUG(), it is often a good time to see\n> if we can restructure the code so that the logic more naturally falls\n> out. Here the issue is that the first if conditional repeats the \"else\"\n> for the other two. So I think we could just write:\n> \n> if (argc == 2)\n> \treturn ...\n> if (argc == 3)\n> \treturn ...\n> \n> die(\"need 1 or 2 arguments\");\n> \n\nHi Jeff,\n\nI like that, I'll see how this plays out after patch 06 which adds\nanother option, and decide whether to use this style; validating\narguments beforehand may still look a little cleaner.\n\nThanks for the comment.\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":"354372","messageId":"20180803130334.3979eedb0733c609815e9fd5@ao2.it","threadId":"49010","inReplyTo":"xmqqd0v01trp.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-03T11:03:34Z","receivedAt":"2018-08-03T11:03:38Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 02 Aug 2018 11:57:14 -0700\nJunio C Hamano wrote:\n\n> Antonio Ospite writes:\n> \n> > Add a --stage option to the 'submodule--helper config' command so that\n> > the .gitmodules file can be staged without referring to it explicitly by\n> > its file path.\n> \n> Sorry, but I have no clue what the above is trying to say.\n>\n\nYou got me :), this was one of the changes I was having the more doubts\nabout, basically about the user interface, and it must have showed in\nthe commit message.\n\n> The original 's--h config []' is quite understandable. It\n> is like \"git config []\", i.e. either get the current value\n> for the name, or set a new value to the name.\n> \n> What does this 's--h config --stage' that does not take any option\n> do? \"git add .gitmodules\"? Something else? In what meaning is the\n> word \"stage\" used? Is it used as a verb \"to stage\"?\n>\n\n\"s--h config --stage\" is meant to replace \"git add -f .gitmodules\" as\nshown in patch 07, maybe the two patches might have been merged to give\nmore context, but I kept the changes separate to follow how things were\ndone with patches 04 and 05.\n\nThe rationale behind the change is to close the circle started with 04\nand 05 and stop referring to .gitmodules explicitly by file path in git\ncommands. The change is not strictly needed for the series, it's for\nconsistency sake.\n\nThe name \"stage\" is meant as a verb and was inspired by the \"git stage\"\ncommand and by the stage_updated_gitmodules() function, I used an\noption flag with no arguments because I saw this has been done for\nverbs before, e.g. \"git config --list\". I did not know about\n\"gitcli.txt\", sorry, I'll check it out.\n\nMaybe a better name could be \"submodule--helper config --add\"?\n\nThe commit message clearly needs improvements.\n\n> In a series that lets us use the data in the .gitmodules file without\n> materializing the file in the working tree, I would have expected\n> that you would want an option to specify which .gitmodules among\n> (1) the one in the working tree (i.e. the only option we currently\n> have), (2) the one in the index, and (3) the one in the HEAD, and\n> when I saw the title, I would have expected that\n> \n> \tgit submodule--helper config --stage name\n> \n> may be a way to read from the .gitmodules in the index to find the\n> value for name (however, we we follow the option naming convention\n> in gitcli.txt, it should be called --cached, I would think).\n>\n\nFor my use case the automatic behavior of falling back to\nHEAD:.gitmodules is enough, so I focused on that to have the\nfunctionality merged.\n\nThe option you suggest may be useful but I'd leave that as a possible\nfuture addition in case someone else needed it.\n\n> > In practice the config will still be written to .gitmodules, there are\n> > no plans to change the file path, but having this level of indirection\n> > makes it possible to perform additional checks before staging the file.\n> \n> Again, a claim without explanation or justification.\n> \n> If you are planning to something like\n> \n> - prepare trial contents in .gitmodules.new file\n> - do whatever \"additional checks\" on .gitmodules.new\n> - add the contents to it to the index as a new .gitmodules blob\n> \n> Then you do not need such an option. \"submodule--helper\" is purely\n> a helper for scripts, and not for human consumption, so scripts can\n> just hash-object the blob contents out and update-index --cacheinfo\n> to register the blob at any location of choice.\n> \n> But as I said, this step is way under-explained, so my guess above\n> may not match what you really wanted to do.\n\nAs stated, the motivation for now is just syntactical: remove hardcoded\nreferences to .gitmodules in scripts.\n\nNo new \"additional checks\" are added as of now, the commit message is\nmisleading indeed.\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":"354404","messageId":"xmqqbmajzadt.fsf@gitster-ct.c.googlers.com","threadId":"49010","inReplyTo":"20180803130334.3979eedb0733c609815e9fd5@ao2.it","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T16:24:14Z","receivedAt":"2018-08-03T16:24:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite writes:\n\n> The rationale behind the change is to close the circle started with 04\n> and 05 and stop referring to .gitmodules explicitly by file path in git\n> commands. The change is not strictly needed for the series, it's for\n> consistency sake.\n\nSorry, but I am still not quite sure what consistency you are\nreferring to.\n\nI also do not see a reason why we want to stop referring to\n.gitmodules explicitly by name. We do not hide the fact that\nin-tree .gitignore and .gitattributes files are used to hold the\nmetainformation about the project tree, saying that it is an\nimplementation detail. Is there a good reason why .gitmodules\nshould be different from these other two?\n"},{"id":"354597","messageId":"20180806125828.66fc99ef0b118e26e0b3e60d@ao2.it","threadId":"49010","inReplyTo":"xmqqbmajzadt.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-06T10:58:28Z","receivedAt":"2018-08-06T10:58:37Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Fri, 03 Aug 2018 09:24:14 -0700\nJunio C Hamano wrote:\n\n> Antonio Ospite writes:\n> \n> > The rationale behind the change is to close the circle started with 04\n> > and 05 and stop referring to .gitmodules explicitly by file path in git\n> > commands. The change is not strictly needed for the series, it's for\n> > consistency sake.\n> \n> Sorry, but I am still not quite sure what consistency you are\n> referring to.\n>\n\nHi Junio,\n\nto recap: in previous patches we removed all instances of \"git config\n-f .gitmodules\" (read/write operations), because in that case\nencapsulating access to .gitmodules would directly enable us to add\nnew functionality (i.e.: read from HEAD:.gitmodules).\n\nSo, to me, it looked more organic to remove also the last explicit\nreference to .gitmodules in git-submodule.sh (\"git add -f .gitmodules\",\nthe \"stage\" operation), even if in this case no new behavior about\nstaging is going to be added for the time being. that is: the change is\nnot strictly necessary.\n\nThe consistency I was referring to is merely in the mechanism used to\ndeal with .gitmodules: by always using \"submodule--helper config\".\n\nAs a side argument: in some previous discussion Stefan mentioned the\npossibility that, in the future, he may be interested to explore\ndifferent locations for submodules configuration, like a special ref,\nto prevent .gitmodules from being in the working tree at all, not even\nfor writing submodule configuration.\n\nHaving an opaque \"stage submodule configuration\" operation would\nprepare for that too, but as I said this is not my immediate goal.\n\n> I also do not see a reason why we want to stop referring to\n> .gitmodules explicitly by name. We do not hide the fact that\n> in-tree .gitignore and .gitattributes files are used to hold the\n> metainformation about the project tree, saying that it is an\n> implementation detail. Is there a good reason why .gitmodules\n> should be different from these other two?\n\nNot sure about that, but one difference I can see\nbetween .gitignore/.gitattributes and .gitmodules is that I got the\nimpression that editing the latter by hand is strongly discouraged, if\nthat is indeed the case a layer of indirection can make sense IMHO to\nmake the actual file path less relevant.\n\nHaving said that I can drop patches 06/07 if this can help moving things\nforward; or if I had some success in convincing you I can improve the\nuser interface (any advice?) and the commit message.\n\nThank 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":"354651","messageId":"xmqq7el3pf8z.fsf@gitster-ct.c.googlers.com","threadId":"49010","inReplyTo":"20180806125828.66fc99ef0b118e26e0b3e60d@ao2.it","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T17:38:20Z","receivedAt":"2018-08-06T17:38:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite writes:\n\n>> I also do not see a reason why we want to stop referring to\n>> .gitmodules explicitly by name. We do not hide the fact that\n>> in-tree .gitignore and .gitattributes files are used to hold the\n>> metainformation about the project tree, saying that it is an\n>> implementation detail. Is there a good reason why .gitmodules\n>> should be different from these other two?\n>\n> Not sure about that, but one difference I can see\n> between .gitignore/.gitattributes and .gitmodules is that I got the\n> impression that editing the latter by hand is strongly discouraged, if\n> that is indeed the case a layer of indirection can make sense IMHO to\n> make the actual file path less relevant.\n\nI do not think we discourage hand editing of .gitmodules more than\nothers, say .gitignore; and I do not see a sane reason to do so.\n\n\"If you commit broken .gitmodules and let another person clone it,\nsubmodules will not be checked out correctly\" is *not* a sane\nreason, as exactly the same thing can be said for incorrect checkout\nof files with broken .gitattributes.\n\nQuite honestly, I just want to get over with this minor detail that\nwon't help any scripts (after all submodule--helper is not meant to\nbe used by humans) and focus on other parts of the patch series.\n"},{"id":"354662","messageId":"CAGZ79kbXtQP+Q+xCn5f6CO5Wk_3dbHeEVgny4jGnTwwW0j_LSw@mail.gmail.com","threadId":"49010","inReplyTo":"20180806125828.66fc99ef0b118e26e0b3e60d@ao2.it","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-06T18:19:10Z","receivedAt":"2018-08-06T18:19:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> The consistency I was referring to is merely in the mechanism used to\n> deal with .gitmodules: by always using \"submodule--helper config\".\n>\n> As a side argument: in some previous discussion Stefan mentioned the\n> possibility that, in the future, he may be interested to explore\n> different locations for submodules configuration, like a special ref,\n> to prevent .gitmodules from being in the working tree at all, not even\n> for writing submodule configuration.\n>\n> Having an opaque \"stage submodule configuration\" operation would\n> prepare for that too, but as I said this is not my immediate goal.\n>\n\nThanks for demonstrating that this will be easy to achieve in the future!\n\n> Having said that I can drop patches 06/07 if this can help moving things\n> forward; or if I had some success in convincing you I can improve the\n> user interface (any advice?) and the commit message.\n>\n\nI think dropping the patches would be a good idea,\ngiven that this is not your immediate goal.\n\nThanks,\nStefan\n"},{"id":"354719","messageId":"20180807111958.1494e8cc1e376a19b54bb788@ao2.it","threadId":"49010","inReplyTo":"xmqq7el3pf8z.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-07T09:19:58Z","receivedAt":"2018-08-07T09:20:05Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Mon, 06 Aug 2018 10:38:20 -0700\nJunio C Hamano wrote:\n\n> Antonio Ospite writes:\n> \n> >> I also do not see a reason why we want to stop referring to\n> >> .gitmodules explicitly by name. We do not hide the fact that\n> >> in-tree .gitignore and .gitattributes files are used to hold the\n> >> metainformation about the project tree, saying that it is an\n> >> implementation detail. Is there a good reason why .gitmodules\n> >> should be different from these other two?\n> >\n> > Not sure about that, but one difference I can see\n> > between .gitignore/.gitattributes and .gitmodules is that I got the\n> > impression that editing the latter by hand is strongly discouraged, if\n> > that is indeed the case a layer of indirection can make sense IMHO to\n> > make the actual file path less relevant.\n> \n> I do not think we discourage hand editing of .gitmodules more than\n> others, say .gitignore; and I do not see a sane reason to do so.\n> \n> \"If you commit broken .gitmodules and let another person clone it,\n> submodules will not be checked out correctly\" is *not* a sane\n> reason, as exactly the same thing can be said for incorrect checkout\n> of files with broken .gitattributes.\n>\n\nOK, I see, maybe I got the wrong impression because\nwhile .gitignore/.gitattributes are expected to be hand edited, in my\nlimited usage of submodules I never had to edit .gitmodules manually\nsince git did it for me; I guess that does not necessarily mean it's\ndiscouraged and it may even be necessary for more advanced usages.\n\n> Quite honestly, I just want to get over with this minor detail that\n> won't help any scripts (after all submodule--helper is not meant to\n> be used by humans) and focus on other parts of the patch series.\n\nAgreed, let's drop 06 and 07 then.\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":"354974","messageId":"20180809111408.3ee7296a8aefd38ea1208547@ao2.it","threadId":"49010","inReplyTo":"CAGZ79kbQ0DsAXZrvpp3_2CrMU6Jburf6UdjTxNSd72JqQCczWQ@mail.gmail.com","subject":"Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-09T09:14:08Z","receivedAt":"2018-08-09T09:14:14Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 2 Aug 2018 13:43:05 -0700\nStefan Beller wrote:\n\n> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n> >\n> > git submodule commands can now access .gitmodules from the current\n> > branch even when it's not in the working tree, add some tests for that\n> > scenario.\n> >\n> > Signed-off-by: Antonio Ospite \n> > ---\n\n[...]\n> > +NOTE: \"git mv\" and \"git rm\" are still supposed to work even without\n> > +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.\n> \n> \"supposed to work\" != \"tested that it works\" ?\n\n\"git mv submod new_submod\" and \"git rm submod\" are actually expected to\nwork without the .gitmodules file, and there are tests about that in\nt3600-rm.sh and t7001-mv.sh:\n\nt3600-rm.sh:\n 'rm does not complain when no .gitmodules file is found'\n\nt7001-mv.sh:\n 'git mv moves a submodule with a .git directory and no .gitmodules' \n 'mv does not complain when no .gitmodules file is found'\n\n> I am not sure what the NOTE wants to tell me? (Should I review those\n> tests to double check them now? or do we just want to tell future readers\n> of this test there are other tangent tests to this?)\n>\n\nAdmittedly the NOTE is not useful without any context: during the\ndevelopment of \"submodule--helper config --stage\" I initially assumed\nthat \"git mv\" and \"git rm\" should fail if .gitmodules was not available,\nbecause these commands modify .gitmodules and I added code for that in\nstage_updated_gitmodules().\n\nBut then later I found out that my assumption was wrong and that git has\ntests to verify that these operations on submodules succeed even when\n.gitmodules does not exist, which was a little of a surprise to me.\n\nSo I removed all my code that was conflicting with git assumptions, and\nadded the NOTE. However I guess that was primarily a note to myself, and\nit should have not slipped in the public patches.\n\nI think I will remove the note, it can be confusing and does not really\nadd anything, and even less considering that \"submodule--helper config\n--stage\" is going to be dropped.\n\n[...]\n> > +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '\n> > + (cd super &&\n> > + test_must_fail git submodule add ../new_submodule\n> > + )\n> > +'\n> > +\n> > +# \"git add\" in the test above fails as expected, however it still leaves the\n> > +# cloned tree in there and adds a config entry to .git/config. This is because\n> > +# no cleanup is done by cmd_add in git-submodule.sh when \"git\n> > +# submodule--helper config\" fails to add a new config setting.\n> > +#\n> > +# If we added the following commands to the test above:\n> > +#\n> > +# rm -rf .git/modules/new_submodule &&\n> > +# git reset HEAD new_submodule &&\n> > +# rm -rf new_submodule\n> \n> Alternatively we could check for the existence of .gitmodules\n> before starting all these things?\n>\n\nYou mean in cmd_add(), before doing anything?\n\nThe following would anticipates the same check which makes \"git submodule\nadd\" fail:\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ff258e2e8c..b261175143 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -159,6 +159,11 @@ cmd_add()\n shift\n done\n\n+ if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules\n+ then\n+ die \"$(eval_gettext \"please make sure that the .gitmodules file in the current branch is checked out\")\"\n+ fi\n+\n if test -n \"$reference_path\"\n then\n is_absolute_path \"$reference_path\" ||\n\nThis refers to .gitmodules explicitly but we said that we do not care\nabout that for now, if opaque access was ever needed in the future,\nsomething like \"submodule--helper config --is-writeable\" could be added.\n\n> I think it is okay to not clean up if we check all \"regular\" or rather expected\n> things such as a non-writable .gitmodules file before actually doing it.\n> (This is similar to 'checkout' that walks the whole tree and checks if the\n> checkout is possible given the dirtyness of the tree, to either abort early\n> or pull through completely. In catastrophic problems such as a full disk\n> we'd still die in the middle of work)\n> \n> > +#\n> > +# then the repository would be in a clean state and the test below would pass.\n> > +#\n> > +# Maybe cmd_add should do the cleanup from above itself when failing to add\n> > +# a submodule.\n> > +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '\n> \n> So this comment and test is about explaining why we can fail mid way through,\n> which we could not before unless we had the catastrophic event.\n> \n> I think we should check for a \"writable\" .gitmodules file at the beginning,\n> which is if (G || (!G && !H)) [using the notation from the cover letter]?\n> \n> > + (cd super &&\n> > + git submodule init\n\nWith the change from above this last test passes.\n\nBTW the check I am using here and in the code of submodule--helper,\ncorresponds indeed to the boolean expression you mentioned, but\nsimplified and negated.\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":"354983","messageId":"20180809121732.02df3be299d42dc53dfa078d@ao2.it","threadId":"49010","inReplyTo":"CAGZ79ka-rbOiwWrwbW2vJW9ZOgSas23LkfmzBRLfK=g_cW=MhA@mail.gmail.com","subject":"Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-09T10:17:32Z","receivedAt":"2018-08-09T10:17:36Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 2 Aug 2018 11:05:02 -0700\nStefan Beller wrote:\n\n> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote:\n> >\n[...]\n> > +extern int print_config_from_gitmodules(const char *key);\n> \n> The only real pushback for this patch I'd have is lack of documentation\n> in public functions, though this is pretty self explanatory; so I'd be fine\n> for lacking the docs here.\n> \n> In case a resend is needed, please drop the extern keyword here.\n> \n\nI'll drop the extern keyword also for the public function added in\npatch 02 then.\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":"354989","messageId":"20180809155940.0505e6d6525d39068a5a6925@ao2.it","threadId":"49010","inReplyTo":"CAGZ79kYn8hCV3z7wLhG+-nOooie-PwHN7n1SrEJdka8bSHc+3w@mail.gmail.com","subject":"Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-08-09T13:59:40Z","receivedAt":"2018-08-09T13:59:45Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Thu, 2 Aug 2018 11:15:03 -0700\nStefan Beller wrote:\n\n> On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor wrote:\n> >\n[...]\n> > >\n> > > Note that test_when_finished is not used here, both to keep the current style\n> > > and also because it does not work in sub-shells.\n> >\n> > That's true, but I think that this:\n> >\n> > test_when_finished git -C super reset --hard HEAD~2\n> >\n> > at the very beginning of the test should work.\n> \n> Yeah that is a better way to do it.\n> Even better would be to have 2 of these for both tests 5 and 8,\n> such that each of them could be skipped individually and any following\n> tests still work fine.\n>\n\nTest 6 also relies on the error introduced in test 5.\n\nSo the options would be either to remove one commit at the time in\ntest 6 and 8 (with a comment in test 6 to note that the commit is from\nthe previous test), or to remove both the commits in test 8. I am going\nto go with the former, using test_when_finished.\n\n> > > t/t7411-submodule-config.sh | 4 +++-\n> > > 1 file changed, 3 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> > > index 0bde5850ac..248da0bc4f 100755\n> > > --- a/t/t7411-submodule-config.sh\n> > > +++ b/t/t7411-submodule-config.sh\n> > > @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> > > HEAD submodule \\\n> > > >actual &&\n> > > test_cmp expect_error actual &&\n> > > - git reset --hard HEAD^\n> > > + # Remove both the commits which add errors to .gitmodules,\n> > > + # the one from this test and the one from a previous test.\n> > > + git reset --hard HEAD~2\n> \n> I am a bit hesitant to removing the commits though, as it is expected to have\n> potentially broken history and submodules still working.\n>\n\nThe commits which are removed only affected .gitmoudles, no \"submodule\ninit\" nor \"submoudle update\" is ever called after they are added, so I\ndon't know what problems there could be.\n\nWould a revert be any different?\n\n> The config --unset already fixes the gitmodules file,\n> so I think we can rather do\n> \n> git commit -a -m 'now the .gitmodules file is fixed at HEAD \\\n> but has a messy history'\n> \n> But as I have only read up to here, not knowing what the future tests will\n> bring this is all speculation at this point.\n\nIIUC the \"config --unset\" is used to cause the error, not to fix it, I\nam not sure I understand this point.\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"}]}