difftool: sanitize $workdir as early as possible
PATCH 1/3 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.
--- git-difftool.perl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
git-difftool.perl +2 -3
@@ -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)) {
Signature · 1 line
2.11.0.26.gb65c994
PATCH 2/3 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.
--- git-difftool.perl | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
git-difftool.perl +14 -12
@@ -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); } }
Signature · 1 line
2.11.0.26.gb65c994
Re: [PATCH 2/3] difftool: chdir as early as possible
git-difftool.perl EOF · line 193
Quoted 11 lines · David Aguilar
git-difftool.perl +0
-4@@ -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";
What codebase are you basing your work on? I do not see these removed four lines in my tree, so it seems that the patch is fixing up some other fix I do not yet have.
Quoted 26 lines
git-difftool.perl +4
-4@@ -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); } }
Re: [PATCH 2/3] difftool: chdir as early as possible
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · David Aguilar
(quoted diff) +0
-0@@ -182,10 +188,6 @@ EOF } }
Quoted 11 lines
git-difftool.perl EOF · line 193
Quoted 7 lines
git-difftool.perl +0
-4- # 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";What codebase are you basing your work on? I do not see these removed four lines in my tree, so it seems that the patch is fixing up some other fix I do not yet have.
Sorry about that.
I forgot to mention that this is based on the patches I recently sent, which were in the "pu" branch. The whats-cooking report mentioned that they'll be merged to "next", so they might be there already too.
The patch this was based upon:
difftool: fix dir-diff index creation when in a subdirectory
Signature · 1 line
David
PATCH 3/3 difftool: rename variables for consistency
In reply to David AguilarAlways call the list of files @files.
Always call the worktree $worktree.
--- git-difftool.perl | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
git-difftool.perl +14 -14
@@ -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;
Signature · 1 line
2.11.0.26.gb65c994