[PATCH v4 0/4] Introduce diff.submodule

STALE3736d

Revision v4 of 3 in this series.

12 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH v4 0/4] Introduce diff.submodule

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:16

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:16

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 <redacted>
---
 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 3/4] diff: rename "set" variable

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:16

From: Jeff King <redacted>

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 <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 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 2/4] diff: introduce diff.submodule configuration variable

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:16

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

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 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 4/4] submodule: display summary header in bold

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:16

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

From: Jeff King <hidden>
Date: 2016-06-15 22:55:18

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

From: Jeff King <hidden>
Date: 2016-06-15 22:55:18

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

From: Jeff King <hidden>
Date: 2016-06-15 22:55:18

On Thu, Nov 15, 2012 at 08:25:26AM -0800, Jeff King wrote:
quoted
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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:18

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:22

Ramkumar Ramachandra [off-list ref] writes:
quoted hunk
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?
quoted hunk
+	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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:23

Junio C Hamano wrote:
Ramkumar Ramachandra [off-list ref] writes:
quoted
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.
quoted
+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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:55:23

Ramkumar Ramachandra wrote:
Junio C Hamano wrote:
quoted
Ramkumar Ramachandra [off-list ref] writes:
quoted
+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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help