[PATCH v2 0/3] Introduce diff.submodule

[PATCH v2 0/3] Introduce diff.submodule

[Ramkumar Ramachandra]

Hi,

v1 is here:
http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com

I've fixed the issues pointed out in the review by Jens.

Sorry about the delay; need to find a new dayjob.

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 ++-
 builtin/diff.c                   |    4 ++++
 cache.h                          |    1 +
 diff.c                           |    7 ++++++-
 submodule.c                      |    8 ++++----
 submodule.h                      |    2 +-
 t/t4041-diff-submodule-option.sh |   30 +++++++++++++++++++++++++++++-
 9 files changed, 60 insertions(+), 14 deletions(-)

-- 
1.7.8.1.362.g5d6df.dirty

[PATCH 1/3] 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 11f320b..d1de857 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 2/3] 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 ++-
 builtin/diff.c                   |    4 ++++
 cache.h                          |    1 +
 diff.c                           |    5 +++++
 t/t4041-diff-submodule-option.sh |   30 +++++++++++++++++++++++++++++-
 6 files changed, 48 insertions(+), 2 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/builtin/diff.c b/builtin/diff.c
index 9650be2..6d00311 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
 
+	/* Set SUBMODULE_LOG if diff.submodule config var was set */
+	if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
+		DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
+
 	if (nongit)
 		die(_("Not a git repository"));
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/cache.h b/cache.h
index a58df84..5fc7ba3 100644
--- a/cache.h
+++ b/cache.h
@@ -1220,6 +1220,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
+extern const char *submodule_format_cfg;
 
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);
diff --git a/diff.c b/diff.c
index 86e5f2a..835eb26 100644
--- a/diff.c
+++ b/diff.c
@@ -29,6 +29,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+const char *submodule_format_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -168,6 +169,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_stat_graph_width = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.submodule"))
+		return git_config_string(&submodule_format_cfg, var, value);
 	if (!strcmp(var, "diff.external"))
 		return git_config_string(&external_diff_cmd_cfg, var, value);
 	if (!strcmp(var, "diff.wordregex"))
@@ -3656,6 +3659,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!prefixcmp(arg, "--submodule=")) {
 		if (!strcmp(arg + 12, "log"))
 			DIFF_OPT_SET(options, SUBMODULE_LOG);
+		if (!strcmp(arg + 12, "short"))
+			DIFF_OPT_CLR(options, SUBMODULE_LOG);
 	}
 
 	/* misc options */
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 3/3] 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 835eb26..115f384 100644
--- a/diff.c
+++ b/diff.c
@@ -2244,7 +2244,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);
+				set, del, add, reset);
 		return;
 	}
 
diff --git a/submodule.c b/submodule.c
index e3e0b45..c10182e 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 *set,
 		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..", set, 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..997fd06 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 *set,
 		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 v2 0/3] Introduce diff.submodule

[Jens Lehmann]

Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:
> Hi,
> 
> v1 is here:
> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> 
> I've fixed the issues pointed out in the review by Jens.

Thanks, looking good to me.

Re: [PATCH v2 0/3] Introduce diff.submodule

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:
>> Hi,
>>
>> v1 is here:
>> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
>>
>> I've fixed the issues pointed out in the review by Jens.
>
> Thanks, looking good to me.

Peff, can we pick this up?

Ram

Re: [PATCH v2 0/3] Introduce diff.submodule

[Jeff King]

On Sun, Nov 04, 2012 at 11:28:17PM +0530, Ramkumar Ramachandra wrote:

> Jens Lehmann wrote:
> > Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:
> >> Hi,
> >>
> >> v1 is here:
> >> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> >>
> >> I've fixed the issues pointed out in the review by Jens.
> >
> > Thanks, looking good to me.
> 
> Peff, can we pick this up?

Thanks for prodding, I missed the original. I have a few comments, but
I'll respond directly.

Also, I was coincidentally looking at the same code today. You might
find this interesting:

  http://article.gmane.org/gmane.comp.version-control.git/209188

-Peff

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

[Jeff King]

On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:

