[BUG] git-diff-* --color oddness

[BUG] git-diff-* --color oddness

[Jeff King]

I encountered some weirdness today with git-diff-{files,tree,index} and
--color.  They parse the --color command line option, but do _not_ call
git_config(git_diff_ui_config), so if the user has colors specified in
the config, they are not used.

Ordinarily, I would say "nobody should use --color with diff-files". But
we do, in git-add--interactive (see Wincent's 4af756f3). So we can
probably fix that with a "git_diff_minimal_ui_config" which does just
the colors, and use that from the plumbing diff scripts (or just move
the color stuff into git_default_config).

But it gets worse. Try this:

  # make a custom color
  git config color.diff.meta yellow
  # now show a plumbing diff
  git diff-tree --color -p 257f3020^ 257f3020

The first two lines of meta-info will be in the stock colors, but
everything after will be in the custom colors. So we are actually
reading the diff_ui options _during_ the diff. The culprit is
funcname_pattern, which calls read_config_if_needed.

It's my understanding that diff_ui_config is about porcelain diff
options, so it is a bit worrisome that in the middle of a plumbing diff
process, we load porcelain config. Fortunately, most of them aren't
looked at except in diff_setup, which has already run. Colors actually
affect globals which are used during the diff, and so are noticed.

So I think the best course is probably:
  1. before v1.5.4, we need to fix the color handling in "git-diff -i",
     either by declaring git-diff-files --color illegal and finding
     another way to do 4af756f3, or by moving the diff color
     functionality into the default config.

  2. Probably post-v1.5.4, some thought should be given to
     how the diff ui config is demand-loaded by
     diff.c:read_config_if_needed. I don't _think_ it is a problem for
     now except for the colors, because most parts just get used in
     diff_setup. But it seems a bit flaky.

-Peff

Re: [BUG] git-diff-* --color oddness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I encountered some weirdness today with git-diff-{files,tree,index} and
> --color.  They parse the --color command line option, but do _not_ call
> git_config(git_diff_ui_config), so if the user has colors specified in
> the config, they are not used.

This is very deliberate.  ui_config is about the Porcelains.

> Ordinarily, I would say "nobody should use --color with diff-files". But
> we do, in git-add--interactive (see Wincent's 4af756f3).

The plumbing deliberately not calling ui_config does not have
anything to do with them supporting explicit --color options.

Re: [BUG] git-diff-* --color oddness

[Jeff King]

On Fri, Jan 04, 2008 at 12:24:01AM -0800, Junio C Hamano wrote:

> > Ordinarily, I would say "nobody should use --color with diff-files". But
> > we do, in git-add--interactive (see Wincent's 4af756f3).
> 
> The plumbing deliberately not calling ui_config does not have
> anything to do with them supporting explicit --color options.

I'm not sure what you mean here. Are you saying that it is the desired
behavior for "git-diff --color" to use my color.diff.* variables, but
for "git-diff-files --color" not to?

I think that's insane, and it makes "git-add -i" depending on
git-diff-file's colorization wrong (since in other parts it uses the
color.diff.* variables).

Not to mention the other bug (that diff-files _does_ read the config,
just halfway through).

-Peff

Re: [BUG] git-diff-* --color oddness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure what you mean here. Are you saying that it is the desired
> behavior for "git-diff --color" to use my color.diff.* variables, but
> for "git-diff-files --color" not to?

What I meant is "git diff" without --color can be colorized
because of config but we should never allow "git diff-files"
without --color to be colorized by user's config.  I realize
that you were talking about the choice of colors, which is a
different issue.

I do not much care ;-), but I guess we would want to be
consistent.

> Not to mention the other bug (that diff-files _does_ read the config,
> just halfway through).

That one _is_ a bug.  diff-files should not be affected by
"diff.color = auto" or somesuch in the config, even when the
user uses custom function header crap.

Re: [BUG] git-diff-* --color oddness

[Jeff King]

On Fri, Jan 04, 2008 at 12:35:42AM -0800, Junio C Hamano wrote:

> What I meant is "git diff" without --color can be colorized
> because of config but we should never allow "git diff-files"
> without --color to be colorized by user's config.  I realize
> that you were talking about the choice of colors, which is a
> different issue.
> 
> I do not much care ;-), but I guess we would want to be
> consistent.

