# [PATCH] Show binary file size change in diff --stat 13 messages from 2007-02-28 to 2007-03-01. Participants: Andy Parkins, Johannes Schindelin, Nicolas Pitre, Rogan Dawes. Thread: https://gitlist.dev/t/7015 ## Andy Parkins, 2007-02-28 13:03 Subject: [PATCH] Show binary file size change in diff --stat Message-ID: <200702281303.11951.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702281303.11951.andyparkins%40gmail.com ``` Previously, a binary file in the diffstat would show as: some-binary-file.bin | Bin The space after the "Bin" was never used. This patch changes binary lines in the diffstat to be: some-binary-file.bin | Bin +123456B -12345B The "+" item is the size of the new version, the "-" item is the size of the old version. If a size is 0 it's not shown (although it would probably be better to treat no-file differently from zero-byte-file). The "B" (for bytes) is shown to highlight the fact that these numbers are not "number of lines", but actual bytes. The user can see what changed in the binary file, and how big the new file is. This is in keeping with the information in the rest of the diffstat. The diffstat_t members "added" and "deleted" were unused when the file was binary, so this patch loads them with the file sizes in builtin_diffstat(). These figures are then read in show_stats() when the file is marked binary. Signed-off-by: Andy Parkins --- diff.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index d1eae72..174b84e 100644 --- a/diff.c +++ b/diff.c @@ -808,7 +808,14 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options) if (data->files[i]->is_binary) { show_name(prefix, name, len, reset, set); - printf(" Bin\n"); + printf(" Bin "); + if (added != 0) + printf("%s+%iB%s", add_c, added, reset); + if (added != 0 && deleted != 0 ) + printf(" "); + if (deleted != 0) + printf("%s-%iB%s", del_c, deleted, reset); + printf("\n"); goto free_diffstat_file; } else if (data->files[i]->is_unmerged) { @@ -1179,9 +1186,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b, if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) die("unable to read files to diff"); - if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) + if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) { data->is_binary = 1; - else { + data->added = mf2.size; + data->deleted = mf1.size; + } else { /* Crazy xdl interfaces.. */ xpparam_t xpp; xdemitconf_t xecfg; -- 1.5.0.2.778.gdcb06 ``` ## Johannes Schindelin, 2007-02-28 14:44 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702281535070.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <200702281303.11951.andyparkins@gmail.com> ``` Hi, On Wed, 28 Feb 2007, Andy Parkins wrote: > Previously, a binary file in the diffstat would show as: > > some-binary-file.bin | Bin > > The space after the "Bin" was never used. This patch changes binary > lines in the diffstat to be: > > some-binary-file.bin | Bin +123456B -12345B This seemed as a good idea to me at first. But I think it is a little misleading: Imagine for example that you have raw image data (which I track with Git, too). If only part of that changes, this representation is misleading. How about "Bin 123456 -> 12345 bytes" instead? Ciao, Dscho ``` ## Nicolas Pitre, 2007-02-28 14:51 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: URL: https://gitlist.dev/e/alpine.LRH.0.82.0702280951080.29426%40xanadu.home In-Reply-To: ``` On Wed, 28 Feb 2007, Johannes Schindelin wrote: > How about "Bin 123456 -> 12345 bytes" instead? I agree. Nicolas ``` ## Andy Parkins, 2007-02-28 15:15 Subject: [PATCH] Show binary file size change in diff --stat Message-ID: <200702281515.10016.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702281515.10016.andyparkins%40gmail.com In-Reply-To: ``` Previously, a binary file in the diffstat would show as: some-binary-file.bin | Bin The space after the "Bin" was never used. This patch changes binary lines in the diffstat to be: some-binary-file.bin | Bin 123456 -> 12345 bytes The very nice "->" notation was suggested by Johannes Schindelin, and shows the before and after sizes more clearly than "+" and "-" would. If a size is 0 it's not shown (although it would probably be better to treat no-file differently from zero-byte-file). The user can see what changed in the binary file, and how big the new file is. This is in keeping with the information in the rest of the diffstat. The diffstat_t members "added" and "deleted" were unused when the file was binary, so this patch loads them with the file sizes in builtin_diffstat(). These figures are then read in show_stats() when the file is marked binary. Signed-off-by: Andy Parkins --- Good call. I like it. "->" is much better than "+" and "-"; they would imply that a set of bytes had been removed and another set added to replace them - obviously nonsense. diff.c | 17 ++++++++++++++--- 1 files changed, 14 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index d1eae72..ea322e2 100644 --- a/diff.c +++ b/diff.c @@ -808,7 +808,16 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options) if (data->files[i]->is_binary) { show_name(prefix, name, len, reset, set); - printf(" Bin\n"); + printf(" Bin "); + if (added != 0) + printf("%s%i%s", add_c, added, reset); + if (added != 0 && deleted != 0 ) + printf(" -> "); + if (deleted != 0) + printf("%s%i%s", del_c, deleted, reset); + if (added != 0 || deleted != 0 ) + printf(" bytes"); + printf("\n"); goto free_diffstat_file; } else if (data->files[i]->is_unmerged) { @@ -1179,9 +1188,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b, if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) die("unable to read files to diff"); - if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) + if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) { data->is_binary = 1; - else { + data->added = mf2.size; + data->deleted = mf1.size; + } else { /* Crazy xdl interfaces.. */ xpparam_t xpp; xdemitconf_t xecfg; -- 1.5.0.2.205.gea888-dirty ``` ## Andy Parkins, 2007-02-28 15:26 Subject: [PATCH] Show binary file size change in diff --stat Message-ID: <200702281526.26247.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702281526.26247.andyparkins%40gmail.com In-Reply-To: ``` Previously, a binary file in the diffstat would show as: some-binary-file.bin | Bin The space after the "Bin" was never used. This patch changes binary lines in the diffstat to be: some-binary-file.bin | Bin 12345 -> 123456 bytes The very nice "->" notation was suggested by Johannes Schindelin, and shows the before and after sizes more clearly than "+" and "-" would. The user can see what changed in the binary file, and how big the new file is. This is in keeping with the information in the rest of the diffstat. The diffstat_t members "added" and "deleted" were unused when the file was binary, so this patch loads them with the file sizes in builtin_diffstat(). These figures are then read in show_stats() when the file is marked binary. Signed-off-by: Andy Parkins --- GAHHH!!! Ignore previous; I forgot to swap the order of the added and deleted items. I've also removed the conditionals on the 0 byte sizes - they're always shown. diff.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index d1eae72..cfddbb4 100644 --- a/diff.c +++ b/diff.c @@ -808,7 +808,12 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options) if (data->files[i]->is_binary) { show_name(prefix, name, len, reset, set); - printf(" Bin\n"); + printf(" Bin "); + printf("%s%i%s", del_c, deleted, reset); + printf(" -> "); + printf("%s%i%s", add_c, added, reset); + printf(" bytes"); + printf("\n"); goto free_diffstat_file; } else if (data->files[i]->is_unmerged) { @@ -1179,9 +1184,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b, if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) die("unable to read files to diff"); - if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) + if (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) { data->is_binary = 1; - else { + data->added = mf2.size; + data->deleted = mf1.size; + } else { /* Crazy xdl interfaces.. */ xpparam_t xpp; xdemitconf_t xecfg; -- 1.5.0.2.205.gea888-dirty ``` ## Johannes Schindelin, 2007-02-28 15:37 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702281630430.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <200702281515.10016.andyparkins@gmail.com> ``` Hi, On Wed, 28 Feb 2007, Andy Parkins wrote: > + printf("%s%i%s", add_c, added, reset); Please use "%d" instead of "%i" (I forgot why, but I remember Junio saying that we should do it...). > + if (added != 0 && deleted != 0 ) > + printf(" -> "); Either you want to have Bin +123456 bytes or Bin 0 -> 123456 bytes for added files (and the obvious thing for deleted ones), but with your patch, both added and deleted get Bin 123456 bytes which is not so optimal. Ciao, Dscho ``` ## Andy Parkins, 2007-02-28 18:42 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: <200702281842.15147.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702281842.15147.andyparkins%40gmail.com In-Reply-To: ``` On Wednesday 2007, February 28, Johannes Schindelin wrote: > for added files (and the obvious thing for deleted ones), but with > your patch, both added and deleted get > > Bin 123456 bytes > > which is not so optimal. See my corrected patch. However, while you're interested: how would one tell the difference between 0 bytes and (no file). I'm thinking of these possibilities: new -> 123456 0 -> 123456 123456 -> deleted 123456 -> 0 For example - does mmfile_t.ptr set to NULL mean anything? Andy -- Dr Andy Parkins, M Eng (hons), MIET andyparkins@gmail.com ``` ## Rogan Dawes, 2007-02-28 18:58 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: <45E5D0D7.5070305@dawes.za.net> URL: https://gitlist.dev/e/45E5D0D7.5070305%40dawes.za.net In-Reply-To: <200702281303.11951.andyparkins@gmail.com> ``` Andy Parkins wrote: > Previously, a binary file in the diffstat would show as: > > some-binary-file.bin | Bin > > The space after the "Bin" was never used. This patch changes binary > lines in the diffstat to be: > > some-binary-file.bin | Bin +123456B -12345B > > The "+" item is the size of the new version, the "-" item is the size of > the old version. If a size is 0 it's not shown (although it would > probably be better to treat no-file differently from zero-byte-file). > > The "B" (for bytes) is shown to highlight the fact that these numbers > are not "number of lines", but actual bytes. > > The user can see what changed in the binary file, and how big the new > file is. This is in keeping with the information in the rest of the > diffstat. > > The diffstat_t members "added" and "deleted" were unused when the file > was binary, so this patch loads them with the file sizes in > builtin_diffstat(). These figures are then read in show_stats() when > the file is marked binary. How about showing the size of the changes between the 2 files via the libxdiff binary patch function? It would probably not be that difficult to generate a binary patch, and then derive some stats from it. Rogan ``` ## Johannes Schindelin, 2007-02-28 19:41 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702282040430.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <200702281842.15147.andyparkins@gmail.com> ``` Hi, On Wed, 28 Feb 2007, Andy Parkins wrote: > On Wednesday 2007, February 28, Johannes Schindelin wrote: > > > for added files (and the obvious thing for deleted ones), but with > > your patch, both added and deleted get > > > > Bin 123456 bytes > > > > which is not so optimal. > > See my corrected patch. However, while you're interested: how would one > tell the difference between 0 bytes and (no file). I'm thinking of > these possibilities: > > new -> 123456 > 0 -> 123456 > 123456 -> deleted > 123456 -> 0 > > For example - does mmfile_t.ptr set to NULL mean anything? In non-binary case, we have only plusses or minusses, so you cannot tell either. You have to look into the summary for that. Ciao, Dscho ``` ## Johannes Schindelin, 2007-02-28 19:42 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702282042000.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <45E5D0D7.5070305@dawes.za.net> ``` Hi, On Wed, 28 Feb 2007, Rogan Dawes wrote: > How about showing the size of the changes between the 2 files via the > libxdiff binary patch function? I briefly considered this, too. But what would it tell you in the case of a jpg? I think it has more disadvantages than advantages... Ciao, Dscho ``` ## Rogan Dawes, 2007-02-28 21:27 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: <45E5F3D4.8000509@dawes.za.net> URL: https://gitlist.dev/e/45E5F3D4.8000509%40dawes.za.net In-Reply-To: ``` Johannes Schindelin wrote: > Hi, > > On Wed, 28 Feb 2007, Rogan Dawes wrote: > >> How about showing the size of the changes between the 2 files via the >> libxdiff binary patch function? > > I briefly considered this, too. But what would it tell you in the case of > a jpg? I think it has more disadvantages than advantages... > > Ciao, > Dscho It would still tell you the extent of the changes. i.e. Did we change only 10 bytes of the file, or is it a dramatic change? This is exactly what the text statistics show. +(new lines + modified lines) -(deleted lines + modified lines) Instead of having a "line-based" record size, simply use individual bytes, since binary files don't have lines (most of them, anyway!). Rogan ``` ## Johannes Schindelin, 2007-03-01 01:09 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0703010208070.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <45E5F3D4.8000509@dawes.za.net> ``` Hi, On Wed, 28 Feb 2007, Rogan Dawes wrote: > Johannes Schindelin wrote: > > > On Wed, 28 Feb 2007, Rogan Dawes wrote: > > > > > How about showing the size of the changes between the 2 files via > > > the libxdiff binary patch function? > > > > I briefly considered this, too. But what would it tell you in the case > > of a jpg? I think it has more disadvantages than advantages... > > It would still tell you the extent of the changes. i.e. Did we change > only 10 bytes of the file, or is it a dramatic change? I was not explicit enough, okay. I was not so worried about the case where only 10 bytes changed. If you insert a single dot in a jpg image, chances are that your binary content will change _a lot_. So, no problem deducing from 10 bytes changed that it was a minor change. But you cannot deduce the opposite of a 1MB change! Hth, Dscho ``` ## Rogan Dawes, 2007-03-01 06:58 Subject: Re: [PATCH] Show binary file size change in diff --stat Message-ID: <45E67978.9030805@dawes.za.net> URL: https://gitlist.dev/e/45E67978.9030805%40dawes.za.net In-Reply-To: ``` Johannes Schindelin wrote: > Hi, > > On Wed, 28 Feb 2007, Rogan Dawes wrote: > >> Johannes Schindelin wrote: >> >>> On Wed, 28 Feb 2007, Rogan Dawes wrote: >>> >>>> How about showing the size of the changes between the 2 files via >>>> the libxdiff binary patch function? >>> I briefly considered this, too. But what would it tell you in the case >>> of a jpg? I think it has more disadvantages than advantages... >> It would still tell you the extent of the changes. i.e. Did we change >> only 10 bytes of the file, or is it a dramatic change? > > I was not explicit enough, okay. I was not so worried about the case where > only 10 bytes changed. If you insert a single dot in a jpg image, chances > are that your binary content will change _a lot_. > > So, no problem deducing from 10 bytes changed that it was a minor change. > But you cannot deduce the opposite of a 1MB change! > > Hth, > Dscho > Yeah, I did understand that. However, if we were to generalise, making a change to an XML document (wrapping the whole thing in an additional tag, with the associated indentation adjustments), we'd still see all the lines in the file change, even though the actual (semantic?) change is small. However, we would still report the full numbers of lines added and deleted regardless. Why should a binary file be any different? It would be up to the observer to make a conclusion as to the significance of the numbers, based on their intrinsic knowledge of the file type, and its properties. I just believe that showing bytes changed for binary files is _consistent_ with what we do for text. Rogan ```