threads / patch / 30925

patch, 2 partsdifftool: only copy back files modified during directory diff

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

## tl;dr

8 messages between Jun 28, 2012 and Jul 21, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Tim Henigan· Jun 28, 2012, 19:39 UTC · lore

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(-)
Show changes to git-difftool.perl +5 −2
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
Tim Henigan· Jun 28, 2012, 19:39 UTC · re: Tim Henigan · lore

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

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(-)
Show changes to git-difftool.perl +1 −1
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
Junio C Hamano· Jun 28, 2012, 20:00 UTC · re: Tim Henigan · lore

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

Tim Henigan <tim.henigan@gmail.com> writes:
Show 18 quoted lines
> 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).

Junio C Hamano· Jun 28, 2012, 19:51 UTC · re: Tim Henigan · lore

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

Tim Henigan <tim.henigan@gmail.com> writes:
Show 33 quoted lines
> 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?

Show 6 quoted lines
> +			copy("$b/$file", "$workdir/$file") or die $!;
> +			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
> +		}
>  	}
>  } else {
>  	if (defined($prompt)) {
David Aguilar· Jul 19, 2012, 08:11 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 28, 2012 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 46 quoted lines
> 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
David Aguilar· Jul 19, 2012, 08:27 UTC · re: David Aguilar · lore

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

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(-)
Show changes to git-difftool.perl +5 −2
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
David Aguilar· Jul 21, 2012, 05:10 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 19, 2012 at 10:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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

← back to recent threads