Skip to content

Commit 288e6ff

Browse files
gitstergitster
committed
Merge branch 'jk/difftool-no-overwrite-on-copyback'
Try to be careful when difftool backend allows the user to write into the temporary files being shown *and* the user makes changes to the working tree at the same time. One of the changes has to be lost in such a case, but at least tell the user what he did. * jk/difftool-no-overwrite-on-copyback: t7800: run --dir-diff tests with and without symlinks t7800: fix tests when difftool uses --no-symlinks t7800: don't hide grep output difftool: don't overwrite modified files t7800: move '--symlinks' specific test to the end
2 parents f30366b + e01afdb commit 288e6ff

File tree

2 files changed

+139
-51
lines changed

2 files changed

+139
-51
lines changed

‎git-difftool.perl‎

Lines changed: 71 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@
1313
use 5.008;
1414
use strict;
1515
use warnings;
16+
use Error qw(:try);
1617
use File::Basename qw(dirname);
1718
use File::Copy;
18-
use File::Compare;
1919
use File::Find;
2020
use File::stat;
2121
use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
8888
my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
8989
my $null_sha1 = '0' x 40;
9090

91-
if ($sha1 eq $null_sha1) {
92-
return 1;
93-
} elsif (not $symlinks) {
91+
if ($sha1 ne $null_sha1 and not $symlinks) {
9492
return 0;
9593
}
9694

9795
my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
98-
return $sha1 eq $wt_sha1;
96+
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
97+
return ($use, $wt_sha1);
98+
}
99+
100+
sub changed_files
101+
{
102+
my ($repo_path, $index, $worktree) = @_;
103+
$ENV{GIT_INDEX_FILE} = $index;
104+
$ENV{GIT_WORK_TREE} = $worktree;
105+
my $must_unset_git_dir = 0;
106+
if (not defined($ENV{GIT_DIR})) {
107+
$must_unset_git_dir = 1;
108+
$ENV{GIT_DIR} = $repo_path;
109+
}
110+
111+
my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
112+
my @gitargs = qw/diff-files --name-only -z/;
113+
try {
114+
Git::command_oneline(@refreshargs);
115+
} catch Git::Error::Command with {};
116+
117+
my $line = Git::command_oneline(@gitargs);
118+
my @files;
119+
if (defined $line) {
120+
@files = split('\0', $line);
121+
} else {
122+
@files = ();
123+
}
124+
125+
delete($ENV{GIT_INDEX_FILE});
126+
delete($ENV{GIT_WORK_TREE});
127+
delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
128+
129+
return map { $_ => 1 } @files;
99130
}
100131

101132
sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
121152
my $null_sha1 = '0' x 40;
122153
my $lindex = '';
123154
my $rindex = '';
155+
my $wtindex = '';
124156
my %submodule;
125157
my %symlink;
126158
my @working_tree = ();
@@ -174,8 +206,12 @@ sub setup_dir_diff
174206
}
175207

176208
if ($rmode ne $null_mode) {
177-
if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
178-
push(@working_tree, $dst_path);
209+
my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
210+
$dst_path, $rsha1,
211+
$symlinks);
212+
if ($use) {
213+
push @working_tree, $dst_path;
214+
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
179215
} else {
180216
$rindex .= "$rmode $rsha1\t$dst_path\0";
181217
}
@@ -218,6 +254,12 @@ sub setup_dir_diff
218254
$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
219255
exit_cleanup($tmpdir, $rc) if $rc != 0;
220256

