Re: [PATCH v2] difftool: don't overwrite modified files
Am 3/25/2013 22:44, schrieb John Keeping:
After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree. This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.
Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not. If both files
have been modified, print a warning and exit with an error.
Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out. These are precisely the files which are not in the
existing indices.
Signed-off-by: John Keeping j...@keeping.me.uk
---
On Mon, Mar 25, 2013 at 10:42:19AM +, John Keeping wrote:
On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an edit collision needs to be checked for.
That's only the case for files that are not copied from the working
tree, so the temporary index doesn't contain the files that are of
interest here.
You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.
I like the idea of creating an index from the working tree files and
using it here. If we create a starting state index for these files,
we should be able to run git-diff-files against both the working tree
and the temporary tree at this point and compare the output.
Here's an attempt at taking this approach, built on
jk/difftool-dir-diff-edit-fix.
git-difftool.perl | 73
+++--
t/t7800-difftool.sh | 26 +++
2 files changed, 85 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..d10f7d2 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
use 5.008;
use strict;
use warnings;
+use Error qw(:try);
use File::Basename qw(dirname);
use File::Copy;
-use File::Compare;
use File::Find;
use File::stat;
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 eq $null_sha1) {
- return 1;
- } elsif (not $symlinks) {
+ if ($sha1 ne $null_sha1 and not $symlinks) {
return 0;
}
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
- return $sha1 eq $wt_sha1;
+ my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+ return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+ my ($repo_path, $index, $worktree) = @_;
+ $ENV{GIT_INDEX_FILE} = $index;
+ $ENV{GIT_WORK_TREE} = $worktree;
+ my $must_unset_git_dir = 0;
+ if (not defined($ENV{GIT_DIR})) {
+ $must_unset_git_dir = 1;
+ $ENV{GIT_DIR} = $repo_path;
+ }
+
+ my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+ my @gitargs = qw/diff-files --name-only -z/;
+ try {
+ Git::command_oneline(@refreshargs);
+ } catch Git::Error::Command with {};
+
+ my $line = Git::command_oneline(@gitargs);
+ my @files;
+ if (defined $line) {
+ @files = split('\0', $line);
+ } else {
+ @files = ();
+ }
+
+ delete($ENV{GIT_INDEX_FILE});
+ delete($ENV{GIT_WORK_TREE});
+ delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+ return map { $_ = 1 } @files;
}
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
my $null_sha1 = '0' x 40;
my $lindex = '';
my $rindex = '';
+ my $wtindex = '';
my %submodule;
my %symlink;
my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
}
if ($rmode ne $null_mode) {
- if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
$symlinks)) {
- push(@working_tree, $dst_path);
+ my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+ $dst_path, $rsha1,
+ $symlinks);
+ if ($use) {
+ push @working_tree, $dst_path;
+ $wtindex .= $rmode
Re: [PATCH v2] difftool: don't overwrite modified files
Forgot to mention: The patch passes t7800 on Windows. Thanks, -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool: don't overwrite modified files
On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:
Am 3/25/2013 22:44, schrieb John Keeping:
After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree. This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.
Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not. If both files
have been modified, print a warning and exit with an error.
Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out. These are precisely the files which are not in the
existing indices.
Signed-off-by: John Keeping j...@keeping.me.uk
---
On Mon, Mar 25, 2013 at 10:42:19AM +, John Keeping wrote:
On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an edit collision needs to be checked for.
That's only the case for files that are not copied from the working
tree, so the temporary index doesn't contain the files that are of
interest here.
You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.
I like the idea of creating an index from the working tree files and
using it here. If we create a starting state index for these files,
we should be able to run git-diff-files against both the working tree
and the temporary tree at this point and compare the output.
Here's an attempt at taking this approach, built on
jk/difftool-dir-diff-edit-fix.
git-difftool.perl | 73
+++--
t/t7800-difftool.sh | 26 +++
2 files changed, 85 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..d10f7d2 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
use 5.008;
use strict;
use warnings;
+use Error qw(:try);
use File::Basename qw(dirname);
use File::Copy;
-use File::Compare;
use File::Find;
use File::stat;
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 eq $null_sha1) {
- return 1;
- } elsif (not $symlinks) {
+ if ($sha1 ne $null_sha1 and not $symlinks) {
return 0;
}
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
- return $sha1 eq $wt_sha1;
+ my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+ return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+ my ($repo_path, $index, $worktree) = @_;
+ $ENV{GIT_INDEX_FILE} = $index;
+ $ENV{GIT_WORK_TREE} = $worktree;
+ my $must_unset_git_dir = 0;
+ if (not defined($ENV{GIT_DIR})) {
+ $must_unset_git_dir = 1;
+ $ENV{GIT_DIR} = $repo_path;
+ }
+
+ my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+ my @gitargs = qw/diff-files --name-only -z/;
+ try {
+ Git::command_oneline(@refreshargs);
+ } catch Git::Error::Command with {};
+
+ my $line = Git::command_oneline(@gitargs);
+ my @files;
+ if (defined $line) {
+ @files = split('\0', $line);
+ } else {
+ @files = ();
+ }
+
+ delete($ENV{GIT_INDEX_FILE});
+ delete($ENV{GIT_WORK_TREE});
+ delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+ return map { $_ = 1 } @files;
}
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
my $null_sha1 = '0' x 40;
my $lindex = '';
my $rindex = '';
+ my $wtindex = '';
my %submodule;
my %symlink;
my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
}
if ($rmode ne $null_mode) {
- if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
$symlinks)) {
- push(@working_tree, $dst_path);
+ my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+ $dst_path, $rsha1,
+ $symlinks);
+ if ($use) {
+
Re: [PATCH v2] difftool: don't overwrite modified files
Am 3/26/2013 10:31, schrieb John Keeping: On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote: One question though: Do I understand correctly that the temporary directories are leaked in the case of an edit conflict? If so, is it worth a warning for the user to clean up the garbage? Do you mean for normal users or for those running the tests? In normal usage we do print a warning - it's in the existing code, triggered by setting $error = 1 - you can see that if you run the tests with -v. I meant for normal users. I see the error now. Thanks. The last test does result in /tmp filling up with temporary directories though, it would be good if the test could clean up after itself. The best I can come up with is adding something like this immediately after running difftool but I'm not entirely happy with the .. in the argument to rm: test_when_finished rm -rf $(cat tmpdir)/.. Wrap the test in ( TMPDIR=$TRASH_DIRECTORY export TMPDIR ... ) It works for me. -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool: don't overwrite modified files
On Tue, Mar 26, 2013 at 10:53:48AM +0100, Johannes Sixt wrote: Am 3/26/2013 10:31, schrieb John Keeping: On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote: The last test does result in /tmp filling up with temporary directories though, it would be good if the test could clean up after itself. The best I can come up with is adding something like this immediately after running difftool but I'm not entirely happy with the .. in the argument to rm: test_when_finished rm -rf $(cat tmpdir)/.. Wrap the test in ( TMPDIR=$TRASH_DIRECTORY export TMPDIR ... ) It works for me. Nice. I've reviewed File::Spec and it looks like that TMPDIR takes priority on every operating system except VMS, and I don't think we care about that. Unless Junio says otherwise, I'll hold off sending this until difftool calms down a bit to avoid too many conflicted merges. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool: don't overwrite modified files
On Mon, Mar 25, 2013 at 5:44 PM, John Keeping j...@keeping.me.uk wrote: Instead of copying unconditionally when the files differ, create and index from the working tree files and only copy the temporary file back if it was modified and the working tree file was not. If both files have been modified, print a warning and exit with an error. When there's a conflict, does difftool save both conflicting files? Or only the working tree copy? I think it should preserve both copies on disk. -- Matt McClure http://www.matthewlmcclure.com http://www.mapmyfitness.com/profile/matthewlmcclure -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool: don't overwrite modified files
On Tue, Mar 26, 2013 at 04:52:02PM -0400, Matt McClure wrote: On Mon, Mar 25, 2013 at 5:44 PM, John Keeping j...@keeping.me.uk wrote: Instead of copying unconditionally when the files differ, create and index from the working tree files and only copy the temporary file back if it was modified and the working tree file was not. If both files have been modified, print a warning and exit with an error. When there's a conflict, does difftool save both conflicting files? Or only the working tree copy? I think it should preserve both copies on disk. It preserves both copies - the clean the temporary directory step is just skipped. This isn't ideal since the temporary copy will be under a temporary directory somewhere but is better than the current behaviour. It might be nice to move the temporary file back with an extension so that the files are at least near each other but I don't think that's needed in the first version of this change. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
