[PATCH 0/5] submodule: introduce diff.submoduleFormat

[PATCH 0/5] submodule: introduce diff.submoduleFormat

[Ramkumar Ramachandra]

Hi,

The fourth patch is the crux of the series.  The first two patches
clear up config.txt, diff-config.txt for the fourth patch.  The third
patch turned out to be a prerequisite.  And the fifth patch is a
bonus.

Thanks for reading.

Ram

Ramkumar Ramachandra (5):
  Documentation: move diff.wordRegex from config.txt to diff-config.txt
  Documentation: sort diff-config.txt alphabetically
  diff: acknowledge --submodule=short command-line option
  diff: introduce diff.submoduleFormat configuration variable
  submodule: display summary header in bold

 Documentation/config.txt         |    6 ------
 Documentation/diff-config.txt    |   21 +++++++++++++++++----
 Documentation/diff-options.txt   |    3 ++-
 diff.c                           |   29 +++++++++++++++++++----------
 diff.h                           |   17 +++++++++--------
 submodule.c                      |    8 ++++----
 submodule.h                      |    2 +-
 t/t4041-diff-submodule-option.sh |   10 ++++++++++
 8 files changed, 62 insertions(+), 34 deletions(-)

-- 
1.7.8.1.362.g5d6df.dirty

[PATCH 1/5] 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 67a90a8..c2b94f9 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -103,6 +103,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/5] Documentation: sort diff-config.txt alphabetically

[Ramkumar Ramachandra]

df44483a (diff --stat: add config option to limit graph width,
2012-03-01) added the option diff.statGraphWidth, but did not place it
in its appropriate lexical place in diff-config.txt.  Fix this.

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

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index c2b94f9..04574f9 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -52,10 +52,6 @@ directories with less than 10% of the total amount of changed files,
 and accumulating child directory counts in the parent directories:
 `files,10,cumulative`.
 
-diff.statGraphWidth::
-	Limit the width of the graph part in --stat output. If set, applies
-	to all commands generating --stat output except format-patch.
-
 diff.external::
 	If this config variable is set, diff generation is not
 	performed using the internal diff machinery, but using the
@@ -99,6 +95,10 @@ diff.renames::
 	will enable basic rename detection.  If set to "copies" or
 	"copy", it will detect copies, as well.
 
+diff.statGraphWidth::
+	Limit the width of the graph part in --stat output. If set, applies
+	to all commands generating --stat output except format-patch.
+
 diff.suppressBlankEmpty::
 	A boolean to inhibit the standard behavior of printing a space
 	before each empty output line. Defaults to false.
-- 
1.7.8.1.362.g5d6df.dirty

[PATCH 3/5] diff: acknowledge --submodule=short command-line option

[Ramkumar Ramachandra]

Currently, the diff code does not differentiate between an explicit
'--submodule=short' being passed, and no submodule option being passed
on the command line.  Making this differentiation will be important
when the command-line option can be used to override a
"diff.submoduleFormat" configuration variable introduced in the next
patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 diff.c |    4 +++-
 diff.h |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 35d3f07..8ea40f9 100644
--- a/diff.c
+++ b/diff.c
@@ -3647,7 +3647,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	} else if (!strcmp(arg, "--submodule"))
 		DIFF_OPT_SET(options, SUBMODULE_LOG);
 	else if (!prefixcmp(arg, "--submodule=")) {
-		if (!strcmp(arg + 12, "log"))
+		if (!strcmp(arg + 12, "short"))
+			DIFF_OPT_SET(options, SUBMODULE_SHORT);
+		else if (!strcmp(arg + 12, "log"))
 			DIFF_OPT_SET(options, SUBMODULE_LOG);
 	}
 
diff --git a/diff.h b/diff.h
index a658f85..4115b49 100644
--- a/diff.h
+++ b/diff.h
@@ -77,14 +77,15 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)
 #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)
 #define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)
