[PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Srinidhi Kaushik]

The `diff-files' command and related commands which call `cmd_diff_files()',
consider the "intent-to-add" files as a part of the index when comparing the
work-tree against it. This was previously addressed in [1] and [2] by turning
the option `--ita-invisible-in-index' (introduced in [3]) on by default.

For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as
as new, `ita_invisible_in_index' will be enabled by default here as well.

[1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)
[2] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in
                index", 2016-10-24)
[3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)

Signed-off-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
---

Hello! This is my first patch in this project.
This issue was mentioned in #leftoverbits on GitHub: [1], and this
patch implements the change proposed in [2].

[1] https://github.com/gitgitgadget/git/issues/647
[2] https://lore.kernel.org/git/20200527230357.GB546534@coredump.intra.peff.net


 builtin/diff-files.c  |  7 +++++++
 t/t2203-add-intent.sh | 25 ++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 86ae474fbf..1e352dd8f7 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -28,6 +28,13 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
+
+	/*
+	 * Consider "intent-to-add" files as new by default, unless
+	 * explicitly specified in the command line or anywhere else.
+	 */
+	rev.diffopt.ita_invisible_in_index = 1;
+
 	precompose_argv(argc, argv);
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 5bbe8dcce4..742f27a935 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -232,7 +232,7 @@ test_expect_success 'double rename detection in status' '
 	)
 '
 
-test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
+test_expect_success 'diff/diff-cached shows ita as new/not-new files' '
 	git reset --hard &&
 	echo new >new-ita &&
 	git add -N new-ita &&
@@ -243,6 +243,29 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
 	test_must_be_empty actual2
 '
 
+test_expect_success 'diff-files shows i-t-a files as new files' '
+	git reset --hard &&
+	touch empty &&
+	content="foo" &&
+	echo $content >not-empty &&
+	git add -N empty not-empty &&
+	git diff-files -p >actual &&
+	hash_e=$(git hash-object empty) &&
+	hash_n=$(git hash-object not-empty) &&
+	cat >expect <<-EOF &&
+	diff --git a/empty b/empty
+	new file mode 100644
+	index 0000000..$(git rev-parse --short $hash_e)
+	diff --git a/not-empty b/not-empty
+	new file mode 100644
+	index 0000000..$(git rev-parse --short $hash_n)
+	--- /dev/null
+	+++ b/not-empty
+	@@ -0,0 +1 @@
+	+$content
+	EOF
+	test_cmp expect actual
+'
 
 test_expect_success '"diff HEAD" includes ita as new files' '
 	git reset --hard &&
-- 
2.27.0

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Junio C Hamano]

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

> The `diff-files' command and related commands which call `cmd_diff_files()',
> consider the "intent-to-add" files as a part of the index when comparing the
> work-tree against it. This was previously addressed in [1] and [2] by turning
> the option `--ita-invisible-in-index' (introduced in [3]) on by default.
>
> For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as
> as new, `ita_invisible_in_index' will be enabled by default here as well.
>
> [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)
> [2] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in
>                 index", 2016-10-24)
> [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)

Is there any place where we still run the internal diff machinery to
compare the index and the working tree without setting the
ita_invisible_in_index bit on with this patch applied, and if so,
why?  Does the justification why that other place needs to leave
the bit off apply to this codepath as well?

What I am trying to get at is if this is helping only one usecase
for "diff-files" while breaking other usecases.

On the other hand, if there is no longer anybody who wants
ita_invisible_in_index off, perhaps we can get rid of the bit and
lose many conditionals.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Srinidhi Kaushik]

Thanks for replying!

On Thu, Jun 11, 2020 at 01:27:22PM -0700, Junio C Hamano wrote:
> Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:
> 
> > The `diff-files' command and related commands which call `cmd_diff_files()',
> > consider the "intent-to-add" files as a part of the index when comparing the
> > work-tree against it. This was previously addressed in [1] and [2] by turning
> > the option `--ita-invisible-in-index' (introduced in [3]) on by default.
> >
> > For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as
> > as new, `ita_invisible_in_index' will be enabled by default here as well.
> >
> > [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)
> > [2] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in
> >                 index", 2016-10-24)
> > [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)
> 
> Is there any place where we still run the internal diff machinery to
> compare the index and the working tree without setting the
> ita_invisible_in_index bit on with this patch applied, and if so,
> why?  Does the justification why that other place needs to leave
> the bit off apply to this codepath as well?

