{"thread":{"id":"23491","subject":"A bug in git 1.6.5.2 with git log --stat: shows a negative number as a size","startedAt":"2010-04-16T13:59:48Z","lastAt":"2010-04-17T17:41:08Z","messageCount":5,"participants":["Heikki Orsila","Tomas Carnecky","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"139666","messageId":"20100416135948.GA26918@zakalwe.fi","threadId":"23491","inReplyTo":null,"subject":"A bug in git 1.6.5.2 with git log --stat: shows a negative number as a size","fromName":"Heikki Orsila","fromEmail":"shdl@zakalwe.fi","sentAt":"2010-04-16T13:59:48Z","receivedAt":"2010-04-16T13:59:48Z","isPatch":false,"sender":{"key":"shdl@zakalwe.fi","avatar":null},"body":"I'm running git version 1.6.5.2. git log --stat shows a negative\ndiffstat size for two files that are each 2049MiB in size.\n\nSteps to reproduce:\n\n$ for f in 0 1 ; do dd bs=$((1024*1024)) if=/dev/zero of=$f count=2049 ; done\n$ git add 0 1\n$ git commit -m \"test commit\"\n$ git log --stat\ncommit 6afe3d3c889daa92bd79956c4bb733eb5cb408dc\nAuthor: Heikki Orsila <heikki.orsila@iki.fi>\nDate:   2010-04-16 16:54:52 +0300\n\n    test commit\n\n 0 |  Bin 0 -> -2146435072 bytes\n 1 |  Bin 0 -> -2146435072 bytes\n 2 files changed, 0 insertions(+), 0 deletions(-)\n\n-- \nHeikki Orsila\nheikki.orsila@iki.fi\nhttp://www.iki.fi/shd\n"},{"id":"139667","messageId":"4BC87BE9.9040704@dbservice.com","threadId":"23491","inReplyTo":"20100416135948.GA26918@zakalwe.fi","subject":"Re: A bug in git 1.6.5.2 with git log --stat: shows a negative number as a size","fromName":"Tomas Carnecky","fromEmail":"tom@dbservice.com","sentAt":"2010-04-16T15:02:01Z","receivedAt":"2010-04-16T15:02:01Z","isPatch":false,"sender":{"key":"tom@dbservice.com","avatar":"https://gravatar.com/avatar/900a300bdd1a8bbe086008ad78210bbee2ad2803b7d50a5cba04c1e9404bd6d2?d=mp&s=160"},"body":"On 4/16/10 3:59 PM, Heikki Orsila wrote:\n> I'm running git version 1.6.5.2. git log --stat shows a negative\n> diffstat size for two files that are each 2049MiB in size.\n>\n> Steps to reproduce:\n>\n> $ for f in 0 1 ; do dd bs=$((1024*1024)) if=/dev/zero of=$f count=2049 ; done\n> $ git add 0 1\n> $ git commit -m \"test commit\"\n> $ git log --stat\n> commit 6afe3d3c889daa92bd79956c4bb733eb5cb408dc\n> Author: Heikki Orsila<heikki.orsila@iki.fi>\n> Date:   2010-04-16 16:54:52 +0300\n>\n>      test commit\n>\n>   0 |  Bin 0 ->  -2146435072 bytes\n>   1 |  Bin 0 ->  -2146435072 bytes\n>   2 files changed, 0 insertions(+), 0 deletions(-)\n\nYep, bug is also in the latest version (1.7.0.5). The code uses 'int' \ninstead of something big enough to hold the size of your files.\n\nhttp://git.kernel.org/?p=git/git.git;a=blob;f=diff.c;h=a1bf1e9cb37104cda8168c5118769ce5bbcfcbb2;hb=HEAD#l1105\n\nand a couple lines below (1124) you see that the stat is printed out.\n\ntom\n"},{"id":"139731","messageId":"20100417102543.GB23110@coredump.intra.peff.net","threadId":"23491","inReplyTo":"4BC87BE9.9040704@dbservice.com","subject":"[PATCH] diff: use 64-bit integers for diffstat calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-17T10:25:43Z","receivedAt":"2010-04-17T10:25:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 16, 2010 at 05:02:01PM +0200, Tomas Carnecky wrote:\n\n> >commit 6afe3d3c889daa92bd79956c4bb733eb5cb408dc\n> >Author: Heikki Orsila<heikki.orsila@iki.fi>\n> >Date:   2010-04-16 16:54:52 +0300\n> >\n> >     test commit\n> >\n> >  0 |  Bin 0 ->  -2146435072 bytes\n> >  1 |  Bin 0 ->  -2146435072 bytes\n> >  2 files changed, 0 insertions(+), 0 deletions(-)\n> \n> Yep, bug is also in the latest version (1.7.0.5). The code uses 'int'\n> instead of something big enough to hold the size of your files.\n\nYuck, we use \"unsigned int\" for the actual storage, and then convert to\na regular \"int\" in some other places. I think we should just do this:\n\n-- >8 --\nSubject: [PATCH] diff: use 64-bit integers for diffstat calculations\n\nThe diffstat \"added\" and \"changed\" fields generally store\nline counts; however, for binary files, they store file\nsizes. Since we store and print these values as ints, a\ndiffstat on a file larger than 2G can show a negative size.\nInstead, let's explicitly use 64-bit integers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c |   21 ++++++++++++---------\n 1 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7effdac..54933cc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -936,7 +936,7 @@ struct diffstat_t {\n \t\tunsigned is_unmerged:1;\n \t\tunsigned is_binary:1;\n \t\tunsigned is_renamed:1;\n-\t\tunsigned int added, deleted;\n+\t\tuint64_t added, deleted;\n \t} **files;\n };\n \n@@ -1028,7 +1028,7 @@ static void fill_print_name(struct diffstat_file *file)\n static void show_stats(struct diffstat_t *data, struct diff_options *options)\n {\n \tint i, len, add, del, adds = 0, dels = 0;\n-\tint max_change = 0, max_len = 0;\n+\tuint64_t max_change = 0, max_len = 0;\n \tint total_files = data->nr;\n \tint width, name_width;\n \tconst char *reset, *set, *add_c, *del_c;\n@@ -1057,7 +1057,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \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+\t\tuint64_t change = file->added + file->deleted;\n \t\tfill_print_name(file);\n \t\tlen = strlen(file->print_name);\n \t\tif (max_len < len)\n@@ -1085,8 +1085,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tfor (i = 0; i < data->nr; i++) {\n \t\tconst char *prefix = \"\";\n \t\tchar *name = data->files[i]->print_name;\n-\t\tint added = data->files[i]->added;\n-\t\tint deleted = data->files[i]->deleted;\n+\t\tuint64_t added = data->files[i]->added;\n+\t\tuint64_t deleted = data->files[i]->deleted;\n \t\tint name_len;\n \n \t\t/*\n@@ -1107,9 +1107,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tif (data->files[i]->is_binary) {\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, \"%s%\"PRIu64\"%s\",\n+\t\t\t\tdel_c, deleted, reset);\n \t\t\tfprintf(options->file, \" -> \");\n-\t\t\tfprintf(options->file, \"%s%d%s\", add_c, added, reset);\n+\t\t\tfprintf(options->file, \"%s%\"PRIu64\"%s\",\n+\t\t\t\tadd_c, added, reset);\n \t\t\tfprintf(options->file, \" bytes\");\n \t\t\tfprintf(options->file, \"\\n\");\n \t\t\tcontinue;\n@@ -1138,7 +1140,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tdel = scale_linear(del, width, max_change);\n \t\t}\n \t\tshow_name(options->file, prefix, name, len);\n-\t\tfprintf(options->file, \"%5d%s\", added + deleted,\n+\t\tfprintf(options->file, \"%5\"PRIu64\"%s\", added + deleted,\n \t\t\t\tadded + deleted ? \" \" : \"\");\n \t\tshow_graph(options->file, '+', add, add_c, reset);\n \t\tshow_graph(options->file, '-', del, del_c, reset);\n@@ -1188,7 +1190,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\"%\"PRIu64\"\\t%\"PRIu64\"\\t\",\n+\t\t\t\tfile->added, file->deleted);\n \t\tif (options->line_termination) {\n \t\t\tfill_print_name(file);\n \t\t\tif (!file->is_renamed)\n-- \n1.7.1.rc1.277.g2c4c9\n"},{"id":"139759","messageId":"7vvdbq2ev7.fsf@alter.siamese.dyndns.org","threadId":"23491","inReplyTo":"20100417102543.GB23110@coredump.intra.peff.net","subject":"Re: [PATCH] diff: use 64-bit integers for diffstat calculations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-17T17:00:44Z","receivedAt":"2010-04-17T17:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yuck, we use \"unsigned int\" for the actual storage, and then convert to\n> a regular \"int\" in some other places. I think we should just do this:\n>\n> -- >8 --\n> Subject: [PATCH] diff: use 64-bit integers for diffstat calculations\n>\n> The diffstat \"added\" and \"changed\" fields generally store\n> line counts; however, for binary files, they store file\n> sizes. Since we store and print these values as ints, a\n> diffstat on a file larger than 2G can show a negative size.\n> Instead, let's explicitly use 64-bit integers.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nYes, but we would probably be better off using using uintmax_t for things\nlike this if the quantity a variable represents is not closely tied to\nexternal file format (e.g. the offset field of pack idx file), nor the\ncode is only for a particular platform (e.g. compat/win32mmap.c), don't\nyou think?\n\nThat is the impression I am getting on the discipline expressed in the\ncurrent codebase, from browsing the output from \"git grep uint64_t\".\n"},{"id":"139764","messageId":"20100417174108.GB23642@coredump.intra.peff.net","threadId":"23491","inReplyTo":"7vvdbq2ev7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: use 64-bit integers for diffstat calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-17T17:41:08Z","receivedAt":"2010-04-17T17:41:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 17, 2010 at 10:00:44AM -0700, Junio C Hamano wrote:\n\n> > The diffstat \"added\" and \"changed\" fields generally store\n> > line counts; however, for binary files, they store file\n> > sizes. Since we store and print these values as ints, a\n> > diffstat on a file larger than 2G can show a negative size.\n> > Instead, let's explicitly use 64-bit integers.\n> \n> Yes, but we would probably be better off using using uintmax_t for things\n> like this if the quantity a variable represents is not closely tied to\n> external file format (e.g. the offset field of pack idx file), nor the\n> code is only for a particular platform (e.g. compat/win32mmap.c), don't\n> you think?\n\nBut 640K^W 64-bits should be large enough for anyone! :)\n\nBut yes, you're right. Updated patch below.\n\n-- >8 --\nSubject: [PATCH] diff: use large integers for diffstat calculations\n\nThe diffstat \"added\" and \"changed\" fields generally store\nline counts; however, for binary files, they store file\nsizes. Since we store and print these values as ints, a\ndiffstat on a file larger than 2G can show a negative size.\nInstead, let's use uintmax_t, which should be at least 64\nbits on modern platforms.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c |   21 ++++++++++++---------\n 1 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7effdac..fbdbd8d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -936,7 +936,7 @@ struct diffstat_t {\n \t\tunsigned is_unmerged:1;\n \t\tunsigned is_binary:1;\n \t\tunsigned is_renamed:1;\n-\t\tunsigned int added, deleted;\n+\t\tuintmax_t added, deleted;\n \t} **files;\n };\n \n@@ -1028,7 +1028,7 @@ static void fill_print_name(struct diffstat_file *file)\n static void show_stats(struct diffstat_t *data, struct diff_options *options)\n {\n \tint i, len, add, del, adds = 0, dels = 0;\n-\tint max_change = 0, max_len = 0;\n+\tuintmax_t max_change = 0, max_len = 0;\n \tint total_files = data->nr;\n \tint width, name_width;\n \tconst char *reset, *set, *add_c, *del_c;\n@@ -1057,7 +1057,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \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+\t\tuintmax_t change = file->added + file->deleted;\n \t\tfill_print_name(file);\n \t\tlen = strlen(file->print_name);\n \t\tif (max_len < len)\n@@ -1085,8 +1085,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tfor (i = 0; i < data->nr; i++) {\n \t\tconst char *prefix = \"\";\n \t\tchar *name = data->files[i]->print_name;\n-\t\tint added = data->files[i]->added;\n-\t\tint deleted = data->files[i]->deleted;\n+\t\tuintmax_t added = data->files[i]->added;\n+\t\tuintmax_t deleted = data->files[i]->deleted;\n \t\tint name_len;\n \n \t\t/*\n@@ -1107,9 +1107,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tif (data->files[i]->is_binary) {\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, \"%s%\"PRIuMAX\"%s\",\n+\t\t\t\tdel_c, deleted, reset);\n \t\t\tfprintf(options->file, \" -> \");\n-\t\t\tfprintf(options->file, \"%s%d%s\", add_c, added, reset);\n+\t\t\tfprintf(options->file, \"%s%\"PRIuMAX\"%s\",\n+\t\t\t\tadd_c, added, reset);\n \t\t\tfprintf(options->file, \" bytes\");\n \t\t\tfprintf(options->file, \"\\n\");\n \t\t\tcontinue;\n@@ -1138,7 +1140,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tdel = scale_linear(del, width, max_change);\n \t\t}\n \t\tshow_name(options->file, prefix, name, len);\n-\t\tfprintf(options->file, \"%5d%s\", added + deleted,\n+\t\tfprintf(options->file, \"%5\"PRIuMAX\"%s\", added + deleted,\n \t\t\t\tadded + deleted ? \" \" : \"\");\n \t\tshow_graph(options->file, '+', add, add_c, reset);\n \t\tshow_graph(options->file, '-', del, del_c, reset);\n@@ -1188,7 +1190,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\"%\"PRIuMAX\"\\t%\"PRIuMAX\"\\t\",\n+\t\t\t\tfile->added, file->deleted);\n \t\tif (options->line_termination) {\n \t\t\tfill_print_name(file);\n \t\t\tif (!file->is_renamed)\n-- \n1.7.1.rc1.277.g2c4c9\n"}]}