-#define DIFF_OPT_SUBMODULE_LOG       (1 << 23)
-#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)
-#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
-#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
-#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
-#define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
-#define DIFF_OPT_FUNCCONTEXT         (1 << 29)
-#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
+#define DIFF_OPT_SUBMODULE_SHORT     (1 << 23)
+#define DIFF_OPT_SUBMODULE_LOG       (1 << 24)
+#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 25)
+#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 26)
+#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 27)
+#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 28)
+#define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 29)
+#define DIFF_OPT_FUNCCONTEXT         (1 << 30)
+#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 31)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
-- 
1.7.8.1.362.g5d6df.dirty

[PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable

[Ramkumar Ramachandra]

Introduce a diff.submoduleFormat 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                           |   25 ++++++++++++++++---------
 t/t4041-diff-submodule-option.sh |   10 ++++++++++
 4 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 04574f9..e445cc8 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -103,6 +103,13 @@ diff.suppressBlankEmpty::
 	A boolean to inhibit the standard behavior of printing a space
 	before each empty output line. Defaults to false.
 
+diff.submoduleFormat::
+	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..034c4e7 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.submoduleFormat` configuration variable.
 
 --color[=<when>]::
 	Show colored diff.
diff --git a/diff.c b/diff.c
index 8ea40f9..4cb8dd2 100644
--- a/diff.c
+++ b/diff.c
@@ -28,6 +28,7 @@ static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *submodule_format_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -161,6 +162,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.submoduleformat"))
+		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"))
@@ -2227,15 +2230,19 @@ static void builtin_diff(const char *name_a,
 		line_prefix = msgbuf->buf;
 	}
 
-	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
-			(!one->mode || S_ISGITLINK(one->mode)) &&
-			(!two->mode || S_ISGITLINK(two->mode))) {
-		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
-		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);
-		return;
+	if (!DIFF_OPT_TST(o, SUBMODULE_SHORT) &&
+		((!one->mode || S_ISGITLINK(one->mode)) &&
+			(!two->mode || S_ISGITLINK(two->mode)))) {
+		if (DIFF_OPT_TST(o, SUBMODULE_LOG) ||
+			(submodule_format_cfg &&
+				!strcmp(submodule_format_cfg, "log"))) {
+			const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
+			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);
+			return;
+		}
 	}
 
 	if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 6c01d0c..ed4f3a8 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -43,6 +43,16 @@ EOF
 	test_cmp expected actual
 "
 
+test_expect_success 'added submodule, set diff.submoduleFormat' "
+	git config diff.submoduleFormat log &&
+	git add sm1 &&
+	git diff --cached >actual &&
+	cat >expected <<-EOF &&
+Submodule sm1 0000000...$head1 (new submodule)
+EOF
+	test_cmp expected actual
+"
+
 commit_file sm1 &&
 head2=$(add_file sm1 foo3)
 
-- 
1.7.8.1.362.g5d6df.dirty

[PATCH 5/5] 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 4cb8dd2..cf369c5 100644
--- a/diff.c
+++ b/diff.c
@@ -2240,7 +2240,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 50f213e..be60e89 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 594b50d..ef5aab6 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 3/5] diff: acknowledge --submodule=short command-line option

[Jens Lehmann]

Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
> Currently, the diff code does not differentiate between an explicit
> '--submodule=short' being passed, and no submodule option being passed
> on the command line.  Making this differentiation will be important
> when the command-line option can be used to override a
> "diff.submoduleFormat" configuration variable introduced in the next
> patch.

Wouldn't it be sufficient here to simply reset the log flag by using
"DIFF_OPT_CLR(options, SUBMODULE_LOG)"? This would avoid having to
use the last bit of the diffopt flags. And if I read the code correctly,
diff_opt_parse() is called by setup_revisions() which is called after
git_config(), so that should be safe. (And "textconv" uses the same
approach)

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  diff.c |    4 +++-
>  diff.h |   17 +++++++++--------
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 35d3f07..8ea40f9 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3647,7 +3647,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>  	} else if (!strcmp(arg, "--submodule"))
>  		DIFF_OPT_SET(options, SUBMODULE_LOG);
>  	else if (!prefixcmp(arg, "--submodule=")) {
> -		if (!strcmp(arg + 12, "log"))
> +		if (!strcmp(arg + 12, "short"))
> +			DIFF_OPT_SET(options, SUBMODULE_SHORT);
> +		else if (!strcmp(arg + 12, "log"))
>  			DIFF_OPT_SET(options, SUBMODULE_LOG);
>  	}
>  
> diff --git a/diff.h b/diff.h
> index a658f85..4115b49 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -77,14 +77,15 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
>  #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)
>  #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)
>  #define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)
> -#define DIFF_OPT_SUBMODULE_LOG       (1 << 23)
> -#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)
> -#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
> -#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
> -#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
> -#define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
> -#define DIFF_OPT_FUNCCONTEXT         (1 << 29)
> -#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
> +#define DIFF_OPT_SUBMODULE_SHORT     (1 << 23)
> +#define DIFF_OPT_SUBMODULE_LOG       (1 << 24)
> +#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 25)
> +#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 26)
> +#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 27)
> +#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 28)
> +#define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 29)
> +#define DIFF_OPT_FUNCCONTEXT         (1 << 30)
> +#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 31)
>  
>  #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
>  #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
> 

Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable

[Jens Lehmann]

Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
> Introduce a diff.submoduleFormat configuration variable corresponding
> to the '--submodule' command-line option of 'git diff'.

Nice. Maybe a better name would be "diff.submodule", as this sets the
default for the "--submodule" option of diff?

And I think you should also test in t4041 that "--submodule=short"
overrides the config setting.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/diff-config.txt    |    7 +++++++
>  Documentation/diff-options.txt   |    3 ++-
>  diff.c                           |   25 ++++++++++++++++---------
>  t/t4041-diff-submodule-option.sh |   10 ++++++++++
>  4 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 04574f9..e445cc8 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -103,6 +103,13 @@ diff.suppressBlankEmpty::
>  	A boolean to inhibit the standard behavior of printing a space
>  	before each empty output line. Defaults to false.
>  
> +diff.submoduleFormat::
> +	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..034c4e7 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.submoduleFormat` configuration variable.
>  
>  --color[=<when>]::
>  	Show colored diff.
> diff --git a/diff.c b/diff.c
> index 8ea40f9..4cb8dd2 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -28,6 +28,7 @@ static int diff_suppress_blank_empty;
>  static int diff_use_color_default = -1;
>  static const char *diff_word_regex_cfg;
>  static const char *external_diff_cmd_cfg;
> +static const char *submodule_format_cfg;
>  int diff_auto_refresh_index = 1;
>  static int diff_mnemonic_prefix;
>  static int diff_no_prefix;
> @@ -161,6 +162,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.submoduleformat"))
> +		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"))
> @@ -2227,15 +2230,19 @@ static void builtin_diff(const char *name_a,
>  		line_prefix = msgbuf->buf;
>  	}
>  
> -	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
> -			(!one->mode || S_ISGITLINK(one->mode)) &&
> -			(!two->mode || S_ISGITLINK(two->mode))) {
> -		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
> -		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);
> -		return;
> +	if (!DIFF_OPT_TST(o, SUBMODULE_SHORT) &&
> +		((!one->mode || S_ISGITLINK(one->mode)) &&
> +			(!two->mode || S_ISGITLINK(two->mode)))) {
> +		if (DIFF_OPT_TST(o, SUBMODULE_LOG) ||
> +			(submodule_format_cfg &&
> +				!strcmp(submodule_format_cfg, "log"))) {
> +			const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
> +			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);
> +			return;
> +		}
>  	}
>  
>  	if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 6c01d0c..ed4f3a8 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -43,6 +43,16 @@ EOF
>  	test_cmp expected actual
>  "
>  
> +test_expect_success 'added submodule, set diff.submoduleFormat' "
> +	git config diff.submoduleFormat log &&
> +	git add sm1 &&
> +	git diff --cached >actual &&
> +	cat >expected <<-EOF &&
> +Submodule sm1 0000000...$head1 (new submodule)
> +EOF
> +	test_cmp expected actual
> +"
> +
>  commit_file sm1 &&
>  head2=$(add_file sm1 foo3)
>  
> 

Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable

