[PATCH 1/2] difftool: only copy back files modified during directory diff

[PATCH 1/2] difftool: only copy back files modified during directory diff

[Tim Henigan]

When 'difftool --dir-diff' is used to compare working tree files,
it always copies files from the tmp dir back to the working tree
when the diff tool is closed, even if the files were not modified
by the diff tool.

This causes the file timestamp to change. Files should only be
copied from the tmp dir back to the working copy if they were
actually modified.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This patch must be applied after commit 304970d on next (diff-no-index:
exit(1) if 'diff --quiet <repo file> <external file>' finds changes).
because it relies on 'git diff --quiet' to compare files outside the
repository.


 git-difftool.perl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index ae1e052..679a56d 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -336,8 +336,11 @@ if (defined($dirdiff)) {
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree
 	for my $file (@working_tree) {
-		copy("$b/$file", "$workdir/$file") or die $!;
-		chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
+		if ((-e "$b/$file") &&
+		    (system('git', 'diff', '--quiet', "$b/$file", "$workdir/$file") != 0)) {
+			copy("$b/$file", "$workdir/$file") or die $!;
+			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
+		}
 	}
 } else {
 	if (defined($prompt)) {
-- 
1.7.11.1.134.gda62046

[PATCH 2/2] difftool: handle uninitialized variable on empty diff

[Tim Henigan]

When 'difftool --dir-diff' finds no changes, it results in an
uninitialized variable warning.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 679a56d..c94557d 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -117,7 +117,7 @@ sub setup_dir_diff
 	# by Git->repository->command*.
 	my $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);
 	my $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);
-	exit(0) if (length($diffrtn) == 0);
+	exit(0) if ((not defined($diffrtn)) or (length($diffrtn) == 0));
 
 	# Setup temp directories
 	my $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);
-- 
1.7.11.1.134.gda62046

Re: [PATCH 2/2] difftool: handle uninitialized variable on empty diff

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> When 'difftool --dir-diff' finds no changes, it results in an
> uninitialized variable warning.
>
> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>  git-difftool.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 679a56d..c94557d 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -117,7 +117,7 @@ sub setup_dir_diff
>  	# by Git->repository->command*.
>  	my $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);
>  	my $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);
> -	exit(0) if (length($diffrtn) == 0);
> +	exit(0) if ((not defined($diffrtn)) or (length($diffrtn) == 0));

Wouldn't it be far more readable to say something like

        if (!$diffrtn) {
        	exit(0);
	}

instead, as "diff --raw" output, when not empty, cannot be a single
"0"?

By the way, I suspect that the use of command_oneline() here is
wrong.  It uses '-z' so that it can handle whitespace characters in
pathnames sensibly (in other words, the output does not use LF as a
record separator), so pathnames with LF in them will be output
literally.  Taking only the first line of the "diff -z" output would
mean that you will stop when you see the first pathname with LF in
it.

This is not a new problem with this patch, but came from your
7e0abce (difftool: teach difftool to handle directory diffs,
2012-04-23).

