[PATCH 0/3] Fix difftool problem with intent-to-add files

[PATCH 0/3] Fix difftool problem with intent-to-add files

[Johannes Schindelin via GitGitGadget]

This problem was reported in 
https://github.com/git-for-windows/git/issues/2677, but the problem actually
lies with git diff --raw, and it seems that the bug has been with us ever
since--intent-to-add was introduced.

Johannes Schindelin (3):
  diff-files: fix incorrect usage of an empty tree
  diff-files --raw: handle intent-to-add files correctly
  difftool -d: ensure that intent-to-add files are handled correctly

 diff-lib.c             | 16 +++++++++++++++-
 t/t4000-diff-format.sh | 10 ++++++++++
 t/t7800-difftool.sh    |  8 ++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)


base-commit: b3d7a52fac39193503a0b6728771d1bf6a161464
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/654
-- 
gitgitgadget

[PATCH 1/3] diff-files: fix incorrect usage of an empty tree

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
2017-05-30), the OID to use for intent-to-add files was inadvertently
changed from the empty blob to the empty tree. Let's revert that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index 61812f48c27..15bb45776e4 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -220,7 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
-					       the_hash_algo->empty_tree, 0,
+					       the_hash_algo->empty_blob, 0,
 					       ce->name, 0);
 				continue;
 			}
-- 
gitgitgadget

[PATCH 2/3] diff-files --raw: handle intent-to-add files correctly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In `run_diff_files()`, files that have been staged with the intention to
add are queued without a valid OID in the `diff_filepair`.

When the output mode is, say, `DIFF_FORMAT_PATCH`, the
`diff_fill_oid_info()` function, called from `run_diff()`, will remedy
that situation by reading the file contents from disk.

However, when the output mode is `DIFF_FORMAT_RAW`, that does not hold
true, and the output will contain a bogus OID (and the flag `M` for
"modified" instead of the correct `A` for "added").

As a consequence, `git difftool -d` (which relies on `git diff-files
--raw`'s output) does not work correctly.

Let's fix this specifically by imitating `diff_fill_oid_info()`.

Note: we can only do that for diff formats that do not actually need the
file contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to
read the blob contents, but that blob might not have been written to
Git's object database.

This fixes https://github.com/git-for-windows/git/issues/2677

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c             | 14 ++++++++++++++
 t/t4000-diff-format.sh | 10 ++++++++++
 2 files changed, 24 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 15bb45776e4..4af8f811ae8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -223,6 +223,20 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       the_hash_algo->empty_blob, 0,
 					       ce->name, 0);
 				continue;