Yes, I believe that there exist some use cases for `ita_invisible_in_index'
to be unset. For instance, `index_differs_from' which is used in a quite a
few places -- like "commit", "revert", and "rebase" -- which require a
"no change" to be returned.

This commit: [1] addressed the issue where the cache-tree was producing
the same tree as the current commit when it involved "intent-to-add"
entries, instead of aborting.

[1] 018ec3c820 (commit: fix empty commit creation when there's no changes
                but ita entries, 2016-10-24) 

> What I am trying to get at is if this is helping only one usecase
> for "diff-files" while breaking other usecases.

Currently, `run_add_p' (for "add"; which this patch addresses
the fix), and `push_to_deploy' (in "receive-pack"; where this
is the intended behavior), call "diff-files" as a subprocess,
in which case the `ita_invisible_in_index' bit is explicitly
set. For all other cases, calls are made directly
to `run_diff_files' and will be unaffected by this change.
 
> On the other hand, if there is no longer anybody who wants
> ita_invisible_in_index off, perhaps we can get rid of the bit and
> lose many conditionals.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Srinidhi Kaushik]

Hello,
Is there any update on this patch?
Please let me know if I missed anything.

Thanks!

On 06/12/2020 04:58, Srinidhi Kaushik wrote:
> Thanks for replying!
> 
> On Thu, Jun 11, 2020 at 01:27:22PM -0700, Junio C Hamano wrote:
> > Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:
> > 
> > > The `diff-files' command and related commands which call `cmd_diff_files()',
> > > consider the "intent-to-add" files as a part of the index when comparing the
> > > work-tree against it. This was previously addressed in [1] and [2] by turning
> > > the option `--ita-invisible-in-index' (introduced in [3]) on by default.
> > >
> > > For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as
> > > as new, `ita_invisible_in_index' will be enabled by default here as well.
> > >
> > > [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)
> > > [2] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist in
> > >                 index", 2016-10-24)
> > > [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)
> > 
> > Is there any place where we still run the internal diff machinery to
> > compare the index and the working tree without setting the
> > ita_invisible_in_index bit on with this patch applied, and if so,
> > why?  Does the justification why that other place needs to leave
> > the bit off apply to this codepath as well?
> 
> Yes, I believe that there exist some use cases for `ita_invisible_in_index'
> to be unset. For instance, `index_differs_from' which is used in a quite a
> few places -- like "commit", "revert", and "rebase" -- which require a
> "no change" to be returned.
> 
> This commit: [1] addressed the issue where the cache-tree was producing
> the same tree as the current commit when it involved "intent-to-add"
> entries, instead of aborting.
> 
> [1] 018ec3c820 (commit: fix empty commit creation when there's no changes
>                 but ita entries, 2016-10-24) 
> 
> > What I am trying to get at is if this is helping only one usecase
> > for "diff-files" while breaking other usecases.
> 
> Currently, `run_add_p' (for "add"; which this patch addresses
> the fix), and `push_to_deploy' (in "receive-pack"; where this
> is the intended behavior), call "diff-files" as a subprocess,
> in which case the `ita_invisible_in_index' bit is explicitly
> set. For all other cases, calls are made directly
> to `run_diff_files' and will be unaffected by this change.
>  
> > On the other hand, if there is no longer anybody who wants
> > ita_invisible_in_index off, perhaps we can get rid of the bit and
> > lose many conditionals.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Junio C Hamano]

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

> Hello,
> Is there any update on this patch?
> Please let me know if I missed anything.

Sorry, the patch totally fell through the cracks.

I just sent out a brief review on the part I didn't review during
the first round.

