{"thread":{"id":"27351","subject":"[PATCH v2 0/2] better handle .gitmodules merge conflicts","startedAt":"2011-05-14T16:25:28Z","lastAt":"2011-05-15T05:00:45Z","messageCount":7,"participants":["Jens Lehmann","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"167849","messageId":"4DCEACF8.50503@web.de","threadId":"27351","inReplyTo":null,"subject":"[PATCH v2 0/2] better handle .gitmodules merge conflicts","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-14T16:25:28Z","receivedAt":"2011-05-14T16:25:28Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.05.2011 08:19, schrieb Junio C Hamano:\n> Jens Lehmann writes:\n>> +static int gitmodules_is_unmerged;\n>\n> Is it too cumbersome to pass this down the callchain as an argument?\n\nI think so. Currently it influences the behavior of fetch, status,\nthe diff family and commit. Having for example gitmodules_config()\nreturn that value and then pass it on would touch a lot of code (and\nthat impact would grow even further with the recursive checkout\nseries I'm working on).\n\nSo I thought it best to have this information statically in the\nsubmodule.c file and let the call sites who need to know that call\nfunctions there to get just the information they need and hide the\ndetails in submodule.c.\n\n\nAll other issues raised have been addressed in this series.\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 | 31 +++++++++++++-\n t/t7506-status-submodule.sh | 92 +++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 117 insertions(+), 6 deletions(-)\n\n-- \n1.7.5.1.291.g080bb\n"},{"id":"167850","messageId":"4DCEAD36.6090605@web.de","threadId":"27351","inReplyTo":"4DCEACF8.50503@web.de","subject":"[PATCH v2 1/2] test that git status works with merge conflict in, .gitmodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-14T16:26:30Z","receivedAt":"2011-05-14T16:26:30Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"From: Heiko Voigt \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 \n---\n t/t7506-status-submodule.sh | 92 +++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 88 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex c8d50a6..61a5680 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,84 @@ test_expect_success 'status -a clean (empty submodule dir)' '\n \ttest_i18ngrep \"nothing to commit\" output\n '\n\n+cat >status_expect <<\\EOF\n+AA .gitmodules\n+A sub1\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 -s >../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.291.g080bb\n"},{"id":"167851","messageId":"4DCEAD52.60707@web.de","threadId":"27351","inReplyTo":"4DCEACF8.50503@web.de","subject":"[PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-14T16:26:58Z","receivedAt":"2011-05-14T16:26:58Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Commands like \"git status\", \"git diff\" and \"git fetch\" would fail when the\n.gitmodules file contained merge conflicts because the config parser would\ncall die() when hitting the conflict markers:\n\n \"fatal: bad config file line in /.gitmodules\"\n\nWhile this behavior was on the safe side, it is really unhelpful to the\nuser to have commands like status and diff fail, as these are needed to\nfind 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.\nUsers of git gui were not shown any conflicts at all when this happened.\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 \n---\n submodule.c | 31 +++++++++++++++++++++++++++++--\n t/t7506-status-submodule.sh | 6 +++---\n 2 files changed, 32 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5294cef..b6dec70 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -14,6 +14,15 @@ 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+/*\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+static int gitmodules_is_unmerged;\n\n static int add_submodule_odb(const char *path)\n {\n@@ -63,6 +72,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 +93,24 @@ 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 < 0) { /* .gitmodules not found or isn't merged */\n+\t\t\tpos = -1 - pos;\n+\t\t\tif (active_nr > pos) { /* there is a .gitmodules */\n+\t\t\t\tconst struct cache_entry *ce = active_cache[pos];\n+\t\t\t\tif (ce_namelen(ce) == 11 &&\n+\t\t\t\t !memcmp(ce->name, \".gitmodules\", 11))\n+\t\t\t\t\tgitmodules_is_unmerged = 1;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (!gitmodules_is_unmerged)\n+\t\t\tgit_config_from_file(submodule_config, gitmodules_path.buf, NULL);\n \t\tstrbuf_release(&gitmodules_path);\n \t}\n }\n@@ -434,7 +460,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 61a5680..d31b34d 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -196,7 +196,7 @@ AA .gitmodules\n A sub1\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@@ -255,7 +255,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@@ -263,7 +263,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.291.g080bb\n"},{"id":"167852","messageId":"BANLkTinvkxDYqNs3Pv0CniGtAHm5dJf0wg@mail.gmail.com","threadId":"27351","inReplyTo":"4DCEAD52.60707@web.de","subject":"Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-05-14T16:59:04Z","receivedAt":"2011-05-14T16:59:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote:\n> Commands like \"git status\", \"git diff\" and \"git fetch\" would fail when the\n> .gitmodules file contained merge conflicts because ...\n\nSide note. .gitattributes and .gitignore likely suffer the same.\nPerhaps the unmerged check in this patch can be generalized for reuse.\n-- \nDuy\n"},{"id":"167856","messageId":"4DCEBFF3.4040809@web.de","threadId":"27351","inReplyTo":"BANLkTinvkxDYqNs3Pv0CniGtAHm5dJf0wg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-05-14T17:46:27Z","receivedAt":"2011-05-14T17:46:27Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 14.05.2011 18:59, schrieb Nguyen Thai Ngoc Duy:\n> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote:\n>> Commands like \"git status\", \"git diff\" and \"git fetch\" would fail when the\n>> .gitmodules file contained merge conflicts because ...\n> \n> Side note. .gitattributes and .gitignore likely suffer the same.\n> Perhaps the unmerged check in this patch can be generalized for reuse.\n\nInteresting thought. But adding conflict markers in those files\ndoesn't break anything the way it did for .gitmodules, so this\nlooks like a non-issue.\n"},{"id":"167858","messageId":"7vfwoh5fom.fsf@alter.siamese.dyndns.org","threadId":"27351","inReplyTo":"BANLkTinvkxDYqNs3Pv0CniGtAHm5dJf0wg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-14T17:56:41Z","receivedAt":"2011-05-14T17:56:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote:\n>> Commands like \"git status\", \"git diff\" and \"git fetch\" would fail when the\n>> .gitmodules file contained merge conflicts because ...\n>\n> Side note. .gitattributes and .gitignore likely suffer the same.\n> Perhaps the unmerged check in this patch can be generalized for reuse.\n\nI am somewhat skeptical about that statement. These files are designed to\nbe one-entry-per-line so that useful information in the non-conflicted\nparts in a conflicted file (which should be majority) can still be used.\nOnly the .gitmodules is an oddball.\n"},{"id":"167880","messageId":"BANLkTim_J7gW1rW+yO74ETEjFcHhyArBRg@mail.gmail.com","threadId":"27351","inReplyTo":"7vfwoh5fom.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-05-15T05:00:45Z","receivedAt":"2011-05-15T05:00:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 15, 2011 at 12:56 AM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote:\n>>> Commands like \"git status\", \"git diff\" and \"git fetch\" would fail when the\n>>> .gitmodules file contained merge conflicts because ...\n>>\n>> Side note. .gitattributes and .gitignore likely suffer the same.\n>> Perhaps the unmerged check in this patch can be generalized for reuse.\n>\n> I am somewhat skeptical about that statement.  These files are designed to\n> be one-entry-per-line so that useful information in the non-conflicted\n> parts in a conflicted file (which should be majority) can still be used.\n> Only the .gitmodules is an oddball.\n\nTrue for .gitignore. A line in .gitattributes is more complex, it's\neasier to break the parser with conflict markers. Anyway unmerged\nentries should not be used until they're cleaned up, or can be used\nwith a big fat warning.\n-- \nDuy\n"}]}