Hi Junio,
On Tue, 21 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html