Thanks.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Junio C Hamano]

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
> +test_expect_success 'diff/diff-cached shows ita as new/not-new files' '
>  	git reset --hard &&
>  	echo new >new-ita &&
>  	git add -N new-ita &&

Interesting.  

I thought that the test originally tested "diff-files" and
"diff-index --cached" and a modernization patch forgot to update the
title when the test body was changed to use "diff" and "diff
--cached", but that is not the case here.  When 0231ae71 (diff: turn
--ita-invisible-in-index on by default, 2018-05-26) added this test,
it gave a wrong title from the beginning.

Nice catch.

> @@ -243,6 +243,29 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
>  	test_must_be_empty actual2
>  '
>  
> +test_expect_success 'diff-files shows i-t-a files as new files' '
> +	git reset --hard &&
> +	touch empty &&

Use of "touch" gives a wrong impression that you care about the file
timestamp; use something like ": >empty &&" instead when you care
about the presence of the file and do not care about its timestamp.

> +	content="foo" &&
> +	echo $content >not-empty &&

The quoting decision is backwards in these two lines.  It is OK not
to quote when the right hand side literal is clearly a single word
without $IFS.  On the other hand, it is a good practice to always
quote when using what is in a "$variable".

> +	git add -N empty not-empty &&
> +	git diff-files -p >actual &&
> +	hash_e=$(git hash-object empty) &&
> +	hash_n=$(git hash-object not-empty) &&
> +	cat >expect <<-EOF &&
> +	diff --git a/empty b/empty
> +	new file mode 100644
> +	index 0000000..$(git rev-parse --short $hash_e)
> +	diff --git a/not-empty b/not-empty
> +	new file mode 100644
> +	index 0000000..$(git rev-parse --short $hash_n)
> +	--- /dev/null
> +	+++ b/not-empty
> +	@@ -0,0 +1 @@
> +	+$content
> +	EOF
> +	test_cmp expect actual
> +'

OK.  Do we want to show what happens when "diff" and "diff --cached"
are run with these two "added but not quite added yet" paths to
contrast with this new case?

Thanks.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	touch empty &&
>
> Use of "touch" gives a wrong impression that you care about the file
> timestamp; use something like ": >empty &&" instead when you care
> about the presence of the file and do not care about its timestamp.

I just realized that this is even more important in this case not to
use "touch".

The test that uses this file cares not just the presence, but it
deeply cares that its contents is empty.  The thing it least cares
about is its timestamp.

The purpose of using "touch" is to update the timestamp, to keep the
current contents if it exists, and to ensure it exists (as a side
effect), in the decreasing order of importance.  Use of the command
here misleads the readers.

Thanks.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Srinidhi Kaushik]

Thank you for reviewing this; I appreciate it!

> > +     content="foo" &&
> > +     echo $content >not-empty &&
>
> The quoting decision is backwards in these two lines.  It is OK not
> to quote when the right hand side literal is clearly a single word
> without $IFS.  On the other hand, it is a good practice to always
> quote when using what is in a "$variable".

Yes, that doesn't look right, I will make changes in v2.

[...]
> > > +  touch empty &&
> >
> > Use of "touch" gives a wrong impression that you care about the file
> > timestamp; use something like ": >empty &&" instead when you care
> > about the presence of the file and do not care about its timestamp.
>
> I just realized that this is even more important in this case not to
> use "touch".
>
> The test that uses this file cares not just the presence, but it
> deeply cares that its contents is empty.  The thing it least cares
> about is its timestamp.
>
> The purpose of using "touch" is to update the timestamp, to keep the
> current contents if it exists, and to ensure it exists (as a side
> effect), in the decreasing order of importance.  Use of the command
> here misleads the readers.

Oops, you are right. That makes sense. Will update to ": >empty".

