{"thread":{"id":"30925","subject":"[PATCH 1/2] difftool: only copy back files modified during directory diff","startedAt":"2012-06-28T19:39:54Z","lastAt":"2012-07-21T05:10:53Z","messageCount":8,"participants":["Tim Henigan","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"194412","messageId":"1340912395-16990-1-git-send-email-tim.henigan@gmail.com","threadId":"30925","inReplyTo":null,"subject":"[PATCH 1/2] difftool: only copy back files modified during directory diff","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-28T19:39:54Z","receivedAt":"2012-06-28T19:39:54Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When 'difftool --dir-diff' is used to compare working tree files,\nit always copies files from the tmp dir back to the working tree\nwhen the diff tool is closed, even if the files were not modified\nby the diff tool.\n\nThis causes the file timestamp to change. Files should only be\ncopied from the tmp dir back to the working copy if they were\nactually modified.\n\nSigned-off-by: Tim Henigan \n---\n\nThis patch must be applied after commit 304970d on next (diff-no-index:\nexit(1) if 'diff --quiet ' finds changes).\nbecause it relies on 'git diff --quiet' to compare files outside the\nrepository.\n\n\n git-difftool.perl | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex ae1e052..679a56d 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -336,8 +336,11 @@ if (defined($dirdiff)) {\n \t# files were modified during the diff, then the changes\n \t# should be copied back to the working tree\n \tfor my $file (@working_tree) {\n-\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n-\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n+\t\tif ((-e \"$b/$file\") &&\n+\t\t (system('git', 'diff', '--quiet', \"$b/$file\", \"$workdir/$file\") != 0)) {\n+\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n+\t\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n+\t\t}\n \t}\n } else {\n \tif (defined($prompt)) {\n-- \n1.7.11.1.134.gda62046\n"},{"id":"194413","messageId":"1340912395-16990-2-git-send-email-tim.henigan@gmail.com","threadId":"30925","inReplyTo":"1340912395-16990-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 2/2] difftool: handle uninitialized variable on empty diff","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-28T19:39:55Z","receivedAt":"2012-06-28T19:39:55Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When 'difftool --dir-diff' finds no changes, it results in an\nuninitialized variable warning.\n\nSigned-off-by: Tim Henigan \n---\n git-difftool.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 679a56d..c94557d 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -117,7 +117,7 @@ sub setup_dir_diff\n \t# by Git->repository->command*.\n \tmy $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);\n \tmy $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n-\texit(0) if (length($diffrtn) == 0);\n+\texit(0) if ((not defined($diffrtn)) or (length($diffrtn) == 0));\n \n \t# Setup temp directories\n \tmy $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);\n-- \n1.7.11.1.134.gda62046\n"},{"id":"194414","messageId":"7vbok35i5s.fsf@alter.siamese.dyndns.org","threadId":"30925","inReplyTo":"1340912395-16990-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 1/2] difftool: only copy back files modified during directory diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-28T19:51:43Z","receivedAt":"2012-06-28T19:51:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> When 'difftool --dir-diff' is used to compare working tree files,\n> it always copies files from the tmp dir back to the working tree\n> when the diff tool is closed, even if the files were not modified\n> by the diff tool.\n>\n> This causes the file timestamp to change. Files should only be\n> copied from the tmp dir back to the working copy if they were\n> actually modified.\n>\n> Signed-off-by: Tim Henigan \n> ---\n>\n> This patch must be applied after commit 304970d on next (diff-no-index:\n> exit(1) if 'diff --quiet ' finds changes).\n> because it relies on 'git diff --quiet' to compare files outside the\n> repository.\n>\n>\n> git-difftool.perl | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index ae1e052..679a56d 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -336,8 +336,11 @@ if (defined($dirdiff)) {\n> \t# files were modified during the diff, then the changes\n> \t# should be copied back to the working tree\n> \tfor my $file (@working_tree) {\n> -\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n> -\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n> +\t\tif ((-e \"$b/$file\") &&\n> +\t\t (system('git', 'diff', '--quiet', \"$b/$file\", \"$workdir/$file\") != 0)) {\n\nWhy waste cycles to spawn \"git diff\" when you only want to find if\nthey are byte-for-byte identical *and* when you are importing many\nperl modules from File::* already into the script?\n\n> +\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n> +\t\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n> +\t\t}\n> \t}\n> } else {\n> \tif (defined($prompt)) {\n"},{"id":"194415","messageId":"7v7gur5hqv.fsf@alter.siamese.dyndns.org","threadId":"30925","inReplyTo":"1340912395-16990-2-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 2/2] difftool: handle uninitialized variable on empty diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-28T20:00:40Z","receivedAt":"2012-06-28T20:00:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> When 'difftool --dir-diff' finds no changes, it results in an\n> uninitialized variable warning.\n>\n> Signed-off-by: Tim Henigan \n> ---\n> git-difftool.perl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 679a56d..c94557d 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -117,7 +117,7 @@ sub setup_dir_diff\n> \t# by Git->repository->command*.\n> \tmy $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);\n> \tmy $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n> -\texit(0) if (length($diffrtn) == 0);\n> +\texit(0) if ((not defined($diffrtn)) or (length($diffrtn) == 0));\n\nWouldn't it be far more readable to say something like\n\n if (!$diffrtn) {\n \texit(0);\n\t}\n\ninstead, as \"diff --raw\" output, when not empty, cannot be a single\n\"0\"?\n\nBy the way, I suspect that the use of command_oneline() here is\nwrong. It uses '-z' so that it can handle whitespace characters in\npathnames sensibly (in other words, the output does not use LF as a\nrecord separator), so pathnames with LF in them will be output\nliterally. Taking only the first line of the \"diff -z\" output would\nmean that you will stop when you see the first pathname with LF in\nit.\n\nThis is not a new problem with this patch, but came from your\n7e0abce (difftool: teach difftool to handle directory diffs,\n2012-04-23).\n"},{"id":"195323","messageId":"CAJDDKr4Q+nQapO0aAQmmpj_guR=_tXwareWsn020BckZR5V+uA@mail.gmail.com","threadId":"30925","inReplyTo":"7vbok35i5s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] difftool: only copy back files modified during directory diff","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-19T08:11:54Z","receivedAt":"2012-07-19T08:11:54Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Jun 28, 2012 at 12:51 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n>> When 'difftool --dir-diff' is used to compare working tree files,\n>> it always copies files from the tmp dir back to the working tree\n>> when the diff tool is closed, even if the files were not modified\n>> by the diff tool.\n>>\n>> This causes the file timestamp to change. Files should only be\n>> copied from the tmp dir back to the working copy if they were\n>> actually modified.\n>>\n>> Signed-off-by: Tim Henigan \n>> ---\n>>\n>> This patch must be applied after commit 304970d on next (diff-no-index:\n>> exit(1) if 'diff --quiet ' finds changes).\n>> because it relies on 'git diff --quiet' to compare files outside the\n>> repository.\n>>\n>>\n>> git-difftool.perl | 7 +++++--\n>> 1 file changed, 5 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index ae1e052..679a56d 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -336,8 +336,11 @@ if (defined($dirdiff)) {\n>> # files were modified during the diff, then the changes\n>> # should be copied back to the working tree\n>> for my $file (@working_tree) {\n>> - copy(\"$b/$file\", \"$workdir/$file\") or die $!;\n>> - chmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n>> + if ((-e \"$b/$file\") &&\n>> + (system('git', 'diff', '--quiet', \"$b/$file\", \"$workdir/$file\") != 0)) {\n>\n> Why waste cycles to spawn \"git diff\" when you only want to find if\n> they are byte-for-byte identical *and* when you are importing many\n> perl modules from File::* already into the script?\n>\n>> + copy(\"$b/$file\", \"$workdir/$file\") or die $!;\n>> + chmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n>> + }\n>> }\n>> } else {\n>> if (defined($prompt)) {\n\n\nHey Tim,\n\nI think what Junio is alluding to here is that we should probably use\nFile::Compare[1] here instead of shelling out to git. I hope that\nhelps.\n\nLet me know if you need any help getting this patch into shape.\n\nThanks,\n-- \nDavid\n\n[1] http://perldoc.perl.org/File/Compare.html\n"},{"id":"195325","messageId":"1342686429-87847-1-git-send-email-davvid@gmail.com","threadId":"30925","inReplyTo":"CAJDDKr4Q+nQapO0aAQmmpj_guR=_tXwareWsn020BckZR5V+uA@mail.gmail.com","subject":"[PATCH v2] difftool: only copy back files modified during directory diff","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-19T08:27:09Z","receivedAt":"2012-07-19T08:27:09Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"From: Tim Henigan \n\nWhen 'difftool --dir-diff' is used to compare working tree files,\nit always copies files from the tmp dir back to the working tree\nwhen the diff tool is closed, even if the files were not modified\nby the diff tool.\n\nThis causes the file timestamp to change. Files should only be\ncopied from the tmp dir back to the working copy if they were\nactually modified.\n\nSigned-off-by: Tim Henigan \nSigned-off-by: David Aguilar \n---\n\nPerhaps something like this...\n\n git-difftool.perl | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex ae1e052..c079854 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -15,6 +15,7 @@ use strict;\n use warnings;\n use File::Basename qw(dirname);\n use File::Copy;\n+use File::Compare;\n use File::Find;\n use File::stat;\n use File::Path qw(mkpath);\n@@ -336,8 +337,10 @@ if (defined($dirdiff)) {\n \t# files were modified during the diff, then the changes\n \t# should be copied back to the working tree\n \tfor my $file (@working_tree) {\n-\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n-\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n+\t\tif (-e \"$b/$file\" && compare(\"$b/$file\", \"$workdir/$file\")) {\n+\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n+\t\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n+\t\t}\n \t}\n } else {\n \tif (defined($prompt)) {\n-- \n1.7.11.2.250.g00b4b9a\n"},{"id":"195341","messageId":"7v394nveni.fsf@alter.siamese.dyndns.org","threadId":"30925","inReplyTo":"1342686429-87847-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2] difftool: only copy back files modified during directory diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-19T17:34:09Z","receivedAt":"2012-07-19T17:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Perhaps something like this...\n\nYeah, like that ;-).\n"},{"id":"195397","messageId":"CAJDDKr7103_S+=THQVLHHXFpiSPjLpDVm8j7Kb-nUh4G=bZa-Q@mail.gmail.com","threadId":"30925","inReplyTo":"7v394nveni.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] difftool: only copy back files modified during directory diff","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-21T05:10:53Z","receivedAt":"2012-07-21T05:10:53Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Jul 19, 2012 at 10:34 AM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> Perhaps something like this...\n>\n> Yeah, like that ;-).\n\nHmm.. this one was potentially data-losing. Sorry for not catching\nthat in 7e0abcec103b3649943b236881cf88e8fd6cf3a4.\n\n$ git tag --contains 7e0abcec103b3649943b236881cf88e8fd6cf3a4\nv1.7.11\nv1.7.11.1\nv1.7.11.2\n\nmaint, please?\n\nI don't like complexity either, but adding a --symlinks option (and\nmaking it the default) to create symlinks instead of copies really\ndoes seem like the way to go long-term. Then we can avoid the whole\ncopy-back business when in this mode.\n\nI'll start exploring this unless you beat me to it, Tim ;-)\n-- \nDavid\n"}]}