[Jens Lehmann]

Am 02.10.2012 21:44, schrieb Jens Lehmann:
> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
>> Introduce a diff.submoduleFormat configuration variable corresponding
>> to the '--submodule' command-line option of 'git diff'.
> 
> Nice. Maybe a better name would be "diff.submodule", as this sets the
> default for the "--submodule" option of diff?
> 
> And I think you should also test in t4041 that "--submodule=short"
> overrides the config setting.

We also need tests which show that setting that config to "log" does
not break one of the many users of "git diff" ("stash", "rebase" and
"format-patch" come to mind, most probably I missed some others). I
suspect we'll have to add "--submodule=short" options to some call
sites to keep them working with submodule changes.

Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
>> Currently, the diff code does not differentiate between an explicit
>> '--submodule=short' being passed, and no submodule option being passed
>> on the command line.  Making this differentiation will be important
>> when the command-line option can be used to override a
>> "diff.submoduleFormat" configuration variable introduced in the next
>> patch.
>
> Wouldn't it be sufficient here to simply reset the log flag by using
> "DIFF_OPT_CLR(options, SUBMODULE_LOG)"? This would avoid having to
> use the last bit of the diffopt flags. And if I read the code correctly,
> diff_opt_parse() is called by setup_revisions() which is called after
> git_config(), so that should be safe. (And "textconv" uses the same
> approach)

