[v4,2/2] difftool -d: ensure that intent-to-add files are handled correctly

Commit Message

Linus Arver via GitGitGadget June 25, 2020, 5:53 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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.

Make sure that the reported `difftool` problem stays fixed.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 t/t7800-difftool.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Junio C Hamano June 25, 2020, 6:11 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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 July 1, 2020, 10:01 a.m. UTC | #2

Hi Junio,

On Thu, 25 Jun 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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 &&
>

Junio C Hamano July 1, 2020, 9:03 p.m. UTC | #3

Johannes Schindelin <[e-mail]> 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.

Johannes Schindelin July 1, 2020, 9:20 p.m. UTC | #4

Hi Junio,

On Wed, 1 Jul 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> 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/[e-mail]/

Ciao,
Dscho

Patch

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 &&