[PATCH v4 0/4] Introduce diff.submodule

[PATCH v4 0/4] Introduce diff.submodule

[Ramkumar Ramachandra]

v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com
v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com

This version was prepared in response to Peff's review of v3.
What changed:
* Functional code simplified and moved to git_diff_ui_config.
* Peff contributed one additional patch to the series.

Thanks.

Ram

Jeff King (1):
  diff: rename "set" variable

Ramkumar Ramachandra (3):
  Documentation: move diff.wordRegex from config.txt to diff-config.txt
  diff: introduce diff.submodule configuration variable
  submodule: display summary header in bold

 Documentation/config.txt         |    6 -----
 Documentation/diff-config.txt    |   13 ++++++++++
 Documentation/diff-options.txt   |    3 +-
 diff.c                           |   46 ++++++++++++++++++++++++++++---------
 submodule.c                      |    8 +++---
 submodule.h                      |    2 +-
 t/t4041-diff-submodule-option.sh |   30 ++++++++++++++++++++++++-
 7 files changed, 84 insertions(+), 24 deletions(-)

-- 
1.7.8.1.362.g5d6df.dirty

[PATCH v4 1/4] Documentation: move diff.wordRegex from config.txt to diff-config.txt

[Ramkumar Ramachandra]

19299a8 (Documentation: Move diff.<driver>.* from config.txt to
diff-config.txt, 2011-04-07) moved the diff configuration options to
diff-config.txt, but forgot about diff.wordRegex, which was left
behind in config.txt.  Fix this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt      |    6 ------
 Documentation/diff-config.txt |    6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9a0544c..e70216d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -962,12 +962,6 @@ difftool.<tool>.cmd::
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
 
-diff.wordRegex::
-	A POSIX Extended Regular Expression used to determine what is a "word"
-	when performing word-by-word difference calculations.  Character
-	sequences that match the regular expression are "words", all other
-	characters are *ignorable* whitespace.
-
 fetch.recurseSubmodules::
 	This option can be either set to a boolean value or to 'on-demand'.
 	Setting it to a boolean changes the behavior of fetch and pull to
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 75ab8a5..decd370 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,12 @@ diff.suppressBlankEmpty::
 	A boolean to inhibit the standard behavior of printing a space
 	before each empty output line. Defaults to false.
 
+diff.wordRegex::
+	A POSIX Extended Regular Expression used to determine what is a "word"
+	when performing word-by-word difference calculations.  Character
+	sequences that match the regular expression are "words", all other
+	characters are *ignorable* whitespace.
+
 diff.<driver>.command::
 	The custom diff driver command.  See linkgit:gitattributes[5]
 	for details.
-- 
1.7.8.1.362.g5d6df.dirty

[PATCH v4 2/4] diff: introduce diff.submodule configuration variable

[Ramkumar Ramachandra]

Introduce a diff.submodule configuration variable corresponding to the
'--submodule' command-line option of 'git diff'.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/diff-config.txt    |    7 +++++++
 Documentation/diff-options.txt   |    3 ++-
 diff.c                           |   32 ++++++++++++++++++++++++++++----
 t/t4041-diff-submodule-option.sh |   30 +++++++++++++++++++++++++++++-
 4 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index decd370..89dd634 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::
 	A boolean to inhibit the standard behavior of printing a space
 	before each empty output line. Defaults to false.
 
+diff.submodule::
+	Specify the format in which differences in submodules are
+	shown.  The "log" format lists the commits in the range like
+	linkgit:git-submodule[1] `summary` does.  The "short" format
+	format just shows the names of the commits at the beginning
+	and end of the range.  Defaults to short.
+
 diff.wordRegex::
 	A POSIX Extended Regular Expression used to determine what is a "word"
 	when performing word-by-word difference calculations.  Character
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cf4b216..f4f7e25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -170,7 +170,8 @@ any of those replacements occurred.
 	the commits in the range like linkgit:git-submodule[1] `summary` does.
 	Omitting the `--submodule` option or specifying `--submodule=short`,
 	uses the 'short' format. This format just shows the names of the commits
-	at the beginning and end of the range.
+	at the beginning and end of the range.  Can be tweaked via the
+	`diff.submodule` configuration variable.
 
 --color[=<when>]::
 	Show colored diff.