+			} else if (ce_intent_to_add(ce) &&
+				   !(revs->diffopt.output_format &
+				     ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {
+				struct object_id oid;
+				int ret = lstat(ce->name, &st);
+
+				if (ret < 0)
+					oidclr(&oid);
+				else
+					ret = index_path(istate, &oid,
+						 ce->name, &st, 0);
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       &oid, ret >= 0, ce->name, 0);
+				continue;
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index e5116a76a1c..48ff4e250b5 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -89,4 +89,14 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'
 	test_must_be_empty err
 '
 
+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '
+	echo 123 >ita &&
+	git add -N ita &&
+	printf ":000000 100644 %s %s A\\tita\n" \
+		$ZERO_OID $(git hash-object --stdin <ita) >expect &&
+	git diff-files --raw ita >actual &&
+	test_cmp expect actual
+'
+
+
 test_done
-- 
gitgitgadget

[PATCH 3/3] difftool -d: ensure that intent-to-add files are handled correctly

[Johannes Schindelin via GitGitGadget]

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.

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

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 29b92907e2a..524f30f7dc7 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 &&
-- 
gitgitgadget

[PATCH v2 0/3] Fix difftool problem with intent-to-add files

[Johannes Schindelin via GitGitGadget]

This problem was reported in 
https://github.com/git-for-windows/git/issues/2677, but the problem actually
lies with git diff --raw, and it seems that the bug has been with us ever
since--intent-to-add was introduced.

Changes since v1:

 * Rebased onto sk/diff-files-show-i-t-a-as-new.
 * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve
   the issue (the --raw output still claims the empty blob as the
   post-image, although the difftool symptom "went away").
 * Amended the central patch of this PR to include a fix for the regression
   test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected
   the raw diff to contain the hash of the empty tree object (which is
   incorrect no matter how you turn it: any hash in any raw diff should
   refer to blob objects).
 * Reordered the patches so that the central patch comes first (otherwise,
   the "empty tree" fix would cause a test failure in t2203).

Johannes Schindelin (3):
  diff-files --raw: handle intent-to-add files correctly
  diff-files: fix incorrect usage of an empty tree
  difftool -d: ensure that intent-to-add files are handled correctly

 diff-lib.c             | 16 +++++++++++++++-
 t/t2203-add-intent.sh  |  4 ++--
 t/t4000-diff-format.sh | 10 ++++++++++
 t/t7800-difftool.sh    |  8 ++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)


base-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/654

Range-diff vs v1:

 2:  6d423928512 ! 1:  640e2255508 diff-files --raw: handle intent-to-add files correctly
     @@ Commit message
      
          This fixes https://github.com/git-for-windows/git/issues/2677
      
     +    This patch _also_ fixes the expectations set by the regression test
     +    introduced in feea6946a5b (diff-files: treat "i-t-a" files as
     +    "not-in-index", 2020-06-20).
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## diff-lib.c ##
      @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
     - 					       the_hash_algo->empty_blob, 0,
     + 					       !is_null_oid(&ce->oid),
       					       ce->name, 0);
       				continue;
      +			} else if (ce_intent_to_add(ce) &&
     @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
      +				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
      +					       &oid, ret >= 0, ce->name, 0);
      +				continue;
     - 			}
     + 			} else if (revs->diffopt.ita_invisible_in_index &&
     + 				   ce_intent_to_add(ce)) {
     + 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
     +
     + ## t/t2203-add-intent.sh ##
     +@@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
     + 	 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
     ++	:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
     ++	:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
     + 	EOF
       
     - 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
     + 	git add -N empty not-empty &&
      
       ## t/t4000-diff-format.sh ##
      @@ t/t4000-diff-format.sh: test_expect_success 'git diff-files --patch --no-patch does not show the patch'
 1:  9c96c43f3d7 ! 2:  b9633315a2f diff-files: fix incorrect usage of an empty tree
     @@ Commit message
      
          In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
          2017-05-30), the OID to use for intent-to-add files was inadvertently
     -    changed from the empty blob to the empty tree. Let's revert that.
     +    changed from the empty blob to the empty tree.
     +
     +    Let's revert that.
     +
     +    To be able to do that, we just taught the regression test introduced in
     +    feea6946a5b (diff-files: treat "i-t-a" files as "not-in-index",
     +    2020-06-20) to _not_ expect the raw diff to contain the hash of the
     +    empty tree (we also had to fix the code to actually produce the expected
     +    output, but for the sake of this here patch, that's beside the point).
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
 3:  a1c889e2cd1 = 3:  d2e9f704c9e difftool -d: ensure that intent-to-add files are handled correctly

-- 
gitgitgadget

[PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In `run_diff_files()`, files that have been staged with the intention to
add are queued without a valid OID in the `diff_filepair`.

When the output mode is, say, `DIFF_FORMAT_PATCH`, the
`diff_fill_oid_info()` function, called from `run_diff()`, will remedy
that situation by reading the file contents from disk.

However, when the output mode is `DIFF_FORMAT_RAW`, that does not hold
true, and the output will contain a bogus OID (and the flag `M` for
"modified" instead of the correct `A` for "added").

As a consequence, `git difftool -d` (which relies on `git diff-files
--raw`'s output) does not work correctly.

Let's fix this specifically by imitating `diff_fill_oid_info()`.

Note: we can only do that for diff formats that do not actually need the
file contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to
read the blob contents, but that blob might not have been written to
Git's object database.

This fixes https://github.com/git-for-windows/git/issues/2677

This patch _also_ fixes the expectations set by the regression test
introduced in feea6946a5b (diff-files: treat "i-t-a" files as
"not-in-index", 2020-06-20).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c             | 14 ++++++++++++++
 t/t2203-add-intent.sh  |  4 ++--
 t/t4000-diff-format.sh | 10 ++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 61812f48c27..ea23169afa2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -217,6 +217,20 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       !is_null_oid(&ce->oid),
 					       ce->name, 0);
 				continue;
+			} else if (ce_intent_to_add(ce) &&
+				   !(revs->diffopt.output_format &
+				     ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {
+				struct object_id oid;
+				int ret = lstat(ce->name, &st);
+
+				if (ret < 0)
+					oidclr(&oid);
+				else
+					ret = index_path(istate, &oid,
+						 ce->name, &st, 0);
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       &oid, ret >= 0, ce->name, 0);
+				continue;
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8a5d55054f2..b000a2bdd1d 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -259,8 +259,8 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 	 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
+	:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
+	:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
 	EOF
 
 	git add -N empty not-empty &&
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index e5116a76a1c..48ff4e250b5 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -89,4 +89,14 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'
 	test_must_be_empty err
 '
 
+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '
+	echo 123 >ita &&
+	git add -N ita &&
+	printf ":000000 100644 %s %s A\\tita\n" \
+		$ZERO_OID $(git hash-object --stdin <ita) >expect &&
+	git diff-files --raw ita >actual &&
+	test_cmp expect actual
+'
+
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In `run_diff_files()`, files that have been staged with the intention to
> add are queued without a valid OID in the `diff_filepair`.
>
> When the output mode is, say, `DIFF_FORMAT_PATCH`, the
> `diff_fill_oid_info()` function, called from `run_diff()`, will remedy
> that situation by reading the file contents from disk.

The above is true.  What do we do for a path that is actually added
to the index but is stat-dirty or actually modified in the working
tree when we are giving the raw output?  Don't we give 0{40} to mean
"we dunno---you go look at the working tree"?  I think we should do
the same for i-t-a file wrt the object name.  In both cases, the
index does not know what the actual object name is, and we do not
want to run the index_path() and write out a new object in the
object database.

Using the status letter 'A' would also be appropriate, as we would
show "new file ..." in the --patch output in this case, which would
be consistent.

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Johannes Schindelin]

Hi Junio,

On Tue, 23 Jun 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In `run_diff_files()`, files that have been staged with the intention to
> > add are queued without a valid OID in the `diff_filepair`.
> >
> > When the output mode is, say, `DIFF_FORMAT_PATCH`, the
> > `diff_fill_oid_info()` function, called from `run_diff()`, will remedy
> > that situation by reading the file contents from disk.
>
> The above is true.  What do we do for a path that is actually added
> to the index but is stat-dirty or actually modified in the working
> tree when we are giving the raw output?  Don't we give 0{40} to mean
> "we dunno---you go look at the working tree"?  I think we should do
> the same for i-t-a file wrt the object name.  In both cases, the
> index does not know what the actual object name is, and we do not
> want to run the index_path() and write out a new object in the
> object database.

Sure, but my intention was to synchronize the `--raw` vs the `--patch`
output: the latter _already_ shows the correct hash. This here patch makes
the hash in the former's output match the latter's.

Besides, we're talking about the post-image of `diff-files`, i.e. the
worktree version, here. I seem to remember that the pre-image already uses
the all-zero hash to indicate precisely what you mentioned above.

Ciao,
Dscho

> Using the status letter 'A' would also be appropriate, as we would
> show "new file ..." in the --patch output in this case, which would
> be consistent.
>
>

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Sure, but my intention was to synchronize the `--raw` vs the `--patch`
> output: the latter _already_ shows the correct hash. This here patch makes
> the hash in the former's output match the latter's.

That is shooting for a wrong uniformity and breaking consistency
among the `--raw` modes.

 $ git reset --hard
 $ echo "/* end */" >cache.h ;# taint
 $ git diff-files --raw
 ... this shows (M)odified with 0{40} on the postimage
 ... 0{40} for side that is known to have contents from low-level diff
 ... means "object name unknown; figure it out yourself if you need it"
 $ git update-index cache.h
 $ git diff-files --raw
 ... of course we see nothing here.  Wait for a bit.
 $ touch cache.h ;# smudge
 $ git diff-files --raw
 ... this shows (M)odified with 0{40} on the postimage
 ... again, it says "it is stat dirty so I do not bother to compute"
 $ git update-index --refresh
 $ git diff-files --raw
 ... again we see nothing.

Any tools that work on "--raw" output must be already prepared to
see 0{40} on the side that is known to have contents and must know
to grab the contents from the working tree file if they need them,
so showing the 0{40} for i-t-a entry (whose definition is "the user
said in the past that the final contents of the file will be added
later, but Git does not know what object it will be yet") cannot
break them.  And the behaviour of giving 0{40} in such a case aligns
well with what is already done for paths already added to the index
when Git does not have an already-computed object name handy.

> Besides, we're talking about the post-image of `diff-files`, i.e. the
> worktree version, here. I seem to remember that the pre-image already uses
> the all-zero hash to indicate precisely what you mentioned above.

The 0{40} you see for pre-image for (A)dded paths means a completely
different thing from the 0{40} I have been explaining in the above,
so that is not relevant here.

By definition, there is *no* contents for the pre-image side of
(A)dded paths (that is why I stressed the "side that must have
contents" in the above description---it is determined by the type of
the change), but because the format requires us to place some
hexadecimal there, we fill the space with 0{40}.  

When we do not know the object name for the side that is known to
have contents without performing extra computation (including "stat
dirty so we cannot tell without rehashing"), we also use 0{40} as a
signal to say "we do not know the actual contents", but the consumer
of "--raw" format is expected to know the difference between "this
side is known to have no data and 0{40} is here as filler" and "this
side must have contents but we see 0{40} because Git does not have
it handy in precomputed form".

The above is the same for "diff-index --raw" without "--cached";
when we have to hash before we can give the object name (e.g. the
path is stat-dirty), we give 0{40} and let the consumer figure it
out if it needs to.

 $ git reset --hard
 $ touch COPYING
 $ git diff-index --raw HEAD
 ... we see (M)odified with 0{40} on the right hand side.

When the caller asks for "--patch" or any other output format that
actually needs contents for output, however, these low-level tools
do read the contents, and as a side effect, they may hash to obtain
the object name and show it [*1*].

By the way, as I do not want to see you waste your time going in a
wrong direction just to be "different", let me make it clear that as
far as the design of low level diff plumbing is concerned, what I
said here is final.  Please don't waste your time on arguing for
changing the design now after 15 years.  I want to see your time
used in a more productive way for the project.

Thanks.


[Footnote]

*1* This division of labor to free "--raw" mode of anything remotely
    unnecessary stems from the original diff plumbing design in May
    2005 where the "--raw" mode was the only output mode, and there
    was a separate "git-diff-helper" (look for it in the mailing
    list archive if you want to learn more) that reads a "--raw"
    output and transforms it into the patch form.  That "once we
    have the raw diff, we can pipe it to post-processing and do more
    interesting things" eventually led to the design of the diffcore
    pipeline where we match up (A)dded and (D)eleted entries to
    detect renames, etc.

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Junio C Hamano]

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Sure, but my intention was to synchronize the `--raw` vs the `--patch`
>> output: the latter _already_ shows the correct hash. This here patch makes
>> the hash in the former's output match the latter's.
>
> That is shooting for a wrong uniformity and breaking consistency
> among the `--raw` modes.
> ...
>
> [Footnote]
>
> *1* This division of labor to free "--raw" mode of anything remotely
>     unnecessary stems from the original diff plumbing design in May
>     2005 where the "--raw" mode was the only output mode, and there
>     was a separate "git-diff-helper" (look for it in the mailing
>     list archive if you want to learn more) that reads a "--raw"
>     output and transforms it into the patch form.  That "once we
>     have the raw diff, we can pipe it to post-processing and do more
>     interesting things" eventually led to the design of the diffcore
>     pipeline where we match up (A)dded and (D)eleted entries to
>     detect renames, etc.

Having said all that.

If somebody wants to shift the burden of computing object names from
the consumers of "diff --raw" output to generators like diff-files
and diff-index, I do not mind if it is done under a new command line
option and done consistently for not just I-T-A additions, but
"modified but not added yet" and "not modified but stat information
is dirty" paths.  As I said, I would not recommend it, though.

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Johannes Schindelin]

Hi Junio,

On Wed, 24 Jun 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Sure, but my intention was to synchronize the `--raw` vs the `--patch`
> > output: the latter _already_ shows the correct hash. This here patch makes
> > the hash in the former's output match the latter's.
>
> That is shooting for a wrong uniformity and breaking consistency
> among the `--raw` modes.
>
>  $ git reset --hard
>  $ echo "/* end */" >cache.h ;# taint
>  $ git diff-files --raw
>  ... this shows (M)odified with 0{40} on the postimage
>  ... 0{40} for side that is known to have contents from low-level diff
>  ... means "object name unknown; figure it out yourself if you need it"
>  $ git update-index cache.h
>  $ git diff-files --raw
>  ... of course we see nothing here.  Wait for a bit.
>  $ touch cache.h ;# smudge
>  $ git diff-files --raw
>  ... this shows (M)odified with 0{40} on the postimage
>  ... again, it says "it is stat dirty so I do not bother to compute"
>  $ git update-index --refresh
>  $ git diff-files --raw
>  ... again we see nothing.
>
> Any tools that work on "--raw" output must be already prepared to
> see 0{40} on the side that is known to have contents and must know
> to grab the contents from the working tree file if they need them,
> so showing the 0{40} for i-t-a entry (whose definition is "the user
> said in the past that the final contents of the file will be added
> later, but Git does not know what object it will be yet") cannot
> break them.  And the behaviour of giving 0{40} in such a case aligns
> well with what is already done for paths already added to the index
> when Git does not have an already-computed object name handy.

Well, don't you know, I never realized that the hash shown by `git
diff-files --raw` for modified files was all-zero while `git diff-files
-p` showed the computed one matching the current worktree version!

> > Besides, we're talking about the post-image of `diff-files`, i.e. the
> > worktree version, here. I seem to remember that the pre-image already uses
> > the all-zero hash to indicate precisely what you mentioned above.
>
> The 0{40} you see for pre-image for (A)dded paths means a completely
> different thing from the 0{40} I have been explaining in the above,
> so that is not relevant here.
>
> By definition, there is *no* contents for the pre-image side of
> (A)dded paths (that is why I stressed the "side that must have
> contents" in the above description---it is determined by the type of
> the change), but because the format requires us to place some
> hexadecimal there, we fill the space with 0{40}.
>
> When we do not know the object name for the side that is known to
> have contents without performing extra computation (including "stat
> dirty so we cannot tell without rehashing"), we also use 0{40} as a
> signal to say "we do not know the actual contents", but the consumer
> of "--raw" format is expected to know the difference between "this
> side is known to have no data and 0{40} is here as filler" and "this
> side must have contents but we see 0{40} because Git does not have
> it handy in precomputed form".
>
> The above is the same for "diff-index --raw" without "--cached";
> when we have to hash before we can give the object name (e.g. the
> path is stat-dirty), we give 0{40} and let the consumer figure it
> out if it needs to.
>
>  $ git reset --hard
>  $ touch COPYING
>  $ git diff-index --raw HEAD
>  ... we see (M)odified with 0{40} on the right hand side.
>
> When the caller asks for "--patch" or any other output format that
> actually needs contents for output, however, these low-level tools
> do read the contents, and as a side effect, they may hash to obtain
> the object name and show it [*1*].
>
> By the way, as I do not want to see you waste your time going in a
> wrong direction just to be "different", let me make it clear that as
> far as the design of low level diff plumbing is concerned, what I
> said here is final.  Please don't waste your time on arguing for
> changing the design now after 15 years.  I want to see your time
> used in a more productive way for the project.

Thank you for patienty explaining to me something I managed to miss for a
decade and a half.

I'll send out v4 in a moment.

Ciao,
Dscho

>
> Thanks.
>
>
> [Footnote]
>
> *1* This division of labor to free "--raw" mode of anything remotely
>     unnecessary stems from the original diff plumbing design in May
>     2005 where the "--raw" mode was the only output mode, and there
>     was a separate "git-diff-helper" (look for it in the mailing
>     list archive if you want to learn more) that reads a "--raw"
>     output and transforms it into the patch form.  That "once we
>     have the raw diff, we can pipe it to post-processing and do more
>     interesting things" eventually led to the design of the diffcore
>     pipeline where we match up (A)dded and (D)eleted entries to
>     detect renames, etc.
>
>

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Srinidhi Kaushik]

Hi Johannes,

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

Thanks for CC-ing me!

[...]

> +			} else if (ce_intent_to_add(ce) &&
> +				   !(revs->diffopt.output_format &
> +				     ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {
> +				struct object_id oid;
> +				int ret = lstat(ce->name, &st);
> +
> +				if (ret < 0)
> +					oidclr(&oid);
> +				else
> +					ret = index_path(istate, &oid,
> +						 ce->name, &st, 0);
> +				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> +					       &oid, ret >= 0, ce->name, 0);
> +				continue;

Instead of showing the hash for empty blobs for all entries previously,
introducing this shows the hash of non-empty "i-t-a" files correctly.
Nice.

[...]

>  			} else if (revs->diffopt.ita_invisible_in_index &&
>  				   ce_intent_to_add(ce)) {
>  				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> -					       the_hash_algo->empty_tree, 0,
> +					       the_hash_algo->empty_blob, 0,
>  					       ce->name, 0);
>  				continue;
>  			}

Oh, I totally missed this in my patch; the change looks good!

> -  :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
> +  :000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
> +  :000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty

Changing the test-case to reflect to the hash of the blob also makes
sense.


[...]

> > +     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...

I guess we can get rid of the `hash_t' assignment here, because it
won't be used anywhere else in the test.

Thanks.

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Johannes Schindelin]

Hi Srinidhi,

On Wed, 24 Jun 2020, Srinidhi Kaushik wrote:

> > 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.
>
> Thanks for CC-ing me!

Of course!

> > -  :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
> > +  :000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
> > +  :000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
>
> Changing the test-case to reflect to the hash of the blob also makes
> sense.

Yes, that's the post-image side. The pre-image side is marked with 0000000
(which is Git's way to say "dunno! no current information in the index").

And it is reflecting the `--patch` case that is unfortunately not visible
in the diff context:

        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

(This comment is not so much directed at you as it really is a
continuation of my reply to Junio, see
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2006241517320.54@tvgsbejvaqbjf.bet/)

> [...]
>
> > > +     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...
>
> I guess we can get rid of the `hash_t' assignment here, because it
> won't be used anywhere else in the test.

Right! I _knew_ there was something I wanted to do, still, but then
forgot all about it...

Thanks,
Dscho

Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > +  :000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
>> > +  :000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
>>
>> Changing the test-case to reflect to the hash of the blob also makes
>> sense.
>
> Yes, that's the post-image side. The pre-image side is marked with 0000000
> (which is Git's way to say "dunno! no current information in the index").

The post-image side for (A)dded and (M)odified can have 0{40} when
Git wants to say "we don't have it in precomputed form.  you can
grab the working tree files and figure it out, if you need it", as
you said, but the pre-image side for (A)dded entries are "we know
there is nothing because we are Adding, but we need to have
something here, so we have 0+ as a filler".

[PATCH v2 2/3] diff-files: fix incorrect usage of an empty tree

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
2017-05-30), the OID to use for intent-to-add files was inadvertently
changed from the empty blob to the empty tree.

Let's revert that.

To be able to do that, we just taught the regression test introduced in
feea6946a5b (diff-files: treat "i-t-a" files as "not-in-index",
2020-06-20) to _not_ expect the raw diff to contain the hash of the
empty tree (we also had to fix the code to actually produce the expected
output, but for the sake of this here patch, that's beside the point).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index ea23169afa2..7aafd7cc376 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -234,7 +234,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
-					       the_hash_algo->empty_tree, 0,
+					       the_hash_algo->empty_blob, 0,
 					       ce->name, 0);
 				continue;
 			}
-- 
gitgitgadget

Re: [PATCH v2 2/3] diff-files: fix incorrect usage of an empty tree

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
> 2017-05-30), the OID to use for intent-to-add files was inadvertently
> changed from the empty blob to the empty tree.

Well spotted.  Regardless of the other two patches, this is
absolutely the right thing to do.  It is quite embarrassing
that nobody noticed the typo so far.

[PATCH v2 3/3] difftool -d: ensure that intent-to-add files are handled correctly

[Johannes Schindelin via GitGitGadget]

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.

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

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 29b92907e2a..524f30f7dc7 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 &&
-- 
gitgitgadget

[PATCH v3 0/3] Fix difftool problem with intent-to-add files

[Johannes Schindelin via GitGitGadget]

This problem was reported in 
https://github.com/git-for-windows/git/issues/2677, but the problem actually
lies with git diff --raw, and it seems that the bug has been with us ever
since --intent-to-add was introduced.

Changes since v2:

 * Now we also drop the no-longer-used definition of hash_t in t2203.

Changes since v1:

 * Rebased onto sk/diff-files-show-i-t-a-as-new.
 * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve
   the issue (the --raw output still claims the empty blob as the
   post-image, although the difftool symptom "went away").
 * Amended the central patch of this PR to include a fix for the regression
   test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected
   the raw diff to contain the hash of the empty tree object (which is
   incorrect no matter how you turn it: any hash in any raw diff should
   refer to blob objects).
 * Reordered the patches so that the central patch comes first (otherwise,
   the "empty tree" fix would cause a test failure in t2203).

Johannes Schindelin (3):
  diff-files --raw: handle intent-to-add files correctly
  diff-files: fix incorrect usage of an empty tree
  difftool -d: ensure that intent-to-add files are handled correctly

 diff-lib.c             | 16 +++++++++++++++-
 t/t2203-add-intent.sh  |  5 ++---
 t/t4000-diff-format.sh | 10 ++++++++++
 t/t7800-difftool.sh    |  8 ++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)


base-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/654

Range-diff vs v2:

 1:  640e225550 ! 1:  8c27c78831 diff-files --raw: handle intent-to-add files correctly
     @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
      
       ## t/t2203-add-intent.sh ##
      @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
     + 
     + 	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
     +@@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
       	 create mode 100644 not-empty
       	EOF
       	cat >expect.diff_a <<-EOF &&
 2:  b9633315a2 = 2:  a9e06427ec diff-files: fix incorrect usage of an empty tree
 3:  d2e9f704c9 = 3:  f51cbedd3f difftool -d: ensure that intent-to-add files are handled correctly

-- 
gitgitgadget

[PATCH v3 1/3] diff-files --raw: handle intent-to-add files correctly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In `run_diff_files()`, files that have been staged with the intention to
add are queued without a valid OID in the `diff_filepair`.

When the output mode is, say, `DIFF_FORMAT_PATCH`, the
`diff_fill_oid_info()` function, called from `run_diff()`, will remedy
that situation by reading the file contents from disk.

However, when the output mode is `DIFF_FORMAT_RAW`, that does not hold
true, and the output will contain a bogus OID (and the flag `M` for
"modified" instead of the correct `A` for "added").

As a consequence, `git difftool -d` (which relies on `git diff-files
--raw`'s output) does not work correctly.

Let's fix this specifically by imitating `diff_fill_oid_info()`.

Note: we can only do that for diff formats that do not actually need the
file contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to
read the blob contents, but that blob might not have been written to
Git's object database.

This fixes https://github.com/git-for-windows/git/issues/2677

This patch _also_ fixes the expectations set by the regression test
introduced in feea6946a5b (diff-files: treat "i-t-a" files as
"not-in-index", 2020-06-20).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c             | 14 ++++++++++++++
 t/t2203-add-intent.sh  |  5 ++---
 t/t4000-diff-format.sh | 10 ++++++++++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 61812f48c2..ea23169afa 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -217,6 +217,20 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       !is_null_oid(&ce->oid),
 					       ce->name, 0);
 				continue;
+			} else if (ce_intent_to_add(ce) &&
+				   !(revs->diffopt.output_format &
+				     ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {
+				struct object_id oid;
+				int ret = lstat(ce->name, &st);
+
+				if (ret < 0)
+					oidclr(&oid);
+				else
+					ret = index_path(istate, &oid,
+						 ce->name, &st, 0);
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       &oid, ret >= 0, ce->name, 0);
+				continue;
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8a5d55054f..7d379bdbc6 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 
 	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
@@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 	 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
+	:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
+	:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
 	EOF
 
 	git add -N empty not-empty &&
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index e5116a76a1..48ff4e250b 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -89,4 +89,14 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'
 	test_must_be_empty err
 '
 
+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '
+	echo 123 >ita &&
+	git add -N ita &&
+	printf ":000000 100644 %s %s A\\tita\n" \
+		$ZERO_OID $(git hash-object --stdin <ita) >expect &&
+	git diff-files --raw ita >actual &&
+	test_cmp expect actual
+'
+
+
 test_done
-- 
gitgitgadget

[PATCH v3 2/3] diff-files: fix incorrect usage of an empty tree

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
2017-05-30), the OID to use for intent-to-add files was inadvertently
changed from the empty blob to the empty tree.

Let's revert that.

To be able to do that, we just taught the regression test introduced in
feea6946a5b (diff-files: treat "i-t-a" files as "not-in-index",
2020-06-20) to _not_ expect the raw diff to contain the hash of the
empty tree (we also had to fix the code to actually produce the expected
output, but for the sake of this here patch, that's beside the point).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index ea23169afa..7aafd7cc37 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -234,7 +234,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
-					       the_hash_algo->empty_tree, 0,
+					       the_hash_algo->empty_blob, 0,
 					       ce->name, 0);
 				continue;
 			}
-- 
gitgitgadget

[PATCH v3 3/3] difftool -d: ensure that intent-to-add files are handled correctly

[Johannes Schindelin via GitGitGadget]

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.

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

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

[PATCH v4 0/2] Fix difftool problem with intent-to-add files

[Johannes Schindelin via GitGitGadget]

This problem was reported in 
https://github.com/git-for-windows/git/issues/2677, but the problem actually
lies with git diff --raw, and it seems that the bug has been with us ever
since --intent-to-add was introduced.

Changes since v3:

 * Instead of showing the same OID in git diff-files --raw as in git
   diff-files -p for intent-to-add files, we now imitate the logic for
   modified (non-intent-to-add) files: we show the "null" OID (i.e.
   all-zero).
   
   
 * As a consequence, we no longer just undo the inadvertent change where
   intent-to-add files were reported with the empty tree instead of the
   empty blob, but we fix the bug that has been with us since 
   run_diff_files() was taught about intent-to-add files, i.e. we fix the
   original bug of 425a28e0a4e (diff-lib: allow ita entries treated as "not
   yet exist in index", 2016-10-24), at long last.
   
   

Changes since v2:

 * Now we also drop the no-longer-used definition of hash_t in t2203.

Changes since v1:

 * Rebased onto sk/diff-files-show-i-t-a-as-new.
 * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve
   the issue (the --raw output still claims the empty blob as the
   post-image, although the difftool symptom "went away").
 * Amended the central patch of this PR to include a fix for the regression
   test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected
   the raw diff to contain the hash of the empty tree object (which is
   incorrect no matter how you turn it: any hash in any raw diff should
   refer to blob objects).
 * Reordered the patches so that the central patch comes first (otherwise,
   the "empty tree" fix would cause a test failure in t2203).

Johannes Schindelin (2):
  diff-files --raw: show correct post-image of intent-to-add files
  difftool -d: ensure that intent-to-add files are handled correctly

 diff-lib.c            | 3 +--
 t/t2203-add-intent.sh | 5 ++---
 t/t7800-difftool.sh   | 8 ++++++++
 3 files changed, 11 insertions(+), 5 deletions(-)


base-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/654

Range-diff vs v3:

 1:  8c27c78831 ! 1:  69256ab910 diff-files --raw: handle intent-to-add files correctly
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    diff-files --raw: handle intent-to-add files correctly
     +    diff-files --raw: show correct post-image of intent-to-add files
      
     -    In `run_diff_files()`, files that have been staged with the intention to
     -    add are queued without a valid OID in the `diff_filepair`.
     +    The documented behavior of `git diff-files --raw` is to display
      
     -    When the output mode is, say, `DIFF_FORMAT_PATCH`, the
     -    `diff_fill_oid_info()` function, called from `run_diff()`, will remedy
     -    that situation by reading the file contents from disk.
     +            [...] 0{40} if creation, unmerged or "look at work tree".
      
     -    However, when the output mode is `DIFF_FORMAT_RAW`, that does not hold
     -    true, and the output will contain a bogus OID (and the flag `M` for
     -    "modified" instead of the correct `A` for "added").
     +    This happens for example when showing modified, unstaged files.
      
     -    As a consequence, `git difftool -d` (which relies on `git diff-files
     -    --raw`'s output) does not work correctly.
     +    For intent-to-add files, we used to show the empty blob's hash instead.
     +    In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
     +    2017-05-30), we made that worse by inadvertently changing that to the
     +    hash of the empty tree.
      
     -    Let's fix this specifically by imitating `diff_fill_oid_info()`.
     +    Let's make the behavior consistent with modified files by showing
     +    all-zero values also for intent-to-add files.
      
     -    Note: we can only do that for diff formats that do not actually need the
     -    file contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to
     -    read the blob contents, but that blob might not have been written to
     -    Git's object database.
     -
     -    This fixes https://github.com/git-for-windows/git/issues/2677
     -
     -    This patch _also_ fixes the expectations set by the regression test
     -    introduced in feea6946a5b (diff-files: treat "i-t-a" files as
     +    Accordingly, this patch adjusts the expectations set by the regression
     +    test introduced in feea6946a5b (diff-files: treat "i-t-a" files as
          "not-in-index", 2020-06-20).
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## diff-lib.c ##
      @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
     - 					       !is_null_oid(&ce->oid),
     - 					       ce->name, 0);
     - 				continue;
     -+			} else if (ce_intent_to_add(ce) &&
     -+				   !(revs->diffopt.output_format &
     -+				     ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {
     -+				struct object_id oid;
     -+				int ret = lstat(ce->name, &st);
     -+
     -+				if (ret < 0)
     -+					oidclr(&oid);
     -+				else
     -+					ret = index_path(istate, &oid,
     -+						 ce->name, &st, 0);
     -+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
     -+					       &oid, ret >= 0, ce->name, 0);
     -+				continue;
       			} else if (revs->diffopt.ita_invisible_in_index &&
       				   ce_intent_to_add(ce)) {
       				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
     +-					       the_hash_algo->empty_tree, 0,
     +-					       ce->name, 0);
     ++					       &null_oid, 0, ce->name, 0);
     + 				continue;
     + 			}
     + 
      
       ## t/t2203-add-intent.sh ##
      @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
     @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff",
       	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
     -+	:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
     -+	:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
     ++	:000000 100644 0000000 0000000 A$(printf "\t")empty
     ++	:000000 100644 0000000 0000000 A$(printf "\t")not-empty
       	EOF
       
       	git add -N empty not-empty &&
     -
     - ## t/t4000-diff-format.sh ##
     -@@ t/t4000-diff-format.sh: test_expect_success 'git diff-files --patch --no-patch does not show the patch'
     - 	test_must_be_empty err
     - '
     - 
     -+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '
     -+	echo 123 >ita &&
     -+	git add -N ita &&
     -+	printf ":000000 100644 %s %s A\\tita\n" \
     -+		$ZERO_OID $(git hash-object --stdin <ita) >expect &&
     -+	git diff-files --raw ita >actual &&
     -+	test_cmp expect actual
     -+'
     -+
     -+
     - test_done
 2:  a9e06427ec < -:  ---------- diff-files: fix incorrect usage of an empty tree
 3:  f51cbedd3f = 2:  9bb8d84ea9 difftool -d: ensure that intent-to-add files are handled correctly

-- 
gitgitgadget

[PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The documented behavior of `git diff-files --raw` is to display

	[...] 0{40} if creation, unmerged or "look at work tree".

This happens for example when showing modified, unstaged files.

For intent-to-add files, we used to show the empty blob's hash instead.
In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
2017-05-30), we made that worse by inadvertently changing that to the
hash of the empty tree.

Let's make the behavior consistent with modified files by showing
all-zero values also for intent-to-add files.

Accordingly, this patch adjusts the expectations set by the regression
test introduced in feea6946a5b (diff-files: treat "i-t-a" files as
"not-in-index", 2020-06-20).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c            | 3 +--
 t/t2203-add-intent.sh | 5 ++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 61812f48c2..25fd2dee19 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
-					       the_hash_algo->empty_tree, 0,
-					       ce->name, 0);
+					       &null_oid, 0, ce->name, 0);
 				continue;
 			}
 
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8a5d55054f..cf0175ad6e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 
 	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
@@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 	 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
+	:000000 100644 0000000 0000000 A$(printf "\t")empty
+	:000000 100644 0000000 0000000 A$(printf "\t")not-empty
 	EOF
 
 	git add -N empty not-empty &&
-- 
gitgitgadget

Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/diff-lib.c b/diff-lib.c
> index 61812f48c2..25fd2dee19 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			} else if (revs->diffopt.ita_invisible_in_index &&
>  				   ce_intent_to_add(ce)) {
>  				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> -					       the_hash_algo->empty_tree, 0,
> -					       ce->name, 0);
> +					       &null_oid, 0, ce->name, 0);

This (even if the preimage were correctly using empty_blob) is *so*
simple a change that it is very surprising that the new test in
[2/2] passes without any other code change.

It means that difftool was written correctly in the first place,
right?

Nicely done.

Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Johannes Schindelin]

Hi Junio,


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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/diff-lib.c b/diff-lib.c
> > index 61812f48c2..25fd2dee19 100644
> > --- a/diff-lib.c
> > +++ b/diff-lib.c
> > @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> >  			} else if (revs->diffopt.ita_invisible_in_index &&
> >  				   ce_intent_to_add(ce)) {
> >  				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> > -					       the_hash_algo->empty_tree, 0,
> > -					       ce->name, 0);
> > +					       &null_oid, 0, ce->name, 0);
>
> This (even if the preimage were correctly using empty_blob) is *so*
> simple a change that it is very surprising that the new test in
> [2/2] passes without any other code change.
>
> It means that difftool was written correctly in the first place,
> right?

Well, it means that difftool does not even consume the OID. Sure, it
parses it, but then it ignores it. All it _really_ is interested in is
that status flag (`A`), so technically, my fix in 1/2 is not even needed
after `sk/diff-files-show-i-t-a-as-new` to let 2/2 pass.

Ciao,
Dscho

>
> Nicely done.
>

Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 25 Jun 2020, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>
>> > diff --git a/diff-lib.c b/diff-lib.c
>> > index 61812f48c2..25fd2dee19 100644
>> > --- a/diff-lib.c
>> > +++ b/diff-lib.c
>> > @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>> >  			} else if (revs->diffopt.ita_invisible_in_index &&
>> >  				   ce_intent_to_add(ce)) {
>> >  				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
>> > -					       the_hash_algo->empty_tree, 0,
>> > -					       ce->name, 0);
>> > +					       &null_oid, 0, ce->name, 0);
>>
>> This (even if the preimage were correctly using empty_blob) is *so*
>> simple a change that it is very surprising that the new test in
>> [2/2] passes without any other code change.
>>
>> It means that difftool was written correctly in the first place,
>> right?
>
> Well, it means that difftool does not even consume the OID. Sure, it
> parses it, but then it ignores it. All it _really_ is interested in is
> that status flag (`A`),

Ah, that's an ultimately defensive code.  No matter what is on the
right hand side of the raw output, as long as it knows that the
right hand side is a file on the working tree, it can safely ignore
the object name and directly go to the working tree file.

Nice ;-)

> so technically, my fix in 1/2 is not even needed
> after `sk/diff-files-show-i-t-a-as-new` to let 2/2 pass.

Sure, but I think it is the right thing to do nevertheless.  Giving
the object name for an empty blob would be wrong unless the working
tree file is known to be empty (and empty tree?  what were we even
thinking, gee...).

Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The documented behavior of `git diff-files --raw` is to display
>
> 	[...] 0{40} if creation, unmerged or "look at work tree".

"on the right hand (i.e. postimage) side" is missing, which is
crucial in this description.

> This happens for example when showing modified, unstaged files.

Modified I would understand.  We notice that the contents on the
work tree is different from what is in the index, and we tell the
consumer "look at work tree".  I do not think you meant "unstaged",
though.  If truly removed from the index, diff-files won't even know
about the path.  You probably meant "removed from the working tree",
but 0{40} in that case means totally different thing (i.e. it would
be a (D)eletion record, so 0{40} on the postimage side is a filler,
not even "look at work tree").  What would make more sense to
describe might be

	This happens for paths that are modified, or unmodified but
	stat-dirty.

Either case, we tell the consumer to check the "work tree".

> For intent-to-add files, we used to show the empty blob's hash instead.
> In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
> 2017-05-30), we made that worse by inadvertently changing that to the
> hash of the empty tree.
>
> Let's make the behavior consistent with modified files by showing
> all-zero values also for intent-to-add files.

Well described.

Thanks.

Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Johannes Schindelin]

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>
> >
> > The documented behavior of `git diff-files --raw` is to display
> >
> > 	[...] 0{40} if creation, unmerged or "look at work tree".
>
> "on the right hand (i.e. postimage) side" is missing, which is
> crucial in this description.

True. Sorry for omitting that.

> > This happens for example when showing modified, unstaged files.
>
> Modified I would understand.  We notice that the contents on the
> work tree is different from what is in the index, and we tell the
> consumer "look at work tree".  I do not think you meant "unstaged",

Right. I probably should have written "[...] when showing files with
unstaged modifications".

> though.  If truly removed from the index, diff-files won't even know
> about the path.  You probably meant "removed from the working tree",
> but 0{40} in that case means totally different thing (i.e. it would
> be a (D)eletion record, so 0{40} on the postimage side is a filler,
> not even "look at work tree").  What would make more sense to
> describe might be
>
> 	This happens for paths that are modified, or unmodified but
> 	stat-dirty.

Yes, but that includes more than I wanted to describe because the
stat-dirty does not matter for intent-to-add files, and I wanted to point
out the analogy between unstaged modifications and intent-to-add-files.

I noticed that you merged this commit into `next` already, so I am
assuming that you do not want me to send a fifth iteration ;-)

Ciao,
Dscho

> Either case, we tell the consumer to check the "work tree".
>
> > For intent-to-add files, we used to show the empty blob's hash instead.
> > In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
> > 2017-05-30), we made that worse by inadvertently changing that to the
> > hash of the empty tree.
> >
> > Let's make the behavior consistent with modified files by showing
> > all-zero values also for intent-to-add files.
>
> Well described.
>
> Thanks.
>

Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Srinidhi Kaushik]

Sorry for the late reply.

> Let's make the behavior consistent with modified files by showing
> all-zero values also for intent-to-add files.
>
-- >8 --
>
>                               diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> -                                            the_hash_algo->empty_tree, 0,
> -                                            ce->name, 0);
> +                                            &null_oid, 0, ce->name, 0);
>                               continue;
>                       }
>
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 8a5d55054f..cf0175ad6e 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
>
>       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
> @@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
>        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
> +     :000000 100644 0000000 0000000 A$(printf "\t")empty
> +     :000000 100644 0000000 0000000 A$(printf "\t")not-empty

This change (and the new test in [PATCH v4 2/2]) looks good to me.

I learnt quite a bit about what the string "0{40}" means in the context
of `diff'  post-image from the conversations around this patch. It was
very helpful.

Thank you.

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

[Johannes Schindelin via GitGitGadget]

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.

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

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

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

[Junio C Hamano]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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

[PATCH v5 0/2] Fix difftool problem with intent-to-add files

[Johannes Schindelin via GitGitGadget]

This problem was reported in 
https://github.com/git-for-windows/git/issues/2677, but the problem actually
lies with git diff --raw, and it seems that the bug has been with us ever
since --intent-to-add was introduced.

Changes since v4:

 * Improved both commit messages after feedback from Junio.

Changes since v3:

 * Instead of showing the same OID in git diff-files --raw as in git
   diff-files -p for intent-to-add files, we now imitate the logic for
   modified (non-intent-to-add) files: we show the "null" OID (i.e.
   all-zero).
   
   
 * As a consequence, we no longer just undo the inadvertent change where
   intent-to-add files were reported with the empty tree instead of the
   empty blob, but we fix the bug that has been with us since 
   run_diff_files() was taught about intent-to-add files, i.e. we fix the
   original bug of 425a28e0a4e (diff-lib: allow ita entries treated as "not
   yet exist in index", 2016-10-24), at long last.
   
   

Changes since v2:

 * Now we also drop the no-longer-used definition of hash_t in t2203.

Changes since v1:

 * Rebased onto sk/diff-files-show-i-t-a-as-new.
 * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve
   the issue (the --raw output still claims the empty blob as the
   post-image, although the difftool symptom "went away").
 * Amended the central patch of this PR to include a fix for the regression
   test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected
   the raw diff to contain the hash of the empty tree object (which is
   incorrect no matter how you turn it: any hash in any raw diff should
   refer to blob objects).
 * Reordered the patches so that the central patch comes first (otherwise,
   the "empty tree" fix would cause a test failure in t2203).

Johannes Schindelin (2):
  diff-files --raw: show correct post-image of intent-to-add files
  difftool -d: ensure that intent-to-add files are handled correctly

 diff-lib.c            | 3 +--
 t/t2203-add-intent.sh | 5 ++---
 t/t7800-difftool.sh   | 8 ++++++++
 3 files changed, 11 insertions(+), 5 deletions(-)


base-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/654

Range-diff vs v4:

 1:  69256ab910 ! 1:  4392ae4be6 diff-files --raw: show correct post-image of intent-to-add files
     @@ Commit message
      
                  [...] 0{40} if creation, unmerged or "look at work tree".
      
     -    This happens for example when showing modified, unstaged files.
     +    on the right hand (i.e. postimage) side. This happens for files that
     +    have unstaged modifications, and for files that are unmodified but
     +    stat-dirty.
      
          For intent-to-add files, we used to show the empty blob's hash instead.
          In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
          2017-05-30), we made that worse by inadvertently changing that to the
          hash of the empty tree.
      
     -    Let's make the behavior consistent with modified files by showing
     +    Let's make the behavior consistent with files that have unstaged
     +    modifications (which applies to intent-to-add files, too) by showing
          all-zero values also for intent-to-add files.
      
          Accordingly, this patch adjusts the expectations set by the regression
 2:  9bb8d84ea9 ! 2:  5f933e852a difftool -d: ensure that intent-to-add files are handled correctly
     @@ Commit message
      
          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.
     +    diff-files --raw` that we just fixed: it reported intent-to-add files
     +    with the empty _tree_ as the post-image OID, when we need to show
     +    an all-zero (or, "null") OID instead, to indicate to the caller that
     +    they have to look at the worktree file.
     +
     +    The symptom of that problem shown by `git difftool` was this:
     +
     +            error: unable to read sha1 file of <path> (<empty-tree-OID>)
     +            error: could not write '<filename>'
      
          Make sure that the reported `difftool` problem stays fixed.
      

-- 
gitgitgadget

[PATCH v5 1/2] diff-files --raw: show correct post-image of intent-to-add files

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The documented behavior of `git diff-files --raw` is to display

	[...] 0{40} if creation, unmerged or "look at work tree".

on the right hand (i.e. postimage) side. This happens for files that
have unstaged modifications, and for files that are unmodified but
stat-dirty.

For intent-to-add files, we used to show the empty blob's hash instead.
In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
2017-05-30), we made that worse by inadvertently changing that to the
hash of the empty tree.

Let's make the behavior consistent with files that have unstaged
modifications (which applies to intent-to-add files, too) by showing
all-zero values also for intent-to-add files.

Accordingly, this patch adjusts the expectations set by the regression
test introduced in feea6946a5b (diff-files: treat "i-t-a" files as
"not-in-index", 2020-06-20).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-lib.c            | 3 +--
 t/t2203-add-intent.sh | 5 ++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 61812f48c2..25fd2dee19 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
 				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
-					       the_hash_algo->empty_tree, 0,
-					       ce->name, 0);
+					       &null_oid, 0, ce->name, 0);
 				continue;
 			}
 
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8a5d55054f..cf0175ad6e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 
 	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
@@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
 	 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
+	:000000 100644 0000000 0000000 A$(printf "\t")empty
+	:000000 100644 0000000 0000000 A$(printf "\t")not-empty
 	EOF
 
 	git add -N empty not-empty &&
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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: it reported intent-to-add files
with the empty _tree_ as the post-image OID, when we need to show
an all-zero (or, "null") OID instead, to indicate to the caller that
they have to look at the worktree file.

The symptom of that problem shown by `git difftool` was this:

	error: unable to read sha1 file of <path> (<empty-tree-OID>)
	error: could not write '<filename>'

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

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