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

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

[Jens Lehmann]

This series contains a test to reproduce and a first attempt to fix the
problem that "git diff", "git status" and "git fetch" die early when the
.gitmodules file contains merge conflict markers. As I am not aware of any
bug reports yet it looks like that either doesn't happen very often in the
wild ("git diff" parses the .gitmodules file since 1.7.3, but even as
heavy submodule users we only hit this once just recently) or the users
encountering this problem just know what to do: resolve the conflict and
carry on.

But that is no excuse to behave so unfriendly, especially as this can
happen in a completely normal workflow, when e.g. two users are adding
different submodules in separate branches and they get merged. So this
RFC patch is my first attempt to avoid those commands dying.

In a second step a merge helper capable of merging inifiles would make
sense. This should be enabled for the .gitmodules file by default and
avoid most of the merge conflicts, e.g. when two users add different
submodules in separate branches. But that is a different patch ...


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                 |   23 +++++++++-
 t/t7506-status-submodule.sh |  100 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 117 insertions(+), 6 deletions(-)

-- 
1.7.5.1.251.ga75dd

[RFC PATCH 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 |  100 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index c8d50a6..3b1806c 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,92 @@ test_expect_success 'status -a clean (empty submodule dir)' '
 	test_i18ngrep "nothing to commit" output
 '

+cat > status_expect << EOF
+# On branch merge_conflict_gitmodules
+# Changes to be committed:
+#
+#	new file:   sub1
+#
+# Unmerged paths:
+#   (use "git add/rm <file>..." as appropriate to mark resolution)
+#
+#	both added:         .gitmodules
+#
+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 > ../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.251.ga75dd

Re: [RFC PATCH 1/2] test that git status works with merge conflict in .gitmodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> 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>

The basic idea (both patches) seems sound.

> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
> index c8d50a6..3b1806c 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 &&

Missing dq, i.e. "$1" (two instances).

> +cat > status_expect << EOF

	cat >status_expect <<\EOF

Unless you actively mean the here-text has variables that need to be
substituted, always quote the end-of-here-text delimiter.  That would
reduce the mental burden of people who have to read the script, as they
do not have to look for a potential substitution that does not exist.

> +# On branch merge_conflict_gitmodules
> +# Changes to be committed:
> +#
> +#	new file:   sub1
> +#
> +# Unmerged paths:
> +#   (use "git add/rm <file>..." as appropriate to mark resolution)
> +#
> +#	both added:         .gitmodules
> +#
> +EOF

Is it absolutely necessary to run this test by comparing the output of
human readable version of "git status" output?  It makes it impossible to
test under gettext-poison.  Can you use "git status -s" output instead?

> ...
> +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
> +'

You have too many ">" redirect followed by an unnecessary SP. There are
existing examples in the test, please do not make things worse.

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

[Jens Lehmann]

Until now 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 was 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.

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                 |   23 +++++++++++++++++++++--
 t/t7506-status-submodule.sh |    6 +++---
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 5294cef..cdf844c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -14,6 +14,13 @@ 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 +70,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 +91,18 @@ 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 >= -1) {
+			/* We have a clean, untracked or missing .gitmodules, try to parse it */
+			git_config_from_file(submodule_config, gitmodules_path.buf, NULL);
+		} else {
+			gitmodules_is_unmerged = 1;
+		}
 		strbuf_release(&gitmodules_path);
 	}
 }
@@ -434,7 +452,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 3b1806c..b31b64e 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -204,7 +204,7 @@ cat > status_expect << EOF
 #
 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 &&
@@ -263,7 +263,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
@@ -271,7 +271,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.251.ga75dd

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Until now commands like "git status", "git diff" and "git fetch" would

s/Until now c/C/;

> 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 was behavior was on the safe side, it is really unhelpful to

was--was

> diff --git a/submodule.c b/submodule.c
> index 5294cef..cdf844c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -14,6 +14,13 @@ 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. */

/*
 * We write our multi-line
 * comments like
 * this.
 */

> +static int gitmodules_is_unmerged;

Is it too cumbersome to pass this down the callchain as an argument?

> +		if (read_cache() < 0)
> +			die("index file corrupt");
> +		pos = cache_name_pos(".gitmodules", 11);
> +		if (pos >= -1) {
> +			/* We have a clean, untracked or missing .gitmodules, try to parse it */
> +			git_config_from_file(submodule_config, gitmodules_path.buf, NULL);
> +		} else {
> +			gitmodules_is_unmerged = 1;
> +		}

I do not think this is correct.

If pos is zero or positive, you know you found ".gitmodules" that is
merged. If it is -1, that means ".gitmodules" at stage #0 would be
inserted at the beginning of the index, but that may perhaps be because
the first cache entry in the index may be an unmerged ".gitmodules", no?

	pos = cache_name_pos(".gitmodules", 11);
	if (0 <= pos)
		; /* .gitmodules found and is merged */
	else {
        	pos = -1 - pos;
		if (active_nr <= pos)
	        	; /* .gitmodules does not exist */
	        ce = active_cache[pos];
	        if (ce_namelen(ce) == 11 &&
                    !memcmp(ce->name, ".gitmodules", 11))
			; /* .gitmodules unmerged */
		else
			; /* there is no .gitmodules */
	}

Re: [RFC PATCH 0/2] better handle .gitmodules merge conflicts

[Marc Branchaud]

On 11-05-12 05:01 PM, Jens Lehmann wrote:
> This series contains a test to reproduce and a first attempt to fix the
> problem that "git diff", "git status" and "git fetch" die early when the
> .gitmodules file contains merge conflict markers. As I am not aware of any
> bug reports yet it looks like that either doesn't happen very often in the
> wild ("git diff" parses the .gitmodules file since 1.7.3, but even as
> heavy submodule users we only hit this once just recently) or the users
> encountering this problem just know what to do: resolve the conflict and
> carry on.

I fell in the latter camp when I ran into this.

I like to examine conflicts with "git gui" but IIRC it doesn't display any
conflicting files when there's a conflict in .gitmodules (it won't even
display conflicts in other files, presumably because some plumbing failed
early on the .gitmodules conflict).  That was quite a "WTF?" moment...

> But that is no excuse to behave so unfriendly, especially as this can
> happen in a completely normal workflow, when e.g. two users are adding
> different submodules in separate branches and they get merged. So this
> RFC patch is my first attempt to avoid those commands dying.

Yup -- thanks for addressing this!

		M.