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

[Jeff King <peff@peff.net>]

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)