Re: [PATCH v4 0/4] Introduce diff.submodule

From

Jeff King <peff@peff.net>

Date

Nov 15, 2012, 16:33 UTC

Message-ID

<20121115163356.GF6157@sigill.intra.peff.net>

In-Reply-To

<20121115162524.GE6157@sigill.intra.peff.net>

On Thu, Nov 15, 2012 at 08:25:26AM -0800, Jeff King wrote:

> > Thanks, this version looks good to me.
> 
> Oh wait. I did not look closely enough. The point was to move the option
> parser _out_ of git_diff_ui_config into git_diff_basic_config, so that
> it only triggers for porcelain, not plumbing.

It may be worth squashing this test into patch 3:

diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index e401814..023439f 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -72,6 +72,21 @@ EOF
 	test_cmp expected actual
 "
 
+test_expect_success 'diff.submodule does not affect plumbing' '
+	test_config diff.submodule log &&
+	git diff-index -p HEAD >actual &&
+	cat >expected <<-EOF &&
+	diff --git a/sm1 b/sm1
+	new file mode 160000
+	index 0000000..a2c4dab
+	--- /dev/null
+	+++ b/sm1
+	@@ -0,0 +1 @@
+	+Subproject commit $fullhead1
+	EOF
+	test_cmp expected actual
+'
+
 commit_file sm1 &&
 head2=$(add_file sm1 foo3)
 

BTW, while writing the test, I noticed two minor nits with your tests:

  1. They can use test_config, which is simpler (you do not need to
     unset yourself after the test) and safer (the unset happens via
     test_when_finished, so it works even if the test fails).

  2. You can still indent expected output when using <<-.

I don't know if it is worth re-rolling for them.

-Peff