Re: [PATCH 5/5] format-patch: avoid freopen()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 22, 2016, 07:24 UTC
Message-ID: <alpine.DEB.2.20.1606220849480.10382@virtualbox>
In-Reply-To: <xmqqvb12qyeu.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

Show 22 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > That is a very convincing argument. So convincing that I wanted to change
> > the patch to guard behind `diff_use_color_default == GIT_COLOR_AUTO`.
> 
> I actually was expecting, instead of your:
> 
>  	if (output_directory) {
> +		rev.diffopt.use_color = 0;
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
> 
> an update would say
> 
>  	if (output_directory) {
> 		if (rev.diffopt.use_color == GIT_COLOR_AUTO)
>                 	rev.diffopt.use_color = 0;
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
> 
> I didn't expect you to check diff_use_color_default exactly for the
> reason why you say "But that is the wrong variable".

In diff_setup() (which is called by init_revisions() which in turn is called by format-patch's cmd_format_patch()), the use_color field is initialized with the value of diff_use_color_default, though:

	https://github.com/git/git/blob/v2.9.0/diff.c#L3283

Please note that diff_use_color_default is initialized to -1:

	https://github.com/git/git/blob/v2.9.0/diff.c#L32

and set to a different value here:

	https://github.com/git/git/blob/v2.9.0/diff.c#L180

when parsing the diff.color or color.diff config settings, which however are *not* parsed in format-patch, thanks to:

	https://github.com/git/git/blob/v2.9.0/builtin/log.c#L740-L743

Therefore, use_color is initialized to -1, and in format-patch's case it remains like this. I was a bit surprised to see that GIT_COLOR_AUTO's numerical value is *not* -1 (which I would have chosen for the "undecided" case, but I guess that -1 was assumed to mean the "unspecified" case), but the numerical value of 2 instead:

	https://github.com/git/git/blob/v2.9.0/color.h#L59

Hence " rev.diffopt.use_color == GIT_COLOR_AUTO" would evaluate to "-1 == 2" in this context.

Further, I think that the commit message of 7787570c (format-patch: ignore ui.color, 2011-09-13) makes a pretty eloquent case that we *want* to switch off color when letting format-patch write to files.

But there's a rub... If you specify --color *explicitly*, use_color is set to GIT_COLOR_ALWAYS and the file indeed contains ANSI sequences (i.e. my analysis above left out the command-line part).

In short, I think you're right, I have to guard the assignment, with the minor adjustment to test use_color != GIT_COLOR_ALWAYS.

Will reroll.

Ciao, Dscho