{"thread":{"id":"5714","subject":"[PATCH 1/3] diff --stat: allow custom diffstat output width.","startedAt":"2006-09-27T02:40:43Z","lastAt":"2006-09-29T06:17:14Z","messageCount":12,"participants":["Junio C Hamano","David Rientjes","Linus Torvalds","Adrian Bunk","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"27739","messageId":"7vr6xyjal0.fsf@assigned-by-dhcp.cox.net","threadId":"5714","inReplyTo":null,"subject":"[PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-27T02:40:43Z","receivedAt":"2006-09-27T02:40:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds two parameters to \"diff --stat\".\n\n . --stat-width=72 tells that the page should fit on 72-column output.\n\n . --stat-name-width=30 tells that the filename part is limited\n   to 30 columns.\n\nWithout these flags, they default to 80 and 50 as before.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n * Adrian Bunk on the kernel list wanted ot have \"diffstat\n   -w72\", so here it is.\n\n diff.c |  132 ++++++++++++++++++++++++++++++++++++++++++++++------------------\n diff.h |    3 +\n 2 files changed, 97 insertions(+), 38 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 443e248..6101365 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -545,21 +545,63 @@ static void diffstat_consume(void *priv,\n \t\tx->deleted++;\n }\n \n-static const char pluses[] = \"++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\";\n-static const char minuses[]= \"----------------------------------------------------------------------\";\n const char mime_boundary_leader[] = \"------------\";\n \n-static void show_stats(struct diffstat_t* data)\n+static int scale_linear(int it, int width, int max_change)\n+{\n+\t/*\n+\t * round(width * it / max_change);\n+\t */\n+\treturn (it * width * 2 + max_change) / (max_change * 2);\n+}\n+\n+static void show_name(const char *prefix, const char *name, int len)\n+{\n+\tprintf(\" %s%-*s |\", prefix, len, name);\n+}\n+\n+static void show_graph(char ch, int cnt)\n+{\n+\tif (!cnt)\n+\t\treturn;\n+\twhile (cnt--)\n+\t\tputchar(ch);\n+}\n+\n+static void show_stats(struct diffstat_t* data, struct diff_options *options)\n {\n \tint i, len, add, del, total, adds = 0, dels = 0;\n-\tint max, max_change = 0, max_len = 0;\n+\tint max_change = 0, max_len = 0;\n \tint total_files = data->nr;\n+\tint width, name_width;\n \n \tif (data->nr == 0)\n \t\treturn;\n \n+\twidth = options->stat_width ? options->stat_width : 80;\n+\tname_width = options->stat_name_width ? options->stat_name_width : 50;\n+\n+\t/* Sanity: give at least 5 columns to the graph,\n+\t * but leave at least 10 columns for the name.\n+\t */\n+\tif (width < name_width + 15) {\n+\t\tif (25 < name_width)\n+\t\t\tname_width = width - 15;\n+\t\telse\n+\t\t\twidth = name_width + 15;\n+\t}\n+\n+\t/* Find the longest filename and max number of changes */\n \tfor (i = 0; i < data->nr; i++) {\n \t\tstruct diffstat_file *file = data->files[i];\n+\t\tint change = file->added + file->deleted;\n+\n+\t\tif (0 < (len = quote_c_style(file->name, NULL, NULL, 0))) {\n+\t\t\tchar *qname = xmalloc(len + 1);\n+\t\t\tquote_c_style(file->name, qname, NULL, 0);\n+\t\t\tfree(file->name);\n+\t\t\tfile->name = qname;\n+\t\t}\n \n \t\tlen = strlen(file->name);\n \t\tif (max_len < len)\n@@ -567,54 +609,53 @@ static void show_stats(struct diffstat_t\n \n \t\tif (file->is_binary || file->is_unmerged)\n \t\t\tcontinue;\n-\t\tif (max_change < file->added + file->deleted)\n-\t\t\tmax_change = file->added + file->deleted;\n+\t\tif (max_change < change)\n+\t\t\tmax_change = change;\n \t}\n \n+\t/* Compute the width of the graph part;\n+\t * 10 is for one blank at the beginning of the line plus\n+\t * \" | count \" between the name and the graph.\n+\t *\n+\t * From here on, name_width is the width of the name area,\n+\t * and width is the width of the graph area.\n+\t */\n+\tname_width = (name_width < max_len) ? name_width : max_len;\n+\tif (width < (name_width + 10) + max_change)\n+\t\twidth = width - (name_width + 10);\n+\telse\n+\t\twidth = max_change;\n+\n \tfor (i = 0; i < data->nr; i++) {\n \t\tconst char *prefix = \"\";\n \t\tchar *name = data->files[i]->name;\n \t\tint added = data->files[i]->added;\n \t\tint deleted = data->files[i]->deleted;\n-\n-\t\tif (0 < (len = quote_c_style(name, NULL, NULL, 0))) {\n-\t\t\tchar *qname = xmalloc(len + 1);\n-\t\t\tquote_c_style(name, qname, NULL, 0);\n-\t\t\tfree(name);\n-\t\t\tdata->files[i]->name = name = qname;\n-\t\t}\n+\t\tint name_len;\n \n \t\t/*\n \t\t * \"scale\" the filename\n \t\t */\n-\t\tlen = strlen(name);\n-\t\tmax = max_len;\n-\t\tif (max > 50)\n-\t\t\tmax = 50;\n-\t\tif (len > max) {\n+\t\tlen = name_width;\n+\t\tname_len = strlen(name);\n+\t\tif (name_width < name_len) {\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n-\t\t\tmax -= 3;\n-\t\t\tname += len - max;\n+\t\t\tlen -= 3;\n+\t\t\tname += name_len - len;\n \t\t\tslash = strchr(name, '/');\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n-\t\tlen = max;\n-\n-\t\t/*\n-\t\t * scale the add/delete\n-\t\t */\n-\t\tmax = max_change;\n-\t\tif (max + len > 70)\n-\t\t\tmax = 70 - len;\n \n \t\tif (data->files[i]->is_binary) {\n-\t\t\tprintf(\" %s%-*s |  Bin\\n\", prefix, len, name);\n+\t\t\tshow_name(prefix, name, len);\n+\t\t\tprintf(\"  Bin\\n\");\n \t\t\tgoto free_diffstat_file;\n \t\t}\n \t\telse if (data->files[i]->is_unmerged) {\n-\t\t\tprintf(\" %s%-*s |  Unmerged\\n\", prefix, len, name);\n+\t\t\tshow_name(prefix, name, len);\n+\t\t\tprintf(\"  Unmerged\\n\");\n \t\t\tgoto free_diffstat_file;\n \t\t}\n \t\telse if (!data->files[i]->is_renamed &&\n@@ -623,27 +664,34 @@ static void show_stats(struct diffstat_t\n \t\t\tgoto free_diffstat_file;\n \t\t}\n \n+\t\t/*\n+\t\t * scale the add/delete\n+\t\t */\n \t\tadd = added;\n \t\tdel = deleted;\n \t\ttotal = add + del;\n \t\tadds += add;\n \t\tdels += del;\n \n-\t\tif (max_change > 0) {\n-\t\t\ttotal = (total * max + max_change / 2) / max_change;\n-\t\t\tadd = (add * max + max_change / 2) / max_change;\n+\t\tif (max_change < width)\n+\t\t\t;\n+\t\telse {\n+\t\t\ttotal = scale_linear(total, width, max_change);\n+\t\t\tadd = scale_linear(add, width, max_change);\n \t\t\tdel = total - add;\n \t\t}\n-\t\tprintf(\" %s%-*s |%5d %.*s%.*s\\n\", prefix,\n-\t\t\t\tlen, name, added + deleted,\n-\t\t\t\tadd, pluses, del, minuses);\n+\t\tshow_name(prefix, name, len);\n+\t\tprintf(\"%5d \", added + deleted);\n+\t\tshow_graph('+', add);\n+\t\tshow_graph('-', del);\n+\t\tputchar('\\n');\n \tfree_diffstat_file:\n \t\tfree(data->files[i]->name);\n \t\tfree(data->files[i]);\n \t}\n \tfree(data->files);\n \tprintf(\" %d files changed, %d insertions(+), %d deletions(-)\\n\",\n-\t\t\ttotal_files, adds, dels);\n+\t       total_files, adds, dels);\n }\n \n struct checkdiff_t {\n@@ -1681,6 +1729,14 @@ int diff_opt_parse(struct diff_options *\n \t}\n \telse if (!strcmp(arg, \"--stat\"))\n \t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n+\telse if (!strncmp(arg, \"--stat-width=\", 13)) {\n+\t\toptions->stat_width = strtoul(arg + 13, NULL, 10);\n+\t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n+\t}\n+\telse if (!strncmp(arg, \"--stat-name-width=\", 18)) {\n+\t\toptions->stat_name_width = strtoul(arg + 18, NULL, 10);\n+\t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n+\t}\n \telse if (!strcmp(arg, \"--check\"))\n \t\toptions->output_format |= DIFF_FORMAT_CHECKDIFF;\n \telse if (!strcmp(arg, \"--summary\"))\n@@ -2438,7 +2494,7 @@ void diff_flush(struct diff_options *opt\n \t\t\tif (check_pair_status(p))\n \t\t\t\tdiff_flush_stat(p, options, &diffstat);\n \t\t}\n-\t\tshow_stats(&diffstat);\n+\t\tshow_stats(&diffstat, options);\n \t\tseparator++;\n \t}\n \ndiff --git a/diff.h b/diff.h\nindex b60a02e..e06d0f4 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -69,6 +69,9 @@ struct diff_options {\n \tconst char *stat_sep;\n \tlong xdl_opts;\n \n+\tint stat_width;\n+\tint stat_name_width;\n+\n \tint nr_paths;\n \tconst char **paths;\n \tint *pathlens;\n-- \n1.4.2.1.gf80a\n"},{"id":"27746","messageId":"Pine.LNX.4.64N.0609262001460.520@attu4.cs.washington.edu","threadId":"5714","inReplyTo":"7vr6xyjal0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"David Rientjes","fromEmail":"rientjes@cs.washington.edu","sentAt":"2006-09-27T03:11:25Z","receivedAt":"2006-09-27T03:11:25Z","isPatch":true,"sender":{"key":"rientjes@cs.washington.edu","avatar":null},"body":"On Tue, 26 Sep 2006, Junio C Hamano wrote:\n\n> +static void show_graph(char ch, int cnt)\n> +{\n> +\tif (!cnt)\n> +\t\treturn;\n> +\twhile (cnt--)\n> +\t\tputchar(ch);\n> +}\n> +\n\n'if (cnt <= 0)' or 'while (cnt-- > 0)' is a better API./\n\n> +static void show_stats(struct diffstat_t* data, struct diff_options *options)\n>  {\n>  \tint i, len, add, del, total, adds = 0, dels = 0;\n> -\tint max, max_change = 0, max_len = 0;\n> +\tint max_change = 0, max_len = 0;\n>  \tint total_files = data->nr;\n> +\tint width, name_width;\n>  \n>  \tif (data->nr == 0)\n>  \t\treturn;\n>  \n> +\twidth = options->stat_width ? options->stat_width : 80;\n> +\tname_width = options->stat_name_width ? options->stat_name_width : 50;\n> +\n> +\t/* Sanity: give at least 5 columns to the graph,\n> +\t * but leave at least 10 columns for the name.\n> +\t */\n> +\tif (width < name_width + 15) {\n> +\t\tif (25 < name_width)\n> +\t\t\tname_width = width - 15;\n> +\t\telse\n> +\t\t\twidth = name_width + 15;\n> +\t}\n> +\n\nConstants go on the right side of comparison expressions.\n\n> +\t/* Find the longest filename and max number of changes */\n>  \tfor (i = 0; i < data->nr; i++) {\n>  \t\tstruct diffstat_file *file = data->files[i];\n> +\t\tint change = file->added + file->deleted;\n> +\n> +\t\tif (0 < (len = quote_c_style(file->name, NULL, NULL, 0))) {\n> +\t\t\tchar *qname = xmalloc(len + 1);\n> +\t\t\tquote_c_style(file->name, qname, NULL, 0);\n> +\t\t\tfree(file->name);\n> +\t\t\tfile->name = qname;\n> +\t\t}\n\nSame.\n\n> @@ -623,27 +664,34 @@ static void show_stats(struct diffstat_t\n>  \t\t\tgoto free_diffstat_file;\n>  \t\t}\n>  \n> +\t\t/*\n> +\t\t * scale the add/delete\n> +\t\t */\n>  \t\tadd = added;\n>  \t\tdel = deleted;\n>  \t\ttotal = add + del;\n>  \t\tadds += add;\n>  \t\tdels += del;\n>  \n> -\t\tif (max_change > 0) {\n> -\t\t\ttotal = (total * max + max_change / 2) / max_change;\n> -\t\t\tadd = (add * max + max_change / 2) / max_change;\n> +\t\tif (max_change < width)\n> +\t\t\t;\n> +\t\telse {\n> +\t\t\ttotal = scale_linear(total, width, max_change);\n> +\t\t\tadd = scale_linear(add, width, max_change);\n>  \t\t\tdel = total - add;\n>  \t\t}\n\nif (max_change >= width)\n\n> diff --git a/diff.h b/diff.h\n> index b60a02e..e06d0f4 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -69,6 +69,9 @@ struct diff_options {\n>  \tconst char *stat_sep;\n>  \tlong xdl_opts;\n>  \n> +\tint stat_width;\n> +\tint stat_name_width;\n> +\n\nCan you use unsigned char here instead?\n\n\t\tDavid\n"},{"id":"27933","messageId":"Pine.LNX.4.64.0609281349110.3952@g5.osdl.org","threadId":"5714","inReplyTo":"7vr6xyjal0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-28T20:54:27Z","receivedAt":"2006-09-28T20:54:27Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Sep 2006, Junio C Hamano wrote:\n>\n> This adds two parameters to \"diff --stat\".\n> \n>  . --stat-width=72 tells that the page should fit on 72-column output.\n> \n>  . --stat-name-width=30 tells that the filename part is limited\n>    to 30 columns.\n\nThinking some more about this, I have to say, I do hate the syntax.\n\nIt may be clear thanks to being verbose, but it's _hell_ to write.\n\nIt has the same problem the \"--stat-with-patch\" argument had: sure, it \nworked, but it was really really inconvenient, and just doing a \ncombination of \"--stat -p\" is much nicer.\n\nSo how about just extending the existing \"--stat\" thing, and just making \nit do something like\n\n\tgit diff --stat=72,30\n\ninstead (perhaps along with a config option to set the defaults to \nsomething else if we want to).\n\nWhat do you think?\n\nI'm just pretty sure I will never _ever_ bother to type \n--stat-name-width=30 in my life except right now to complain about it. I \njust can't see myself ever caring _that_ much. But \"--stat=100\" to see a \nwider stat, that I could see myself doing. Or \"--stat=100,100\" if I want \nto see long filenames too.\n\n\t\tLinus\n"},{"id":"27936","messageId":"7vac4ju1f1.fsf@assigned-by-dhcp.cox.net","threadId":"5714","inReplyTo":"Pine.LNX.4.64.0609281349110.3952@g5.osdl.org","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-28T21:27:46Z","receivedAt":"2006-09-28T21:27:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> So how about just extending the existing \"--stat\" thing, and just making \n> it do something like\n>\n> \tgit diff --stat=72,30\n>\n> instead (perhaps along with a config option to set the defaults to \n> something else if we want to).\n>\n> What do you think?\n\nI am all for it.\n"},{"id":"27937","messageId":"Pine.LNX.4.64.0609281458420.3952@g5.osdl.org","threadId":"5714","inReplyTo":"7vac4ju1f1.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-28T22:07:16Z","receivedAt":"2006-09-28T22:07:16Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Sep 2006, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@osdl.org> writes:\n> \n> > So how about just extending the existing \"--stat\" thing, and just making \n> > it do something like\n> >\n> > \tgit diff --stat=72,30\n> >\n> > instead (perhaps along with a config option to set the defaults to \n> > something else if we want to).\n> >\n> > What do you think?\n> \n> I am all for it.\n\nHere. Something like this.\n\nYou should probably check the \"width\" and \"name_width\" values for sanity. \nThe old code didn't, so I didn't do it, but this actually does check that \nthe number was a real number (and didn't have crap after it). And it's \neasy to check the values, since all cases go through the same point.\n\nI could have made it a more obvious \"stupid\" parser, I just think it's \nbetter to do it this way.\n\n\t\tLinus\n\n----\ndiff --git a/diff.c b/diff.c\nindex 98c29bf..8546dde 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1825,15 +1825,33 @@ int diff_opt_parse(struct diff_options *\n \telse if (!strcmp(arg, \"--patch-with-raw\")) {\n \t\toptions->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;\n \t}\n-\telse if (!strcmp(arg, \"--stat\"))\n-\t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n-\telse if (!strncmp(arg, \"--stat-width=\", 13)) {\n-\t\toptions->stat_width = strtoul(arg + 13, NULL, 10);\n-\t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n-\t}\n-\telse if (!strncmp(arg, \"--stat-name-width=\", 18)) {\n-\t\toptions->stat_name_width = strtoul(arg + 18, NULL, 10);\n+\telse if (!strncmp(arg, \"--stat\", 6)) {\n+\t\tchar *end;\n+\t\tint width = options->stat_width;\n+\t\tint name_width = options->stat_name_width;\n+\t\targ += 6;\n+\t\tend = arg;\n+\n+\t\tswitch (*arg) {\n+\t\tcase '-':\n+\t\t\tif (!strncmp(arg, \"-width=\", 7))\n+\t\t\t\twidth = strtoul(arg + 7, &end, 10);\n+\t\t\telse if (!strncmp(arg, \"-name-width=\", 12))\n+\t\t\t\tname_width = strtoul(arg + 12, &end, 10);\n+\t\t\tbreak;\n+\n+\t\tcase '=':\n+\t\t\twidth = strtoul(arg+1, &end, 10);\n+\t\t\tif (*end == ',')\n+\t\t\t\tname_width = strtoul(end+1, &end, 10);\n+\t\t}\n+\n+\t\t/* Important! This checks all the error cases! */\n+\t\tif (*end)\n+\t\t\treturn 0;\n \t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n+\t\toptions->stat_name_width = name_width;\n+\t\toptions->stat_width = width;\n \t}\n \telse if (!strcmp(arg, \"--check\"))\n \t\toptions->output_format |= DIFF_FORMAT_CHECKDIFF;\n"},{"id":"27941","messageId":"20060928222402.GC3469@stusta.de","threadId":"5714","inReplyTo":"Pine.LNX.4.64.0609281349110.3952@g5.osdl.org","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Adrian Bunk","fromEmail":"bunk@stusta.de","sentAt":"2006-09-28T22:24:02Z","receivedAt":"2006-09-28T22:24:02Z","isPatch":true,"sender":{"key":"bunk@stusta.de","avatar":null},"body":"On Thu, Sep 28, 2006 at 01:54:27PM -0700, Linus Torvalds wrote:\n> \n> \n> On Tue, 26 Sep 2006, Junio C Hamano wrote:\n> >\n> > This adds two parameters to \"diff --stat\".\n> > \n> >  . --stat-width=72 tells that the page should fit on 72-column output.\n> > \n> >  . --stat-name-width=30 tells that the filename part is limited\n> >    to 30 columns.\n> \n> Thinking some more about this, I have to say, I do hate the syntax.\n> \n> It may be clear thanks to being verbose, but it's _hell_ to write.\n> \n> It has the same problem the \"--stat-with-patch\" argument had: sure, it \n> worked, but it was really really inconvenient, and just doing a \n> combination of \"--stat -p\" is much nicer.\n> \n> So how about just extending the existing \"--stat\" thing, and just making \n> it do something like\n> \n> \tgit diff --stat=72,30\n> \n> instead (perhaps along with a config option to set the defaults to \n> something else if we want to).\n> \n> What do you think?\n>...\n\nWhat about staying compatible with diffstat?\n\nIOW, change --stat-width=72 to -w72, or at least allow it alternatively?\n\n> \t\tLinus\n\ncu\nAdrian\n\n-- \n\n       \"Is there not promise of rain?\" Ling Tan asked suddenly out\n        of the darkness. There had been need of rain for many days.\n       \"Only a promise,\" Lao Er said.\n                                       Pearl S. Buck - Dragon Seed\n"},{"id":"27943","messageId":"Pine.LNX.4.63.0609290025030.14200@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5714","inReplyTo":"20060928222402.GC3469@stusta.de","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-09-28T22:26:40Z","receivedAt":"2006-09-28T22:26:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 29 Sep 2006, Adrian Bunk wrote:\n\n> IOW, change --stat-width=72 to -w72, or at least allow it alternatively?\n\nWith diff options, -w is identical to --ignore-all-space. (Yes, I know, it \nis not in Documentation/diff-options.txt, but it is in \"man diff\".)\n\nCiao,\nDscho\n"},{"id":"27964","messageId":"7vk63nsbec.fsf@assigned-by-dhcp.cox.net","threadId":"5714","inReplyTo":"Pine.LNX.4.64.0609281458420.3952@g5.osdl.org","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-29T01:35:07Z","receivedAt":"2006-09-29T01:35:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> You should probably check the \"width\" and \"name_width\" values for sanity.\n\nThe output code forces (well, at least tries to force) some\nsanity into these values when they are not, so probably we do\nnot need it.  If we want, we could check something like:\n\n    \tif (name_width < 10 || width - name_width < 15)\n\t\tdie(\"at least 10 for name, and 15+name for width please\");\n\nThe number 10 is totally arbitrary; among the total area that is\ngiven by width, after subtracting name_width, 10 columns are\ntaken by left and right end space, vertical bar and numbers, so\nthe above gives at least 5 columns to draw the graph.\n"},{"id":"27972","messageId":"7vr6xvp7jm.fsf@assigned-by-dhcp.cox.net","threadId":"5714","inReplyTo":"Pine.LNX.4.64.0609281458420.3952@g5.osdl.org","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-29T05:26:37Z","receivedAt":"2006-09-29T05:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> I could have made it a more obvious \"stupid\" parser, I just think it's \n> better to do it this way.\n>\n> +\telse if (!strncmp(arg, \"--stat\", 6)) {\n> +\t\tchar *end;\n> +\t\tint width = options->stat_width;\n> +\t\tint name_width = options->stat_name_width;\n> +\t\targ += 6;\n> +\t\tend = arg;\n> +\n> +\t\tswitch (*arg) {\n> +\t\tcase '-':\n> +\t\t\tif (!strncmp(arg, \"-width=\", 7))\n> +\t\t\t\twidth = strtoul(arg + 7, &end, 10);\n> +\t\t\telse if (!strncmp(arg, \"-name-width=\", 12))\n> +\t\t\t\tname_width = strtoul(arg + 12, &end, 10);\n> +\t\t\tbreak;\n> +\n> +\t\tcase '=':\n> +\t\t\twidth = strtoul(arg+1, &end, 10);\n> +\t\t\tif (*end == ',')\n> +\t\t\t\tname_width = strtoul(end+1, &end, 10);\n> +\t\t}\n> +\n> +\t\t/* Important! This checks all the error cases! */\n> +\t\tif (*end)\n> +\t\t\treturn 0;\n>  \t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n> +\t\toptions->stat_name_width = name_width;\n> +\t\toptions->stat_width = width;\n>  \t}\n\nThis is simply too clever; -pedantic does not like assignment of\narg to end (constness -- and strtoul takes pointer to non-const\nchar *, so making the type of end const char * is not an answer\neither).\n\nAnd I do not like casting constness away: end = (char *) arg.\n\nHmmmm.\n"},{"id":"27973","messageId":"Pine.LNX.4.64.0609282252430.3952@g5.osdl.org","threadId":"5714","inReplyTo":"7vr6xvp7jm.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-29T05:58:10Z","receivedAt":"2006-09-29T05:58:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Sep 2006, Junio C Hamano wrote:\n> \n> This is simply too clever; -pedantic does not like assignment of\n> arg to end (constness -- and strtoul takes pointer to non-const\n> char *, so making the type of end const char * is not an answer\n> either).\n\nThe _code_ really is right. The problem is \"strtoul()\" interfaces and a \nC typing oddity.\n\n> And I do not like casting constness away: end = (char *) arg.\n\nYou could fix it by doing something like this:\n\n\tstatic inline unsigned long sane_strtoul(const char *n,\n\t\t\t\t\t\t const char **p,\n\t\t\t\t\t\t int base)\n\t{\n\t\tchar *end;\n\t\tunsigned long res;\n\n\t\tres = strtoul(n, &end, base);\n\t\t*p = end;\n\t\treturn res;\n\t}\n\nbecause the only reason strtoul() warns now is that C type-rules don't \nallow the (obviously safe - but pointers migth have strange \nrepresentations) conversion of \"char **\" into \"const char **\", even though \n\"char *\" can be converted into \"const char *\".\n\nAt that point, the cast is probably simpler, but the above should be \nstrictly correct pedantic ANSI C.\n\nI didn't even try it, though.\n\n\t\t\tLinus\n"},{"id":"27976","messageId":"7vhcyrp5hh.fsf@assigned-by-dhcp.cox.net","threadId":"5714","inReplyTo":"Pine.LNX.4.64.0609282252430.3952@g5.osdl.org","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-29T06:11:06Z","receivedAt":"2006-09-29T06:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> On Thu, 28 Sep 2006, Junio C Hamano wrote:\n>> \n>> This is simply too clever; -pedantic does not like assignment of\n>> arg to end (constness -- and strtoul takes pointer to non-const\n>> char *, so making the type of end const char * is not an answer\n>> either).\n>\n> The _code_ really is right. The problem is \"strtoul()\" interfaces and a \n> C typing oddity.\n\nYes, the code is right (not only because you wrote it ;-) and\nnot just right but it is crystal clear to humans what is going\non and why the final \"if (*end)\" is all that is needed without\nany \"return 0\" in the middle.\n\n> At that point, the cast is probably simpler,...\n\n... which I ended up doing anyway.\n\nThanks.\n"},{"id":"27978","messageId":"Pine.LNX.4.64.0609282307220.3952@g5.osdl.org","threadId":"5714","inReplyTo":"Pine.LNX.4.64.0609282252430.3952@g5.osdl.org","subject":"Re: [PATCH 1/3] diff --stat: allow custom diffstat output width.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-29T06:17:14Z","receivedAt":"2006-09-29T06:17:14Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Sep 2006, Linus Torvalds wrote:\n> \n> because the only reason strtoul() warns now is that C type-rules don't \n> allow the (obviously safe - but pointers migth have strange \n> representations) conversion of \"char **\" into \"const char **\", even though \n> \"char *\" can be converted into \"const char *\".\n\nI phrased that badly. \n\nIF C pointer conversion allowed implicit addition of \"const\" past the \ntop-most level, ANSI C would have just done \"strtoul()\" as \n\n\tunsigned long strtoul(const char *n, const char **p, int);\n\nie they could just have added the \"const\" not just to the first argument, \nand legact programs (without const) would still have worked fine.\n\nBut _because_ that's not how C type rules work, we have the current \nsituation where the first argument is a \"const char *\", and the second \nargument _logically_ should be a pointer to such an entity, but because \nthat would have caused bogus warnings for any code that just used a \nregular \"char **\" without any const at all, that wasn't an option.\n\nSo that explains why ANSI C has insane imbalances like this. It's easy to \nadd a \"const\" to a _first-level_ pointer to say \"we allow both const and \nregular pointers to this thing\", but sadly you can't do it for a pointer \nto such a pointer.\n\n\t\t\tLinus\n"}]}