OK, yes, I knew that about diff.color already. But I think it is a bug
to not use the user's colors in "git add -i", and I think the right fix
is to make diff-files consistent in its color choices. Patch will
follow.

> That one _is_ a bug.  diff-files should not be affected by
> "diff.color = auto" or somesuch in the config, even when the
> user uses custom function header crap.

It doesn't use it, actually, but I think that is a happy accident of the
way most config options are used (i.e., we _do_ read and change the
config, but we just never look at it again because we have done our
diff_setup). If you could provoke any of the diff plumbing to call
diff_setup twice, you would be in trouble (but even diff-tree --stdin
doesn't do that).

-Peff

Re: [BUG] git-diff-* --color oddness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The first two lines of meta-info will be in the stock colors, but
> everything after will be in the custom colors. So we are actually
> reading the diff_ui options _during_ the diff. The culprit is
> funcname_pattern, which calls read_config_if_needed.

Yuck.  Why is funcname_pattern do ui-config stuff?  I know it
wants to get custom regexp crap, but that should belong to the
plumbing part, not Porcelain-only thing, shouldn't it?

Re: [BUG] git-diff-* --color oddness

[Jeff King]

On Fri, Jan 04, 2008 at 12:26:55AM -0800, Junio C Hamano wrote:

> > The first two lines of meta-info will be in the stock colors, but
> > everything after will be in the custom colors. So we are actually
> > reading the diff_ui options _during_ the diff. The culprit is
> > funcname_pattern, which calls read_config_if_needed.
> 
> Yuck.  Why is funcname_pattern do ui-config stuff?  I know it
> wants to get custom regexp crap, but that should belong to the
> plumbing part, not Porcelain-only thing, shouldn't it?

It's for user diff drivers, and it looks like the funcname pattern. Not
sure why we want to demand-load that stuff at all...I wonder if it
should just go into git_default_config (or a git_basic_diff_config).

-Peff

Re: [BUG] git-diff-* --color oddness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 04, 2008 at 12:26:55AM -0800, Junio C Hamano wrote:
>
>> > The first two lines of meta-info will be in the stock colors, but
>> > everything after will be in the custom colors. So we are actually
>> > reading the diff_ui options _during_ the diff. The culprit is
>> > funcname_pattern, which calls read_config_if_needed.
>> 
>> Yuck.  Why is funcname_pattern do ui-config stuff?  I know it
>> wants to get custom regexp crap, but that should belong to the
>> plumbing part, not Porcelain-only thing, shouldn't it?
>
> It's for user diff drivers, and it looks like the funcname pattern. Not
> sure why we want to demand-load that stuff at all...I wonder if it
> should just go into git_default_config (or a git_basic_diff_config).

Yeah, moving some to the diff-basic-config sounds sane.

 * I'd prefer to keep low-level produce consistent diff that can
   be reliably applied with git-apply without getting broken by
   user configuration while Porcelain level diff can be tweaked
   by the user to do whatever "human readable" crap they would
   want to see on the screen (such as "side by side"), and my
   gut feeling is that we should keep the user-level driver
   definition in ui-config, never to affect plumbing.

 * using or not-using colors should stay in ui-config;

 * funcname-pattern can go either way; that affects what appears
   at the end of @@ context @@ lines, and would not have risk to
   corrupt the patch for plumbing.

 * color choice can also go either way, but probably is better
   to be in the low-level.  Cnce color is used the output cannot
   be fed sanely to patch or git-apply _anyway_ so we can be
   sure that "git diff-plumbing --color" is run to produce human
   readable output.

Re: [BUG] git-diff-* --color oddness

[Jeff King]

On Fri, Jan 04, 2008 at 12:46:18AM -0800, Junio C Hamano wrote:

> > It's for user diff drivers, and it looks like the funcname pattern. Not
> > sure why we want to demand-load that stuff at all...I wonder if it
> > should just go into git_default_config (or a git_basic_diff_config).
> 
> Yeah, moving some to the diff-basic-config sounds sane.

OK, below is a patch that fixes the color issues I was having.

>  * I'd prefer to keep low-level produce consistent diff that can
>    be reliably applied with git-apply without getting broken by
>    user configuration while Porcelain level diff can be tweaked
>    by the user to do whatever "human readable" crap they would
>    want to see on the screen (such as "side by side"), and my
>    gut feeling is that we should keep the user-level driver
>    definition in ui-config, never to affect plumbing.

OK. In that case, we need a way for the plumbing to tell the diff
machinery "don't ever try loading the ui config."

>  * using or not-using colors should stay in ui-config;

Agreed.

>  * funcname-pattern can go either way; that affects what appears
>    at the end of @@ context @@ lines, and would not have risk to
>    corrupt the patch for plumbing.

I don't personally use this, so I don't care too much. But I am inclined
to say low-level since it cannot break the patch (and it will be used
and presented by porcelains like "git add -i").

>  * color choice can also go either way, but probably is better
>    to be in the low-level.  Cnce color is used the output cannot
>    be fed sanely to patch or git-apply _anyway_ so we can be
>    sure that "git diff-plumbing --color" is run to produce human
>    readable output.

Definitely low-level IMHO, since we have porcelain which relies on the
low-level's output (and there is no way to set it manually from the
command line).

