threads / rfc / 27341

RFC patch, 2 partsbetter handle .gitmodules merge conflicts

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

## tl;dr

6 messages between May 12, 2011 and May 13, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jens Lehmann· May 12, 2011, 21:01 UTC · lore

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
Jens Lehmann· May 12, 2011, 21:02 UTC · re: Jens Lehmann · lore

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

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(-)
Show changes to t/t7506-status-submodule.sh +88 −4
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
Junio C Hamano· May 13, 2011, 05:47 UTC · re: Jens Lehmann · lore

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

Jens Lehmann <Jens.Lehmann@web.de> writes:
Show 9 quoted lines
> 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.
Show 15 quoted lines
> 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.

Show 11 quoted lines
> +# 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?

Show 8 quoted lines
> ...
> +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.

Jens Lehmann· May 12, 2011, 21:03 UTC · re: Jens Lehmann · lore

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

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(-)
Show changes to 2 files +24 −5

submodule.c, t/t7506-status-submodule.sh

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
Junio C Hamano· May 13, 2011, 06:19 UTC · re: Jens Lehmann · lore

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

Jens Lehmann <Jens.Lehmann@web.de> writes:
> Until now commands like "git status", "git diff" and "git fetch" would
s/Until now c/C/;
Show 6 quoted lines
> 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
Show 14 quoted lines
> 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?
Show 9 quoted lines
> +		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 */
	}
Marc Branchaud· May 13, 2011, 15:01 UTC · re: Jens Lehmann · lore

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

On 11-05-12 05:01 PM, Jens Lehmann wrote:
Show 8 quoted lines
> 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.

← back to recent threads