Re: [PATCH 1/2] difftool: only copy back files modified during directory diff

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> When 'difftool --dir-diff' is used to compare working tree files,
> it always copies files from the tmp dir back to the working tree
> when the diff tool is closed, even if the files were not modified
> by the diff tool.
>
> This causes the file timestamp to change. Files should only be
> copied from the tmp dir back to the working copy if they were
> actually modified.
>
> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>
> This patch must be applied after commit 304970d on next (diff-no-index:
> exit(1) if 'diff --quiet <repo file> <external file>' finds changes).
> because it relies on 'git diff --quiet' to compare files outside the
> repository.
>
>
>  git-difftool.perl | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index ae1e052..679a56d 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -336,8 +336,11 @@ if (defined($dirdiff)) {
>  	# files were modified during the diff, then the changes
>  	# should be copied back to the working tree
>  	for my $file (@working_tree) {
> -		copy("$b/$file", "$workdir/$file") or die $!;
> -		chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
> +		if ((-e "$b/$file") &&
> +		    (system('git', 'diff', '--quiet', "$b/$file", "$workdir/$file") != 0)) {

Why waste cycles to spawn "git diff" when you only want to find if
they are byte-for-byte identical *and* when you are importing many
perl modules from File::* already into the script?

> +			copy("$b/$file", "$workdir/$file") or die $!;
> +			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
> +		}
>  	}
>  } else {
>  	if (defined($prompt)) {

Re: [PATCH 1/2] difftool: only copy back files modified during directory diff

[David Aguilar]

On Thu, Jun 28, 2012 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> When 'difftool --dir-diff' is used to compare working tree files,
>> it always copies files from the tmp dir back to the working tree
>> when the diff tool is closed, even if the files were not modified
>> by the diff tool.
>>
>> This causes the file timestamp to change. Files should only be
>> copied from the tmp dir back to the working copy if they were
>> actually modified.
>>
>> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
>> ---
>>
>> This patch must be applied after commit 304970d on next (diff-no-index:
>> exit(1) if 'diff --quiet <repo file> <external file>' finds changes).
>> because it relies on 'git diff --quiet' to compare files outside the
>> repository.
>>
>>
>>  git-difftool.perl | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-difftool.perl b/git-difftool.perl
>> index ae1e052..679a56d 100755
>> --- a/git-difftool.perl
>> +++ b/git-difftool.perl
>> @@ -336,8 +336,11 @@ if (defined($dirdiff)) {
>>       # files were modified during the diff, then the changes
>>       # should be copied back to the working tree
>>       for my $file (@working_tree) {
>> -             copy("$b/$file", "$workdir/$file") or die $!;
>> -             chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
>> +             if ((-e "$b/$file") &&
>> +                 (system('git', 'diff', '--quiet', "$b/$file", "$workdir/$file") != 0)) {
>
> Why waste cycles to spawn "git diff" when you only want to find if
> they are byte-for-byte identical *and* when you are importing many
> perl modules from File::* already into the script?
>
>> +                     copy("$b/$file", "$workdir/$file") or die $!;
>> +                     chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
>> +             }
>>       }
>>  } else {
>>       if (defined($prompt)) {


Hey Tim,

I think what Junio is alluding to here is that we should probably use
File::Compare[1] here instead of shelling out to git.  I hope that
helps.

Let me know if you need any help getting this patch into shape.

Thanks,
-- 
David

[1] http://perldoc.perl.org/File/Compare.html

[PATCH v2] difftool: only copy back files modified during directory diff

[David Aguilar]

From: Tim Henigan <tim.henigan@gmail.com>

When 'difftool --dir-diff' is used to compare working tree files,
it always copies files from the tmp dir back to the working tree
when the diff tool is closed, even if the files were not modified
by the diff tool.

This causes the file timestamp to change. Files should only be
copied from the tmp dir back to the working copy if they were
actually modified.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---

Perhaps something like this...

 git-difftool.perl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index ae1e052..c079854 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -15,6 +15,7 @@ use strict;
 use warnings;
 use File::Basename qw(dirname);
 use File::Copy;
+use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath);
@@ -336,8 +337,10 @@ if (defined($dirdiff)) {
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree
 	for my $file (@working_tree) {
-		copy("$b/$file", "$workdir/$file") or die $!;
-		chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
+		if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {
+			copy("$b/$file", "$workdir/$file") or die $!;
+			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
+		}
 	}
 } else {
 	if (defined($prompt)) {
-- 
1.7.11.2.250.g00b4b9a

Re: [PATCH v2] difftool: only copy back files modified during directory diff

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Perhaps something like this...

Yeah, like that ;-).

Re: [PATCH v2] difftool: only copy back files modified during directory diff

[David Aguilar]

On Thu, Jul 19, 2012 at 10:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Perhaps something like this...
>
> Yeah, like that ;-).

Hmm.. this one was potentially data-losing.  Sorry for not catching
that in 7e0abcec103b3649943b236881cf88e8fd6cf3a4.

$ git tag --contains 7e0abcec103b3649943b236881cf88e8fd6cf3a4
v1.7.11
v1.7.11.1
v1.7.11.2

maint, please?

I don't like complexity either, but adding a --symlinks option (and
making it the default) to create symlinks instead of copies really
does seem like the way to go long-term.  Then we can avoid the whole
copy-back business when in this mode.

I'll start exploring this unless you beat me to it, Tim ;-)
-- 
David