{"thread":{"id":"19012","subject":"text in --stat is colored like color.diff.plain","startedAt":"2009-04-22T21:13:49Z","lastAt":"2009-04-25T11:09:39Z","messageCount":6,"participants":["Markus Heidelberg","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"112009","messageId":"200904222313.50081.markus.heidelberg@web.de","threadId":"19012","inReplyTo":null,"subject":"text in --stat is colored like color.diff.plain","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-22T21:13:49Z","receivedAt":"2009-04-22T21:13:49Z","isPatch":false,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"I noticed, that the filenames and the text \"X files changed, Y\ninsertions(+), Z deletions(-)\" in the --stat output ended with an ESC[m,\nalthough they were not colored. Further investigation showed, that\nthese parts of the output indeed can use colors from color.diff.plain.\n\nIs this intention? Commit 785f743 (diff --stat: color output.,\n2006-09-26) introduced it, but only mentioned colored output of the\n+++++++---- diffstat graph in the commit message. color.diff.plain is\ndocumented for coloring context text and I don't think it looks good\neither as it is now, nor is this really \"context text\".\nNote: --shortstat and --summary don't use colors like --stat. Or is this\nreally intention and I don't see the reason for it?\n\nIf not, should we remove the colors for --stat?\nAfter that maybe we could do some more color.diff.old/new coloring for\n\"Y insertions(+)\", \"Z deletions(-)\", \"create mode ...\", \"delete mode ...\"\n\nMarkus\n"},{"id":"112244","messageId":"1240610808-7285-1-git-send-email-markus.heidelberg@web.de","threadId":"19012","inReplyTo":"200904222313.50081.markus.heidelberg@web.de","subject":"[PATCH 1/2] diff: do not color --stat output like patch context","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-24T22:06:47Z","receivedAt":"2009-04-24T22:06:47Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"The diffstat used the color.diff.plain slot (context text) for coloring\nfilenames and the whole summary line. This didn't look nice and the\naffected text isn't patch context at all.\n\nSigned-off-by: Markus Heidelberg <markus.heidelberg@web.de>\n---\n\n    Maybe it's easier to begin with a patch :)\n\n diff.c |   15 +++++++--------\n 1 files changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3ac7168..d581d4d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -839,10 +839,9 @@ static int scale_linear(int it, int width, int max_change)\n }\n \n static void show_name(FILE *file,\n-\t\t      const char *prefix, const char *name, int len,\n-\t\t      const char *reset, const char *set)\n+\t\t      const char *prefix, const char *name, int len)\n {\n-\tfprintf(file, \" %s%s%-*s%s |\", set, prefix, len, name, reset);\n+\tfprintf(file, \" %s%-*s |\", prefix, len, name);\n }\n \n static void show_graph(FILE *file, char ch, int cnt, const char *set, const char *reset)\n@@ -956,7 +955,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t}\n \n \t\tif (data->files[i]->is_binary) {\n-\t\t\tshow_name(options->file, prefix, name, len, reset, set);\n+\t\t\tshow_name(options->file, prefix, name, len);\n \t\t\tfprintf(options->file, \"  Bin \");\n \t\t\tfprintf(options->file, \"%s%d%s\", del_c, deleted, reset);\n \t\t\tfprintf(options->file, \" -> \");\n@@ -966,7 +965,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\telse if (data->files[i]->is_unmerged) {\n-\t\t\tshow_name(options->file, prefix, name, len, reset, set);\n+\t\t\tshow_name(options->file, prefix, name, len);\n \t\t\tfprintf(options->file, \"  Unmerged\\n\");\n \t\t\tcontinue;\n \t\t}\n@@ -988,7 +987,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\tadd = scale_linear(add, width, max_change);\n \t\t\tdel = scale_linear(del, width, max_change);\n \t\t}\n-\t\tshow_name(options->file, prefix, name, len, reset, set);\n+\t\tshow_name(options->file, prefix, name, len);\n \t\tfprintf(options->file, \"%5d%s\", added + deleted,\n \t\t\t\tadded + deleted ? \" \" : \"\");\n \t\tshow_graph(options->file, '+', add, add_c, reset);\n@@ -996,8 +995,8 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\tfprintf(options->file, \"\\n\");\n \t}\n \tfprintf(options->file,\n-\t       \"%s %d files changed, %d insertions(+), %d deletions(-)%s\\n\",\n-\t       set, total_files, adds, dels, reset);\n+\t       \" %d files changed, %d insertions(+), %d deletions(-)\\n\",\n+\t       total_files, adds, dels);\n }\n \n static void show_shortstats(struct diffstat_t* data, struct diff_options *options)\n-- \n1.6.3.rc1.84.g1036b\n"},{"id":"112245","messageId":"1240610808-7285-2-git-send-email-markus.heidelberg@web.de","threadId":"19012","inReplyTo":"200904222313.50081.markus.heidelberg@web.de","subject":"[PATCH 2/2] diff: color statistics (stat, shortstat, numstat)","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-24T22:06:48Z","receivedAt":"2009-04-24T22:06:48Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"stat/shortstat:\nColor added and removed lines and the corresponding signs ('+' and '-')\nin the summary.\n\nnumstat:\nColor added and removed lines per file.\n\nSigned-off-by: Markus Heidelberg <markus.heidelberg@web.de>\n---\n\n    I didn't consider --summary and --name-status. Also in --stat it\n    would be possible to color filenames in renames and copies and the\n    mode in mode changes. Not sure if it would be nice or distracting.\n    At least it wouldn't look so consistent, I think.\n\n diff.c |   26 ++++++++++++++++++++------\n 1 files changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d581d4d..bc8377d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -994,18 +994,24 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\tshow_graph(options->file, '-', del, del_c, reset);\n \t\tfprintf(options->file, \"\\n\");\n \t}\n-\tfprintf(options->file,\n-\t       \" %d files changed, %d insertions(+), %d deletions(-)\\n\",\n-\t       total_files, adds, dels);\n+\tfprintf(options->file, \" %d files changed, %s%d%s insertions(%s+%s), \"\n+\t\t\t\t\t\t  \"%s%d%s deletions(%s-%s)\\n\",\n+\t\t\t\ttotal_files, add_c, adds, reset, add_c, reset,\n+\t\t\t\t\t     del_c, dels, reset, del_c, reset);\n }\n \n static void show_shortstats(struct diffstat_t* data, struct diff_options *options)\n {\n \tint i, adds = 0, dels = 0, total_files = data->nr;\n+\tconst char *reset, *add_c, *del_c;\n \n \tif (data->nr == 0)\n \t\treturn;\n \n+\treset = diff_get_color_opt(options, DIFF_RESET);\n+\tadd_c = diff_get_color_opt(options, DIFF_FILE_NEW);\n+\tdel_c = diff_get_color_opt(options, DIFF_FILE_OLD);\n+\n \tfor (i = 0; i < data->nr; i++) {\n \t\tif (!data->files[i]->is_binary &&\n \t\t    !data->files[i]->is_unmerged) {\n@@ -1020,17 +1026,24 @@ static void show_shortstats(struct diffstat_t* data, struct diff_options *option\n \t\t\t}\n \t\t}\n \t}\n-\tfprintf(options->file, \" %d files changed, %d insertions(+), %d deletions(-)\\n\",\n-\t       total_files, adds, dels);\n+\tfprintf(options->file, \" %d files changed, %s%d%s insertions(%s+%s), \"\n+\t\t\t\t\t\t  \"%s%d%s deletions(%s-%s)\\n\",\n+\t\t\t\ttotal_files, add_c, adds, reset, add_c, reset,\n+\t\t\t\t\t     del_c, dels, reset, del_c, reset);\n }\n \n static void show_numstat(struct diffstat_t* data, struct diff_options *options)\n {\n \tint i;\n+\tconst char *reset, *add_c, *del_c;\n \n \tif (data->nr == 0)\n \t\treturn;\n \n+\treset = diff_get_color_opt(options, DIFF_RESET);\n+\tadd_c = diff_get_color_opt(options, DIFF_FILE_NEW);\n+\tdel_c = diff_get_color_opt(options, DIFF_FILE_OLD);\n+\n \tfor (i = 0; i < data->nr; i++) {\n \t\tstruct diffstat_file *file = data->files[i];\n \n@@ -1038,7 +1051,8 @@ static void show_numstat(struct diffstat_t* data, struct diff_options *options)\n \t\t\tfprintf(options->file, \"-\\t-\\t\");\n \t\telse\n \t\t\tfprintf(options->file,\n-\t\t\t\t\"%d\\t%d\\t\", file->added, file->deleted);\n+\t\t\t\t\"%s%d%s\\t%s%d%s\\t\", add_c, file->added, reset,\n+\t\t\t\t\t\t    del_c, file->deleted, reset);\n \t\tif (options->line_termination) {\n \t\t\tfill_print_name(file);\n \t\t\tif (!file->is_renamed)\n-- \n1.6.3.rc1.84.g1036b\n"},{"id":"112250","messageId":"20090424222635.GA14604@coredump.intra.peff.net","threadId":"19012","inReplyTo":"1240610808-7285-2-git-send-email-markus.heidelberg@web.de","subject":"Re: [PATCH 2/2] diff: color statistics (stat, shortstat, numstat)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-24T22:26:35Z","receivedAt":"2009-04-24T22:26:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 25, 2009 at 12:06:48AM +0200, Markus Heidelberg wrote:\n\n> numstat:\n> Color added and removed lines per file.\n\nSomehow I thought numstat was about being easy to parse for plumbing.\nAre people really using it for human-readable output?\n\n-Peff\n"},{"id":"112255","messageId":"7vbpqlac6n.fsf@gitster.siamese.dyndns.org","threadId":"19012","inReplyTo":"1240610808-7285-2-git-send-email-markus.heidelberg@web.de","subject":"Re: [PATCH 2/2] diff: color statistics (stat, shortstat, numstat)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-24T23:01:36Z","receivedAt":"2009-04-24T23:01:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Heidelberg <markus.heidelberg@web.de> writes:\n\n> stat/shortstat:\n> Color added and removed lines and the corresponding signs ('+' and '-')\n> in the summary.\n>\n> numstat:\n> Color added and removed lines per file.\n\nI find this extremely unreadable.  Also numstat being for porcelain use, I\ndo not see the point.\n\nI think [1/2] that removes the (plain)coloring of the stat summary text is\nOk.  The code is painting the stat summary in the same color as the\nfilenames in the stat graph, and the default \"plain\" color happens to be\n\"do not color--just use the terminal default\", so it probably does not\nhave any practical difference.\n"},{"id":"112292","messageId":"200904251309.39816.markus.heidelberg@web.de","threadId":"19012","inReplyTo":"7vbpqlac6n.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] diff: color statistics (stat, shortstat, numstat)","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-04-25T11:09:39Z","receivedAt":"2009-04-25T11:09:39Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Junio C Hamano, 25.04.2009:\n> Markus Heidelberg <markus.heidelberg@web.de> writes:\n> \n> > stat/shortstat:\n> > Color added and removed lines and the corresponding signs ('+' and '-')\n> > in the summary.\n> >\n> > numstat:\n> > Color added and removed lines per file.\n> \n> I find this extremely unreadable.\n\nI know one can overdo it with colors, but I don't think it's the case\nfor the stat/shortstat summary line here. Of course this is very\nsubjective and if the consensus is not to have colors there, then it's OK.\n\n> Also numstat being for porcelain use, I\n> do not see the point.\n\nIt gives you an information that --stat doesn't give you: the exact\ncount of added and removed lines per file. Also for long path names you\ndon't have to bother with --stat=x,y\n\nHaving said this, I have to admit that I didn't use it myself up to now.\n\n> I think [1/2] that removes the (plain)coloring of the stat summary text is\n> Ok.\n\nDo you mean the whole patch 1/2 is OK or only the summary part and the\nfilenames should stay with plain coloring? I think the former.\n\n> The code is painting the stat summary in the same color as the\n> filenames in the stat graph, and the default \"plain\" color happens to be\n> \"do not color--just use the terminal default\", so it probably does not\n> have any practical difference.\n\nEh, yes, for people setting color.diff.plain\n"}]}