{"thread":{"id":"33854","subject":"[PATCH] difftool: fix dir-diff when file does not exist in working tree","startedAt":"2013-05-17T17:29:37Z","lastAt":"2013-05-17T18:46:15Z","messageCount":4,"participants":["John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"217742","messageId":"de6690bea81de561747ca49893fbc77fa3eb8529.1368811736.git.john@keeping.me.uk","threadId":"33854","inReplyTo":null,"subject":"[PATCH] difftool: fix dir-diff when file does not exist in working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-17T17:29:37Z","receivedAt":"2013-05-17T17:29:37Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Commit 02c5631 (difftool --dir-diff: symlink all files matching the\nworking tree, 2013-03-14) does not handle the case where a file that is\nbeing compared does not exist in the working tree.  Fix this by checking\nfor existence explicitly before running git-hash-object.\n\nReported-by: Kevin Bracey <kevin@bracey.fi>\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\nThis fixes a regression in 1.8.3-rc0.\n\n git-difftool.perl   | 9 ++++++++-\n t/t7800-difftool.sh | 7 +++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 6780292..0a1cb0a 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -92,7 +92,14 @@ sub use_wt_file\n \t\treturn 0;\n \t}\n \n-\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n+\tmy $wt_sha1;\n+\tif (-e \"$workdir/$file\") {\n+\t\t$wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n+\t} else {\n+\t\t# If the file doesn't exist in the working tree, use something\n+\t\t# that cannot match a SHA-1.\n+\t\t$wt_sha1 = '';\n+\t};\n \tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n \treturn ($use, $wt_sha1);\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex a6bd99e..d46f041 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n \t)\n '\n \n+run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '\n+\ttest_when_finished git reset --hard &&\n+\trm file2 &&\n+\tgit difftool --dir-diff $symlinks --extcmd ls branch master >output &&\n+\tgrep file2 output\n+'\n+\n write_script .git/CHECK_SYMLINKS <<\\EOF\n for f in file file2 sub/sub\n do\n-- \n1.8.3.rc2.285.gfc18c2c\n"},{"id":"217749","messageId":"7v38tlzdcv.fsf@alter.siamese.dyndns.org","threadId":"33854","inReplyTo":"de6690bea81de561747ca49893fbc77fa3eb8529.1368811736.git.john@keeping.me.uk","subject":"Re: [PATCH] difftool: fix dir-diff when file does not exist in working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-17T18:10:40Z","receivedAt":"2013-05-17T18:10:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Commit 02c5631 (difftool --dir-diff: symlink all files matching the\n> working tree, 2013-03-14) does not handle the case where a file that is\n> being compared does not exist in the working tree.  Fix this by checking\n> for existence explicitly before running git-hash-object.\n>\n> Reported-by: Kevin Bracey <kevin@bracey.fi>\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n> This fixes a regression in 1.8.3-rc0.\n>\n>  git-difftool.perl   | 9 ++++++++-\n>  t/t7800-difftool.sh | 7 +++++++\n>  2 files changed, 15 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 6780292..0a1cb0a 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -92,7 +92,14 @@ sub use_wt_file\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> +\tmy $wt_sha1;\n> +\tif (-e \"$workdir/$file\") {\n> +\t\t$wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> +\t} else {\n> +\t\t# If the file doesn't exist in the working tree, use something\n> +\t\t# that cannot match a SHA-1.\n> +\t\t$wt_sha1 = '';\n\nYuck.\n\n\"that cannot match\" might be a good justification to say \"this does\nnot break the next line to set $use and forces it to false\", but\n\"when we return false in $use, the value of $wt_sha1 is not used\"\nneeds to be said to convince why this is a safe change.\n\nBut if $sha1 is $null_sha1, we do end up setting $use to true and\nthe caller would stuff the empty $wt_sha1 to form:\n\n\t$wtindex .= \"$rmode \\$dst_path\\0\";\n\nIs that what we want to do here, or is it a \"will never happen\"\ncondition?  If the latter, the reason need to be described in this\ncomment (and in the log).\n\nThanks.\n\n> +\t};\n>  \tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n>  \treturn ($use, $wt_sha1);\n>  }\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index a6bd99e..d46f041 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n>  \t)\n>  '\n>  \n> +run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '\n> +\ttest_when_finished git reset --hard &&\n> +\trm file2 &&\n> +\tgit difftool --dir-diff $symlinks --extcmd ls branch master >output &&\n> +\tgrep file2 output\n> +'\n> +\n>  write_script .git/CHECK_SYMLINKS <<\\EOF\n>  for f in file file2 sub/sub\n>  do\n"},{"id":"217751","messageId":"20130517182608.GB27005@serenity.lan","threadId":"33854","inReplyTo":"7v38tlzdcv.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] difftool: fix dir-diff when file does not exist in working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-17T18:26:08Z","receivedAt":"2013-05-17T18:26:08Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Commit 02c5631 (difftool --dir-diff: symlink all files matching the\nworking tree, 2013-03-14) does not handle the case where a file that is\nbeing compared does not exist in the working tree.  Fix this by checking\nfor existence explicitly before running git-hash-object.\n\nReported-by: Kevin Bracey <kevin@bracey.fi>\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\nOn Fri, May 17, 2013 at 11:10:40AM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > Commit 02c5631 (difftool --dir-diff: symlink all files matching the\n> > working tree, 2013-03-14) does not handle the case where a file that is\n> > being compared does not exist in the working tree.  Fix this by checking\n> > for existence explicitly before running git-hash-object.\n> >\n> > Reported-by: Kevin Bracey <kevin@bracey.fi>\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> > This fixes a regression in 1.8.3-rc0.\n> >\n> >  git-difftool.perl   | 9 ++++++++-\n> >  t/t7800-difftool.sh | 7 +++++++\n> >  2 files changed, 15 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index 6780292..0a1cb0a 100755\n> > --- a/git-difftool.perl\n> > +++ b/git-difftool.perl\n> > @@ -92,7 +92,14 @@ sub use_wt_file\n> >  \t\treturn 0;\n> >  \t}\n> >  \n> > -\tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> > +\tmy $wt_sha1;\n> > +\tif (-e \"$workdir/$file\") {\n> > +\t\t$wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> > +\t} else {\n> > +\t\t# If the file doesn't exist in the working tree, use something\n> > +\t\t# that cannot match a SHA-1.\n> > +\t\t$wt_sha1 = '';\n> \n> Yuck.\n> \n> \"that cannot match\" might be a good justification to say \"this does\n> not break the next line to set $use and forces it to false\", but\n> \"when we return false in $use, the value of $wt_sha1 is not used\"\n> needs to be said to convince why this is a safe change.\n> \n> But if $sha1 is $null_sha1, we do end up setting $use to true and\n> the caller would stuff the empty $wt_sha1 to form:\n> \n> \t$wtindex .= \"$rmode \\$dst_path\\0\";\n> \n> Is that what we want to do here, or is it a \"will never happen\"\n> condition?  If the latter, the reason need to be described in this\n> comment (and in the log).\n\nIt can't ever happen because we only call this if the mode is non-zero\nin which case the SHA-1 is only null if there are unstaged changes.\n\nHowever, I think this revised version is much clearer.\n\n git-difftool.perl   | 6 ++++++\n t/t7800-difftool.sh | 7 +++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 6780292..8a75205 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -92,6 +92,12 @@ sub use_wt_file\n \t\treturn 0;\n \t}\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+\t\treturn (0, $null_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);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex a6bd99e..d46f041 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n \t)\n '\n \n+run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '\n+\ttest_when_finished git reset --hard &&\n+\trm file2 &&\n+\tgit difftool --dir-diff $symlinks --extcmd ls branch master >output &&\n+\tgrep file2 output\n+'\n+\n write_script .git/CHECK_SYMLINKS <<\\EOF\n for f in file file2 sub/sub\n do\n-- \n1.8.3.rc2.285.gfc18c2c\n"},{"id":"217754","messageId":"7vhai1xx54.fsf@alter.siamese.dyndns.org","threadId":"33854","inReplyTo":"20130517182608.GB27005@serenity.lan","subject":"Re: [PATCH v2] difftool: fix dir-diff when file does not exist in working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-17T18:46:15Z","receivedAt":"2013-05-17T18:46:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> It can't ever happen because we only call this if the mode is non-zero\n> in which case the SHA-1 is only null if there are unstaged changes.\n>\n> However, I think this revised version is much clearer.\n\nYes, that makes the intention crystal clear.  I like it.\n\n>\n>  git-difftool.perl   | 6 ++++++\n>  t/t7800-difftool.sh | 7 +++++++\n>  2 files changed, 13 insertions(+)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 6780292..8a75205 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -92,6 +92,12 @@ sub use_wt_file\n>  \t\treturn 0;\n>  \t}\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> +\t\treturn (0, $null_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> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index a6bd99e..d46f041 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n>  \t)\n>  '\n>  \n> +run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '\n> +\ttest_when_finished git reset --hard &&\n> +\trm file2 &&\n> +\tgit difftool --dir-diff $symlinks --extcmd ls branch master >output &&\n> +\tgrep file2 output\n> +'\n> +\n>  write_script .git/CHECK_SYMLINKS <<\\EOF\n>  for f in file file2 sub/sub\n>  do\n"}]}