threads / patch / 7015

patchShow binary file size change in diff --stat

Subject: [PATCH] Show binary file size change in diff --stat

## tl;dr

13 messages between Feb 28, 2007 and Mar 1, 2007. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Andy Parkins· Feb 28, 2007, 13:03 UTC · lore
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 <andyparkins@gmail.com>
---
 diff.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)
Show changes to diff.c +12 −3
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· Feb 28, 2007, 14:44 UTC · re: Andy Parkins · lore

Re: [PATCH] Show binary file size change in diff --stat

Hi,
On Wed, 28 Feb 2007, Andy Parkins wrote:
Show 8 quoted lines
> 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

Andy Parkins· Feb 28, 2007, 15:15 UTC · re: Johannes Schindelin · lore
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 <andyparkins@gmail.com>
---
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(-)
Show changes to diff.c +14 −3
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
Johannes Schindelin· Feb 28, 2007, 15:37 UTC · re: Andy Parkins · lore

Re: [PATCH] Show binary file size change in diff --stat

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· Feb 28, 2007, 18:42 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Show binary file size change in diff --stat

On Wednesday 2007, February 28, Johannes Schindelin wrote:
Show 6 quoted lines
> 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
Johannes Schindelin· Feb 28, 2007, 19:41 UTC · re: Andy Parkins · lore

Re: [PATCH] Show binary file size change in diff --stat

Hi,
On Wed, 28 Feb 2007, Andy Parkins wrote:
Show 19 quoted lines
> 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

Andy Parkins· Feb 28, 2007, 15:26 UTC · re: Johannes Schindelin · lore
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 <andyparkins@gmail.com>
---
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(-)
Show changes to diff.c +10 −3
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
Rogan Dawes· Feb 28, 2007, 18:58 UTC · re: Andy Parkins · lore

Re: [PATCH] Show binary file size change in diff --stat

Andy Parkins wrote:
Show 24 quoted lines
> 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· Feb 28, 2007, 19:42 UTC · re: Rogan Dawes · lore

Re: [PATCH] Show binary file size change in diff --stat

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· Feb 28, 2007, 21:27 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Show binary file size change in diff --stat

Johannes Schindelin wrote:
Show 12 quoted lines
> 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· Mar 1, 2007, 01:09 UTC · re: Rogan Dawes · lore

Re: [PATCH] Show binary file size change in diff --stat

Hi,
On Wed, 28 Feb 2007, Rogan Dawes wrote:
Show 12 quoted lines
> 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· Mar 1, 2007, 06:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Show binary file size change in diff --stat

Johannes Schindelin wrote:
Show 25 quoted lines
> 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

← back to recent threads