{"thread":{"id":"40659","subject":"[PATCH] difftool: avoid symlinks when reusing worktree files","startedAt":"2015-10-27T21:24:48Z","lastAt":"2015-10-30T16:19:10Z","messageCount":8,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"272344","messageId":"1445981088-6285-1-git-send-email-davvid@gmail.com","threadId":"40659","inReplyTo":null,"subject":"[PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-10-27T21:24:48Z","receivedAt":"2015-10-27T21:24:48Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"difftool's dir-diff should never reuse a symlink, regardless of\nwhat it points to.  Tighten use_wt_file() so that it rejects all\nsymlinks.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 1abe647..873db57 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -70,13 +70,13 @@ sub use_wt_file\n \tmy ($repo, $workdir, $file, $sha1) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif (! -f \"$workdir/$file\") {\n-\t\treturn (0, $null_sha1);\n+\tmy $workfile = \"$workdir/$file\";\n+\tif (-f $workfile && ! -l $workfile) {\n+\t\tmy $wt_sha1 = $repo->command_oneline('hash-object', $workfile);\n+\t\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n+\t\treturn ($use, $wt_sha1);\n \t}\n-\n-\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n-\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n-\treturn ($use, $wt_sha1);\n+\treturn (0, $null_sha1);\n }\n \n sub changed_files\n-- \n2.6.2.282.gfefd36e\n"},{"id":"272353","messageId":"xmqq1tcgne4u.fsf@gitster.mtv.corp.google.com","threadId":"40659","inReplyTo":"1445981088-6285-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T22:24:49Z","receivedAt":"2015-10-27T22:24:49Z","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 should never reuse a symlink, regardless of\n> what it points to.  Tighten use_wt_file() so that it rejects all\n> symlinks.\n>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n\nSorry.  I do recall saying \"it is wrong to feed the contents of a\nfile that a symlink points at to hash-object\" but other than that,\nI completely lost track.\n\nWhat purpose does this function play in its callchain?  What does\nits caller wants it to compute?  Is use of the entity in the working\ntree completely optional?  Would the caller happily produce correct\nresult even if we changed this function to unconditionally return\n($use=0, $wt_sha1='0'x40) regardless of the result of lstat(2) on\n\"$workdir/$file\"?\n\nThe conclusion of the thought process that starts from \"it is wrong\nto feed the contents of a file that a symlink points at to\nhash-object\" may not be \"so let's return $use=0 for all symlinks\",\nwhich is this patch. Depending on what its caller wants it to\ncompute, the right conclusion may be \"we need to call hash-object\ncorrectly by first running readlink and then feeding the result to\nit\".\n\nAnd if the answer is \"the caller wants us to compute the hash for a\nsymbolic link and say $use=1\", then we would instead need to do\nan equivalent of\n\n\twt_sha1=$(readlink \"$workdir/$file\" | hash-object --stdin)\n\nI cannot quite tell which from the patch and explanation.\n\nPerhaps an additional test or two would help illustrate what issues\nare being addressed better?\n\nThanks.\n\n>  git-difftool.perl | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 1abe647..873db57 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -70,13 +70,13 @@ sub use_wt_file\n>  \tmy ($repo, $workdir, $file, $sha1) = @_;\n>  \tmy $null_sha1 = '0' x 40;\n>  \n> -\tif (! -f \"$workdir/$file\") {\n> -\t\treturn (0, $null_sha1);\n> +\tmy $workfile = \"$workdir/$file\";\n> +\tif (-f $workfile && ! -l $workfile) {\n> +\t\tmy $wt_sha1 = $repo->command_oneline('hash-object', $workfile);\n> +\t\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n> +\t\treturn ($use, $wt_sha1);\n>  \t}\n> -\n> -\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> -\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n> -\treturn ($use, $wt_sha1);\n> +\treturn (0, $null_sha1);\n>  }\n>  \n>  sub changed_files\n"},{"id":"272354","messageId":"xmqqwpu8lysi.fsf@gitster.mtv.corp.google.com","threadId":"40659","inReplyTo":"1445981088-6285-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T22:41:33Z","receivedAt":"2015-10-27T22:41:33Z","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> -\tif (! -f \"$workdir/$file\") {\n> -\t\treturn (0, $null_sha1);\n> +\tmy $workfile = \"$workdir/$file\";\n> +\tif (-f $workfile && ! -l $workfile) {\n\nI still don't know if return (0, $null) is the right thing to do,\nbut in any case, I find the original flow easier to read, i.e. \"we\nnotice a few cases we cannot do the main 'hash-object' thing this\nfunction is meant to do and return early\".  I.e.\n\n\tif (-l \"$workdir/$file\" || ! -f _) {\n        \treturn (0, $null_sha1);\n\t}\n\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n\treturn ($use, $wt_sha1);\n"},{"id":"272473","messageId":"20151029015539.GA12513@gmail.com","threadId":"40659","inReplyTo":"xmqq1tcgne4u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-10-29T01:55:39Z","receivedAt":"2015-10-29T01:55:39Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Oct 27, 2015 at 03:24:49PM -0700, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > difftool's dir-diff should never reuse a symlink, regardless of\n> > what it points to.  Tighten use_wt_file() so that it rejects all\n> > symlinks.\n> >\n> > Helped-by: Junio C Hamano <gitster@pobox.com>\n> > Signed-off-by: David Aguilar <davvid@gmail.com>\n> > ---\n> \n> Sorry.  I do recall saying \"it is wrong to feed the contents of a\n> file that a symlink points at to hash-object\" but other than that,\n> I completely lost track.\n> \n> What purpose does this function play in its callchain?  What does\n> its caller wants it to compute?  Is use of the entity in the working\n> tree completely optional?  Would the caller happily produce correct\n> result even if we changed this function to unconditionally return\n> ($use=0, $wt_sha1='0'x40) regardless of the result of lstat(2) on\n> \"$workdir/$file\"?\n> \n> The conclusion of the thought process that starts from \"it is wrong\n> to feed the contents of a file that a symlink points at to\n> hash-object\" may not be \"so let's return $use=0 for all symlinks\",\n> which is this patch. Depending on what its caller wants it to\n> compute, the right conclusion may be \"we need to call hash-object\n> correctly by first running readlink and then feeding the result to\n> it\".\n> \n> And if the answer is \"the caller wants us to compute the hash for a\n> symbolic link and say $use=1\", then we would instead need to do\n> an equivalent of\n> \n> \twt_sha1=$(readlink \"$workdir/$file\" | hash-object --stdin)\n> \n> I cannot quite tell which from the patch and explanation.\n> \n> Perhaps an additional test or two would help illustrate what issues\n> are being addressed better?\n> \n> Thanks.\n\nRight.  At first I thought I could revise the commit message to\nmake it clearer that we simply want to skip all symlinks, since\nit never makes sense to reuse a worktree symlinks, but looking\nat the tests and implementation makes me realize that it's not\nthat simple.\n\nThis is going to take a bit more time to get right.  John, I was\nhoping you'd be able to take a look -- I'm playing catch-up too.\nWhen it was first reported I let it sit for a while in hopes\nthat the original author would pickup the issue, but months\npassed and I figured I'd take a stab at helping the user out.\n\nAnyways, it'll take me a bit more time to understand the code\nand work out a sensible solution.  My gut feeling is that we\nshould adjust the dir-diff feature so that it ignores all\nsymlinks.  That seems like a simple answer since we're deciding\nto skip that chunk of complexity.\n\nJohn, do you have any thoughts on how we can best handle this?\n-- \nDavid\n"},{"id":"272491","messageId":"xmqq8u6lsggx.fsf@gitster.mtv.corp.google.com","threadId":"40659","inReplyTo":"20151029015539.GA12513@gmail.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-29T17:59:58Z","receivedAt":"2015-10-29T17:59:58Z","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> Right.  At first I thought I could revise the commit message to\n> make it clearer that we simply want to skip all symlinks, since\n> it never makes sense to reuse a worktree symlinks, but looking\n> at the tests and implementation makes me realize that it's not\n> that simple.\n>\n> This is going to take a bit more time to get right.  John, I was\n> hoping you'd be able to take a look -- I'm playing catch-up too.\n> When it was first reported I let it sit for a while in hopes\n> that the original author would pickup the issue, but months\n> passed and I figured I'd take a stab at helping the user out.\n>\n> Anyways, it'll take me a bit more time to understand the code\n> and work out a sensible solution.  My gut feeling is that we\n> should adjust the dir-diff feature so that it ignores all\n> symlinks.  That seems like a simple answer since we're deciding\n> to skip that chunk of complexity.\n\nWhat dir-diff wants to do is to prepare two directory hierarchies on\nthe filesystem and run \"diff -r\" (or an equivalent command of user's\nchoice) on them, e.g. \"diff -r left/ right/\".  \"left/\" tree is\ntypically what you want to compare your working tree files agaist\n(e.g. a clean checkout of \"the other version\"), and \"right/\" tree is\npopulated with either copies of the working tree or symbolic links.\nThe copying to \"right/\" feels wasteful, but your working tree may be\nlittered with build artifacts, and making a clean copy with only\ntracked files is one way to make sure that \"diff -r\" with a clean\nchecout of \"the other version\" will not show them.\n\nIn the loop that walks the @rawdiff array, there are a lot of \"if we\nsee a symbolic link on the left, do this\" before the last one that\nsays \"if the working tree side is not $null (i.e. not missing), ask\nut_wt_file()\".  That code remembers which path on either side had\nsymbolic links.\n\nLater in the same function, there is this comment \"Symbolic links\nrequire special treatment.\"  The intent of the code here is that any\npath that involves a symbolic link should be tweaked there.  The\nloop over %symlink expects left/ and right/ to be populated normally\nby the loop over @working_tree, and then for any path that is a\nsymbolic link is replaced with a phony regular file (not a symbolic\nlink) that says \"Here is a symbolic link\".\n\nSo I think it is fine to return $use=0 for any symbolic link from\nuse_wt_file.  Anything you do there will be replaced by the loop\nover %symlink that appears later in the caller.  The caller discards\n$wt_sha1 when $use=0 is returned, so the second return value does\nnot matter.\n"},{"id":"272493","messageId":"xmqq4mh9sfl6.fsf@gitster.mtv.corp.google.com","threadId":"40659","inReplyTo":"xmqq8u6lsggx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-29T18:19:01Z","receivedAt":"2015-10-29T18:19:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> So I think it is fine to return $use=0 for any symbolic link from\n> use_wt_file.  Anything you do there will be replaced by the loop\n> over %symlink that appears later in the caller.  The caller discards\n> $wt_sha1 when $use=0 is returned, so the second return value does\n> not matter.\n\nSo let me try to update your patch with the result of the study of\nthe codeflow.\n\n-- >8 --\nFrom: David Aguilar <davvid@gmail.com>\nSubject: difftool: ignore symbolic links in use_wt_file\n\nThe caller is preparing a narrowed-down copy of the working tree and\nthis function is asked if the path should be included in that copy.\nIf we say yes, the path from the working tree will be either symlinked\nor copied into the narrowed-down copy.\n\nFor any path that is a symbolic link, the caller later fixes up the\nnarrowed-down copy by unlinking the path and replacing it with a\nregular file it writes out that mimics the way how \"git diff\"\ncompares symbolic links.\n\nLet's answer \"no, you do not want to copy/symlink the working tree\nfile\" for all symbolic links from this function, as we know the\nresult will not be used because it will be overwritten anyway.\n\nIncidentally, this also stops the function from feeding a symbolic\nlink in the working tree to hash-object, which is a wrong thing to\ndo to begin with. The link may be pointing at a directory, or worse\nmay be dangling (both would be noticed as an error).  Even if the\nlink points at a regular file, hashing the contents of a file that\nis pointed at by the link is not correct (Git hashes the contents of\nthe link itself, not the pointee).\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.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..488d14b 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 (-l \"$workdir/$file\" || ! -e _) {\n \t\treturn (0, $null_sha1);\n \t}\n \ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex ea35a02..a771cf7 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":"272519","messageId":"20151030072857.GA15031@gmail.com","threadId":"40659","inReplyTo":"xmqq4mh9sfl6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2015-10-30T07:28:57Z","receivedAt":"2015-10-30T07:28:57Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Oct 29, 2015 at 11:19:01AM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > So I think it is fine to return $use=0 for any symbolic link from\n> > use_wt_file.  Anything you do there will be replaced by the loop\n> > over %symlink that appears later in the caller.  The caller discards\n> > $wt_sha1 when $use=0 is returned, so the second return value does\n> > not matter.\n> \n> So let me try to update your patch with the result of the study of\n> the codeflow.\n> \n> -- >8 --\n> From: David Aguilar <davvid@gmail.com>\n> Subject: difftool: ignore symbolic links in use_wt_file\n> \n> The caller is preparing a narrowed-down copy of the working tree and\n> this function is asked if the path should be included in that copy.\n> If we say yes, the path from the working tree will be either symlinked\n> or copied into the narrowed-down copy.\n> \n> For any path that is a symbolic link, the caller later fixes up the\n> narrowed-down copy by unlinking the path and replacing it with a\n> regular file it writes out that mimics the way how \"git diff\"\n> compares symbolic links.\n> \n> Let's answer \"no, you do not want to copy/symlink the working tree\n> file\" for all symbolic links from this function, as we know the\n> result will not be used because it will be overwritten anyway.\n> \n> Incidentally, this also stops the function from feeding a symbolic\n> link in the working tree to hash-object, which is a wrong thing to\n> do to begin with. The link may be pointing at a directory, or worse\n> may be dangling (both would be noticed as an error).  Even if the\n> link points at a regular file, hashing the contents of a file that\n> is pointed at by the link is not correct (Git hashes the contents of\n> the link itself, not the pointee).\n> \n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n\nThis is a very nicely worded commit message.  Thanks for the\nthorough explanation.\n\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..488d14b 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 (-l \"$workdir/$file\" || ! -e _) {\n>  \t\treturn (0, $null_sha1);\n>  \t}\n\nThe \"-e _\" shorthand caught my eye ~ I didn't know perl could do that!\nNice.\n\nUnderline is barely mentioned in perlvar, but it's obvious what\n(I think) it means, and since Perl is DWIM, it must be right. ;-)\n-- \nDavid\n"},{"id":"272530","messageId":"xmqqio5omirl.fsf@gitster.mtv.corp.google.com","threadId":"40659","inReplyTo":"20151030072857.GA15031@gmail.com","subject":"Re: [PATCH] difftool: avoid symlinks when reusing worktree files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T16:19:10Z","receivedAt":"2015-10-30T16:19:10Z","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>> +\tif (-l \"$workdir/$file\" || ! -e _) {\n>>  \t\treturn (0, $null_sha1);\n>>  \t}\n>\n> The \"-e _\" shorthand caught my eye ~ I didn't know perl could do that!\n> Nice.\n>\n> Underline is barely mentioned in perlvar, but it's obvious what\n> (I think) it means, and since Perl is DWIM, it must be right. ;-)\n\nIt is mentioned under -X (i.e. the headline for -f, -e, -d,\n... tests) in perlfunc with an interesting example of doing\nan explicit stat() first and then running many variants of -X\nwith _ as the target.\n"}]}