From: Johannes Sixt <j.sixt@viscovery.net>
To: John Keeping <john@keeping.me.uk>
Cc: Matt McClure <matthewlmcclure@gmail.com>,
Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org, David Aguilar <davvid@gmail.com>,
Sitaram Chamarty <sitaramc@gmail.com>,
Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks
Date: Mon, 25 Mar 2013 08:41:59 +0100 [thread overview]
Message-ID: <514FFFC7.3090004@viscovery.net> (raw)
In-Reply-To: <20130324151557.GB2286@serenity.lan>
Am 3/24/2013 16:15, schrieb John Keeping:
> Subject: [PATCH] difftool: don't overwrite modified files
>
> 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, store the
> initial hash of the working tree file 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.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> git-difftool.perl | 35 +++++++++++++++++++++--------------
> t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index c433e86..be82b5a 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -15,7 +15,6 @@ 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 rmtree);
> @@ -123,7 +122,7 @@ sub setup_dir_diff
> my $rindex = '';
> my %submodule;
> my %symlink;
> - my @working_tree = ();
> + my %working_tree;
> my @rawdiff = split('\0', $diffrtn);
>
> my $i = 0;
> @@ -175,7 +174,9 @@ EOF
>
> if ($rmode ne $null_mode) {
> if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> - push(@working_tree, $dst_path);
> + $working_tree{$dst_path} =
> + $repo->command_oneline('hash-object',
> + "$workdir/$dst_path");
> } else {
> $rindex .= "$rmode $rsha1\t$dst_path\0";
> }
> @@ -227,7 +228,7 @@ EOF
> # not part of the index. Remove any trailing slash from $workdir
> # before starting to avoid double slashes in symlink targets.
> $workdir =~ s|/$||;
> - for my $file (@working_tree) {
> + for my $file (keys %working_tree) {
> my $dir = dirname($file);
> unless (-d "$rdir/$dir") {
> mkpath("$rdir/$dir") or
> @@ -278,7 +279,7 @@ EOF
> exit_cleanup($tmpdir, 1) if not $ok;
> }
>
> - return ($ldir, $rdir, $tmpdir, @working_tree);
> + return ($ldir, $rdir, $tmpdir, %working_tree);
> }
>
> sub write_to_file
> @@ -376,7 +377,7 @@ sub dir_diff
> my $error = 0;
> my $repo = Git->repository();
> my $workdir = find_worktree($repo);
> - my ($a, $b, $tmpdir, @worktree) =
> + my ($a, $b, $tmpdir, %worktree) =
> setup_dir_diff($repo, $workdir, $symlinks);
>
> if (defined($extcmd)) {
> @@ -390,19 +391,25 @@ 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.
> - for my $file (@worktree) {
> + for my $file (keys %worktree) {
> next if $symlinks && -l "$b/$file";
> next if ! -f "$b/$file";
>
> - my $diff = compare("$b/$file", "$workdir/$file");
> - if ($diff == 0) {
> - next;
> - } elsif ($diff == -1) {
> - my $errmsg = "warning: Could not compare ";
> - $errmsg += "'$b/$file' with '$workdir/$file'\n";
> + my $wt_hash = $repo->command_oneline('hash-object',
> + "$workdir/$file");
> + my $tmp_hash = $repo->command_oneline('hash-object',
> + "$b/$file");
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.
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.
> + my $wt_modified = $wt_hash ne $worktree{$file};
> + my $tmp_modified = $tmp_hash ne $worktree{$file};
> +
> + if ($wt_modified and $tmp_modified) {
> + my $errmsg = "warning: Both files modified: ";
> + $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
> + $errmsg .= "warning: Working tree file has been left.\n";
> + $errmsg .= "warning:\n";
> warn $errmsg;
> $error = 1;
> - } elsif ($diff == 1) {
> + } elsif ($tmp_modified) {
> my $mode = stat("$b/$file")->mode;
> copy("$b/$file", "$workdir/$file") or
> exit_cleanup($tmpdir, 1);
-- Hannes
next prev parent reply other threads:[~2013-03-25 7:42 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-21 4:03 [PATCH v3 1/4] difftool: silence uninitialized variable warning David Aguilar
2013-02-21 4:03 ` [PATCH 2/4] t7800: update copyright notice David Aguilar
2013-02-21 4:03 ` [PATCH v3 3/4] t7800: modernize tests David Aguilar
2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar
2013-02-21 4:55 ` Junio C Hamano
2013-02-21 5:00 ` Junio C Hamano
2013-02-21 23:31 ` David Aguilar
2013-03-20 9:48 ` [PATCH v3 3/4] t7800: modernize tests Johannes Sixt
2013-03-20 22:59 ` David Aguilar
2013-03-21 7:41 ` Johannes Sixt
2013-03-22 7:13 ` Johannes Sixt
2013-03-22 10:00 ` John Keeping
2013-03-22 11:14 ` Johannes Sixt
2013-03-22 11:53 ` John Keeping
2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping
2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping
2013-03-22 22:32 ` Johannes Sixt
2013-03-22 22:45 ` Junio C Hamano
2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping
2013-03-22 22:27 ` Johannes Sixt
2013-03-22 22:53 ` Junio C Hamano
2013-03-22 23:05 ` John Keeping
2013-03-23 3:24 ` David Aguilar
2013-03-22 19:36 ` [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping
2013-03-22 21:05 ` [PATCH 3/3 v2] " John Keeping
2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping
2013-03-23 13:31 ` [PATCH v2 1/3] t7800: don't hide grep output John Keeping
2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping
2013-03-24 5:19 ` Junio C Hamano
2013-03-24 12:36 ` John Keeping
2013-03-24 13:31 ` Matt McClure
2013-03-24 15:15 ` John Keeping
2013-03-25 7:41 ` Johannes Sixt [this message]
2013-03-25 10:42 ` John Keeping
2013-03-25 21:44 ` [PATCH v2] difftool: don't overwrite modified files John Keeping
2013-03-26 8:38 ` Johannes Sixt
2013-03-26 8:47 ` Johannes Sixt
2013-03-26 9:31 ` John Keeping
2013-03-26 9:53 ` Johannes Sixt
2013-03-26 19:34 ` John Keeping
2013-03-26 20:52 ` Matt McClure
2013-03-26 21:01 ` John Keeping
2013-03-25 16:15 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks Junio C Hamano
2013-03-24 21:29 ` David Aguilar
2013-03-25 10:57 ` John Keeping
2013-03-25 14:54 ` Junio C Hamano
2013-03-24 13:24 ` Matt McClure
2013-03-24 6:20 ` Eric Sunshine
2013-03-23 13:31 ` [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping
2013-03-25 7:26 ` Johannes Sixt
2013-03-25 10:35 ` John Keeping
2013-03-25 10:59 ` Johannes Sixt
2013-03-25 11:02 ` John Keeping
2013-03-25 21:50 ` Junio C Hamano
2013-03-26 9:22 ` John Keeping
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=514FFFC7.3090004@viscovery.net \
--to=j.sixt@viscovery.net \
--cc=davvid@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=john@keeping.me.uk \
--cc=jrnieder@gmail.com \
--cc=matthewlmcclure@gmail.com \
--cc=sitaramc@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).