threads / patch / 45575

patch, 2 partsRe: git diff --submodule=diff fails with submodules in a submodule

Subject: [PATCH 0/2] Re: git diff --submodule=diff fails with submodules in a submodule

## tl;dr

3 messages between Mar 31, 2017 and Mar 31, 2017. Diffs are folded; open one to read it.

replies: 2people: 1as markdown or json

Stefan Beller· Mar 31, 2017, 23:17 UTC · lore

I came up with a patch that fixes the issue locally. (It is the first patch, such that we can track the first patch down to maint)

While doing so, I noticed 2 issues:
* when having nested submodules, we probably want to have --submodule=diff
  to recurse into the nested submodules, so pass on the option.
  (2nd patch)
* the tests in t4060 hardcode hashes literally. I do not have a patch for that
  yet, but I will send patches later. (c.f. "sanitize_output" in t4202)
  
Thanks,
Stefan

Stefan Beller (2):
  diff: submodule inline diff to initialize env array.
  diff: recurse into nested submodules for inline diff

 submodule.c                                  |  4 +-
 t/t4060-diff-submodule-option-diff-format.sh | 70 ++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 1 deletion(-)

-- 
2.12.2.576.g7be6e4ba40.dirty

Stefan Beller· Mar 31, 2017, 23:17 UTC · re: Stefan Beller · lore

[PATCH 1/2] diff: submodule inline diff to initialize env array.

David reported:

> When I try to run `git diff --submodule=diff` in a submodule which has
> it's own submodules that have changes I get the error: fatal: bad
> object.

This happens, because we do not properly initialize the environment in which the diff is run in the submodule. That means we inherit the environment from the main process, which sets environment variables. (Apparently we do set environment variables which we do not set when not in a submodules, i.e. the .git directory is linked)

This commit, just like fd47ae6a5b (diff: teach diff to display submodule difference with an inline diff, 2016-08-31) introduces bad test code (i.e. hard coded hash values), which will be cleanup up in a later patch.

Reported-by: David Parrish <daveparrish@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                                  |  1 +
 t/t4060-diff-submodule-option-diff-format.sh | 29 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

Show changes to 2 files +30 −0

submodule.c, t/t4060-diff-submodule-option-diff-format.sh

diff --git a/submodule.c b/submodule.c
index c52d6634c5..d84109908f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -576,6 +576,7 @@ void show_submodule_inline_diff(FILE *f, const char *path,
 	if (!(dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
 		argv_array_push(&cp.args, oid_to_hex(new));
 
+	prepare_submodule_repo_env(&cp.env_array);
 	if (run_command(&cp))
 		fprintf(f, "(diff failed)\n");
 
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index 7e23b55ea4..d4a3ffa69c 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -746,4 +746,33 @@ test_expect_success 'diff --submodule=diff with .git file' '
 	test_cmp expected actual
 '
 
+test_expect_success 'setup nested submodule' '
+	git submodule add -f ./sm2 &&
+	git commit -a -m "add sm2" &&
+	git -C sm2 submodule add ../sm2 nested &&
+	git -C sm2 commit -a -m "nested sub"
+'
+
+test_expect_success 'move nested submodule HEAD' '
+	echo "nested content" >sm2/nested/file &&
+	git -C sm2/nested add file &&
+	git -C sm2/nested commit --allow-empty -m "new HEAD"
+'
+
+test_expect_success 'diff --submodule=diff with moved nested submodule HEAD' '
+	cat >expected <<-EOF &&
+	Submodule nested a5a65c9..b55928c:
+	diff --git a/nested/file b/nested/file
+	new file mode 100644
+	index 0000000..ca281f5
+	--- /dev/null
+	+++ b/nested/file
+	@@ -0,0 +1 @@
+	+nested content
+	EOF
+	git -C sm2 diff --submodule=diff >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp expected actual
+'
+
 test_done

-- 
2.12.2.576.g7be6e4ba40.dirty

Stefan Beller· Mar 31, 2017, 23:17 UTC · re: Stefan Beller · lore

[PATCH 2/2] diff: recurse into nested submodules for inline diff

When fd47ae6a5b (diff: teach diff to display submodule difference with an inline diff, 2016-08-31) was introduced, we did not think of recursing into nested submodules.

When showing the inline diff for submodules, automatically recurse into nested submodules as well with inline submodule diffs.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                                  |  3 +-
 t/t4060-diff-submodule-option-diff-format.sh | 41 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

Show changes to 2 files +43 −1

submodule.c, t/t4060-diff-submodule-option-diff-format.sh

diff --git a/submodule.c b/submodule.c
index d84109908f..471ca9ce7d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -553,7 +553,8 @@ void show_submodule_inline_diff(FILE *f, const char *path,
 	cp.no_stdin = 1;
 
 	/* TODO: other options may need to be passed here. */
-	argv_array_push(&cp.args, "diff");
+	argv_array_pushl(&cp.args, "diff", "--submodule=diff", NULL);
+
 	argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
 	if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
 		argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index d4a3ffa69c..33ec26d755 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -775,4 +775,45 @@ test_expect_success 'diff --submodule=diff with moved nested submodule HEAD' '
 	test_cmp expected actual
 '
 
+test_expect_success 'diff --submodule=diff recurses into nested submodules' '
+	cat >expected <<-EOF &&
+	Submodule sm2 contains modified content
+	Submodule sm2 a5a65c9..280969a:
+	diff --git a/sm2/.gitmodules b/sm2/.gitmodules
+	new file mode 100644
+	index 0000000..3a816b8
+	--- /dev/null
+	+++ b/sm2/.gitmodules
+	@@ -0,0 +1,3 @@
+	+[submodule "nested"]
+	+	path = nested
+	+	url = ../sm2
+	Submodule nested 0000000...b55928c (new submodule)
+	diff --git a/sm2/nested/file b/sm2/nested/file
+	new file mode 100644
+	index 0000000..ca281f5
+	--- /dev/null
+	+++ b/sm2/nested/file
+	@@ -0,0 +1 @@
+	+nested content
+	diff --git a/sm2/nested/foo8 b/sm2/nested/foo8
+	new file mode 100644
+	index 0000000..db9916b
+	--- /dev/null
+	+++ b/sm2/nested/foo8
+	@@ -0,0 +1 @@
+	+foo8
+	diff --git a/sm2/nested/foo9 b/sm2/nested/foo9
+	new file mode 100644
+	index 0000000..9c3b4f6
+	--- /dev/null
+	+++ b/sm2/nested/foo9
+	@@ -0,0 +1 @@
+	+foo9
+	EOF
+	git diff --submodule=diff >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp expected actual
+'
+
 test_done

-- 
2.12.2.576.g7be6e4ba40.dirty