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

8 messages from 2012-06-28 to 2012-07-21. Participants: Tim Henigan, Junio C Hamano, David Aguilar.
Thread: https://gitlist.dev/t/30925

## Tim Henigan, 2012-06-28 19:39

Subject: [PATCH 1/2] difftool: only copy back files modified during directory diff
Message-ID: <1340912395-16990-1-git-send-email-tim.henigan@gmail.com>
URL: https://gitlist.dev/e/1340912395-16990-1-git-send-email-tim.henigan%40gmail.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>
---

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

```

## Tim Henigan, 2012-06-28 19:39

Subject: [PATCH 2/2] difftool: handle uninitialized variable on empty diff
Message-ID: <1340912395-16990-2-git-send-email-tim.henigan@gmail.com>
URL: https://gitlist.dev/e/1340912395-16990-2-git-send-email-tim.henigan%40gmail.com
In-Reply-To: <1340912395-16990-1-git-send-email-tim.henigan@gmail.com>

```
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

```

## Junio C Hamano, 2012-06-28 19:51

Subject: Re: [PATCH 1/2] difftool: only copy back files modified during directory diff
Message-ID: <7vbok35i5s.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vbok35i5s.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1340912395-16990-1-git-send-email-tim.henigan@gmail.com>

```
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)) {

```

## Junio C Hamano, 2012-06-28 20:00

Subject: Re: [PATCH 2/2] difftool: handle uninitialized variable on empty diff
Message-ID: <7v7gur5hqv.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v7gur5hqv.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1340912395-16990-2-git-send-email-tim.henigan@gmail.com>

```
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).

```

## David Aguilar, 2012-07-19 08:11

Subject: Re: [PATCH 1/2] difftool: only copy back files modified during directory diff
Message-ID: <CAJDDKr4Q+nQapO0aAQmmpj_guR=_tXwareWsn020BckZR5V+uA@mail.gmail.com>
URL: https://gitlist.dev/e/CAJDDKr4Q%2BnQapO0aAQmmpj_guR%3D_tXwareWsn020BckZR5V%2BuA%40mail.gmail.com
In-Reply-To: <7vbok35i5s.fsf@alter.siamese.dyndns.org>

```
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

```

## David Aguilar, 2012-07-19 08:27

Subject: [PATCH v2] difftool: only copy back files modified during directory diff
Message-ID: <1342686429-87847-1-git-send-email-davvid@gmail.com>
URL: https://gitlist.dev/e/1342686429-87847-1-git-send-email-davvid%40gmail.com
In-Reply-To: <CAJDDKr4Q+nQapO0aAQmmpj_guR=_tXwareWsn020BckZR5V+uA@mail.gmail.com>

```
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

```

## Junio C Hamano, 2012-07-19 17:34

Subject: Re: [PATCH v2] difftool: only copy back files modified during directory diff
Message-ID: <7v394nveni.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v394nveni.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1342686429-87847-1-git-send-email-davvid@gmail.com>

```
David Aguilar <davvid@gmail.com> writes:

> Perhaps something like this...

Yeah, like that ;-).

```

## David Aguilar, 2012-07-21 05:10

Subject: Re: [PATCH v2] difftool: only copy back files modified during directory diff
Message-ID: <CAJDDKr7103_S+=THQVLHHXFpiSPjLpDVm8j7Kb-nUh4G=bZa-Q@mail.gmail.com>
URL: https://gitlist.dev/e/CAJDDKr7103_S%2B%3DTHQVLHHXFpiSPjLpDVm8j7Kb-nUh4G%3DbZa-Q%40mail.gmail.com
In-Reply-To: <7v394nveni.fsf@alter.siamese.dyndns.org>

```
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

```