[...]
> > +     git add -N empty not-empty &&
> > +     git diff-files -p >actual &&
> > +     hash_e=$(git hash-object empty) &&
> > +     hash_n=$(git hash-object not-empty) &&
> > +     cat >expect <<-EOF &&
> > +     diff --git a/empty b/empty
> > +     new file mode 100644
> > +     index 0000000..$(git rev-parse --short $hash_e)
> > +     diff --git a/not-empty b/not-empty
> > +     new file mode 100644
> > +     index 0000000..$(git rev-parse --short $hash_n)
> > +     --- /dev/null
> > +     +++ b/not-empty
> > +     @@ -0,0 +1 @@
> > +     +$content
> > +     EOF
> > +     test_cmp expect actual
> > +'
>
> OK.  Do we want to show what happens when "diff" and "diff --cached"
> are run with these two "added but not quite added yet" paths to
> contrast with this new case?

I'm not sure if we want to repeat an older test. The test (which was
renamed in this patch) in t2203-add-intent.sh: "diff/diff-cached shows
ita as new/not-new files" is already doing that. Should  the "diff" and
"diff --cached" steps be appended here again?

Thanks.

Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"

[Junio C Hamano]

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

>> > +     git add -N empty not-empty &&
>> > +     git diff-files -p >actual &&
>> > +     hash_e=$(git hash-object empty) &&
>> > +     hash_n=$(git hash-object not-empty) &&
>> > +     cat >expect <<-EOF &&
>> > +     diff --git a/empty b/empty
>> > +     new file mode 100644
>> > +     index 0000000..$(git rev-parse --short $hash_e)
>> > +     diff --git a/not-empty b/not-empty
>> > +     new file mode 100644
>> > +     index 0000000..$(git rev-parse --short $hash_n)
>> > +     --- /dev/null
>> > +     +++ b/not-empty
>> > +     @@ -0,0 +1 @@
>> > +     +$content
>> > +     EOF
>> > +     test_cmp expect actual
>> > +'
>>
>> OK.  Do we want to show what happens when "diff" and "diff --cached"
>> are run with these two "added but not quite added yet" paths to
>> contrast with this new case?
>
> I'm not sure if we want to repeat an older test. The test (which was
> renamed in this patch) in t2203-add-intent.sh: "diff/diff-cached shows
> ita as new/not-new files" is already doing that. Should  the "diff" and
> "diff --cached" steps be appended here again?

No, there is no need to repeat essentially the same test that exists
elsewhere.  I wonder if it reduces duplication even further if we
extend that existing test that checks "diff" and "diff --cached" so
that it also checks "diff-files" as well?, instead of adding this
new one?  The existing one checks diff and diff-cached only with a
new non-empty path, and it can use tests with a new empty path at
the same time, with a unified "set up" code that is in the early
part of the test, e.g.

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 5bbe8dcce4..cfde790ac7 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -235,7 +235,8 @@ test_expect_success 'double rename detection in status' '
 test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
 	git reset --hard &&
 	echo new >new-ita &&
-	git add -N new-ita &&
+	: >new-ita-empty &&
+	git add -N new-ita new-ita-empty &&
 	git diff --summary >actual &&
 	...

Then the existing tests can be updated to see not just --summary but
also for the contents like you did in the new test---and another test
that examines what "git diff-files" sees (which is what you added)
can happen in the same test (that way, the same set-up can be reused
for three tests).

Thanks.

[PATCH v2] diff-files: treat "i-t-a" files as "not-in-index"

[Srinidhi Kaushik]