diff --git a/diff.c b/diff.c
index e89a201..7f2a255 100644
--- a/diff.c
+++ b/diff.c
@@ -123,6 +123,17 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
 	return ret;
 }
 
+static int parse_submodule_params(struct diff_options *options, const char *value)
+{
+	if (!strcmp(value, "log"))
+		DIFF_OPT_SET(options, SUBMODULE_LOG);
+	else if (!strcmp(value, "short"))
+		DIFF_OPT_CLR(options, SUBMODULE_LOG);
+	else
+		return -1;
+	return 0;
+}
+
 static int git_config_rename(const char *var, const char *value)
 {
 	if (!value)
@@ -178,6 +189,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
 
+	if (!strcmp(var, "diff.submodule")) {
+		if (parse_submodule_params(&default_diff_options, value))
+			warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
+				value);
+		return 0;
+	}
+
 	if (git_color_config(var, value, cb) < 0)
 		return -1;
 
@@ -3475,6 +3493,14 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
 	return 1;
 }
 
+static int parse_submodule_opt(struct diff_options *options, const char *value)
+{
+	if (parse_submodule_params(options, value))
+		die(_("Failed to parse --submodule option parameter: '%s'"),
+			value);
+	return 1;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
@@ -3655,10 +3681,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		handle_ignore_submodules_arg(options, arg + 20);
 	} else if (!strcmp(arg, "--submodule"))
 		DIFF_OPT_SET(options, SUBMODULE_LOG);
-	else if (!prefixcmp(arg, "--submodule=")) {
-		if (!strcmp(arg + 12, "log"))
-			DIFF_OPT_SET(options, SUBMODULE_LOG);
-	}
+	else if (!prefixcmp(arg, "--submodule="))
+		return parse_submodule_opt(options, arg + 12);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 6c01d0c..e401814 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -33,6 +33,7 @@ test_create_repo sm1 &&
 add_file . foo >/dev/null
 
 head1=$(add_file sm1 foo1 foo2)
+fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
 
 test_expect_success 'added submodule' "
 	git add sm1 &&
@@ -43,6 +44,34 @@ EOF
 	test_cmp expected actual
 "
 
+test_expect_success 'added submodule, set diff.submodule' "
+	git config diff.submodule log &&
+	git add sm1 &&
+	git diff --cached >actual &&
+	cat >expected <<-EOF &&
+Submodule sm1 0000000...$head1 (new submodule)
+EOF
+	git config --unset diff.submodule &&
+	test_cmp expected actual
+"
+
+test_expect_success '--submodule=short overrides diff.submodule' "
+	git config diff.submodule log &&
+	git add sm1 &&
+	git diff --submodule=short --cached >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
+	git config --unset diff.submodule &&
+	test_cmp expected actual
+"
+
 commit_file sm1 &&
 head2=$(add_file sm1 foo3)
 
@@ -73,7 +102,6 @@ EOF
 	test_cmp expected actual
 "
 
-fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
 fullhead2=$(cd sm1; git rev-list --max-count=1 $head2)
 test_expect_success 'modified submodule(forward) --submodule=short' "
 	git diff --submodule=short >actual &&
-- 
1.7.8.1.362.g5d6df.dirty

[PATCH v4 3/4] diff: rename "set" variable

[Ramkumar Ramachandra]

From: Jeff King <peff@peff.net>

Once upon a time the builtin_diff function used one color, and the color
variables were called "set" and "reset". Nowadays it is a much longer
function and we use several colors (e.g., "add", "del"). Rename "set" to
"meta" to show that it is the color for showing diff meta-info (it still
does not indicate that it is a "color", but at least it matches the
scheme of the other color variables).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 diff.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 7f2a255..ffaed72 100644
--- a/diff.c
+++ b/diff.c
@@ -2240,7 +2240,7 @@ static void builtin_diff(const char *name_a,
 	mmfile_t mf1, mf2;
 	const char *lbl[2];
 	char *a_one, *b_two;
-	const char *set = diff_get_color_opt(o, DIFF_METAINFO);
+	const char *meta = diff_get_color_opt(o, DIFF_METAINFO);
 	const char *reset = diff_get_color_opt(o, DIFF_RESET);
 	const char *a_prefix, *b_prefix;
 	struct userdiff_driver *textconv_one = NULL;
@@ -2287,24 +2287,24 @@ static void builtin_diff(const char *name_a,
 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
-	strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, set, a_one, b_two, reset);
+	strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset);
 	if (lbl[0][0] == '/') {
 		/* /dev/null */
-		strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, set, two->mode, reset);
+		strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset);
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
 		must_show_header = 1;
 	}
 	else if (lbl[1][0] == '/') {
-		strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, set, one->mode, reset);
+		strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset);
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
 		must_show_header = 1;
 	}
 	else {
 		if (one->mode != two->mode) {
-			strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, set, one->mode, reset);
-			strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, set, two->mode, reset);
+			strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset);
+			strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset);
 			must_show_header = 1;
 		}
 		if (xfrm_msg)