257+
$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
258+
($inpipe, $ctx) =
259+
$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
260+
print($inpipe $wtindex);
261+
$repo->command_close_pipe($inpipe, $ctx);
262+
221263
# If $GIT_DIR was explicitly set just for the update/checkout
222264
# commands, then it should be unset before continuing.
223265
delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,34 @@ sub dir_diff
390432
# should be copied back to the working tree.
391433
# Do not copy back files when symlinks are used and the
392434
# external tool did not replace the original link with a file.
435+
#
436+
# These hashes are loaded lazily since they aren't needed
437+
# in the common case of --symlinks and the difftool updating
438+
# files through the symlink.
439+
my %wt_modified;
440+
my %tmp_modified;
441+
my $indices_loaded = 0;
442+
393443
for my $file (@worktree) {
394444
next if $symlinks && -l "$b/$file";
395445
next if ! -f "$b/$file";
396446

397-
my $diff = compare("$b/$file", "$workdir/$file");
398-
if ($diff == 0) {
399-
next;
400-
} elsif ($diff == -1) {
401-
my $errmsg = "warning: Could not compare ";
402-
$errmsg += "'$b/$file' with '$workdir/$file'\n";
447+
if (!$indices_loaded) {
448+
%wt_modified = changed_files($repo->repo_path(),
449+
"$tmpdir/wtindex", "$workdir");
450+
%tmp_modified = changed_files($repo->repo_path(),
451+
"$tmpdir/wtindex", "$b");
452+
$indices_loaded = 1;
453+
}
454+
455+
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
456+
my $errmsg = "warning: Both files modified: ";
457+
$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
458+
$errmsg .= "warning: Working tree file has been left.\n";
459+
$errmsg .= "warning:\n";
403460
warn $errmsg;
404461
$error = 1;
405-
} elsif ($diff == 1) {
462+
} elsif (exists $tmp_modified{$file}) {
406463
my $mode = stat("$b/$file")->mode;
407464
copy("$b/$file", "$workdir/$file") or
408465
exit_cleanup($tmpdir, 1);

‎t/t7800-difftool.sh‎

Lines changed: 68 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,6 @@ prompt_given ()
2323
test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
2424
}
2525

26-
stdin_contains ()
27-
{
28-
grep >/dev/null "$1"
29-
}
30-
31-
stdin_doesnot_contain ()
32-
{
33-
! stdin_contains "$1"
34-
}
35-
3626
# Create a file on master and change it on branch
3727
test_expect_success PERL 'setup' '
3828
echo master >file &&
@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '
296286
test_expect_success PERL 'say no to the first file' '
297287
(echo n && echo) >input &&
298288
git difftool -x cat branch <input >output &&
299-
stdin_contains m2 <output &&
300-
stdin_contains br2 <output &&
301-
stdin_doesnot_contain master <output &&
302-
stdin_doesnot_contain branch <output
289+
grep m2 output &&
290+
grep br2 output &&
291+
! grep master output &&
292+
! grep branch output
303293
'
304294

305295
test_expect_success PERL 'say no to the second file' '
306296
(echo && echo n) >input &&
307297
git difftool -x cat branch <input >output &&
308-
stdin_contains master <output &&
309-
stdin_contains branch <output &&
310-
stdin_doesnot_contain m2 <output &&
311-
stdin_doesnot_contain br2 <output
298+
grep master output &&
299+
grep branch output &&
300+
! grep m2 output &&
301+
! grep br2 output
312302
'
313303

314304
test_expect_success PERL 'difftool --tool-help' '
315305
git difftool --tool-help >output &&
316-
stdin_contains tool <output
306+
grep tool output
317307
'
318308

319309
test_expect_success PERL 'setup change in subdirectory' '
@@ -324,20 +314,46 @@ test_expect_success PERL 'setup change in subdirectory' '
324314
git commit -m "added sub/sub" &&
325315
echo test >>file &&
326316
echo test >>sub/sub &&
327-
git add . &&
317+
git add file sub/sub &&
328318
git commit -m "modified both"
329319
'
330320

331-
test_expect_success PERL 'difftool -d' '
332-
git difftool -d --extcmd ls branch >output &&
333-
stdin_contains sub <output &&
334-
stdin_contains file <output
321+
run_dir_diff_test () {
322+
test_expect_success PERL "$1 --no-symlinks" "
323+
symlinks=--no-symlinks &&
324+
$2
325+
"
326+
test_expect_success PERL,SYMLINKS "$1 --symlinks" "
327+
symlinks=--symlinks &&
328+
$2
329+
"
330+
}
331+
332+
run_dir_diff_test 'difftool -d' '
333+
git difftool -d $symlinks --extcmd ls branch >output &&
334+
grep sub output &&
335+
grep file output
336+
'
337+
338+
run_dir_diff_test 'difftool --dir-diff' '
339+
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
340+
grep sub output &&
341+
grep file output
335342
'
336343

337-
test_expect_success PERL 'difftool --dir-diff' '
338-
git difftool --dir-diff --extcmd ls branch >output &&
339-
stdin_contains sub <output &&
340-
stdin_contains file <output
344+
run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
345+
git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
346+
grep sub output &&
347+
grep file output
348+
'
349+
350+
run_dir_diff_test 'difftool --dir-diff from subdirectory' '
351+
(
352+
cd sub &&
353+
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
354+
grep sub output &&
355+
grep file output
356+
)
341357
'
342358

343359
write_script .git/CHECK_SYMLINKS <<\EOF
@@ -362,18 +378,33 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
362378
test_cmp actual expect
363379
'
364380

365-
test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
366-
git difftool --dir-diff --prompt --extcmd ls branch >output &&
367-
stdin_contains sub <output &&
368-
stdin_contains file <output
381+
write_script modify-file <<\EOF
382+
echo "new content" >file
383+
EOF
384+
385+
test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
386+
echo "orig content" >file &&
387+
git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
388+
echo "new content" >expect &&
389+
test_cmp expect file
369390
'
370391

371-
test_expect_success PERL 'difftool --dir-diff from subdirectory' '
392+
write_script modify-both-files <<\EOF
393+
echo "wt content" >file &&
394+
echo "tmp content" >"$2/file" &&
395+
echo "$2" >tmpdir
396+
EOF
397+
398+
test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
372399
(
373-
cd sub &&
374-
git difftool --dir-diff --extcmd ls branch >output &&
375-
stdin_contains sub <output &&
376-
stdin_contains file <output
400+
TMPDIR=$TRASH_DIRECTORY &&
401+
export TMPDIR &&
402+
echo "orig content" >file &&
403+
test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
404+
echo "wt content" >expect &&
405+
test_cmp expect file &&
406+
echo "tmp content" >expect &&
407+
test_cmp expect "$(cat tmpdir)/file"
377408
)
378409
'
379410

0 commit comments

Comments
 (0)