Message ID | 9bb8d84ea956dcddefbe7b62baa3a5ff23b6b1e2.1593107621.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fix difftool problem with intent-to-add files | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In https://github.com/git-for-windows/git/issues/2677, a `git difftool > -d` problem was reported. The underlying cause was a bug in `git > diff-files --raw` that we just fixed. That leaves a gap between "there is some unspecified problem" and "the problem was cased by such and such" that forces readers to either know the problem at heart (may apply to you and me right now, but I am not sure about me 3 months in the future) or go check the external website. Can we fill the gap by saying how seeing the object name of empty blob (or worse, tree) instead of 0{40} made "difftool -d" upset? Thanks. > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > t/t7800-difftool.sh | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index 29b92907e2..524f30f7dc 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' > test_cmp expect actual > ' > > +test_expect_success 'add -N and difftool -d' ' > + test_when_finished git reset --hard && > + > + test_write_lines A B C >intent-to-add && > + git add -N intent-to-add && > + git difftool --dir-diff --extcmd ls > +' > + > test_expect_success 'outside worktree' ' > echo 1 >1 && > echo 2 >2 &&
Hi Junio, On Thu, 25 Jun 2020, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > In https://github.com/git-for-windows/git/issues/2677, a `git difftool > > -d` problem was reported. The underlying cause was a bug in `git > > diff-files --raw` that we just fixed. > > That leaves a gap between "there is some unspecified problem" and > "the problem was cased by such and such" that forces readers to > either know the problem at heart (may apply to you and me right now, > but I am not sure about me 3 months in the future) or go check the > external website. > > Can we fill the gap by saying how seeing the object name of empty > blob (or worse, tree) instead of 0{40} made "difftool -d" upset? Sorry about catching this only now, after the commit hit `next`. Filling the gap is a slightly more complicated. And now that I looked at the code again, to make sure that I don't say anything stupid, I realize that I just provided incorrect information in my reply elsewhere in this thread: Srinidhi's fix is _not_ enough to fix t7800 with this here patch. Your guess was almost spot on: the empty blob would have worked (as in: not caused an error, but it would have shown incorrect information). The problem really is the attempt trying to read the empty tree as if it was a blob. That results in something like this: error: unable to read sha1 file of /tmp/git-difftool.O8CoK9/right/intent-to-add (4b825dc642cb6eb9a060e54bf8d69288fbee4904) error: could not write 'intent-to-add' And yes, it would have been good to adjust the commit message as you suggested. Sorry for not getting to it in time before it hit `next`. Do you want me to send out a v5 and drop v4 from `next` in favor of the new iteration? Ciao, Dscho > > Thanks. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > t/t7800-difftool.sh | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > > index 29b92907e2..524f30f7dc 100755 > > --- a/t/t7800-difftool.sh > > +++ b/t/t7800-difftool.sh > > @@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' > > test_cmp expect actual > > ' > > > > +test_expect_success 'add -N and difftool -d' ' > > + test_when_finished git reset --hard && > > + > > + test_write_lines A B C >intent-to-add && > > + git add -N intent-to-add && > > + git difftool --dir-diff --extcmd ls > > +' > > + > > test_expect_success 'outside worktree' ' > > echo 1 >1 && > > echo 2 >2 && >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Can we fill the gap by saying how seeing the object name of empty >> blob (or worse, tree) instead of 0{40} made "difftool -d" upset? > > Sorry about catching this only now, after the commit hit `next`. > > Filling the gap is a slightly more complicated. > > And now that I looked at the code again, to make sure that I don't say > anything stupid, I realize that I just provided incorrect information in > my reply elsewhere in this thread: Srinidhi's fix is _not_ enough to fix > t7800 with this here patch. > > Your guess was almost spot on: the empty blob would have worked (as in: > not caused an error, but it would have shown incorrect information). The > problem really is the attempt trying to read the empty tree as if it was a > blob. That results in something like this: > > error: unable to read sha1 file of /tmp/git-difftool.O8CoK9/right/intent-to-add (4b825dc642cb6eb9a060e54bf8d69288fbee4904) > error: could not write 'intent-to-add' > > And yes, it would have been good to adjust the commit message as you > suggested. Sorry for not getting to it in time before it hit `next`. > > Do you want me to send out a v5 and drop v4 from `next` in favor of the > new iteration? That would help the future "us" quite a lot. Thanks for carefully thinking it through.
Hi Junio, On Wed, 1 Jul 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> Can we fill the gap by saying how seeing the object name of empty > >> blob (or worse, tree) instead of 0{40} made "difftool -d" upset? > > > > Sorry about catching this only now, after the commit hit `next`. > > > > Filling the gap is a slightly more complicated. > > > > And now that I looked at the code again, to make sure that I don't say > > anything stupid, I realize that I just provided incorrect information in > > my reply elsewhere in this thread: Srinidhi's fix is _not_ enough to fix > > t7800 with this here patch. > > > > Your guess was almost spot on: the empty blob would have worked (as in: > > not caused an error, but it would have shown incorrect information). The > > problem really is the attempt trying to read the empty tree as if it was a > > blob. That results in something like this: > > > > error: unable to read sha1 file of /tmp/git-difftool.O8CoK9/right/intent-to-add (4b825dc642cb6eb9a060e54bf8d69288fbee4904) > > error: could not write 'intent-to-add' > > > > And yes, it would have been good to adjust the commit message as you > > suggested. Sorry for not getting to it in time before it hit `next`. > > > > Do you want me to send out a v5 and drop v4 from `next` in favor of the > > new iteration? > > That would help the future "us" quite a lot. Thanks for carefully > thinking it through. You're welcome. Here goes v5: https://lore.kernel.org/git/pull.654.v5.git.1593638347.gitgitgadget@gmail.com/ Ciao, Dscho
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 29b92907e2..524f30f7dc 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' test_cmp expect actual ' +test_expect_success 'add -N and difftool -d' ' + test_when_finished git reset --hard && + + test_write_lines A B C >intent-to-add && + git add -N intent-to-add && + git difftool --dir-diff --extcmd ls +' + test_expect_success 'outside worktree' ' echo 1 >1 && echo 2 >2 &&