* [PATCH] difftool: fix dir-diff when file does not exist in working tree @ 2013-05-17 17:29 John Keeping 2013-05-17 18:10 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: John Keeping @ 2013-05-17 17:29 UTC (permalink / raw) To: git; +Cc: Kevin Bracey, Junio C Hamano, John Keeping Commit 02c5631 (difftool --dir-diff: symlink all files matching the working tree, 2013-03-14) does not handle the case where a file that is being compared does not exist in the working tree. Fix this by checking for existence explicitly before running git-hash-object. Reported-by: Kevin Bracey <kevin@bracey.fi> Signed-off-by: John Keeping <john@keeping.me.uk> --- This fixes a regression in 1.8.3-rc0. git-difftool.perl | 9 ++++++++- t/t7800-difftool.sh | 7 +++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/git-difftool.perl b/git-difftool.perl index 6780292..0a1cb0a 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -92,7 +92,14 @@ sub use_wt_file return 0; } - my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); + my $wt_sha1; + if (-e "$workdir/$file") { + $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); + } else { + # If the file doesn't exist in the working tree, use something + # that cannot match a SHA-1. + $wt_sha1 = ''; + }; my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1); return ($use, $wt_sha1); } diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index a6bd99e..d46f041 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' ' ) ' +run_dir_diff_test 'difftool --dir-diff when worktree file is missing' ' + test_when_finished git reset --hard && + rm file2 && + git difftool --dir-diff $symlinks --extcmd ls branch master >output && + grep file2 output +' + write_script .git/CHECK_SYMLINKS <<\EOF for f in file file2 sub/sub do -- 1.8.3.rc2.285.gfc18c2c ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] difftool: fix dir-diff when file does not exist in working tree 2013-05-17 17:29 [PATCH] difftool: fix dir-diff when file does not exist in working tree John Keeping @ 2013-05-17 18:10 ` Junio C Hamano 2013-05-17 18:26 ` [PATCH v2] " John Keeping 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2013-05-17 18:10 UTC (permalink / raw) To: John Keeping; +Cc: git, Kevin Bracey John Keeping <john@keeping.me.uk> writes: > Commit 02c5631 (difftool --dir-diff: symlink all files matching the > working tree, 2013-03-14) does not handle the case where a file that is > being compared does not exist in the working tree. Fix this by checking > for existence explicitly before running git-hash-object. > > Reported-by: Kevin Bracey <kevin@bracey.fi> > Signed-off-by: John Keeping <john@keeping.me.uk> > --- > This fixes a regression in 1.8.3-rc0. > > git-difftool.perl | 9 ++++++++- > t/t7800-difftool.sh | 7 +++++++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/git-difftool.perl b/git-difftool.perl > index 6780292..0a1cb0a 100755 > --- a/git-difftool.perl > +++ b/git-difftool.perl > @@ -92,7 +92,14 @@ sub use_wt_file > return 0; > } > > - my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); > + my $wt_sha1; > + if (-e "$workdir/$file") { > + $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); > + } else { > + # If the file doesn't exist in the working tree, use something > + # that cannot match a SHA-1. > + $wt_sha1 = ''; Yuck. "that cannot match" might be a good justification to say "this does not break the next line to set $use and forces it to false", but "when we return false in $use, the value of $wt_sha1 is not used" needs to be said to convince why this is a safe change. But if $sha1 is $null_sha1, we do end up setting $use to true and the caller would stuff the empty $wt_sha1 to form: $wtindex .= "$rmode \$dst_path\0"; Is that what we want to do here, or is it a "will never happen" condition? If the latter, the reason need to be described in this comment (and in the log). Thanks. > + }; > my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1); > return ($use, $wt_sha1); > } > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index a6bd99e..d46f041 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' ' > ) > ' > > +run_dir_diff_test 'difftool --dir-diff when worktree file is missing' ' > + test_when_finished git reset --hard && > + rm file2 && > + git difftool --dir-diff $symlinks --extcmd ls branch master >output && > + grep file2 output > +' > + > write_script .git/CHECK_SYMLINKS <<\EOF > for f in file file2 sub/sub > do ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] difftool: fix dir-diff when file does not exist in working tree 2013-05-17 18:10 ` Junio C Hamano @ 2013-05-17 18:26 ` John Keeping 2013-05-17 18:46 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: John Keeping @ 2013-05-17 18:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Kevin Bracey Commit 02c5631 (difftool --dir-diff: symlink all files matching the working tree, 2013-03-14) does not handle the case where a file that is being compared does not exist in the working tree. Fix this by checking for existence explicitly before running git-hash-object. Reported-by: Kevin Bracey <kevin@bracey.fi> Signed-off-by: John Keeping <john@keeping.me.uk> --- On Fri, May 17, 2013 at 11:10:40AM -0700, Junio C Hamano wrote: > John Keeping <john@keeping.me.uk> writes: > > > Commit 02c5631 (difftool --dir-diff: symlink all files matching the > > working tree, 2013-03-14) does not handle the case where a file that is > > being compared does not exist in the working tree. Fix this by checking > > for existence explicitly before running git-hash-object. > > > > Reported-by: Kevin Bracey <kevin@bracey.fi> > > Signed-off-by: John Keeping <john@keeping.me.uk> > > --- > > This fixes a regression in 1.8.3-rc0. > > > > git-difftool.perl | 9 ++++++++- > > t/t7800-difftool.sh | 7 +++++++ > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/git-difftool.perl b/git-difftool.perl > > index 6780292..0a1cb0a 100755 > > --- a/git-difftool.perl > > +++ b/git-difftool.perl > > @@ -92,7 +92,14 @@ sub use_wt_file > > return 0; > > } > > > > - my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); > > + my $wt_sha1; > > + if (-e "$workdir/$file") { > > + $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); > > + } else { > > + # If the file doesn't exist in the working tree, use something > > + # that cannot match a SHA-1. > > + $wt_sha1 = ''; > > Yuck. > > "that cannot match" might be a good justification to say "this does > not break the next line to set $use and forces it to false", but > "when we return false in $use, the value of $wt_sha1 is not used" > needs to be said to convince why this is a safe change. > > But if $sha1 is $null_sha1, we do end up setting $use to true and > the caller would stuff the empty $wt_sha1 to form: > > $wtindex .= "$rmode \$dst_path\0"; > > Is that what we want to do here, or is it a "will never happen" > condition? If the latter, the reason need to be described in this > comment (and in the log). It can't ever happen because we only call this if the mode is non-zero in which case the SHA-1 is only null if there are unstaged changes. However, I think this revised version is much clearer. git-difftool.perl | 6 ++++++ t/t7800-difftool.sh | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/git-difftool.perl b/git-difftool.perl index 6780292..8a75205 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -92,6 +92,12 @@ sub use_wt_file return 0; } + if (! -e "$workdir/$file") { + # If the file doesn't exist in the working tree, we cannot + # use it. + return (0, $null_sha1); + } + my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1); return ($use, $wt_sha1); diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index a6bd99e..d46f041 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' ' ) ' +run_dir_diff_test 'difftool --dir-diff when worktree file is missing' ' + test_when_finished git reset --hard && + rm file2 && + git difftool --dir-diff $symlinks --extcmd ls branch master >output && + grep file2 output +' + write_script .git/CHECK_SYMLINKS <<\EOF for f in file file2 sub/sub do -- 1.8.3.rc2.285.gfc18c2c ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] difftool: fix dir-diff when file does not exist in working tree 2013-05-17 18:26 ` [PATCH v2] " John Keeping @ 2013-05-17 18:46 ` Junio C Hamano 0 siblings, 0 replies; 4+ messages in thread From: Junio C Hamano @ 2013-05-17 18:46 UTC (permalink / raw) To: John Keeping; +Cc: git, Kevin Bracey John Keeping <john@keeping.me.uk> writes: > It can't ever happen because we only call this if the mode is non-zero > in which case the SHA-1 is only null if there are unstaged changes. > > However, I think this revised version is much clearer. Yes, that makes the intention crystal clear. I like it. > > git-difftool.perl | 6 ++++++ > t/t7800-difftool.sh | 7 +++++++ > 2 files changed, 13 insertions(+) > > diff --git a/git-difftool.perl b/git-difftool.perl > index 6780292..8a75205 100755 > --- a/git-difftool.perl > +++ b/git-difftool.perl > @@ -92,6 +92,12 @@ sub use_wt_file > return 0; > } > > + if (! -e "$workdir/$file") { > + # If the file doesn't exist in the working tree, we cannot > + # use it. > + return (0, $null_sha1); > + } > + > my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file"); > my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1); > return ($use, $wt_sha1); > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index a6bd99e..d46f041 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' ' > ) > ' > > +run_dir_diff_test 'difftool --dir-diff when worktree file is missing' ' > + test_when_finished git reset --hard && > + rm file2 && > + git difftool --dir-diff $symlinks --extcmd ls branch master >output && > + grep file2 output > +' > + > write_script .git/CHECK_SYMLINKS <<\EOF > for f in file file2 sub/sub > do ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-17 18:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-17 17:29 [PATCH] difftool: fix dir-diff when file does not exist in working tree John Keeping 2013-05-17 18:10 ` Junio C Hamano 2013-05-17 18:26 ` [PATCH v2] " John Keeping 2013-05-17 18:46 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox