git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Nov 11, 2012, 15:09 UTC
Message-ID
<20121111150922.GA10140@sigill.intra.peff.net>
In-Reply-To
<CALkWK0=zTCXki2c=ugRXE485ps2=OWag7mdzVJW93cnsypwkiA@mail.gmail.com>
On Sun, Nov 11, 2012 at 08:20:27PM +0530, Ramkumar Ramachandra wrote:
Show 17 quoted lines
> >> 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)
Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 6 of 10 in “Introduce diff.submodule”
  1. 0/3 Introduce diff.submoduleRamkumar Ramachandra, Nov 1, 2012
  2. 1/3 Documentation: move diff.wordRegex from config.txt to diff-config.txtRamkumar Ramachandra, Nov 1, 2012
  3. 2/3 diff: introduce diff.submodule configuration variableRamkumar Ramachandra, Nov 1, 2012
  4. Jeff KingNov 8, 2012
  5. Ramkumar RamachandraNov 11, 2012
  6. Jeff KingNov 11, 2012
  7. 3/3 submodule: display summary header in boldRamkumar Ramachandra, Nov 1, 2012
  8. Jens LehmannNov 1, 2012
  9. Ramkumar RamachandraNov 4, 2012
  10. Jeff KingNov 8, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.