{"thread":{"id":"40605","subject":"[PATCH] difftool: gracefully handle symlinks to directories","startedAt":"2015-10-21T08:04:33Z","lastAt":"2015-10-27T21:30:56Z","messageCount":3,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"272028","messageId":"1445414673-15307-1-git-send-email-davvid@gmail.com","threadId":"40605","inReplyTo":null,"subject":"[PATCH] difftool: gracefully handle symlinks to directories","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-10-21T08:04:33Z","receivedAt":"2015-10-21T08:04:33Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"difftool's dir-diff feature was blindly feeding worktree paths\nto hash-object without checking whether the path was indeed a\nfile, causing the feature to fail when repositories contain\nsymlinks to directories.\n\nEnsure that only files are ever given to hash-object.\nAdd a test to demonstrate the breakage.\n\nReported-by: Ismail Badawi <ismail@badawi.io>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl   |  4 +---\n t/t7800-difftool.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 7df7c8a..1abe647 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -70,9 +70,7 @@ sub use_wt_file\n \tmy ($repo, $workdir, $file, $sha1) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif (! -e \"$workdir/$file\") {\n-\t\t# If the file doesn't exist in the working tree, we cannot\n-\t\t# use it.\n+\tif (! -f \"$workdir/$file\") {\n \t\treturn (0, $null_sha1);\n \t}\n \ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 48c6e2b..ec8bc8c 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -504,4 +504,23 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '\n \t)\n '\n \n+test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '\n+\tgit init dirlinks &&\n+\t(\n+\t\tcd dirlinks &&\n+\t\tgit config diff.tool checktrees &&\n+\t\tgit config difftool.checktrees.cmd \"echo good\" &&\n+\t\tmkdir foo &&\n+\t\t: >foo/bar &&\n+\t\tgit add foo/bar &&\n+\t\ttest_commit symlink-one &&\n+\t\tln -s foo link &&\n+\t\tgit add link &&\n+\t\ttest_commit symlink-two &&\n+\t\techo good >expect &&\n+\t\tgit difftool --tool=checktrees --dir-diff HEAD~ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.6.2.281.gac28444\n"},{"id":"272114","messageId":"xmqq8u6usqx1.fsf@gitster.mtv.corp.google.com","threadId":"40605","inReplyTo":"1445414673-15307-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: gracefully handle symlinks to directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-22T18:23:54Z","receivedAt":"2015-10-22T18:23:54Z","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> difftool's dir-diff feature was blindly feeding worktree paths\n> to hash-object without checking whether the path was indeed a\n> file, causing the feature to fail when repositories contain\n> symlinks to directories.\n\nWait.  Anything that considers symlinks \"to directories\" any special\nsmells like a misdesign here.  Why is it safe to substitute a\nsymbolic link that happens to point at a file with the file it\npoints at?\n\nBecause the way you would hash a symblic link is not by hashing the\nfile it points at, but by hashing the result of readlink(2) of it,\nwe must not reuse the working tree files for any symbolic link,\nregardless of its target, I would think.\n\nAfter all, a symbolic link may even be dangling and not pointing at\nanything.\n\n>\n> Ensure that only files are ever given to hash-object.\n> Add a test to demonstrate the breakage.\n>\n> Reported-by: Ismail Badawi <ismail@badawi.io>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-difftool.perl   |  4 +---\n>  t/t7800-difftool.sh | 19 +++++++++++++++++++\n>  2 files changed, 20 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 7df7c8a..1abe647 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -70,9 +70,7 @@ sub use_wt_file\n>  \tmy ($repo, $workdir, $file, $sha1) = @_;\n>  \tmy $null_sha1 = '0' x 40;\n>  \n> -\tif (! -e \"$workdir/$file\") {\n> -\t\t# If the file doesn't exist in the working tree, we cannot\n> -\t\t# use it.\n> +\tif (! -f \"$workdir/$file\") {\n>  \t\treturn (0, $null_sha1);\n>  \t}\n>  \n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 48c6e2b..ec8bc8c 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -504,4 +504,23 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '\n>  \t)\n>  '\n>  \n> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '\n> +\tgit init dirlinks &&\n> +\t(\n> +\t\tcd dirlinks &&\n> +\t\tgit config diff.tool checktrees &&\n> +\t\tgit config difftool.checktrees.cmd \"echo good\" &&\n> +\t\tmkdir foo &&\n> +\t\t: >foo/bar &&\n> +\t\tgit add foo/bar &&\n> +\t\ttest_commit symlink-one &&\n> +\t\tln -s foo link &&\n> +\t\tgit add link &&\n> +\t\ttest_commit symlink-two &&\n> +\t\techo good >expect &&\n> +\t\tgit difftool --tool=checktrees --dir-diff HEAD~ >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"272350","messageId":"20151027213056.GA6527@gmail.com","threadId":"40605","inReplyTo":"xmqq8u6usqx1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: gracefully handle symlinks to directories","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-10-27T21:30:56Z","receivedAt":"2015-10-27T21:30:56Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Oct 22, 2015 at 11:23:54AM -0700, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > difftool's dir-diff feature was blindly feeding worktree paths\n> > to hash-object without checking whether the path was indeed a\n> > file, causing the feature to fail when repositories contain\n> > symlinks to directories.\n> \n> Wait.  Anything that considers symlinks \"to directories\" any special\n> smells like a misdesign here.  Why is it safe to substitute a\n> symbolic link that happens to point at a file with the file it\n> points at?\n> \n> Because the way you would hash a symblic link is not by hashing the\n> file it points at, but by hashing the result of readlink(2) of it,\n> we must not reuse the working tree files for any symbolic link,\n> regardless of its target, I would think.\n> \n> After all, a symbolic link may even be dangling and not pointing at\n> anything.\n\nAh, right.  I think the simplest thing to do is to tighten\nuse_wt_file() so that it always rejects symlinks.  That seems\nlike a safe way to go for now without needing to invent a new\nparadigm for how to handle symlinks in the dir-diff code.\n\nI just sent a follow-up patch that does just that.  Let me know\nif you'd like a replacement patch that combines the two patches\ninstead.\n\nThanks for the review,\n-- \nDavid\n"}]}