> diff --git a/builtin/diff.c b/builtin/diff.c
> index 9650be2..6d00311 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
>  	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
>  
> +	/* Set SUBMODULE_LOG if diff.submodule config var was set */
> +	if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
> +		DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
> +

Yuck. Why is this parsing happening in cmd_diff?

Wouldn't you want it to kick in for "git log --submodule", too? It seems
like it should go into diff_setup(), and the porcelain/plumbing aspect
should be controlled by parsing it in git_diff_ui_config or
git_diff_basic_config. See how diff_no_prefix and diff_mnemonic prefix
are handled for an example.

Then you can keep the parsing logic for "log" in diff.c. And you should
factor it out into a function so that the command-line option and the
config parser can share the same code. I know it's only one line now,
but anybody who wants to add an option will have to update both places.
See the parsing of diff.dirstat for an example.

>  	else if (!prefixcmp(arg, "--submodule=")) {
>  		if (!strcmp(arg + 12, "log"))
>  			DIFF_OPT_SET(options, SUBMODULE_LOG);
> +		if (!strcmp(arg + 12, "short"))
> +			DIFF_OPT_CLR(options, SUBMODULE_LOG);
>  	}

Much better (although arguably should go in a separate patch). Should we
also produce an error if somebody says "--submodule=foobar"?

-Peff

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

[Ramkumar Ramachandra]

Jeff King wrote:
> On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:
>
>> diff --git a/builtin/diff.c b/builtin/diff.c
>> index 9650be2..6d00311 100644
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
>>       DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
>>
>> +     /* Set SUBMODULE_LOG if diff.submodule config var was set */
>> +     if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
>> +             DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
>> +
>
> Yuck. Why is this parsing happening in cmd_diff?

Blame Jens- see this thread |
http://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815

> Then you can keep the parsing logic for "log" in diff.c. And you should
> factor it out into a function so that the command-line option and the
> config parser can share the same code. I know it's only one line now,
> but anybody who wants to add an option will have to update both places.
> See the parsing of diff.dirstat for an example.

Done.  I'll send out the new series shortly.

Ram

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

[Jeff King]

On Sun, Nov 11, 2012 at 08:20:27PM +0530, Ramkumar Ramachandra wrote:

> >> diff --git a/builtin/diff.c b/builtin/diff.c
> >> index 9650be2..6d00311 100644
> >> --- a/builtin/diff.c
> >> +++ b/builtin/diff.c
> >> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> >>       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
> >>       DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
> >>
> >> +     /* Set SUBMODULE_LOG if diff.submodule config var was set */
> >> +     if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
> >> +             DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
> >> +
> >
> > Yuck. Why is this parsing happening in cmd_diff?
> 
> Blame Jens- see this thread |
> http://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815

I don't think that is the right path, as at means that the option can
only ever affect diff, not other porcelains. I was thinking something
more like this (completely untested):

diff --git a/diff.c b/diff.c
index e89a201..74f4fc6 100644
--- a/diff.c
+++ b/diff.c
@@ -37,6 +37,13 @@ static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 
+/*
+ * 0 for "short", 1 for "log". This should probably just be an enum, and
+ * SUBMODULE_LOG lifted up from being a bit in the options to being its own
+ * struct member.
+ */
+static int diff_submodule_default;
+
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
 	GIT_COLOR_NORMAL,	/* PLAIN */
@@ -178,6 +185,19 @@ 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")) {
+		/* XXX this should be factored out from the command-line parser */
+		if (!value)
+			return config_error_nonbool(var);
+		else if (!strcmp(var, "short"))
+			diff_submodule_default = 0;
+		else if (!strcmp(var, "log"))
+			diff_submodule_default = 1;
+		else
+			return error("unknown %s value: %s", var, value);
+		return 0;
+	}
+
 	if (git_color_config(var, value, cb) < 0)
 		return -1;
 
@@ -3193,6 +3213,9 @@ void diff_setup(struct diff_options *options)
 		options->a_prefix = "a/";
 		options->b_prefix = "b/";
 	}
+
+	if (diff_submodule_default)
+		DIFF_OPT_SET(options, SUBMODULE_LOG);
 }
 
 void diff_setup_done(struct diff_options *options)

[PATCH 1/3] 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