How is it sufficient?  In git_diff_ui_config(), I set
submodule_format_cfg, which has nothing to do with SUBMODULE_LOG.  In
builtin_diff(), I'll have to check SUBMODULE_LOG and
submodule_format_cfg.  The tricky bit is that I should check
submodule_format_cfg if and only if "--submodule=short" was NOT passed
on the command line-  now, that's not the same thing is checking if
SUBMODULE_LOG is unset, because SUBMODULE_LOG is unset (or cleared) if
no argument was passed or if "--submodule=short" is passed.
Therefore, I need a SUBMODULE_SHORT to differentiate between the two
cases.

What am I missing?

Ram

Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option

[Jens Lehmann]

Am 07.10.2012 17:22, schrieb Ramkumar Ramachandra:
> Jens Lehmann wrote:
>> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
>>> Currently, the diff code does not differentiate between an explicit
>>> '--submodule=short' being passed, and no submodule option being passed
>>> on the command line.  Making this differentiation will be important
>>> when the command-line option can be used to override a
>>> "diff.submoduleFormat" configuration variable introduced in the next
>>> patch.
>>
>> Wouldn't it be sufficient here to simply reset the log flag by using
>> "DIFF_OPT_CLR(options, SUBMODULE_LOG)"? This would avoid having to
>> use the last bit of the diffopt flags. And if I read the code correctly,
>> diff_opt_parse() is called by setup_revisions() which is called after
>> git_config(), so that should be safe. (And "textconv" uses the same
>> approach)
> 
> How is it sufficient?  In git_diff_ui_config(), I set
> submodule_format_cfg, which has nothing to do with SUBMODULE_LOG.  In
> builtin_diff(), I'll have to check SUBMODULE_LOG and
> submodule_format_cfg.  The tricky bit is that I should check
> submodule_format_cfg if and only if "--submodule=short" was NOT passed
> on the command line-  now, that's not the same thing is checking if
> SUBMODULE_LOG is unset, because SUBMODULE_LOG is unset (or cleared) if
> no argument was passed or if "--submodule=short" is passed.
> Therefore, I need a SUBMODULE_SHORT to differentiate between the two
> cases.
> 
> What am I missing?