Patch to add diff_basic_config infrastructure and fix the color
selection issue is below. It just does the color, but we can move other
stuff over as discussion ensues.

-- >8 --
add a "basic" diff config callback

The diff porcelain uses git_diff_ui_config to set
porcelain-ish config options, like automatically turning on
color. The plumbing specifically avoids calling this
function, since it doesn't want things like automatic color
or rename detection.

However, some diff options should be set for both plumbing
and porcelain. For example, one can still turn on color in
git-diff-files using the --color command line option. This
means we want the color config from color.diff.* (so that
once color is on, we use the user's preferred scheme), but
_not_ the color.diff variable.

We split the diff config into "ui" and "basic", where
"basic" is suitable for use by plumbing (so _most_ things
affecting the output should still go into the "ui" part).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-diff-files.c |    2 +-
 builtin-diff-index.c |    2 +-
 builtin-diff-tree.c  |    2 +-
 diff.c               |    6 ++++++
 diff.h               |    1 +
 5 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 9c04111..4abe3c2 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -21,7 +21,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 
 	prefix = setup_git_directory_gently(&nongit);
 	init_revisions(&rev, prefix);
-	git_config(git_default_config); /* no "diff" UI options */
+	git_config(git_diff_basic_config); /* no "diff" UI options */
 	rev.abbrev = 0;
 
 	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 0f2390a..2b955de 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -17,7 +17,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int result;
 
 	init_revisions(&rev, prefix);
-	git_config(git_default_config); /* no "diff" UI options */
+	git_config(git_diff_basic_config); /* no "diff" UI options */
 	rev.abbrev = 0;
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index ebc50ef..832797f 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -68,7 +68,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	int read_stdin = 0;
 
 	init_revisions(opt, prefix);
-	git_config(git_default_config); /* no "diff" UI options */
+	git_config(git_diff_basic_config); /* no "diff" UI options */
 	nr_sha1 = 0;
 	opt->abbrev = 0;
 	opt->diff = 1;
diff --git a/diff.c b/diff.c
index f6a8515..6bb0f62 100644
--- a/diff.c
+++ b/diff.c
@@ -183,6 +183,12 @@ int git_diff_ui_config(const char *var, const char *value)
 				return parse_funcname_pattern(var, ep, value);
 		}
 	}