-- 
1.7.8.1.362.g5d6df.dirty

[PATCH v4 4/4] submodule: display summary header in bold

[Ramkumar Ramachandra]

Currently, 'git diff --submodule' displays output with a bold diff
header for non-submodules.  So this part is in bold:

    diff --git a/file1 b/file1
    index 30b2f6c..2638038 100644
    --- a/file1
    +++ b/file1

For submodules, the header looks like this:

    Submodule submodule1 012b072..248d0fd:

Unfortunately, it's easy to miss in the output because it's not bold.
Change this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 diff.c      |    2 +-
 submodule.c |    8 ++++----
 submodule.h |    2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index ffaed72..1065978 100644
--- a/diff.c
+++ b/diff.c
@@ -2261,7 +2261,7 @@ static void builtin_diff(const char *name_a,
 		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
 		show_submodule_summary(o->file, one ? one->path : two->path,
 				one->sha1, two->sha1, two->dirty_submodule,
-				del, add, reset);
+				meta, del, add, reset);
 		return;
 	}
 
diff --git a/submodule.c b/submodule.c
index e3e0b45..2f55436 100644
--- a/submodule.c
+++ b/submodule.c
@@ -258,7 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
 
 void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
-		unsigned dirty_submodule,
+		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset)
 {
 	struct rev_info rev;
@@ -292,15 +292,15 @@ void show_submodule_summary(FILE *f, const char *path,
 		return;
 	}
 
-	strbuf_addf(&sb, "Submodule %s %s..", path,
+	strbuf_addf(&sb, "%sSubmodule %s %s..", meta, path,
 			find_unique_abbrev(one, DEFAULT_ABBREV));
 	if (!fast_backward && !fast_forward)
 		strbuf_addch(&sb, '.');
 	strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
 	if (message)
-		strbuf_addf(&sb, " %s\n", message);
+		strbuf_addf(&sb, " %s%s\n", message, reset);
 	else
-		strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
 	fwrite(sb.buf, sb.len, 1, f);
 
 	if (!message) {
diff --git a/submodule.h b/submodule.h
index f2e8271..3dc1b3f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -20,7 +20,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
-		unsigned dirty_submodule,
+		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset);
 void set_config_fetch_recurse_submodules(int value);
 void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-- 
1.7.8.1.362.g5d6df.dirty

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

[Jeff King]

On Tue, Nov 13, 2012 at 09:12:43PM +0530, Ramkumar Ramachandra wrote:

> v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com
> v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com
> 
> This version was prepared in response to Peff's review of v3.
> What changed:
> * Functional code simplified and moved to git_diff_ui_config.
> * Peff contributed one additional patch to the series.

Thanks, this version looks good to me.

-Peff

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

[Jeff King]

On Thu, Nov 15, 2012 at 08:23:34AM -0800, Jeff King wrote:

> On Tue, Nov 13, 2012 at 09:12:43PM +0530, Ramkumar Ramachandra wrote:
> 
> > v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> > v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com
> > v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com
> > 
> > This version was prepared in response to Peff's review of v3.
> > What changed:
> > * Functional code simplified and moved to git_diff_ui_config.
> > * Peff contributed one additional patch to the series.
> 
> 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.

-Peff

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

[Jeff King]

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.

Oh no, I am having a muddled morning. It _is_ right. We want to move it
out of basic into ui. Sorry for the noise. I just woke up and am
thinking backwards.

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

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

[Ramkumar Ramachandra]

Jeff King wrote:
> It may be worth squashing this test into patch 3:

Looks good.  Thanks.

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

I see.  Can you squash this in?

-- 8< --
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index e401814..876800f 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -56,7 +56,7 @@ EOF
 "

 test_expect_success '--submodule=short overrides diff.submodule' "
-	git config diff.submodule log &&
+	test_config diff.submodule log &&
 	git add sm1 &&
 	git diff --submodule=short --cached >actual &&
 	cat >expected <<-EOF &&
@@ -68,7 +68,6 @@ index 0000000..a2c4dab
 @@ -0,0 +1 @@
 +Subproject commit $fullhead1
 EOF
-	git config --unset diff.submodule &&
 	test_cmp expected actual
 "

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

I know;  however, I wanted to be consistent with the surrounding code.

Ram

Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 6c01d0c..e401814 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -33,6 +33,7 @@ test_create_repo sm1 &&
>  add_file . foo >/dev/null
>  
>  head1=$(add_file sm1 foo1 foo2)
> +fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)

That looks like quite a roundabout way to say

	$(cd sm1; git rev-parse --verify HEAD)

doesn't it?  I know this is just moved code from the original, so I
am not saying this should be fixed in this commit.

Existing code in t7401 may want to be cleaned up, perhaps after this
topic settles.  The add_file() function, for example, has at least
these points:

 - do we know 7 hexdigits is always the right precision?
 - what happens when it fails to create a commit in one of the steps
   while looping over "$@" in its loop?
 - the function uses the "cd there and then come back", not
   "go there in a subshell and do whatever it needs to" pattern.

> +test_expect_success 'added submodule, set diff.submodule' "

s/added/add/;

Shouldn't it test the base case where no diff.submodule is set?  For
those people, the patch should not change the output when they do or
do not pass --submodule option, right?

> +	git config diff.submodule log &&
> +	git add sm1 &&
> +	git diff --cached >actual &&
> +	cat >expected <<-EOF &&
> +Submodule sm1 0000000...$head1 (new submodule)
> +EOF
> +	git config --unset diff.submodule &&
> +	test_cmp expected actual
> +"
> +
> +test_expect_success '--submodule=short overrides diff.submodule' "
> +	git config diff.submodule log &&
> +	git add sm1 &&
> +	git diff --submodule=short --cached >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
> +	git config --unset diff.submodule &&
> +	test_cmp expected actual
> +"
> +
>  commit_file sm1 &&
>  head2=$(add_file sm1 foo3)
>  
> @@ -73,7 +102,6 @@ EOF
>  	test_cmp expected actual
>  "
>  
> -fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
>  fullhead2=$(cd sm1; git rev-list --max-count=1 $head2)
>  test_expect_success 'modified submodule(forward) --submodule=short' "
>  	git diff --submodule=short >actual &&

Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
>> index 6c01d0c..e401814 100755
>> --- a/t/t4041-diff-submodule-option.sh
>> +++ b/t/t4041-diff-submodule-option.sh
>> @@ -33,6 +33,7 @@ test_create_repo sm1 &&
>>  add_file . foo >/dev/null
>>
>>  head1=$(add_file sm1 foo1 foo2)
>> +fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
>
> That looks like quite a roundabout way to say
>
>         $(cd sm1; git rev-parse --verify HEAD)
>
> doesn't it?  I know this is just moved code from the original, so I
> am not saying this should be fixed in this commit.

Exactly what I was thinking.

> Existing code in t7401 may want to be cleaned up, perhaps after this
> topic settles.  The add_file() function, for example, has at least
> these points:
>
>  - do we know 7 hexdigits is always the right precision?
>  - what happens when it fails to create a commit in one of the steps
>    while looping over "$@" in its loop?
>  - the function uses the "cd there and then come back", not
>    "go there in a subshell and do whatever it needs to" pattern.

Okay, will do.

>> +test_expect_success 'added submodule, set diff.submodule' "
>
> s/added/add/;

I see that the topic is already in `next`.  Do you want to fix it up there?

> Shouldn't it test the base case where no diff.submodule is set?  For
> those people, the patch should not change the output when they do or
> do not pass --submodule option, right?

When no diff.submodule is set, `git diff --submodule` should just work
as before; isn't this tested by the other tests in the file?

Ram

Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>> +test_expect_success 'added submodule, set diff.submodule' "
>>
>> s/added/add/;
>
> I see that the topic is already in `next`.  Do you want to fix it up there?

I was simply following the example set by the previous test in the
file: "added submodule".  I can fix up these two after the topic
graduates to `master`, if you prefer that, instead of rewriting `next`
for a small change like this.

Ram