I forgot to mention that testing submodule_format_cfg would have to
happen in cmd_diff() (between reading the config and parsing the
command line options) instead of builtin_diff(). Something like this
should do the trick (untested):

diff --git a/builtin/diff.c b/builtin/diff.c
index 9650be2..180bf44 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -297,6 +297,9 @@ 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);

+       if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
+               DIFF_OPT_SET(options, SUBMODULE_LOG);
+
        if (nongit)
                die(_("Not a git repository"));
        argc = setup_revisions(argc, argv, &rev, NULL);

Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option

[Jens Lehmann]

Am 07.10.2012 21:49, schrieb Jens Lehmann:
> I forgot to mention that testing submodule_format_cfg would have to
> happen in cmd_diff() (between reading the config and parsing the
> command line options) instead of builtin_diff(). Something like this
> should do the trick (untested):
> 
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 9650be2..180bf44 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -297,6 +297,9 @@ 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);
> 
> +       if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
> +               DIFF_OPT_SET(options, SUBMODULE_LOG);

Obviously not even compile tested, the line above should read:

+               DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);

> +
>         if (nongit)
>                 die(_("Not a git repository"));
>         argc = setup_revisions(argc, argv, &rev, NULL);

Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> Am 02.10.2012 21:44, schrieb Jens Lehmann:
>> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
>>> Introduce a diff.submoduleFormat configuration variable corresponding
>>> to the '--submodule' command-line option of 'git diff'.
>>
>> Nice. Maybe a better name would be "diff.submodule", as this sets the
>> default for the "--submodule" option of diff?
>>
>> And I think you should also test in t4041 that "--submodule=short"
>> overrides the config setting.
>
> We also need tests which show that setting that config to "log" does
> not break one of the many users of "git diff" ("stash", "rebase" and
> "format-patch" come to mind, most probably I missed some others). I
> suspect we'll have to add "--submodule=short" options to some call
> sites to keep them working with submodule changes.

Um, why would "stash", "rebase" or "format-patch" be affected by this
setting?  They don't operate on submodules at all.  To be sure, I ran
all the tests with the following diff and nothing broke.

Confused,

Ram

-- 8< --
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 514282c..904a81c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -608,6 +608,8 @@ fi
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$test" || exit 1

+git config test.submodule log
+
 this_test=${0##*/}
 this_test=${this_test%%-*}
 for skp in $GIT_SKIP_TESTS

Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable

[Jens Lehmann]

Am 29.10.2012 11:30, schrieb Ramkumar Ramachandra:
> Jens Lehmann wrote:
>> Am 02.10.2012 21:44, schrieb Jens Lehmann:
>>> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
>>>> Introduce a diff.submoduleFormat configuration variable corresponding
>>>> to the '--submodule' command-line option of 'git diff'.
>>>
>>> Nice. Maybe a better name would be "diff.submodule", as this sets the
>>> default for the "--submodule" option of diff?
>>>
>>> And I think you should also test in t4041 that "--submodule=short"
>>> overrides the config setting.
>>
>> We also need tests which show that setting that config to "log" does
>> not break one of the many users of "git diff" ("stash", "rebase" and
>> "format-patch" come to mind, most probably I missed some others). I
>> suspect we'll have to add "--submodule=short" options to some call
>> sites to keep them working with submodule changes.
> 
> Um, why would "stash", "rebase" or "format-patch" be affected by this
> setting?  They don't operate on submodules at all.  To be sure, I ran
> all the tests with the following diff and nothing broke.

They do operate on the submodule commits too (while they don't touch
submodule work trees) and IIRC rebase applies diffs, so that could
break when the output of diff changes due to the new config option.
But it looks like your test did prove that nothing goes wrong there,
I assume they they use plumbing diff commands which aren't affected
by the new option.