+
+	return git_diff_basic_config(var, value);
+}
+
+int git_diff_basic_config(const char *var, const char *value)
+{
 	if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
 		int slot = parse_diff_color_slot(var, 11);
 		color_parse(value, var, diff_colors[slot]);
diff --git a/diff.h b/diff.h
index beccf85..073d5cb 100644
--- a/diff.h
+++ b/diff.h
@@ -172,6 +172,7 @@ extern void diff_unmerge(struct diff_options *,
 #define DIFF_SETUP_USE_CACHE		2
 #define DIFF_SETUP_USE_SIZE_CACHE	4
 
+extern int git_diff_basic_config(const char *var, const char *value);
 extern int git_diff_ui_config(const char *var, const char *value);
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
-- 
1.5.4.rc2.1123.gce734-dirty

Re: [BUG] git-diff-* --color oddness

[Jeff King]

On Fri, Jan 04, 2008 at 03:59:34AM -0500, Jeff King wrote:

> OK. In that case, we need a way for the plumbing to tell the diff
> machinery "don't ever try loading the ui config."

> >  * funcname-pattern can go either way; that affects what appears
> >    at the end of @@ context @@ lines, and would not have risk to
> >    corrupt the patch for plumbing.

I just sent a patch to put the funcname pattern in the "basic" config,
and to get rid of the lazy config loading. So that fixes one call by the
plumbing to read_config_if_needed.

And it looks like the second call is already OK. We don't try parsing
the config to get the external diff command unless ALLOW_EXTERNAL is
set, which the plumbing already disallows (though I am still confused
why it would need to be loaded lazily in the first place -- I wonder if
read_config_if_needed is needed at all).

-Peff

Re: [BUG] git-diff-* --color oddness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 04, 2008 at 03:59:34AM -0500, Jeff King wrote:
>
>> OK. In that case, we need a way for the plumbing to tell the diff
>> machinery "don't ever try loading the ui config."
>
>> >  * funcname-pattern can go either way; that affects what appears
>> >    at the end of @@ context @@ lines, and would not have risk to
>> >    corrupt the patch for plumbing.
>
> I just sent a patch to put the funcname pattern in the "basic" config,
> and to get rid of the lazy config loading. So that fixes one call by the
> plumbing to read_config_if_needed.
>
> And it looks like the second call is already OK. We don't try parsing
> the config to get the external diff command unless ALLOW_EXTERNAL is
> set, which the plumbing already disallows (though I am still confused
> why it would need to be loaded lazily in the first place -- I wonder if
> read_config_if_needed is needed at all).

I think that was a premature optimization without benching.  It
is expected that most trees would not have attributes to define
custom low-level diff types, and without them we do not need to
parse the configuration to find out the external commands to be
used.

Re: [BUG] git-diff-* --color oddness

[Jeff King]

On Fri, Jan 04, 2008 at 01:37:46AM -0800, Junio C Hamano wrote:

> > And it looks like the second call is already OK. We don't try parsing
> > the config to get the external diff command unless ALLOW_EXTERNAL is
> > set, which the plumbing already disallows (though I am still confused
> > why it would need to be loaded lazily in the first place -- I wonder if
> > read_config_if_needed is needed at all).
> 
> I think that was a premature optimization without benching.  It
> is expected that most trees would not have attributes to define
> custom low-level diff types, and without them we do not need to
> parse the configuration to find out the external commands to be
> used.

Ah. But we were parsing them anyway in git_diff_ui_config at the
beginning of the program (and we need to, since you will always have
diff.default.*). So I think this is safe to do, and can replace my other
"only read config once" patch from a few minutes ago:

-- >8 --
diff: remove lazy config loading

There is no point to this. Either:

  1. The program has already loaded git_diff_ui_config, in
     which case this is a noop.
  2. The program didn't, which means it is plumbing that
     does not _want_ git_diff_ui_config to be loaded.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/diff.c b/diff.c
index ecf2fd6..2c78d74 100644
--- a/diff.c
+++ b/diff.c
@@ -59,17 +59,6 @@ static struct ll_diff_driver {
 	char *cmd;
 } *user_diff, **user_diff_tail;
 
-static void read_config_if_needed(void)
-{
-	static int done = 0;
-	if (done)
-		return;
-
-	user_diff_tail = &user_diff;
-	git_config(git_diff_ui_config);
-	done = 1;
-}
-
 /*
  * Currently there is only "diff.<drivername>.command" variable;
  * because there are "diff.color.<slot>" variables, we are parsing
@@ -1825,7 +1814,6 @@ static const char *external_diff_attr(const char *name)
 		    !ATTR_UNSET(value)) {
 			struct ll_diff_driver *drv;
 
-			read_config_if_needed();
 			for (drv = user_diff; drv; drv = drv->next)
 				if (!strcmp(drv->name, value))
 					return drv->cmd;
-- 
1.5.4.rc2.1125.ga305e-dirty