The `diff-files' command and related commands which call the function
`cmd_diff_files()', consider the "intent-to-add" files as a part of the
index when comparing the work-tree against it. This was previously
addressed in commits [1] and [2] by turning the option
`--ita-invisible-in-index' (introduced in [3]) on by default.

For `diff-files' (and `add -p' as a consequence) to show the i-t-a
files as as new, `ita_invisible_in_index' will be enabled by default
here as well.

[1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default,
                2018-05-26)
[2] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
                in index", 2016-10-24)
[3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)

Signed-off-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
---

Summary of changes since v1:
  - Combine the existing "diff", "diff --cached" test with the new one.
  - Fix quoting, empty file creation in tests.
  - Add an additional test for "diff-files --abbrev".

 builtin/diff-files.c  |  7 ++++++
 t/t2203-add-intent.sh | 53 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 86ae474fbf..1e352dd8f7 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -28,6 +28,13 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
+
+	/*
+	 * Consider "intent-to-add" files as new by default, unless
+	 * explicitly specified in the command line or anywhere else.
+	 */
+	rev.diffopt.ita_invisible_in_index = 1;
+
 	precompose_argv(argc, argv);

 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 5bbe8dcce4..8a5d55054f 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -232,17 +232,54 @@ test_expect_success 'double rename detection in status' '
 	)
 '

-test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
+test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new for "diff --cached"' '
 	git reset --hard &&
-	echo new >new-ita &&
-	git add -N new-ita &&
+	: >empty &&
+	content="foo" &&
+	echo "$content" >not-empty &&
+
+	hash_e=$(git hash-object empty) &&
+	hash_n=$(git hash-object not-empty) &&
+	hash_t=$(git hash-object -t tree /dev/null) &&
+
+	cat >expect.diff_p <<-EOF &&
+	diff --git a/empty b/empty
+	new file mode 100644
+	index 0000000..$(git rev-parse --short $hash_e)
+	diff --git a/not-empty b/not-empty
+	new file mode 100644
+	index 0000000..$(git rev-parse --short $hash_n)
+	--- /dev/null
+	+++ b/not-empty
+	@@ -0,0 +1 @@
+	+$content
+	EOF
+	cat >expect.diff_s <<-EOF &&
+	 create mode 100644 empty
+	 create mode 100644 not-empty
+	EOF
+	cat >expect.diff_a <<-EOF &&
+	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")empty
+	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")not-empty
+	EOF
+
+	git add -N empty not-empty &&
+
+	git diff >actual &&
+	test_cmp expect.diff_p actual &&
+
 	git diff --summary >actual &&
-	echo " create mode 100644 new-ita" >expected &&
-	test_cmp expected actual &&
-	git diff --cached --summary >actual2 &&
-	test_must_be_empty actual2
-'
+	test_cmp expect.diff_s actual &&
+
+	git diff-files -p >actual &&
+	test_cmp expect.diff_p actual &&

+	git diff-files --abbrev >actual &&
+	test_cmp expect.diff_a actual &&
+
+	git diff --cached >actual &&
+	test_must_be_empty actual
+'

 test_expect_success '"diff HEAD" includes ita as new files' '
 	git reset --hard &&
--
2.27.0

Re: [PATCH v2] diff-files: treat "i-t-a" files as "not-in-index"

[Junio C Hamano]

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

> The `diff-files' command and related commands which call the function
> `cmd_diff_files()', consider the "intent-to-add" files as a part of the
> index when comparing the work-tree against it. This was previously
> addressed in commits [1] and [2] by turning the option
> `--ita-invisible-in-index' (introduced in [3]) on by default.
>
> For `diff-files' (and `add -p' as a consequence) to show the i-t-a
> files as as new, `ita_invisible_in_index' will be enabled by default
> here as well.
>
> [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default,
>                 2018-05-26)
> [2] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
>                 in index", 2016-10-24)
> [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)
>
> Signed-off-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
> ---

Thanks.

> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
> +test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new for "diff --cached"' '
>  	git reset --hard &&
> +	: >empty &&
> +	content="foo" &&
> +	echo "$content" >not-empty &&
> +
> +	hash_e=$(git hash-object empty) &&
> +	hash_n=$(git hash-object not-empty) &&
> +	hash_t=$(git hash-object -t tree /dev/null) &&
> +
> +	cat >expect.diff_p <<-EOF &&
> +	diff --git a/empty b/empty
> +	new file mode 100644
> +	index 0000000..$(git rev-parse --short $hash_e)
> +	diff --git a/not-empty b/not-empty
> +	new file mode 100644
> +	index 0000000..$(git rev-parse --short $hash_n)
> +	--- /dev/null
> +	+++ b/not-empty
> +	@@ -0,0 +1 @@
> +	+$content
> +	EOF
> +	cat >expect.diff_s <<-EOF &&
> +	 create mode 100644 empty
> +	 create mode 100644 not-empty
> +	EOF
> +	cat >expect.diff_a <<-EOF &&
> +	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")empty
> +	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")not-empty

This is good, but just FYI you did not have to use printf --- "<<-"
treats only leading tab specially, and not in the middle of a line.

> +	EOF
> +
> +	git add -N empty not-empty &&

OK.  So with two paths...

> +
> +	git diff >actual &&
> +	test_cmp expect.diff_p actual &&

... the patch output of "git diff", and

>  	git diff --summary >actual &&

... the summary part, and

> -	echo " create mode 100644 new-ita" >expected &&
> -	test_cmp expected actual &&
> -	git diff --cached --summary >actual2 &&
> -	test_must_be_empty actual2
> -'
> +	test_cmp expect.diff_s actual &&
> +
> +	git diff-files -p >actual &&
> +	test_cmp expect.diff_p actual &&
>
> +	git diff-files --abbrev >actual &&
> +	test_cmp expect.diff_a actual &&

... the same for "diff-files" and

> +	git diff --cached >actual &&

... "diff -cached" are all checked.

Looking good.

> +	test_must_be_empty actual
> +'
>
>  test_expect_success '"diff HEAD" includes ita as new files' '
>  	git reset --hard &&
> --
> 2.27.0

Re: [PATCH v2] diff-files: treat "i-t-a" files as "not-in-index"

[Johannes Schindelin]

Hi Srinidhi,

On Sat, 20 Jun 2020, Srinidhi Kaushik wrote:

> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 5bbe8dcce4..8a5d55054f 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -232,17 +232,54 @@ test_expect_success 'double rename detection in status' '
>  	)
>  '
>
> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
> +test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new for "diff --cached"' '
>  	git reset --hard &&
> -	echo new >new-ita &&
> -	git add -N new-ita &&
> +	: >empty &&
> +	content="foo" &&
> +	echo "$content" >not-empty &&
> +
> +	hash_e=$(git hash-object empty) &&
> +	hash_n=$(git hash-object not-empty) &&
> +	hash_t=$(git hash-object -t tree /dev/null) &&

So this is the hash of the empty tree object, and...

> +
> +	cat >expect.diff_p <<-EOF &&
> +	diff --git a/empty b/empty
> +	new file mode 100644
> +	index 0000000..$(git rev-parse --short $hash_e)
> +	diff --git a/not-empty b/not-empty
> +	new file mode 100644
> +	index 0000000..$(git rev-parse --short $hash_n)
> +	--- /dev/null
> +	+++ b/not-empty
> +	@@ -0,0 +1 @@
> +	+$content
> +	EOF
> +	cat >expect.diff_s <<-EOF &&
> +	 create mode 100644 empty
> +	 create mode 100644 not-empty
> +	EOF
> +	cat >expect.diff_a <<-EOF &&
> +	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")empty
> +	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")not-empty

... here we expect `git diff --raw` to claim that the contents of `empty`
and of `non-empty` are actually the empty tree.

The underlying problem is that some time ago, the (already incorrect)
empty blob constant was replaced by the empty tree constant, by mistake. I
contributed a patch series to fix that, and Cc:ed you you in v2 that I
sent out earlier today.

It would be helpful if you reviewed it carefully, as you are already
familiar with the code in question.

Thank you,
Johannes

> +	EOF
> +
> +	git add -N empty not-empty &&
> +
> +	git diff >actual &&
> +	test_cmp expect.diff_p actual &&
> +
>  	git diff --summary >actual &&
> -	echo " create mode 100644 new-ita" >expected &&
> -	test_cmp expected actual &&
> -	git diff --cached --summary >actual2 &&
> -	test_must_be_empty actual2
> -'
> +	test_cmp expect.diff_s actual &&
> +
> +	git diff-files -p >actual &&
> +	test_cmp expect.diff_p actual &&
>
> +	git diff-files --abbrev >actual &&
> +	test_cmp expect.diff_a actual &&
> +
> +	git diff --cached >actual &&
> +	test_must_be_empty actual
> +'
>
>  test_expect_success '"diff HEAD" includes ita as new files' '
>  	git reset --hard &&
> --
> 2.27.0
>
>