{"thread":{"id":"44660","subject":"[PATCH 1/3] difftool: sanitize $workdir as early as possible","startedAt":"2016-12-09T08:59:02Z","lastAt":"2016-12-10T00:04:10Z","messageCount":5,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"307331","messageId":"20161209085848.10929-1-davvid@gmail.com","threadId":"44660","inReplyTo":null,"subject":"[PATCH 1/3] difftool: sanitize $workdir as early as possible","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-12-09T08:58:46Z","receivedAt":"2016-12-09T08:59:02Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The double-slash fixup on the $workdir variable was being\nperformed just-in-time to avoid double-slashes in symlink\ntargets, but the rest of the code was silently using paths with\nembedded \"//\" in them.\n\nA recent user-reported error message contained double-slashes.\nEliminate the issue by sanitizing inputs as soon as they arrive.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 959822d5f3..17c336321f 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -224,9 +224,7 @@ EOF\n \tdelete($ENV{GIT_INDEX_FILE});\n \n \t# Changes in the working tree need special treatment since they are\n-\t# not part of the index. Remove any trailing slash from $workdir\n-\t# before starting to avoid double slashes in symlink targets.\n-\t$workdir =~ s|/$||;\n+\t# not part of the index.\n \tfor my $file (@working_tree) {\n \t\tmy $dir = dirname($file);\n \t\tunless (-d \"$rdir/$dir\") {\n@@ -389,6 +387,7 @@ sub dir_diff\n \tmy $repo = Git->repository();\n \tmy $repo_path = $repo->repo_path();\n \tmy $workdir = $repo->wc_path();\n+\t$workdir =~ s|/$||; # Avoid double slashes in symlink targets\n \tmy ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);\n \n \tif (defined($extcmd)) {\n-- \n2.11.0.26.gb65c994\n\n"},{"id":"307332","messageId":"20161209085848.10929-3-davvid@gmail.com","threadId":"44660","inReplyTo":"20161209085848.10929-1-davvid@gmail.com","subject":"[PATCH 3/3] difftool: rename variables for consistency","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-12-09T08:58:48Z","receivedAt":"2016-12-09T08:59:05Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Always call the list of files @files.\nAlways call the worktree $worktree.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 99b03949bf..4e4f5d8138 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -100,7 +100,7 @@ sub changed_files\n \n sub setup_dir_diff\n {\n-\tmy ($workdir, $symlinks) = @_;\n+\tmy ($worktree, $symlinks) = @_;\n \tmy @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n \tmy $diffrtn = Git::command_oneline(@gitargs);\n \texit(0) unless defined($diffrtn);\n@@ -109,7 +109,7 @@ sub setup_dir_diff\n \t# changed files.  The paths returned by diff --raw are relative to the\n \t# top-level of the repository, but we defer changing directories so\n \t# that @ARGV can perform pathspec limiting in the current directory.\n-\tchdir($workdir);\n+\tchdir($worktree);\n \n \t# Build index info for left and right sides of the diff\n \tmy $submodule_mode = '160000';\n@@ -121,7 +121,7 @@ sub setup_dir_diff\n \tmy $wtindex = '';\n \tmy %submodule;\n \tmy %symlink;\n-\tmy @working_tree = ();\n+\tmy @files = ();\n \tmy %working_tree_dups = ();\n \tmy @rawdiff = split('\\0', $diffrtn);\n \n@@ -173,14 +173,14 @@ EOF\n \t\t}\n \n \t\tif ($rmode ne $null_mode) {\n-\t\t\t# Avoid duplicate working_tree entries\n+\t\t\t# Avoid duplicate entries\n \t\t\tif ($working_tree_dups{$dst_path}++) {\n \t\t\t\tnext;\n \t\t\t}\n \t\t\tmy ($use, $wt_sha1) =\n \t\t\t\tuse_wt_file($dst_path, $rsha1);\n \t\t\tif ($use) {\n-\t\t\t\tpush @working_tree, $dst_path;\n+\t\t\t\tpush @files, $dst_path;\n \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n \t\t\t} else {\n \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n@@ -227,14 +227,14 @@ EOF\n \n \t# Changes in the working tree need special treatment since they are\n \t# not part of the index.\n-\tfor my $file (@working_tree) {\n+\tfor my $file (@files) {\n \t\tmy $dir = dirname($file);\n \t\tunless (-d \"$rdir/$dir\") {\n \t\t\tmkpath(\"$rdir/$dir\") or\n \t\t\texit_cleanup($tmpdir, 1);\n \t\t}\n \t\tif ($symlinks) {\n-\t\t\tsymlink(\"$workdir/$file\", \"$rdir/$file\") or\n+\t\t\tsymlink(\"$worktree/$file\", \"$rdir/$file\") or\n \t\t\texit_cleanup($tmpdir, 1);\n \t\t} else {\n \t\t\tcopy($file, \"$rdir/$file\") or\n@@ -278,7 +278,7 @@ EOF\n \t\texit_cleanup($tmpdir, 1) if not $ok;\n \t}\n \n-\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n+\treturn ($ldir, $rdir, $tmpdir, @files);\n }\n \n sub write_to_file\n@@ -388,9 +388,9 @@ sub dir_diff\n \tmy $error = 0;\n \tmy $repo = Git->repository();\n \tmy $repo_path = $repo->repo_path();\n-\tmy $workdir = $repo->wc_path();\n-\t$workdir =~ s|/$||; # Avoid double slashes in symlink targets\n-\tmy ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);\n+\tmy $worktree = $repo->wc_path();\n+\t$worktree =~ s|/$||; # Avoid double slashes in symlink targets\n+\tmy ($a, $b, $tmpdir, @files) = setup_dir_diff($worktree, $symlinks);\n \n \tif (defined($extcmd)) {\n \t\t$rc = system($extcmd, $a, $b);\n@@ -411,13 +411,13 @@ sub dir_diff\n \tmy %tmp_modified;\n \tmy $indices_loaded = 0;\n \n-\tfor my $file (@worktree) {\n+\tfor my $file (@files) {\n \t\tnext if $symlinks && -l \"$b/$file\";\n \t\tnext if ! -f \"$b/$file\";\n \n \t\tif (!$indices_loaded) {\n \t\t\t%wt_modified = changed_files(\n-\t\t\t\t$repo_path, \"$tmpdir/wtindex\", $workdir);\n+\t\t\t\t$repo_path, \"$tmpdir/wtindex\", $worktree);\n \t\t\t%tmp_modified = changed_files(\n \t\t\t\t$repo_path, \"$tmpdir/wtindex\", $b);\n \t\t\t$indices_loaded = 1;\n@@ -425,7 +425,7 @@ sub dir_diff\n \n \t\tif (exists $wt_modified{$file} and exists $tmp_modified{$file}) {\n \t\t\tmy $errmsg = \"warning: Both files modified: \";\n-\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n+\t\t\t$errmsg .= \"'$worktree/$file' and '$b/$file'.\\n\";\n \t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n \t\t\t$errmsg .= \"warning:\\n\";\n \t\t\twarn $errmsg;\n-- \n2.11.0.26.gb65c994\n\n"},{"id":"307333","messageId":"20161209085848.10929-2-davvid@gmail.com","threadId":"44660","inReplyTo":"20161209085848.10929-1-davvid@gmail.com","subject":"[PATCH 2/3] difftool: chdir as early as possible","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-12-09T08:58:47Z","receivedAt":"2016-12-09T08:59:07Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Make difftool chdir to the top-level of the repository as soon as it can\nso that we can simplify how paths are handled.  Replace construction of\nabsolute paths via string concatenation with relative paths wherever\npossible.  The bulk of the code no longer needs to use absolute paths.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 17c336321f..99b03949bf 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -59,14 +59,14 @@ sub exit_cleanup\n \n sub use_wt_file\n {\n-\tmy ($workdir, $file, $sha1) = @_;\n+\tmy ($file, $sha1) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif (-l \"$workdir/$file\" || ! -e _) {\n+\tif (-l $file || ! -e _) {\n \t\treturn (0, $null_sha1);\n \t}\n \n-\tmy $wt_sha1 = Git::command_oneline('hash-object', \"$workdir/$file\");\n+\tmy $wt_sha1 = Git::command_oneline('hash-object', $file);\n \tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n \treturn ($use, $wt_sha1);\n }\n@@ -105,6 +105,12 @@ sub setup_dir_diff\n \tmy $diffrtn = Git::command_oneline(@gitargs);\n \texit(0) unless defined($diffrtn);\n \n+\t# Go to the root of the worktree now that we've captured the list of\n+\t# changed files.  The paths returned by diff --raw are relative to the\n+\t# top-level of the repository, but we defer changing directories so\n+\t# that @ARGV can perform pathspec limiting in the current directory.\n+\tchdir($workdir);\n+\n \t# Build index info for left and right sides of the diff\n \tmy $submodule_mode = '160000';\n \tmy $symlink_mode = '120000';\n@@ -172,7 +178,7 @@ EOF\n \t\t\t\tnext;\n \t\t\t}\n \t\t\tmy ($use, $wt_sha1) =\n-\t\t\t\tuse_wt_file($workdir, $dst_path, $rsha1);\n+\t\t\t\tuse_wt_file($dst_path, $rsha1);\n \t\t\tif ($use) {\n \t\t\t\tpush @working_tree, $dst_path;\n \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n@@ -182,10 +188,6 @@ EOF\n \t\t}\n \t}\n \n-\t# Go to the root of the worktree so that the left index files\n-\t# are properly setup -- the index is toplevel-relative.\n-\tchdir($workdir);\n-\n \t# Setup temp directories\n \tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n \tmy $ldir = \"$tmpdir/left\";\n@@ -235,10 +237,10 @@ EOF\n \t\t\tsymlink(\"$workdir/$file\", \"$rdir/$file\") or\n \t\t\texit_cleanup($tmpdir, 1);\n \t\t} else {\n-\t\t\tcopy(\"$workdir/$file\", \"$rdir/$file\") or\n+\t\t\tcopy($file, \"$rdir/$file\") or\n \t\t\texit_cleanup($tmpdir, 1);\n \n-\t\t\tmy $mode = stat(\"$workdir/$file\")->mode;\n+\t\t\tmy $mode = stat($file)->mode;\n \t\t\tchmod($mode, \"$rdir/$file\") or\n \t\t\texit_cleanup($tmpdir, 1);\n \t\t}\n@@ -430,10 +432,10 @@ sub dir_diff\n \t\t\t$error = 1;\n \t\t} elsif (exists $tmp_modified{$file}) {\n \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n-\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n+\t\t\tcopy(\"$b/$file\", $file) or\n \t\t\texit_cleanup($tmpdir, 1);\n \n-\t\t\tchmod($mode, \"$workdir/$file\") or\n+\t\t\tchmod($mode, $file) or\n \t\t\texit_cleanup($tmpdir, 1);\n \t\t}\n \t}\n-- \n2.11.0.26.gb65c994\n\n"},{"id":"307402","messageId":"xmqqbmwkr9ji.fsf@gitster.mtv.corp.google.com","threadId":"44660","inReplyTo":"20161209085848.10929-2-davvid@gmail.com","subject":"Re: [PATCH 2/3] difftool: chdir as early as possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-09T23:02:09Z","receivedAt":"2016-12-09T23:02:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> @@ -182,10 +188,6 @@ EOF\n>  \t\t}\n>  \t}\n>  \n> -\t# Go to the root of the worktree so that the left index files\n> -\t# are properly setup -- the index is toplevel-relative.\n> -\tchdir($workdir);\n> -\n>  \t# Setup temp directories\n>  \tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n>  \tmy $ldir = \"$tmpdir/left\";\n\nWhat codebase are you basing your work on?  I do not see these\nremoved four lines in my tree, so it seems that the patch is fixing\nup some other fix I do not yet have.\n\n> @@ -235,10 +237,10 @@ EOF\n>  \t\t\tsymlink(\"$workdir/$file\", \"$rdir/$file\") or\n>  \t\t\texit_cleanup($tmpdir, 1);\n>  \t\t} else {\n> -\t\t\tcopy(\"$workdir/$file\", \"$rdir/$file\") or\n> +\t\t\tcopy($file, \"$rdir/$file\") or\n>  \t\t\texit_cleanup($tmpdir, 1);\n>  \n> -\t\t\tmy $mode = stat(\"$workdir/$file\")->mode;\n> +\t\t\tmy $mode = stat($file)->mode;\n>  \t\t\tchmod($mode, \"$rdir/$file\") or\n>  \t\t\texit_cleanup($tmpdir, 1);\n>  \t\t}\n> @@ -430,10 +432,10 @@ sub dir_diff\n>  \t\t\t$error = 1;\n>  \t\t} elsif (exists $tmp_modified{$file}) {\n>  \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n> -\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n> +\t\t\tcopy(\"$b/$file\", $file) or\n>  \t\t\texit_cleanup($tmpdir, 1);\n>  \n> -\t\t\tchmod($mode, \"$workdir/$file\") or\n> +\t\t\tchmod($mode, $file) or\n>  \t\t\texit_cleanup($tmpdir, 1);\n>  \t\t}\n>  \t}\n"},{"id":"307410","messageId":"20161210000359.4uypoxg67zlsthc7@gmail.com","threadId":"44660","inReplyTo":"xmqqbmwkr9ji.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] difftool: chdir as early as possible","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-12-10T00:03:59Z","receivedAt":"2016-12-10T00:04:10Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Dec 09, 2016 at 03:02:09PM -0800, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > @@ -182,10 +188,6 @@ EOF\n> >  \t\t}\n> >  \t}\n> >  \n> > -\t# Go to the root of the worktree so that the left index files\n> > -\t# are properly setup -- the index is toplevel-relative.\n> > -\tchdir($workdir);\n> > -\n> >  \t# Setup temp directories\n> >  \tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n> >  \tmy $ldir = \"$tmpdir/left\";\n> \n> What codebase are you basing your work on?  I do not see these\n> removed four lines in my tree, so it seems that the patch is fixing\n> up some other fix I do not yet have.\n\nSorry about that.\n\nI forgot to mention that this is based on the patches I recently\nsent, which were in the \"pu\" branch.  The whats-cooking report\nmentioned that they'll be merged to \"next\", so they might be\nthere already too.\n\nThe patch this was based upon:\n\ndifftool: fix dir-diff index creation when in a subdirectory\n-- \nDavid\n"}]}