{"thread":{"id":"27341","subject":"[RFC PATCH 0/2] better handle .gitmodules merge conflicts","startedAt":"2011-05-12T21:01:17Z","lastAt":"2011-05-13T15:01:09Z","messageCount":6,"participants":["Jens Lehmann","Junio C Hamano","Marc Branchaud"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"167760","messageId":"4DCC4A9D.3060007@web.de","threadId":"27341","inReplyTo":null,"subject":"[RFC PATCH 0/2] better handle .gitmodules merge conflicts","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-12T21:01:17Z","receivedAt":"2011-05-12T21:01:17Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"This series contains a test to reproduce and a first attempt to fix the\nproblem that \"git diff\", \"git status\" and \"git fetch\" die early when the\n.gitmodules file contains merge conflict markers. As I am not aware of any\nbug reports yet it looks like that either doesn't happen very often in the\nwild (\"git diff\" parses the .gitmodules file since 1.7.3, but even as\nheavy submodule users we only hit this once just recently) or the users\nencountering this problem just know what to do: resolve the conflict and\ncarry on.\n\nBut that is no excuse to behave so unfriendly, especially as this can\nhappen in a completely normal workflow, when e.g. two users are adding\ndifferent submodules in separate branches and they get merged. So this\nRFC patch is my first attempt to avoid those commands dying.\n\nIn a second step a merge helper capable of merging inifiles would make\nsense. This should be enabled for the .gitmodules file by default and\navoid most of the merge conflicts, e.g. when two users add different\nsubmodules in separate branches. But that is a different patch ...\n\n\nHeiko Voigt (1):\n  test that git status works with merge conflict in .gitmodules\n\nJens Lehmann (1):\n  Submodules: Don't parse .gitmodules when it contains merge conflicts\n\n submodule.c                 |   23 +++++++++-\n t/t7506-status-submodule.sh |  100 +++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 117 insertions(+), 6 deletions(-)\n\n-- \n1.7.5.1.251.ga75dd\n"},{"id":"167761","messageId":"4DCC4AE0.6020902@web.de","threadId":"27341","inReplyTo":"4DCC4A9D.3060007@web.de","subject":"[RFC PATCH 1/2] test that git status works with merge conflict in .gitmodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-12T21:02:24Z","receivedAt":"2011-05-12T21:02:24Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"From: Heiko Voigt <hvoigt@hvoigt.net>\n\nFor example: Two users independently adding a submodule will result in a\nmerge conflict in .gitmodules. Since configuration of the status and\ndiff machinery depends on the file being parseable they currently\nfail to produce useable output in case .gitmodules is marked with a\nmerge conflict.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n t/t7506-status-submodule.sh |  100 +++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 96 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex c8d50a6..3b1806c 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -4,17 +4,21 @@ test_description='git status for submodule'\n\n . ./test-lib.sh\n\n-test_expect_success 'setup' '\n-\ttest_create_repo sub &&\n+test_create_repo_with_commit () {\n+\ttest_create_repo $1 &&\n \t(\n-\t\tcd sub &&\n+\t\tcd $1 &&\n \t\t: >bar &&\n \t\tgit add bar &&\n \t\tgit commit -m \" Add bar\" &&\n \t\t: >foo &&\n \t\tgit add foo &&\n \t\tgit commit -m \" Add foo\"\n-\t) &&\n+\t)\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_create_repo_with_commit sub &&\n \techo output > .gitignore &&\n \tgit add sub .gitignore &&\n \tgit commit -m \"Add submodule sub\"\n@@ -187,4 +191,92 @@ test_expect_success 'status -a clean (empty submodule dir)' '\n \ttest_i18ngrep \"nothing to commit\" output\n '\n\n+cat > status_expect << EOF\n+# On branch merge_conflict_gitmodules\n+# Changes to be committed:\n+#\n+#\tnew file:   sub1\n+#\n+# Unmerged paths:\n+#   (use \"git add/rm <file>...\" as appropriate to mark resolution)\n+#\n+#\tboth added:         .gitmodules\n+#\n+EOF\n+\n+test_expect_failure 'status with merge conflict in .gitmodules' '\n+\tgit clone . super &&\n+\ttest_create_repo_with_commit sub1 &&\n+\ttest_tick &&\n+\ttest_create_repo_with_commit sub2 &&\n+\t(\n+\t\tcd super &&\n+\t\tprev=$(git rev-parse HEAD) &&\n+\t\tgit checkout -b add_sub1 &&\n+\t\tgit submodule add ../sub1 &&\n+\t\tgit commit -m \"add sub1\" &&\n+\t\tgit checkout -b add_sub2 $prev &&\n+\t\tgit submodule add ../sub2 &&\n+\t\tgit commit -m \"add sub2\" &&\n+\t\tgit checkout -b merge_conflict_gitmodules &&\n+\t\ttest_must_fail git merge add_sub1 &&\n+\t\tgit status > ../status_actual 2>&1\n+\t) &&\n+\ttest_cmp status_actual status_expect\n+'\n+\n+sha1_merge_sub1=$(cd sub1 && git rev-parse HEAD)\n+sha1_merge_sub2=$(cd sub2 && git rev-parse HEAD)\n+short_sha1_merge_sub1=$(cd sub1 && git rev-parse --short HEAD)\n+short_sha1_merge_sub2=$(cd sub2 && git rev-parse --short HEAD)\n+cat > diff_expect << EOF\n+diff --cc .gitmodules\n+index badaa4c,44f999a..0000000\n+--- a/.gitmodules\n++++ b/.gitmodules\n+@@@ -1,3 -1,3 +1,9 @@@\n+++<<<<<<< HEAD\n+ +[submodule \"sub2\"]\n+ +\tpath = sub2\n+ +\turl = ../sub2\n+++=======\n++ [submodule \"sub1\"]\n++ \tpath = sub1\n++ \turl = ../sub1\n+++>>>>>>> add_sub1\n+EOF\n+\n+cat > diff_submodule_expect << EOF\n+diff --cc .gitmodules\n+index badaa4c,44f999a..0000000\n+--- a/.gitmodules\n++++ b/.gitmodules\n+@@@ -1,3 -1,3 +1,9 @@@\n+++<<<<<<< HEAD\n+ +[submodule \"sub2\"]\n+ +\tpath = sub2\n+ +\turl = ../sub2\n+++=======\n++ [submodule \"sub1\"]\n++ \tpath = sub1\n++ \turl = ../sub1\n+++>>>>>>> add_sub1\n+EOF\n+\n+test_expect_failure 'diff with merge conflict in .gitmodules' '\n+\t(\n+\t\tcd super &&\n+\t\tgit diff > ../diff_actual 2>&1\n+\t) &&\n+\ttest_cmp diff_actual diff_expect\n+'\n+\n+test_expect_failure 'diff --submodule with merge conflict in .gitmodules' '\n+\t(\n+\t\tcd super &&\n+\t\tgit diff --submodule > ../diff_submodule_actual 2>&1\n+\t) &&\n+\ttest_cmp diff_submodule_actual diff_submodule_expect\n+'\n+\n test_done\n-- \n1.7.5.1.251.ga75dd\n"},{"id":"167762","messageId":"4DCC4B05.9070205@web.de","threadId":"27341","inReplyTo":"4DCC4A9D.3060007@web.de","subject":"[RFC PATCH 2/2] Submodules: Don't parse .gitmodules when it contains merge conflicts","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-12T21:03:01Z","receivedAt":"2011-05-12T21:03:01Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Until now commands like \"git status\", \"git diff\" and \"git fetch\" would\nfail when the .gitmodules file contained merge conflicts because the\nconfig parser would call die() when hitting the conflict markers:\n\n    \"fatal: bad config file line <n> in <path>/.gitmodules\"\n\nWhile this was behavior was on the safe side, it is really unhelpful to\nthe user to have commands like status and diff fail, as these are needed\nto find out what's going on. And the error message is only mildly helpful,\nas it points to the right file but doesn't mention that it is unmerged.\n\nImprove the situation by checking if the index records .gitmodules as\nunmerged. When that is the case we can't make any assumptions about the\nconfiguration to be found there after the merge conflict is resolved by\nthe user, so assume that all recursion is disabled unless .git/config or\nthe global config say otherwise.\n\nAs soon as the merge conflict is resolved and the .gitmodules file has\nbeen staged subsequent commands again honor any configuration done there.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n submodule.c                 |   23 +++++++++++++++++++++--\n t/t7506-status-submodule.sh |    6 +++---\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5294cef..cdf844c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -14,6 +14,13 @@ static struct string_list config_fetch_recurse_submodules_for_name;\n static struct string_list config_ignore_for_name;\n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static struct string_list changed_submodule_paths;\n+/* The following flag is set if the .gitmodules file is unmerged. We then\n+ * disable recursion for all submodules where .git/config doesn't have a\n+ * matching config entry because we can't guess what might be configured in\n+ * .gitmodules unless the user resolves the conflict. When a command line\n+ * option is given (which always overrides configuration) this flag will be\n+ * ignored. */\n+static int gitmodules_is_unmerged;\n\n static int add_submodule_odb(const char *path)\n {\n@@ -63,6 +70,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\tignore_option = unsorted_string_list_lookup(&config_ignore_for_name, path_option->util);\n \t\tif (ignore_option)\n \t\t\thandle_ignore_submodules_arg(diffopt, ignore_option->util);\n+\t\telse if (gitmodules_is_unmerged)\n+\t\t\tDIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);\n \t}\n }\n\n@@ -82,9 +91,18 @@ void gitmodules_config(void)\n \tconst char *work_tree = get_git_work_tree();\n \tif (work_tree) {\n \t\tstruct strbuf gitmodules_path = STRBUF_INIT;\n+\t\tint pos;\n \t\tstrbuf_addstr(&gitmodules_path, work_tree);\n \t\tstrbuf_addstr(&gitmodules_path, \"/.gitmodules\");\n-\t\tgit_config_from_file(submodule_config, gitmodules_path.buf, NULL);\n+\t\tif (read_cache() < 0)\n+\t\t\tdie(\"index file corrupt\");\n+\t\tpos = cache_name_pos(\".gitmodules\", 11);\n+\t\tif (pos >= -1) {\n+\t\t\t/* We have a clean, untracked or missing .gitmodules, try to parse it */\n+\t\t\tgit_config_from_file(submodule_config, gitmodules_path.buf, NULL);\n+\t\t} else {\n+\t\t\tgitmodules_is_unmerged = 1;\n+\t\t}\n \t\tstrbuf_release(&gitmodules_path);\n \t}\n }\n@@ -434,7 +452,8 @@ int fetch_populated_submodules(int num_options, const char **options,\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t} else {\n-\t\t\t\tif (config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF)\n+\t\t\t\tif ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) ||\n+\t\t\t\t    gitmodules_is_unmerged)\n \t\t\t\t\tcontinue;\n \t\t\t\tif (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 3b1806c..b31b64e 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -204,7 +204,7 @@ cat > status_expect << EOF\n #\n EOF\n\n-test_expect_failure 'status with merge conflict in .gitmodules' '\n+test_expect_success 'status with merge conflict in .gitmodules' '\n \tgit clone . super &&\n \ttest_create_repo_with_commit sub1 &&\n \ttest_tick &&\n@@ -263,7 +263,7 @@ index badaa4c,44f999a..0000000\n ++>>>>>>> add_sub1\n EOF\n\n-test_expect_failure 'diff with merge conflict in .gitmodules' '\n+test_expect_success 'diff with merge conflict in .gitmodules' '\n \t(\n \t\tcd super &&\n \t\tgit diff > ../diff_actual 2>&1\n@@ -271,7 +271,7 @@ test_expect_failure 'diff with merge conflict in .gitmodules' '\n \ttest_cmp diff_actual diff_expect\n '\n\n-test_expect_failure 'diff --submodule with merge conflict in .gitmodules' '\n+test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \t(\n \t\tcd super &&\n \t\tgit diff --submodule > ../diff_submodule_actual 2>&1\n-- \n1.7.5.1.251.ga75dd\n"},{"id":"167767","messageId":"7v1v03b19e.fsf@alter.siamese.dyndns.org","threadId":"27341","inReplyTo":"4DCC4AE0.6020902@web.de","subject":"Re: [RFC PATCH 1/2] test that git status works with merge conflict in .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-13T05:47:09Z","receivedAt":"2011-05-13T05:47:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> From: Heiko Voigt <hvoigt@hvoigt.net>\n>\n> For example: Two users independently adding a submodule will result in a\n> merge conflict in .gitmodules. Since configuration of the status and\n> diff machinery depends on the file being parseable they currently\n> fail to produce useable output in case .gitmodules is marked with a\n> merge conflict.\n>\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n\nThe basic idea (both patches) seems sound.\n\n> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\n> index c8d50a6..3b1806c 100755\n> --- a/t/t7506-status-submodule.sh\n> +++ b/t/t7506-status-submodule.sh\n> @@ -4,17 +4,21 @@ test_description='git status for submodule'\n>\n>  . ./test-lib.sh\n>\n> -test_expect_success 'setup' '\n> -\ttest_create_repo sub &&\n> +test_create_repo_with_commit () {\n> +\ttest_create_repo $1 &&\n>  \t(\n> -\t\tcd sub &&\n> +\t\tcd $1 &&\n\nMissing dq, i.e. \"$1\" (two instances).\n\n> +cat > status_expect << EOF\n\n\tcat >status_expect <<\\EOF\n\nUnless you actively mean the here-text has variables that need to be\nsubstituted, always quote the end-of-here-text delimiter.  That would\nreduce the mental burden of people who have to read the script, as they\ndo not have to look for a potential substitution that does not exist.\n\n> +# On branch merge_conflict_gitmodules\n> +# Changes to be committed:\n> +#\n> +#\tnew file:   sub1\n> +#\n> +# Unmerged paths:\n> +#   (use \"git add/rm <file>...\" as appropriate to mark resolution)\n> +#\n> +#\tboth added:         .gitmodules\n> +#\n> +EOF\n\nIs it absolutely necessary to run this test by comparing the output of\nhuman readable version of \"git status\" output?  It makes it impossible to\ntest under gettext-poison.  Can you use \"git status -s\" output instead?\n\n> ...\n> +test_expect_failure 'diff --submodule with merge conflict in .gitmodules' '\n> +\t(\n> +\t\tcd super &&\n> +\t\tgit diff --submodule > ../diff_submodule_actual 2>&1\n> +\t) &&\n> +\ttest_cmp diff_submodule_actual diff_submodule_expect\n> +'\n\nYou have too many \">\" redirect followed by an unnecessary SP. There are\nexisting examples in the test, please do not make things worse.\n"},{"id":"167768","messageId":"7vmxir9l7s.fsf@alter.siamese.dyndns.org","threadId":"27341","inReplyTo":"4DCC4B05.9070205@web.de","subject":"Re: [RFC PATCH 2/2] Submodules: Don't parse .gitmodules when it contains merge conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-13T06:19:03Z","receivedAt":"2011-05-13T06:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Until now commands like \"git status\", \"git diff\" and \"git fetch\" would\n\ns/Until now c/C/;\n\n> fail when the .gitmodules file contained merge conflicts because the\n> config parser would call die() when hitting the conflict markers:\n>\n>     \"fatal: bad config file line <n> in <path>/.gitmodules\"\n>\n> While this was behavior was on the safe side, it is really unhelpful to\n\nwas--was\n\n> diff --git a/submodule.c b/submodule.c\n> index 5294cef..cdf844c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -14,6 +14,13 @@ static struct string_list config_fetch_recurse_submodules_for_name;\n>  static struct string_list config_ignore_for_name;\n>  static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n>  static struct string_list changed_submodule_paths;\n> +/* The following flag is set if the .gitmodules file is unmerged. We then\n> + * disable recursion for all submodules where .git/config doesn't have a\n> + * matching config entry because we can't guess what might be configured in\n> + * .gitmodules unless the user resolves the conflict. When a command line\n> + * option is given (which always overrides configuration) this flag will be\n> + * ignored. */\n\n/*\n * We write our multi-line\n * comments like\n * this.\n */\n\n> +static int gitmodules_is_unmerged;\n\nIs it too cumbersome to pass this down the callchain as an argument?\n\n> +\t\tif (read_cache() < 0)\n> +\t\t\tdie(\"index file corrupt\");\n> +\t\tpos = cache_name_pos(\".gitmodules\", 11);\n> +\t\tif (pos >= -1) {\n> +\t\t\t/* We have a clean, untracked or missing .gitmodules, try to parse it */\n> +\t\t\tgit_config_from_file(submodule_config, gitmodules_path.buf, NULL);\n> +\t\t} else {\n> +\t\t\tgitmodules_is_unmerged = 1;\n> +\t\t}\n\nI do not think this is correct.\n\nIf pos is zero or positive, you know you found \".gitmodules\" that is\nmerged. If it is -1, that means \".gitmodules\" at stage #0 would be\ninserted at the beginning of the index, but that may perhaps be because\nthe first cache entry in the index may be an unmerged \".gitmodules\", no?\n\n\tpos = cache_name_pos(\".gitmodules\", 11);\n\tif (0 <= pos)\n\t\t; /* .gitmodules found and is merged */\n\telse {\n        \tpos = -1 - pos;\n\t\tif (active_nr <= pos)\n\t        \t; /* .gitmodules does not exist */\n\t        ce = active_cache[pos];\n\t        if (ce_namelen(ce) == 11 &&\n                    !memcmp(ce->name, \".gitmodules\", 11))\n\t\t\t; /* .gitmodules unmerged */\n\t\telse\n\t\t\t; /* there is no .gitmodules */\n\t}\n"},{"id":"167806","messageId":"4DCD47B5.8050505@xiplink.com","threadId":"27341","inReplyTo":"4DCC4A9D.3060007@web.de","subject":"Re: [RFC PATCH 0/2] better handle .gitmodules merge conflicts","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-05-13T15:01:09Z","receivedAt":"2011-05-13T15:01:09Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-05-12 05:01 PM, Jens Lehmann wrote:\n> This series contains a test to reproduce and a first attempt to fix the\n> problem that \"git diff\", \"git status\" and \"git fetch\" die early when the\n> .gitmodules file contains merge conflict markers. As I am not aware of any\n> bug reports yet it looks like that either doesn't happen very often in the\n> wild (\"git diff\" parses the .gitmodules file since 1.7.3, but even as\n> heavy submodule users we only hit this once just recently) or the users\n> encountering this problem just know what to do: resolve the conflict and\n> carry on.\n\nI fell in the latter camp when I ran into this.\n\nI like to examine conflicts with \"git gui\" but IIRC it doesn't display any\nconflicting files when there's a conflict in .gitmodules (it won't even\ndisplay conflicts in other files, presumably because some plumbing failed\nearly on the .gitmodules conflict).  That was quite a \"WTF?\" moment...\n\n> But that is no excuse to behave so unfriendly, especially as this can\n> happen in a completely normal workflow, when e.g. two users are adding\n> different submodules in separate branches and they get merged. So this\n> RFC patch is my first attempt to avoid those commands dying.\n\nYup -- thanks for addressing this!\n\n\t\tM.\n"}]}