Re: [PATCH] config.txt,diff-options.txt: porcelain vs. plumbing for color.diff

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 27, 2011, 09:03 UTC
Message-ID: <20110427090323.GA14849@elie>
In-Reply-To: <24a6907cade7aedb51dc20ab5977603ca21e70bb.1303889849.git.git@drmicha.warpmail.net>

Michael J Gruber wrote:

> Reading the diff-family and config man pages one may think that the
> color.diff and color.ui settings apply to all diff commands. Make it
> clearer that they do not apply to the plumbing variants
> diff-{files,index,tree}.

Sounds like a good idea.  Quick reactions:

Show 10 quoted lines

> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -708,7 +708,7 @@ second is the background.  The position of the attribute, if any,
>  doesn't matter.
>  
>  color.diff::
> -	When set to `always`, always use colors in patch.
> +	When set to `always`, always use colors in patch for porcelain commands.
>  	When false (or `never`), never.  When set to `true` or `auto`, use
>  	colors only when the output is to the terminal. Defaults to false.

I am not sure I like promising that all porcelain commands will use color; for example, "git commit -v" does not (though "commit -v --dry-run" does). Maybe:

	Whether to use ANSI escape sequences to add color to patches.
	If this is set to `always`, linkgit:git-diff[1],
	linkgit:git-log[1], and linkgit:git-show[1] will use color
	for all patches.  If it is set to `true` or `auto`, those
	commands will only use color when output is to the terminal.
	Defaults to false.
+
This does not affect linkgit:git-format-patch[1] or the
'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
command line with the `--color[=<when>]` option.

Show 7 quoted lines

> @@ -796,7 +796,7 @@ color.status.<slot>::
>  	color.branch.<slot>.
>  
>  color.ui::
> -	When set to `always`, always use colors in all git commands which
> +	When set to `always`, always use colors in all porcelain commands which
>  	are capable of colored output. When false (or `never`), never. When

Nitpick: the grammatical subject of the dependent clause ("when set to
always") and the rest of the sentence ("always use colors") differ, so
the sentence sounds awkward.  Not your fault, but while we're here, it
might be nice to change that.

I think the intent is something like

	This variable determines the default for color.branch,
	color.diff, color.grep, color.interactive, color.showbranch,
	and color.status, and its scope will expand as other commands
	learn configuration to set a default for the --color option.
	Set it to "always" if you want all output not intended for
	machine consumption to use color, to "true" or "auto" if you
	want such output to use color when written to the terminal,
	or to "false" or "never" if you prefer git commands not to
	use color unless enabled explicitly with some other
	configuration or the --color option.

In other words, it's closer to "all appropriate commands" than "all porcelain". format-patch is porcelain. Not sure what a concise way to say that is, though.

Thanks, and hope that helps. Jonathan