Re: [PATCH 2/5] difftool: don't overwrite modified files

2013-03-29 Thread Junio C Hamano
John Keeping j...@keeping.me.uk writes:

 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.

Thanks.

I should drop the t7800-modernize topic and queue this under a
better name.  Perhaps difftool-no-overwrite-on-copyback, or
something.

 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

 ---
 Changes since v2:
 - Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails

  git-difftool.perl   | 73 
 +++--
  t/t7800-difftool.sh | 30 ++
  2 files changed, 89 insertions(+), 14 deletions(-)

 diff --git a/git-difftool.perl b/git-difftool.perl
 index 663640d..844f619 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 $wt_sha1\t$dst_path\0;
   } else {
   $rindex .= $rmode $rsha1\t$dst_path\0;
   }
 @@ -218,6 +254,12 @@ EOF
   $rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
   exit_cleanup($tmpdir, $rc) if $rc != 0;
  
 + $ENV{GIT_INDEX_FILE} = $tmpdir/wtindex;
 + ($inpipe, $ctx) =
 + $repo-command_input_pipe(qw(update-index --info-only -z 
 --index-info));
 + print($inpipe $wtindex);
 + $repo-command_close_pipe($inpipe, $ctx);
 +
   # If $GIT_DIR was explicitly set just for the update/checkout
   # commands, then it should be unset before continuing.
   delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
 @@ -390,19 +432,22 @@ sub dir_diff
   # should be copied back to the working tree.
   # Do not copy back files when symlinks are used and the
   # external tool did not replace the original link with a file.
 + my %wt_modified = 

Re: [PATCH 2/5] difftool: don't overwrite modified files

2013-03-29 Thread John Keeping
On Fri, Mar 29, 2013 at 01:20:50PM -0700, Junio C Hamano wrote:
 John Keeping j...@keeping.me.uk writes:
 
  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.
 
 Thanks.
 
 I should drop the t7800-modernize topic and queue this under a
 better name.  Perhaps difftool-no-overwrite-on-copyback, or
 something.
 
  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
 
  ---
  Changes since v2:
  - Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails
 
   git-difftool.perl   | 73 
  +++--
   t/t7800-difftool.sh | 30 ++
   2 files changed, 89 insertions(+), 14 deletions(-)
 
  diff --git a/git-difftool.perl b/git-difftool.perl
  index 663640d..844f619 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 $wt_sha1\t$dst_path\0;
  } else {
  $rindex .= $rmode $rsha1\t$dst_path\0;
  }
  @@ -218,6 +254,12 @@ EOF
  $rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
  exit_cleanup($tmpdir, $rc) if $rc != 0;
   
  +   $ENV{GIT_INDEX_FILE} = $tmpdir/wtindex;
  +   ($inpipe, $ctx) =
  +   $repo-command_input_pipe(qw(update-index --info-only -z 
  --index-info));
  +   print($inpipe $wtindex);
  +   $repo-command_close_pipe($inpipe, $ctx);
  +
  # If $GIT_DIR was explicitly set just for the update/checkout
  # commands, then it should be unset before continuing.
  delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
  @@ -390,19 +432,22 @@ sub dir_diff
  # should be copied back to the working tree.
  # Do not copy back files when symlinks are used and the
  # external tool