# A bug in git 1.6.5.2 with git log --stat: shows a negative number as a size 5 messages from 2010-04-16 to 2010-04-17. Participants: Heikki Orsila, Tomas Carnecky, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/23491 ## Heikki Orsila, 2010-04-16 13:59 Subject: A bug in git 1.6.5.2 with git log --stat: shows a negative number as a size Message-ID: <20100416135948.GA26918@zakalwe.fi> URL: https://gitlist.dev/e/20100416135948.GA26918%40zakalwe.fi ``` I'm running git version 1.6.5.2. git log --stat shows a negative diffstat size for two files that are each 2049MiB in size. Steps to reproduce: $ for f in 0 1 ; do dd bs=$((1024*1024)) if=/dev/zero of=$f count=2049 ; done $ git add 0 1 $ git commit -m "test commit" $ git log --stat commit 6afe3d3c889daa92bd79956c4bb733eb5cb408dc Author: Heikki Orsila Date: 2010-04-16 16:54:52 +0300 test commit 0 | Bin 0 -> -2146435072 bytes 1 | Bin 0 -> -2146435072 bytes 2 files changed, 0 insertions(+), 0 deletions(-) -- Heikki Orsila heikki.orsila@iki.fi http://www.iki.fi/shd ``` ## Tomas Carnecky, 2010-04-16 15:02 Subject: Re: A bug in git 1.6.5.2 with git log --stat: shows a negative number as a size Message-ID: <4BC87BE9.9040704@dbservice.com> URL: https://gitlist.dev/e/4BC87BE9.9040704%40dbservice.com In-Reply-To: <20100416135948.GA26918@zakalwe.fi> ``` On 4/16/10 3:59 PM, Heikki Orsila wrote: > I'm running git version 1.6.5.2. git log --stat shows a negative > diffstat size for two files that are each 2049MiB in size. > > Steps to reproduce: > > $ for f in 0 1 ; do dd bs=$((1024*1024)) if=/dev/zero of=$f count=2049 ; done > $ git add 0 1 > $ git commit -m "test commit" > $ git log --stat > commit 6afe3d3c889daa92bd79956c4bb733eb5cb408dc > Author: Heikki Orsila > Date: 2010-04-16 16:54:52 +0300 > > test commit > > 0 | Bin 0 -> -2146435072 bytes > 1 | Bin 0 -> -2146435072 bytes > 2 files changed, 0 insertions(+), 0 deletions(-) Yep, bug is also in the latest version (1.7.0.5). The code uses 'int' instead of something big enough to hold the size of your files. http://git.kernel.org/?p=git/git.git;a=blob;f=diff.c;h=a1bf1e9cb37104cda8168c5118769ce5bbcfcbb2;hb=HEAD#l1105 and a couple lines below (1124) you see that the stat is printed out. tom ``` ## Jeff King, 2010-04-17 10:25 Subject: [PATCH] diff: use 64-bit integers for diffstat calculations Message-ID: <20100417102543.GB23110@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100417102543.GB23110%40coredump.intra.peff.net In-Reply-To: <4BC87BE9.9040704@dbservice.com> ``` On Fri, Apr 16, 2010 at 05:02:01PM +0200, Tomas Carnecky wrote: > >commit 6afe3d3c889daa92bd79956c4bb733eb5cb408dc > >Author: Heikki Orsila > >Date: 2010-04-16 16:54:52 +0300 > > > > test commit > > > > 0 | Bin 0 -> -2146435072 bytes > > 1 | Bin 0 -> -2146435072 bytes > > 2 files changed, 0 insertions(+), 0 deletions(-) > > Yep, bug is also in the latest version (1.7.0.5). The code uses 'int' > instead of something big enough to hold the size of your files. Yuck, we use "unsigned int" for the actual storage, and then convert to a regular "int" in some other places. I think we should just do this: -- >8 -- Subject: [PATCH] diff: use 64-bit integers for diffstat calculations The diffstat "added" and "changed" fields generally store line counts; however, for binary files, they store file sizes. Since we store and print these values as ints, a diffstat on a file larger than 2G can show a negative size. Instead, let's explicitly use 64-bit integers. Signed-off-by: Jeff King --- diff.c | 21 ++++++++++++--------- 1 files changed, 12 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index 7effdac..54933cc 100644 --- a/diff.c +++ b/diff.c @@ -936,7 +936,7 @@ struct diffstat_t { unsigned is_unmerged:1; unsigned is_binary:1; unsigned is_renamed:1; - unsigned int added, deleted; + uint64_t added, deleted; } **files; }; @@ -1028,7 +1028,7 @@ static void fill_print_name(struct diffstat_file *file) static void show_stats(struct diffstat_t *data, struct diff_options *options) { int i, len, add, del, adds = 0, dels = 0; - int max_change = 0, max_len = 0; + uint64_t max_change = 0, max_len = 0; int total_files = data->nr; int width, name_width; const char *reset, *set, *add_c, *del_c; @@ -1057,7 +1057,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) for (i = 0; i < data->nr; i++) { struct diffstat_file *file = data->files[i]; - int change = file->added + file->deleted; + uint64_t change = file->added + file->deleted; fill_print_name(file); len = strlen(file->print_name); if (max_len < len) @@ -1085,8 +1085,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) for (i = 0; i < data->nr; i++) { const char *prefix = ""; char *name = data->files[i]->print_name; - int added = data->files[i]->added; - int deleted = data->files[i]->deleted; + uint64_t added = data->files[i]->added; + uint64_t deleted = data->files[i]->deleted; int name_len; /* @@ -1107,9 +1107,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) if (data->files[i]->is_binary) { show_name(options->file, prefix, name, len); fprintf(options->file, " Bin "); - fprintf(options->file, "%s%d%s", del_c, deleted, reset); + fprintf(options->file, "%s%"PRIu64"%s", + del_c, deleted, reset); fprintf(options->file, " -> "); - fprintf(options->file, "%s%d%s", add_c, added, reset); + fprintf(options->file, "%s%"PRIu64"%s", + add_c, added, reset); fprintf(options->file, " bytes"); fprintf(options->file, "\n"); continue; @@ -1138,7 +1140,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) del = scale_linear(del, width, max_change); } show_name(options->file, prefix, name, len); - fprintf(options->file, "%5d%s", added + deleted, + fprintf(options->file, "%5"PRIu64"%s", added + deleted, added + deleted ? " " : ""); show_graph(options->file, '+', add, add_c, reset); show_graph(options->file, '-', del, del_c, reset); @@ -1188,7 +1190,8 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options) fprintf(options->file, "-\t-\t"); else fprintf(options->file, - "%d\t%d\t", file->added, file->deleted); + "%"PRIu64"\t%"PRIu64"\t", + file->added, file->deleted); if (options->line_termination) { fill_print_name(file); if (!file->is_renamed) -- 1.7.1.rc1.277.g2c4c9 ``` ## Junio C Hamano, 2010-04-17 17:00 Subject: Re: [PATCH] diff: use 64-bit integers for diffstat calculations Message-ID: <7vvdbq2ev7.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdbq2ev7.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100417102543.GB23110@coredump.intra.peff.net> ``` Jeff King writes: > Yuck, we use "unsigned int" for the actual storage, and then convert to > a regular "int" in some other places. I think we should just do this: > > -- >8 -- > Subject: [PATCH] diff: use 64-bit integers for diffstat calculations > > The diffstat "added" and "changed" fields generally store > line counts; however, for binary files, they store file > sizes. Since we store and print these values as ints, a > diffstat on a file larger than 2G can show a negative size. > Instead, let's explicitly use 64-bit integers. > > Signed-off-by: Jeff King > --- Yes, but we would probably be better off using using uintmax_t for things like this if the quantity a variable represents is not closely tied to external file format (e.g. the offset field of pack idx file), nor the code is only for a particular platform (e.g. compat/win32mmap.c), don't you think? That is the impression I am getting on the discipline expressed in the current codebase, from browsing the output from "git grep uint64_t". ``` ## Jeff King, 2010-04-17 17:41 Subject: Re: [PATCH] diff: use 64-bit integers for diffstat calculations Message-ID: <20100417174108.GB23642@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100417174108.GB23642%40coredump.intra.peff.net In-Reply-To: <7vvdbq2ev7.fsf@alter.siamese.dyndns.org> ``` On Sat, Apr 17, 2010 at 10:00:44AM -0700, Junio C Hamano wrote: > > The diffstat "added" and "changed" fields generally store > > line counts; however, for binary files, they store file > > sizes. Since we store and print these values as ints, a > > diffstat on a file larger than 2G can show a negative size. > > Instead, let's explicitly use 64-bit integers. > > Yes, but we would probably be better off using using uintmax_t for things > like this if the quantity a variable represents is not closely tied to > external file format (e.g. the offset field of pack idx file), nor the > code is only for a particular platform (e.g. compat/win32mmap.c), don't > you think? But 640K^W 64-bits should be large enough for anyone! :) But yes, you're right. Updated patch below. -- >8 -- Subject: [PATCH] diff: use large integers for diffstat calculations The diffstat "added" and "changed" fields generally store line counts; however, for binary files, they store file sizes. Since we store and print these values as ints, a diffstat on a file larger than 2G can show a negative size. Instead, let's use uintmax_t, which should be at least 64 bits on modern platforms. Signed-off-by: Jeff King --- diff.c | 21 ++++++++++++--------- 1 files changed, 12 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index 7effdac..fbdbd8d 100644 --- a/diff.c +++ b/diff.c @@ -936,7 +936,7 @@ struct diffstat_t { unsigned is_unmerged:1; unsigned is_binary:1; unsigned is_renamed:1; - unsigned int added, deleted; + uintmax_t added, deleted; } **files; }; @@ -1028,7 +1028,7 @@ static void fill_print_name(struct diffstat_file *file) static void show_stats(struct diffstat_t *data, struct diff_options *options) { int i, len, add, del, adds = 0, dels = 0; - int max_change = 0, max_len = 0; + uintmax_t max_change = 0, max_len = 0; int total_files = data->nr; int width, name_width; const char *reset, *set, *add_c, *del_c; @@ -1057,7 +1057,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) for (i = 0; i < data->nr; i++) { struct diffstat_file *file = data->files[i]; - int change = file->added + file->deleted; + uintmax_t change = file->added + file->deleted; fill_print_name(file); len = strlen(file->print_name); if (max_len < len) @@ -1085,8 +1085,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) for (i = 0; i < data->nr; i++) { const char *prefix = ""; char *name = data->files[i]->print_name; - int added = data->files[i]->added; - int deleted = data->files[i]->deleted; + uintmax_t added = data->files[i]->added; + uintmax_t deleted = data->files[i]->deleted; int name_len; /* @@ -1107,9 +1107,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) if (data->files[i]->is_binary) { show_name(options->file, prefix, name, len); fprintf(options->file, " Bin "); - fprintf(options->file, "%s%d%s", del_c, deleted, reset); + fprintf(options->file, "%s%"PRIuMAX"%s", + del_c, deleted, reset); fprintf(options->file, " -> "); - fprintf(options->file, "%s%d%s", add_c, added, reset); + fprintf(options->file, "%s%"PRIuMAX"%s", + add_c, added, reset); fprintf(options->file, " bytes"); fprintf(options->file, "\n"); continue; @@ -1138,7 +1140,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) del = scale_linear(del, width, max_change); } show_name(options->file, prefix, name, len); - fprintf(options->file, "%5d%s", added + deleted, + fprintf(options->file, "%5"PRIuMAX"%s", added + deleted, added + deleted ? " " : ""); show_graph(options->file, '+', add, add_c, reset); show_graph(options->file, '-', del, del_c, reset); @@ -1188,7 +1190,8 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options) fprintf(options->file, "-\t-\t"); else fprintf(options->file, - "%d\t%d\t", file->added, file->deleted); + "%"PRIuMAX"\t%"PRIuMAX"\t", + file->added, file->deleted); if (options->line_termination) { fill_print_name(file); if (!file->is_renamed) -- 1.7.1.rc1.277.g2c4c9 ```