[PATCH v2 0/2] better handle .gitmodules merge conflicts

[PATCH v2 0/2] better handle .gitmodules merge conflicts

[Jens Lehmann]

Am 13.05.2011 08:19, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> 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

[PATCH v2 1/2] test that git status works with merge conflict in, .gitmodules

[Jens Lehmann]

From: Heiko Voigt <hvoigt@hvoigt.net>

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 <hvoigt@hvoigt.net>
---
 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

[PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts

[Jens Lehmann]

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 <n> in <path>/.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 <Jens.Lehmann@web.de>
---
 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

Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts

[Nguyen Thai Ngoc Duy]

On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann <Jens.Lehmann@web.de> 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

Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts

[Jens Lehmann]

Am 14.05.2011 18:59, schrieb Nguyen Thai Ngoc Duy:
> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann <Jens.Lehmann@web.de> 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.

Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann <Jens.Lehmann@web.de> 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.

Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts

[Nguyen Thai Ngoc Duy]

On Sun, May 15, 2011 at 12:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Sat, May 14, 2011 at 11:26 PM, Jens Lehmann <Jens.Lehmann@web.de> 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