* [PATCH 0/4] report chmod'ed binary files the same as text files
@ 2012-05-01 17:10 Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 1/4] test: modernize style of t4006 Zbigniew Jędrzejewski-Szmek
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 17:10 UTC (permalink / raw)
To: git, gitster; +Cc: mj, Zbigniew Jędrzejewski-Szmek
This patch series fixes a small discrepancy between the way that
text files and binary files are treated. Reported by Martin Mareš in [1].
Firt patch is cleanup, second describes current behaviour, third does
the change, and fourth is a bonus micro-opt.
[1] http://article.gmane.org/gmane.comp.version-control.git/179361
Zbigniew Jędrzejewski-Szmek (4):
test: modernize style of t4006
tests: check --[short]stat output after chmod
diff --stat: report chmoded binary files like text files
diff --stat: do not run diff on indentical files
diff.c | 30 +++++++++++++----------
t/t4006-diff-mode.sh | 65 ++++++++++++++++++++++++++++++++++++--------------
2 files changed, 65 insertions(+), 30 deletions(-)
--
1.7.10.539.g288dd
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] test: modernize style of t4006
2012-05-01 17:10 [PATCH 0/4] report chmod'ed binary files the same as text files Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 17:10 ` Zbigniew Jędrzejewski-Szmek
2012-05-01 18:00 ` Junio C Hamano
2012-05-01 17:10 ` [PATCH 2/4] tests: check --[short]stat output after chmod Zbigniew Jędrzejewski-Szmek
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 17:10 UTC (permalink / raw)
To: git, gitster; +Cc: mj, Zbigniew Jędrzejewski-Szmek
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t4006-diff-mode.sh | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
index ff8c2f7..c8f5180 100755
--- a/t/t4006-diff-mode.sh
+++ b/t/t4006-diff-mode.sh
@@ -8,23 +8,21 @@ test_description='Test mode change diffs.
'
. ./test-lib.sh
-test_expect_success \
- 'setup' \
- 'echo frotz >rezrov &&
- git update-index --add rezrov &&
- tree=`git write-tree` &&
- echo $tree'
-
-test_expect_success \
- 'chmod' \
- 'test_chmod +x rezrov &&
- git diff-index $tree >current'
-
-sed -e 's/\(:100644 100755\) \('"$_x40"'\) \2 /\1 X X /' <current >check
-echo ":100644 100755 X X M rezrov" >expected
+test_expect_success 'setup' '
+ echo frotz >rezrov &&
+ git update-index --add rezrov &&
+ tree=`git write-tree` &&
+ echo $tree
+'
-test_expect_success \
- 'verify' \
- 'test_cmp expected check'
+# $_x40 is defined in test-lib.sh
+sed_script='s/\(:100644 100755\) \('"$_x40"'\) \2 /\1 X X /'
+test_expect_success 'chmod' '
+ test_chmod +x rezrov &&
+ git diff-index $tree >current &&
+ sed -e "$sed_script" <current >check &&
+ echo ":100644 100755 X X M rezrov" >expected &&
+ test_cmp expected check
+'
test_done
--
1.7.10.539.g288dd
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] tests: check --[short]stat output after chmod
2012-05-01 17:10 [PATCH 0/4] report chmod'ed binary files the same as text files Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 1/4] test: modernize style of t4006 Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 17:10 ` Zbigniew Jędrzejewski-Szmek
2012-05-02 7:36 ` Johannes Sixt
2012-05-01 17:10 ` [PATCH 3/4] diff --stat: report chmoded binary files like text files Zbigniew Jędrzejewski-Szmek
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 17:10 UTC (permalink / raw)
To: git, gitster; +Cc: mj, Zbigniew Jędrzejewski-Szmek
Add a test to check 'diff --stat' output with a text file after chmod,
and the same for a binary file. This demonstrates that text and binary
files are treated differently, which can be misleading.
While at it, duplicate the tests to check --shortstat output too.
Reported-by: Martin Mareš <mj@ucw.cz>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t4006-diff-mode.sh | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
index c8f5180..a81c095 100755
--- a/t/t4006-diff-mode.sh
+++ b/t/t4006-diff-mode.sh
@@ -25,4 +25,41 @@ test_expect_success 'chmod' '
test_cmp expected check
'
+test_expect_success 'prepare binary file' '
+ git commit -m rezrov &&
+ dd if=/dev/zero of=binbin bs=1024 count=1 &&
+ git add binbin &&
+ git commit -m binbin
+'
+
+test_expect_success '--stat output after text chmod' '
+ test_chmod -x rezrov &&
+ echo " 0 files changed" >expect &&
+ git diff HEAD --stat >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--shortstat output after text chmod' '
+ git diff HEAD --shortstat >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--stat output after binary chmod' '
+ test_chmod +x binbin &&
+ cat >expect <<-EOF &&
+ binbin | Bin 1024 -> 1024 bytes
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+ EOF
+ git diff HEAD --stat >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--shortstat output after binary chmod' '
+ cat >expect <<-EOF &&
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+ EOF
+ git diff HEAD --shortstat >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.10.539.g288dd
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] diff --stat: report chmoded binary files like text files
2012-05-01 17:10 [PATCH 0/4] report chmod'ed binary files the same as text files Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 1/4] test: modernize style of t4006 Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 2/4] tests: check --[short]stat output after chmod Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 17:10 ` Zbigniew Jędrzejewski-Szmek
2012-05-01 18:27 ` Junio C Hamano
2012-05-01 17:10 ` [PATCH 4/4] diff --stat: do not run diff on indentical files Zbigniew Jędrzejewski-Szmek
2012-05-03 11:45 ` [PATCH 0/4] report chmod'ed binary files the same as text files Martin Mares
4 siblings, 1 reply; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 17:10 UTC (permalink / raw)
To: git, gitster; +Cc: mj, Zbigniew Jędrzejewski-Szmek
Binary files chmoded without content change were reported as if they
were rewritten. At the same time, text files in the same situation
were reported as "unchanged". Let's treat binary files like text files
here, and simply say that they are unchanged.
For text files, we knew that they were unchanged if the numbers of
lines added and deleted were both 0. For binary files this metric does
not make sense and is not calculated, so a new way of conveying this
information is needed. A new flag is_unchanged is added in struct
diffstat_t that is set if the contents of both files are identical.
For consistency, this new flag is used both for text files and binary
files.
Output of --shortstat is modified in the same way.
Reported-by: Martin Mareš <mj@ucw.cz>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
diff.c | 28 +++++++++++++++++-----------
t/t4006-diff-mode.sh | 8 +-------
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/diff.c b/diff.c
index 7da16c9..6eb2946 100644
--- a/diff.c
+++ b/diff.c
@@ -1299,6 +1299,7 @@ struct diffstat_t {
unsigned is_unmerged:1;
unsigned is_binary:1;
unsigned is_renamed:1;
+ unsigned is_unchanged:1;
uintmax_t added, deleted;
} **files;
};
@@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
struct diffstat_file *file = data->files[i];
uintmax_t change = file->added + file->deleted;
if (!data->files[i]->is_renamed &&
- (change == 0)) {
+ data->files[i]->is_unchanged) {
count++; /* not shown == room for one more */
continue;
}
@@ -1565,7 +1566,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
int name_len;
if (!data->files[i]->is_renamed &&
- (added + deleted == 0)) {
+ data->files[i]->is_unchanged) {
total_files--;
continue;
}
@@ -1587,8 +1588,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (data->files[i]->is_binary) {
fprintf(options->file, "%s", line_prefix);
show_name(options->file, prefix, name, len);
- fprintf(options->file, " Bin ");
- fprintf(options->file, "%s%"PRIuMAX"%s",
+ fprintf(options->file, " Bin");
+ if (data->files[i]->is_unchanged) {
+ fprintf(options->file, "\n");
+ continue;
+ }
+ fprintf(options->file, " %s%"PRIuMAX"%s",
del_c, deleted, reset);
fprintf(options->file, " -> ");
fprintf(options->file, "%s%"PRIuMAX"%s",
@@ -1661,16 +1666,15 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
return;
for (i = 0; i < data->nr; i++) {
- if (!data->files[i]->is_binary &&
- !data->files[i]->is_unmerged) {
- int added = data->files[i]->added;
- int deleted= data->files[i]->deleted;
+ if (!data->files[i]->is_unmerged) {
if (!data->files[i]->is_renamed &&
- (added + deleted == 0)) {
+ data->files[i]->is_unchanged) {
total_files--;
+ } else if (data->files[i]->is_binary) {
+ ; /* do nothing */
} else {
- adds += added;
- dels += deleted;
+ adds += data->files[i]->added;
+ dels += data->files[i]->deleted;
}
}
}
@@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
return;
}
+ data->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;
+
if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
data->is_binary = 1;
data->added = diff_filespec_size(two);
diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
index a81c095..e85a1d6 100755
--- a/t/t4006-diff-mode.sh
+++ b/t/t4006-diff-mode.sh
@@ -46,18 +46,12 @@ test_expect_success '--shortstat output after text chmod' '
test_expect_success '--stat output after binary chmod' '
test_chmod +x binbin &&
- cat >expect <<-EOF &&
- binbin | Bin 1024 -> 1024 bytes
- 1 file changed, 0 insertions(+), 0 deletions(-)
- EOF
+ echo " 0 files changed" >expect &&
git diff HEAD --stat >actual &&
test_cmp expect actual
'
test_expect_success '--shortstat output after binary chmod' '
- cat >expect <<-EOF &&
- 1 file changed, 0 insertions(+), 0 deletions(-)
- EOF
git diff HEAD --shortstat >actual &&
test_cmp expect actual
'
--
1.7.10.539.g288dd
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] diff --stat: do not run diff on indentical files
2012-05-01 17:10 [PATCH 0/4] report chmod'ed binary files the same as text files Zbigniew Jędrzejewski-Szmek
` (2 preceding siblings ...)
2012-05-01 17:10 ` [PATCH 3/4] diff --stat: report chmoded binary files like text files Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 17:10 ` Zbigniew Jędrzejewski-Szmek
2012-05-03 11:45 ` [PATCH 0/4] report chmod'ed binary files the same as text files Martin Mares
4 siblings, 0 replies; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 17:10 UTC (permalink / raw)
To: git, gitster; +Cc: mj, Zbigniew Jędrzejewski-Szmek
If sha1's are equal, then there's no point in performing the diff.
In a very unscientific test:
git init &&
dd if=/dev/urandom bs=1M count=30 | hexdump >file1 &&
git add file1 && git commit -m 'add file' &&
git mv file1 file1-moved && chmod +x file1-moved &&
command time git diff --stat
(before) git diff --stat 2.00s user 0.31s system 99% cpu 2.323 total
(after) git diff --stat 0.80s user 0.10s system 98% cpu 0.913 total
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
diff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index 6eb2946..7cb9893 100644
--- a/diff.c
+++ b/diff.c
@@ -2398,7 +2398,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
data->added = count_lines(two->data, two->size);
}
- else {
+ else if (!data->is_unchanged) {
/* Crazy xdl interfaces.. */
xpparam_t xpp;
xdemitconf_t xecfg;
--
1.7.10.539.g288dd
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] test: modernize style of t4006
2012-05-01 17:10 ` [PATCH 1/4] test: modernize style of t4006 Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 18:00 ` Junio C Hamano
2012-05-01 19:55 ` Zbigniew Jędrzejewski-Szmek
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2012-05-01 18:00 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, mj
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
> t/t4006-diff-mode.sh | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
Style update is welcome, but shouldn't the assignment to sed_script
be done in the second test if it is the only user? If you are going to
add more tests at the end, then it should be away from the second test to
make it clear that it is not part of it.
Thanks.
> diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
> index ff8c2f7..c8f5180 100755
> --- a/t/t4006-diff-mode.sh
> +++ b/t/t4006-diff-mode.sh
> @@ -8,23 +8,21 @@ test_description='Test mode change diffs.
> '
> . ./test-lib.sh
>
> -test_expect_success \
> - 'setup' \
> - 'echo frotz >rezrov &&
> - git update-index --add rezrov &&
> - tree=`git write-tree` &&
> - echo $tree'
> -
> -test_expect_success \
> - 'chmod' \
> - 'test_chmod +x rezrov &&
> - git diff-index $tree >current'
> -
> -sed -e 's/\(:100644 100755\) \('"$_x40"'\) \2 /\1 X X /' <current >check
> -echo ":100644 100755 X X M rezrov" >expected
> +test_expect_success 'setup' '
> + echo frotz >rezrov &&
> + git update-index --add rezrov &&
> + tree=`git write-tree` &&
> + echo $tree
> +'
>
> -test_expect_success \
> - 'verify' \
> - 'test_cmp expected check'
> +# $_x40 is defined in test-lib.sh
> +sed_script='s/\(:100644 100755\) \('"$_x40"'\) \2 /\1 X X /'
> +test_expect_success 'chmod' '
> + test_chmod +x rezrov &&
> + git diff-index $tree >current &&
> + sed -e "$sed_script" <current >check &&
> + echo ":100644 100755 X X M rezrov" >expected &&
> + test_cmp expected check
> +'
>
> test_done
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files
2012-05-01 17:10 ` [PATCH 3/4] diff --stat: report chmoded binary files like text files Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 18:27 ` Junio C Hamano
2012-05-01 19:39 ` Zbigniew Jędrzejewski-Szmek
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2012-05-01 18:27 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, mj
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> Binary files chmoded without content change were reported as if they
> were rewritten. At the same time, text files in the same situation
> were reported as "unchanged". Let's treat binary files like text files
> here, and simply say that they are unchanged.
>
> For text files, we knew that they were unchanged if the numbers of
> lines added and deleted were both 0. For binary files this metric does
> not make sense and is not calculated, so a new way of conveying this
> information is needed. A new flag is_unchanged is added in struct
> diffstat_t that is set if the contents of both files are identical.
> For consistency, this new flag is used both for text files and binary
> files.
>
> Output of --shortstat is modified in the same way.
>
> Reported-by: Martin Mareš <mj@ucw.cz>
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
> diff.c | 28 +++++++++++++++++-----------
> t/t4006-diff-mode.sh | 8 +-------
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 7da16c9..6eb2946 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1299,6 +1299,7 @@ struct diffstat_t {
> unsigned is_unmerged:1;
> unsigned is_binary:1;
> unsigned is_renamed:1;
> + unsigned is_unchanged:1;
The name is somewhat misleading, as a filepair that consists of two blobs
with the same contents with different mode bits is still "changed", and
you are trying to say that they have the same contents.
> @@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> struct diffstat_file *file = data->files[i];
> uintmax_t change = file->added + file->deleted;
> if (!data->files[i]->is_renamed &&
> - (change == 0)) {
> + data->files[i]->is_unchanged) {
I am not sure if all these hunks are needed. If you are going to show
only " Bin\n" for a filepair with the same binary contents, perhaps it is
simpler to set added/deleted fields of such a filepair to 0? Then most of
the hunks in this patch can disappear, no?
> @@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
> return;
> }
>
> + data->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;
Please write it as "!hashcmp(a, b)", not "hashcmp(a, b) == 0".
In any case, how about doing it like this instead?
diff.c | 38 +++++++++++++++++++++++---------------
t/t4006-diff-mode.sh | 8 +-------
2 files changed, 24 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index 22288b0..338ef41 100644
--- a/diff.c
+++ b/diff.c
@@ -1583,8 +1583,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (data->files[i]->is_binary) {
fprintf(options->file, "%s", line_prefix);
show_name(options->file, prefix, name, len);
- fprintf(options->file, " Bin ");
- fprintf(options->file, "%s%"PRIuMAX"%s",
+ fprintf(options->file, " Bin");
+ if (!added && !deleted) {
+ putc('\n', options->file);
+ continue;
+ }
+ fprintf(options->file, " %s%"PRIuMAX"%s",
del_c, deleted, reset);
fprintf(options->file, " -> ");
fprintf(options->file, "%s%"PRIuMAX"%s",
@@ -1657,17 +1661,16 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
return;
for (i = 0; i < data->nr; i++) {
- if (!data->files[i]->is_binary &&
- !data->files[i]->is_unmerged) {
- int added = data->files[i]->added;
- int deleted= data->files[i]->deleted;
- if (!data->files[i]->is_renamed &&
- (added + deleted == 0)) {
- total_files--;
- } else {
- adds += added;
- dels += deleted;
- }
+ int added = data->files[i]->added;
+ int deleted= data->files[i]->deleted;
+
+ if (data->files[i]->is_unmerged)
+ continue;
+ if (!data->files[i]->is_renamed && (added + deleted == 0)) {
+ total_files--;
+ } else {
+ adds += added;
+ dels += deleted;
}
}
if (options->output_prefix) {
@@ -2377,8 +2380,13 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
data->is_binary = 1;
- data->added = diff_filespec_size(two);
- data->deleted = diff_filespec_size(one);
+ if (!hashcmp(one->sha1, two->sha1)) {
+ data->added = 0;
+ data->deleted = 0;
+ } else {
+ data->added = diff_filespec_size(one);
+ data->deleted = diff_filespec_size(two);
+ }
}
else if (complete_rewrite) {
diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
index 392dfef..693bfc4 100755
--- a/t/t4006-diff-mode.sh
+++ b/t/t4006-diff-mode.sh
@@ -46,18 +46,12 @@ test_expect_success '--shortstat output after text chmod' '
test_expect_success '--stat output after binary chmod' '
test_chmod +x binbin &&
- cat >expect <<-EOF &&
- binbin | Bin 1024 -> 1024 bytes
- 1 file changed, 0 insertions(+), 0 deletions(-)
- EOF
+ echo " 0 files changed" >expect &&
git diff HEAD --stat >actual &&
test_cmp expect actual
'
test_expect_success '--shortstat output after binary chmod' '
- cat >expect <<-EOF &&
- 1 file changed, 0 insertions(+), 0 deletions(-)
- EOF
git diff HEAD --shortstat >actual &&
test_cmp expect actual
'
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files
2012-05-01 18:27 ` Junio C Hamano
@ 2012-05-01 19:39 ` Zbigniew Jędrzejewski-Szmek
0 siblings, 0 replies; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 19:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, mj
On 05/01/2012 08:27 PM, Junio C Hamano wrote:
> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
>
>> Binary files chmoded without content change were reported as if they
>> were rewritten. At the same time, text files in the same situation
>> were reported as "unchanged". Let's treat binary files like text files
>> here, and simply say that they are unchanged.
>>
>> For text files, we knew that they were unchanged if the numbers of
>> lines added and deleted were both 0. For binary files this metric does
>> not make sense and is not calculated, so a new way of conveying this
>> information is needed. A new flag is_unchanged is added in struct
>> diffstat_t that is set if the contents of both files are identical.
>> For consistency, this new flag is used both for text files and binary
>> files.
>>
>> Output of --shortstat is modified in the same way.
>>
>> Reported-by: Martin Mareš <mj@ucw.cz>
>> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
>> ---
>> diff.c | 28 +++++++++++++++++-----------
>> t/t4006-diff-mode.sh | 8 +-------
>> 2 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index 7da16c9..6eb2946 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -1299,6 +1299,7 @@ struct diffstat_t {
>> unsigned is_unmerged:1;
>> unsigned is_binary:1;
>> unsigned is_renamed:1;
>> + unsigned is_unchanged:1;
>
> The name is somewhat misleading, as a filepair that consists of two blobs
> with the same contents with different mode bits is still "changed", and
> you are trying to say that they have the same contents.
>
>> @@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> struct diffstat_file *file = data->files[i];
>> uintmax_t change = file->added + file->deleted;
>> if (!data->files[i]->is_renamed &&
>> - (change == 0)) {
>> + data->files[i]->is_unchanged) {
>
> I am not sure if all these hunks are needed. If you are going to show
> only " Bin\n" for a filepair with the same binary contents, perhaps it is
> simpler to set added/deleted fields of such a filepair to 0? Then most of
> the hunks in this patch can disappear, no?
>
>> @@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
>> return;
>> }
>>
>> + data->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;
>
> Please write it as "!hashcmp(a, b)", not "hashcmp(a, b) == 0".
>
> In any case, how about doing it like this instead?
Yeah, this is much nicer.
On top of this, 4/4 becomes:
- else {
+ else if (hashcmp(one->sha1, two->sha1)) {
and the time improvement is the same (0.8 vs 2.0 s).
Do you want me to resend with your replacement patch?
Zbyszek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] test: modernize style of t4006
2012-05-01 18:00 ` Junio C Hamano
@ 2012-05-01 19:55 ` Zbigniew Jędrzejewski-Szmek
0 siblings, 0 replies; 11+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, mj
On 05/01/2012 08:00 PM, Junio C Hamano wrote:
> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
>
>> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
>> ---
>> t/t4006-diff-mode.sh | 32 +++++++++++++++-----------------
>> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> Style update is welcome, but shouldn't the assignment to sed_script
> be done in the second test if it is the only user? If you are going to
> add more tests at the end, then it should be away from the second test to
> make it clear that it is not part of it.
Hi,
$sed_script is indeed only used in that one test. But moving the
assignment inside would complicate the quoting rules (the script is now
quoted with ', but this would have to change inside the test case which
is quoted with ' too). I actually think it's simpler this way.
Thanks,
Zbyszek
>> -sed -e 's/\(:100644 100755\) \('"$_x40"'\) \2 /\1 X X /' <current >check
>> -echo ":100644 100755 X X M rezrov" >expected
>> +# $_x40 is defined in test-lib.sh
>> +sed_script='s/\(:100644 100755\) \('"$_x40"'\) \2 /\1 X X /'
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] tests: check --[short]stat output after chmod
2012-05-01 17:10 ` [PATCH 2/4] tests: check --[short]stat output after chmod Zbigniew Jędrzejewski-Szmek
@ 2012-05-02 7:36 ` Johannes Sixt
0 siblings, 0 replies; 11+ messages in thread
From: Johannes Sixt @ 2012-05-02 7:36 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster, mj
Am 5/1/2012 19:10, schrieb Zbigniew Jędrzejewski-Szmek:
> +test_expect_success 'prepare binary file' '
> + git commit -m rezrov &&
> + dd if=/dev/zero of=binbin bs=1024 count=1 &&
> + git add binbin &&
> + git commit -m binbin
> +'
Please squash in this fixup; we do not have /dev/zero on Windows.
diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
index 693bfc4..7a3e1f9 100755
--- a/t/t4006-diff-mode.sh
+++ b/t/t4006-diff-mode.sh
@@ -27,7 +27,7 @@ test_expect_success 'chmod' '
test_expect_success 'prepare binary file' '
git commit -m rezrov &&
- dd if=/dev/zero of=binbin bs=1024 count=1 &&
+ printf "\00\01\02\03\04\05\06" >binbin &&
git add binbin &&
git commit -m binbin
'
--
1.7.10.1.1568.gede6096
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] report chmod'ed binary files the same as text files
2012-05-01 17:10 [PATCH 0/4] report chmod'ed binary files the same as text files Zbigniew Jędrzejewski-Szmek
` (3 preceding siblings ...)
2012-05-01 17:10 ` [PATCH 4/4] diff --stat: do not run diff on indentical files Zbigniew Jędrzejewski-Szmek
@ 2012-05-03 11:45 ` Martin Mares
4 siblings, 0 replies; 11+ messages in thread
From: Martin Mares @ 2012-05-03 11:45 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster
Hi!
> This patch series fixes a small discrepancy between the way that
> text files and binary files are treated. Reported by Martin Mareš in [1].
> Firt patch is cleanup, second describes current behaviour, third does
> the change, and fourth is a bonus micro-opt.
Thanks for the fix!
Have a nice fortnight
--
Martin `MJ' Mares <mj@ucw.cz> http://mj.ucw.cz/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
This mail doesn't contain viruses, because it wasn't sent from MS Windows. Checked by eyes.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-05-03 11:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-01 17:10 [PATCH 0/4] report chmod'ed binary files the same as text files Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 1/4] test: modernize style of t4006 Zbigniew Jędrzejewski-Szmek
2012-05-01 18:00 ` Junio C Hamano
2012-05-01 19:55 ` Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 2/4] tests: check --[short]stat output after chmod Zbigniew Jędrzejewski-Szmek
2012-05-02 7:36 ` Johannes Sixt
2012-05-01 17:10 ` [PATCH 3/4] diff --stat: report chmoded binary files like text files Zbigniew Jędrzejewski-Szmek
2012-05-01 18:27 ` Junio C Hamano
2012-05-01 19:39 ` Zbigniew Jędrzejewski-Szmek
2012-05-01 17:10 ` [PATCH 4/4] diff --stat: do not run diff on indentical files Zbigniew Jędrzejewski-Szmek
2012-05-03 11:45 ` [PATCH 0/4] report chmod'ed binary files the same as text files Martin Mares
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).