# [PATCH v2 0/2] better handle .gitmodules merge conflicts 7 messages from 2011-05-14 to 2011-05-15. Participants: Jens Lehmann, Nguyen Thai Ngoc Duy, Junio C Hamano. Thread: https://gitlist.dev/t/27351 ## Jens Lehmann, 2011-05-14 16:25 Subject: [PATCH v2 0/2] better handle .gitmodules merge conflicts Message-ID: <4DCEACF8.50503@web.de> URL: https://gitlist.dev/e/4DCEACF8.50503%40web.de ``` Am 13.05.2011 08:19, schrieb Junio C Hamano: > Jens Lehmann writes: >> +static int gitmodules_is_unmerged; > > Is it too cumbersome to pass this down the callchain as an argument? I think so. Currently it influences the behavior of fetch, status, the diff family and commit. Having for example gitmodules_config() return that value and then pass it on would touch a lot of code (and that impact would grow even further with the recursive checkout series I'm working on). So I thought it best to have this information statically in the submodule.c file and let the call sites who need to know that call functions there to get just the information they need and hide the details in submodule.c. All other issues raised have been addressed in this series. Heiko Voigt (1): test that git status works with merge conflict in .gitmodules Jens Lehmann (1): Submodules: Don't parse .gitmodules when it contains merge conflicts submodule.c | 31 +++++++++++++- t/t7506-status-submodule.sh | 92 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 117 insertions(+), 6 deletions(-) -- 1.7.5.1.291.g080bb ``` ## Jens Lehmann, 2011-05-14 16:26 Subject: [PATCH v2 1/2] test that git status works with merge conflict in, .gitmodules Message-ID: <4DCEAD36.6090605@web.de> URL: https://gitlist.dev/e/4DCEAD36.6090605%40web.de In-Reply-To: <4DCEACF8.50503@web.de> ``` From: Heiko Voigt For example: Two users independently adding a submodule will result in a merge conflict in .gitmodules. Since configuration of the status and diff machinery depends on the file being parseable they currently fail to produce useable output in case .gitmodules is marked with a merge conflict. Signed-off-by: Heiko Voigt --- t/t7506-status-submodule.sh | 92 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 88 insertions(+), 4 deletions(-) diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index c8d50a6..61a5680 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -4,17 +4,21 @@ test_description='git status for submodule' . ./test-lib.sh -test_expect_success 'setup' ' - test_create_repo sub && +test_create_repo_with_commit () { + test_create_repo "$1" && ( - cd sub && + cd "$1" && : >bar && git add bar && git commit -m " Add bar" && : >foo && git add foo && git commit -m " Add foo" - ) && + ) +} + +test_expect_success 'setup' ' + test_create_repo_with_commit sub && echo output > .gitignore && git add sub .gitignore && git commit -m "Add submodule sub" @@ -187,4 +191,84 @@ test_expect_success 'status -a clean (empty submodule dir)' ' test_i18ngrep "nothing to commit" output ' +cat >status_expect <<\EOF +AA .gitmodules +A sub1 +EOF + +test_expect_failure 'status with merge conflict in .gitmodules' ' + git clone . super && + test_create_repo_with_commit sub1 && + test_tick && + test_create_repo_with_commit sub2 && + ( + cd super && + prev=$(git rev-parse HEAD) && + git checkout -b add_sub1 && + git submodule add ../sub1 && + git commit -m "add sub1" && + git checkout -b add_sub2 $prev && + git submodule add ../sub2 && + git commit -m "add sub2" && + git checkout -b merge_conflict_gitmodules && + test_must_fail git merge add_sub1 && + git status -s >../status_actual 2>&1 + ) && + test_cmp status_actual status_expect +' + +sha1_merge_sub1=$(cd sub1 && git rev-parse HEAD) +sha1_merge_sub2=$(cd sub2 && git rev-parse HEAD) +short_sha1_merge_sub1=$(cd sub1 && git rev-parse --short HEAD) +short_sha1_merge_sub2=$(cd sub2 && git rev-parse --short HEAD) +cat >diff_expect <<\EOF +diff --cc .gitmodules +index badaa4c,44f999a..0000000 +--- a/.gitmodules ++++ b/.gitmodules +@@@ -1,3 -1,3 +1,9 @@@ +++<<<<<<< HEAD + +[submodule "sub2"] + + path = sub2 + + url = ../sub2 +++======= ++ [submodule "sub1"] ++ path = sub1 ++ url = ../sub1 +++>>>>>>> add_sub1 +EOF + +cat >diff_submodule_expect <<\EOF +diff --cc .gitmodules +index badaa4c,44f999a..0000000 +--- a/.gitmodules ++++ b/.gitmodules +@@@ -1,3 -1,3 +1,9 @@@ +++<<<<<<< HEAD + +[submodule "sub2"] + + path = sub2 + + url = ../sub2 +++======= ++ [submodule "sub1"] ++ path = sub1 ++ url = ../sub1 +++>>>>>>> add_sub1 +EOF + +test_expect_failure 'diff with merge conflict in .gitmodules' ' + ( + cd super && + git diff >../diff_actual 2>&1 + ) && + test_cmp diff_actual diff_expect +' + +test_expect_failure 'diff --submodule with merge conflict in .gitmodules' ' + ( + cd super && + git diff --submodule >../diff_submodule_actual 2>&1 + ) && + test_cmp diff_submodule_actual diff_submodule_expect +' + test_done -- 1.7.5.1.291.g080bb ``` ## Jens Lehmann, 2011-05-14 16:26 Subject: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts Message-ID: <4DCEAD52.60707@web.de> URL: https://gitlist.dev/e/4DCEAD52.60707%40web.de In-Reply-To: <4DCEACF8.50503@web.de> ``` Commands like "git status", "git diff" and "git fetch" would fail when the .gitmodules file contained merge conflicts because the config parser would call die() when hitting the conflict markers: "fatal: bad config file line in /.gitmodules" While this behavior was on the safe side, it is really unhelpful to the user to have commands like status and diff fail, as these are needed to find out what's going on. And the error message is only mildly helpful, as it points to the right file but doesn't mention that it is unmerged. Users of git gui were not shown any conflicts at all when this happened. Improve the situation by checking if the index records .gitmodules as unmerged. When that is the case we can't make any assumptions about the configuration to be found there after the merge conflict is resolved by the user, so assume that all recursion is disabled unless .git/config or the global config say otherwise. As soon as the merge conflict is resolved and the .gitmodules file has been staged subsequent commands again honor any configuration done there. Signed-off-by: Jens Lehmann --- submodule.c | 31 +++++++++++++++++++++++++++++-- t/t7506-status-submodule.sh | 6 +++--- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/submodule.c b/submodule.c index 5294cef..b6dec70 100644 --- a/submodule.c +++ b/submodule.c @@ -14,6 +14,15 @@ static struct string_list config_fetch_recurse_submodules_for_name; static struct string_list config_ignore_for_name; static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; static struct string_list changed_submodule_paths; +/* + * The following flag is set if the .gitmodules file is unmerged. We then + * disable recursion for all submodules where .git/config doesn't have a + * matching config entry because we can't guess what might be configured in + * .gitmodules unless the user resolves the conflict. When a command line + * option is given (which always overrides configuration) this flag will be + * ignored. + */ +static int gitmodules_is_unmerged; static int add_submodule_odb(const char *path) { @@ -63,6 +72,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt, ignore_option = unsorted_string_list_lookup(&config_ignore_for_name, path_option->util); if (ignore_option) handle_ignore_submodules_arg(diffopt, ignore_option->util); + else if (gitmodules_is_unmerged) + DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES); } } @@ -82,9 +93,24 @@ void gitmodules_config(void) const char *work_tree = get_git_work_tree(); if (work_tree) { struct strbuf gitmodules_path = STRBUF_INIT; + int pos; strbuf_addstr(&gitmodules_path, work_tree); strbuf_addstr(&gitmodules_path, "/.gitmodules"); - git_config_from_file(submodule_config, gitmodules_path.buf, NULL); + if (read_cache() < 0) + die("index file corrupt"); + pos = cache_name_pos(".gitmodules", 11); + if (pos < 0) { /* .gitmodules not found or isn't merged */ + pos = -1 - pos; + if (active_nr > pos) { /* there is a .gitmodules */ + const struct cache_entry *ce = active_cache[pos]; + if (ce_namelen(ce) == 11 && + !memcmp(ce->name, ".gitmodules", 11)) + gitmodules_is_unmerged = 1; + } + } + + if (!gitmodules_is_unmerged) + git_config_from_file(submodule_config, gitmodules_path.buf, NULL); strbuf_release(&gitmodules_path); } } @@ -434,7 +460,8 @@ int fetch_populated_submodules(int num_options, const char **options, default_argv = "on-demand"; } } else { - if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) + if ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) || + gitmodules_is_unmerged) continue; if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) { if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name)) diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index 61a5680..d31b34d 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -196,7 +196,7 @@ AA .gitmodules A sub1 EOF -test_expect_failure 'status with merge conflict in .gitmodules' ' +test_expect_success 'status with merge conflict in .gitmodules' ' git clone . super && test_create_repo_with_commit sub1 && test_tick && @@ -255,7 +255,7 @@ index badaa4c,44f999a..0000000 ++>>>>>>> add_sub1 EOF -test_expect_failure 'diff with merge conflict in .gitmodules' ' +test_expect_success 'diff with merge conflict in .gitmodules' ' ( cd super && git diff >../diff_actual 2>&1 @@ -263,7 +263,7 @@ test_expect_failure 'diff with merge conflict in .gitmodules' ' test_cmp diff_actual diff_expect ' -test_expect_failure 'diff --submodule with merge conflict in .gitmodules' ' +test_expect_success 'diff --submodule with merge conflict in .gitmodules' ' ( cd super && git diff --submodule >../diff_submodule_actual 2>&1 -- 1.7.5.1.291.g080bb ``` ## Nguyen Thai Ngoc Duy, 2011-05-14 16:59 Subject: Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts Message-ID: URL: https://gitlist.dev/e/BANLkTinvkxDYqNs3Pv0CniGtAHm5dJf0wg%40mail.gmail.com In-Reply-To: <4DCEAD52.60707@web.de> ``` On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote: > Commands like "git status", "git diff" and "git fetch" would fail when the > .gitmodules file contained merge conflicts because ... Side note. .gitattributes and .gitignore likely suffer the same. Perhaps the unmerged check in this patch can be generalized for reuse. -- Duy ``` ## Jens Lehmann, 2011-05-14 17:46 Subject: Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts Message-ID: <4DCEBFF3.4040809@web.de> URL: https://gitlist.dev/e/4DCEBFF3.4040809%40web.de In-Reply-To: ``` Am 14.05.2011 18:59, schrieb Nguyen Thai Ngoc Duy: > On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote: >> Commands like "git status", "git diff" and "git fetch" would fail when the >> .gitmodules file contained merge conflicts because ... > > Side note. .gitattributes and .gitignore likely suffer the same. > Perhaps the unmerged check in this patch can be generalized for reuse. Interesting thought. But adding conflict markers in those files doesn't break anything the way it did for .gitmodules, so this looks like a non-issue. ``` ## Junio C Hamano, 2011-05-14 17:56 Subject: Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts Message-ID: <7vfwoh5fom.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfwoh5fom.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nguyen Thai Ngoc Duy writes: > On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote: >> Commands like "git status", "git diff" and "git fetch" would fail when the >> .gitmodules file contained merge conflicts because ... > > Side note. .gitattributes and .gitignore likely suffer the same. > Perhaps the unmerged check in this patch can be generalized for reuse. I am somewhat skeptical about that statement. These files are designed to be one-entry-per-line so that useful information in the non-conflicted parts in a conflicted file (which should be majority) can still be used. Only the .gitmodules is an oddball. ``` ## Nguyen Thai Ngoc Duy, 2011-05-15 05:00 Subject: Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts Message-ID: URL: https://gitlist.dev/e/BANLkTim_J7gW1rW%2ByO74ETEjFcHhyArBRg%40mail.gmail.com In-Reply-To: <7vfwoh5fom.fsf@alter.siamese.dyndns.org> ``` On Sun, May 15, 2011 at 12:56 AM, Junio C Hamano wrote: > Nguyen Thai Ngoc Duy writes: > >> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann wrote: >>> Commands like "git status", "git diff" and "git fetch" would fail when the >>> .gitmodules file contained merge conflicts because ... >> >> Side note. .gitattributes and .gitignore likely suffer the same. >> Perhaps the unmerged check in this patch can be generalized for reuse. > > I am somewhat skeptical about that statement.  These files are designed to > be one-entry-per-line so that useful information in the non-conflicted > parts in a conflicted file (which should be majority) can still be used. > Only the .gitmodules is an oddball. True for .gitignore. A line in .gitattributes is more complex, it's easier to break the parser with conflict markers. Anyway unmerged entries should not be used until they're cleaned up, or can be used with a big fat warning. -- Duy ```