From 86defcbe3f6c54a31dc898cb3adb4a3b25f1eb1a Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Wed, 7 Dec 2016 02:16:08 -0800 Subject: difftool: fix dir-diff index creation when in a subdirectory 9ec26e7977 (difftool: fix argument handling in subdirs, 2016-07-18) corrected how path arguments are handled in a subdirectory, but it introduced a regression in how entries outside of the subdirectory are handled by dir-diff. When preparing the right-side of the diff we only include the changed paths in the temporary area. The left side of the diff is constructed from a temporary index that is built from the same set of changed files, but it was being constructed from within the subdirectory. This is a problem because the indexed paths are toplevel-relative, and thus they were not getting added to the index. Teach difftool to chdir to the toplevel of the repository before preparing its temporary indexes. This ensures that all of the toplevel-relative paths are valid. Add test cases to more thoroughly exercise this scenario. Reported-by: Frank Becker Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- git-difftool.perl | 4 ++++ t/t7800-difftool.sh | 44 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/git-difftool.perl b/git-difftool.perl index a5790d03a0..959822d5f3 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -182,6 +182,10 @@ EOF } } + # Go to the root of the worktree so that the left index files + # are properly setup -- the index is toplevel-relative. + chdir($workdir); + # Setup temp directories my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1); my $ldir = "$tmpdir/left"; diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index cb25480e1f..c7bd688537 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -368,6 +368,7 @@ test_expect_success PERL 'setup change in subdirectory' ' echo master >sub/sub && git add sub/sub && git commit -m "added sub/sub" && + git tag v1 && echo test >>file && echo test >>sub/sub && git add file sub/sub && @@ -403,12 +404,49 @@ run_dir_diff_test 'difftool --dir-diff ignores --prompt' ' grep file output ' -run_dir_diff_test 'difftool --dir-diff from subdirectory' ' +run_dir_diff_test 'difftool --dir-diff branch from subdirectory' ' ( cd sub && git difftool --dir-diff $symlinks --extcmd ls branch >output && - grep sub output && - grep file output + # "sub" must only exist in "right" + # "file" and "file2" must be listed in both "left" and "right" + test "1" = $(grep sub output | wc -l) && + test "2" = $(grep file"$" output | wc -l) && + test "2" = $(grep file2 output | wc -l) + ) +' + +run_dir_diff_test 'difftool --dir-diff v1 from subdirectory' ' + ( + cd sub && + git difftool --dir-diff $symlinks --extcmd ls v1 >output && + # "sub" and "file" exist in both v1 and HEAD. + # "file2" is unchanged. + test "2" = $(grep sub output | wc -l) && + test "2" = $(grep file output | wc -l) && + test "0" = $(grep file2 output | wc -l) + ) +' + +run_dir_diff_test 'difftool --dir-diff branch from subdirectory w/ pathspec' ' + ( + cd sub && + git difftool --dir-diff $symlinks --extcmd ls branch -- .>output && + # "sub" only exists in "right" + # "file" and "file2" must not be listed + test "1" = $(grep sub output | wc -l) && + test "0" = $(grep file output | wc -l) + ) +' + +run_dir_diff_test 'difftool --dir-diff v1 from subdirectory w/ pathspec' ' + ( + cd sub && + git difftool --dir-diff $symlinks --extcmd ls v1 -- .>output && + # "sub" exists in v1 and HEAD + # "file" is filtered out by the pathspec + test "2" = $(grep sub output | wc -l) && + test "0" = $(grep file output | wc -l) ) ' -- cgit v1.2.3 From e6e3e2a67c8dacb0ed726f09cf203568f24f8e74 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Fri, 9 Dec 2016 00:58:46 -0800 Subject: difftool: sanitize $workdir as early as possible The double-slash fixup on the $workdir variable was being performed just-in-time to avoid double-slashes in symlink targets, but the rest of the code was silently using paths with embedded "//" in them. A recent user-reported error message contained double-slashes. Eliminate the issue by sanitizing inputs as soon as they arrive. Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- git-difftool.perl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/git-difftool.perl b/git-difftool.perl index 959822d5f3..17c336321f 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -224,9 +224,7 @@ EOF delete($ENV{GIT_INDEX_FILE}); # Changes in the working tree need special treatment since they are - # not part of the index. Remove any trailing slash from $workdir - # before starting to avoid double slashes in symlink targets. - $workdir =~ s|/$||; + # not part of the index. for my $file (@working_tree) { my $dir = dirname($file); unless (-d "$rdir/$dir") { @@ -389,6 +387,7 @@ sub dir_diff my $repo = Git->repository(); my $repo_path = $repo->repo_path(); my $workdir = $repo->wc_path(); + $workdir =~ s|/$||; # Avoid double slashes in symlink targets my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks); if (defined($extcmd)) { -- cgit v1.2.3 From f242a03d7330a68baf0748e595c0b2290d3a05a5 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Fri, 9 Dec 2016 00:58:47 -0800 Subject: difftool: chdir as early as possible Make difftool chdir to the top-level of the repository as soon as it can so that we can simplify how paths are handled. Replace construction of absolute paths via string concatenation with relative paths wherever possible. The bulk of the code no longer needs to use absolute paths. Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- git-difftool.perl | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/git-difftool.perl b/git-difftool.perl index 17c336321f..99b03949bf 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -59,14 +59,14 @@ sub exit_cleanup sub use_wt_file { - my ($workdir, $file, $sha1) = @_; + my ($file, $sha1) = @_; my $null_sha1 = '0' x 40; - if (-l "$workdir/$file" || ! -e _) { + if (-l $file || ! -e _) { return (0, $null_sha1); } - my $wt_sha1 = Git::command_oneline('hash-object', "$workdir/$file"); + my $wt_sha1 = Git::command_oneline('hash-object', $file); my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1); return ($use, $wt_sha1); } @@ -105,6 +105,12 @@ sub setup_dir_diff my $diffrtn = Git::command_oneline(@gitargs); exit(0) unless defined($diffrtn); + # Go to the root of the worktree now that we've captured the list of + # changed files. The paths returned by diff --raw are relative to the + # top-level of the repository, but we defer changing directories so + # that @ARGV can perform pathspec limiting in the current directory. + chdir($workdir); + # Build index info for left and right sides of the diff my $submodule_mode = '160000'; my $symlink_mode = '120000'; @@ -172,7 +178,7 @@ EOF next; } my ($use, $wt_sha1) = - use_wt_file($workdir, $dst_path, $rsha1); + use_wt_file($dst_path, $rsha1); if ($use) { push @working_tree, $dst_path; $wtindex .= "$rmode $wt_sha1\t$dst_path\0"; @@ -182,10 +188,6 @@ EOF } } - # Go to the root of the worktree so that the left index files - # are properly setup -- the index is toplevel-relative. - chdir($workdir); - # Setup temp directories my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1); my $ldir = "$tmpdir/left"; @@ -235,10 +237,10 @@ EOF symlink("$workdir/$file", "$rdir/$file") or exit_cleanup($tmpdir, 1); } else { - copy("$workdir/$file", "$rdir/$file") or + copy($file, "$rdir/$file") or exit_cleanup($tmpdir, 1); - my $mode = stat("$workdir/$file")->mode; + my $mode = stat($file)->mode; chmod($mode, "$rdir/$file") or exit_cleanup($tmpdir, 1); } @@ -430,10 +432,10 @@ sub dir_diff $error = 1; } elsif (exists $tmp_modified{$file}) { my $mode = stat("$b/$file")->mode; - copy("$b/$file", "$workdir/$file") or + copy("$b/$file", $file) or exit_cleanup($tmpdir, 1); - chmod($mode, "$workdir/$file") or + chmod($mode, $file) or exit_cleanup($tmpdir, 1); } } -- cgit v1.2.3 From ce6926974ec7b28ad43c387d5aa8ae7ea9f72e65 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Fri, 9 Dec 2016 00:58:48 -0800 Subject: difftool: rename variables for consistency Always call the list of files @files. Always call the worktree $worktree. Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- git-difftool.perl | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/git-difftool.perl b/git-difftool.perl index 99b03949bf..4e4f5d8138 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -100,7 +100,7 @@ sub changed_files sub setup_dir_diff { - my ($workdir, $symlinks) = @_; + my ($worktree, $symlinks) = @_; my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV); my $diffrtn = Git::command_oneline(@gitargs); exit(0) unless defined($diffrtn); @@ -109,7 +109,7 @@ sub setup_dir_diff # changed files. The paths returned by diff --raw are relative to the # top-level of the repository, but we defer changing directories so # that @ARGV can perform pathspec limiting in the current directory. - chdir($workdir); + chdir($worktree); # Build index info for left and right sides of the diff my $submodule_mode = '160000'; @@ -121,7 +121,7 @@ sub setup_dir_diff my $wtindex = ''; my %submodule; my %symlink; - my @working_tree = (); + my @files = (); my %working_tree_dups = (); my @rawdiff = split('\0', $diffrtn); @@ -173,14 +173,14 @@ EOF } if ($rmode ne $null_mode) { - # Avoid duplicate working_tree entries + # Avoid duplicate entries if ($working_tree_dups{$dst_path}++) { next; } my ($use, $wt_sha1) = use_wt_file($dst_path, $rsha1); if ($use) { - push @working_tree, $dst_path; + push @files, $dst_path; $wtindex .= "$rmode $wt_sha1\t$dst_path\0"; } else { $rindex .= "$rmode $rsha1\t$dst_path\0"; @@ -227,14 +227,14 @@ EOF # Changes in the working tree need special treatment since they are # not part of the index. - for my $file (@working_tree) { + for my $file (@files) { my $dir = dirname($file); unless (-d "$rdir/$dir") { mkpath("$rdir/$dir") or exit_cleanup($tmpdir, 1); } if ($symlinks) { - symlink("$workdir/$file", "$rdir/$file") or + symlink("$worktree/$file", "$rdir/$file") or exit_cleanup($tmpdir, 1); } else { copy($file, "$rdir/$file") or @@ -278,7 +278,7 @@ EOF exit_cleanup($tmpdir, 1) if not $ok; } - return ($ldir, $rdir, $tmpdir, @working_tree); + return ($ldir, $rdir, $tmpdir, @files); } sub write_to_file @@ -388,9 +388,9 @@ sub dir_diff my $error = 0; my $repo = Git->repository(); my $repo_path = $repo->repo_path(); - my $workdir = $repo->wc_path(); - $workdir =~ s|/$||; # Avoid double slashes in symlink targets - my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks); + my $worktree = $repo->wc_path(); + $worktree =~ s|/$||; # Avoid double slashes in symlink targets + my ($a, $b, $tmpdir, @files) = setup_dir_diff($worktree, $symlinks); if (defined($extcmd)) { $rc = system($extcmd, $a, $b); @@ -411,13 +411,13 @@ sub dir_diff my %tmp_modified; my $indices_loaded = 0; - for my $file (@worktree) { + for my $file (@files) { next if $symlinks && -l "$b/$file"; next if ! -f "$b/$file"; if (!$indices_loaded) { %wt_modified = changed_files( - $repo_path, "$tmpdir/wtindex", $workdir); + $repo_path, "$tmpdir/wtindex", $worktree); %tmp_modified = changed_files( $repo_path, "$tmpdir/wtindex", $b); $indices_loaded = 1; @@ -425,7 +425,7 @@ sub dir_diff if (exists $wt_modified{$file} and exists $tmp_modified{$file}) { my $errmsg = "warning: Both files modified: "; - $errmsg .= "'$workdir/$file' and '$b/$file'.\n"; + $errmsg .= "'$worktree/$file' and '$b/$file'.\n"; $errmsg .= "warning: Working tree file has been left.\n"; $errmsg .= "warning:\n"; warn $errmsg; -- cgit v1.2.3