Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Duy Nguyen]

On Fri, May 25, 2018 at 6:43 PM, Jonathan Nieder  wrote:
> I think it should be reverted from 'next' because of the unintended
> change to the behavior of "git diff HEAD".

Ah. That is indeed unintended. I still don't know how this change
affects that (but that's probably why it slipped through in the first
place). While there, perhaps you could add a test in t2203 just to
make sure I will not break it again in the next attempt?

I agree that it should be reverted. Or if it's possible to eject it
from next, then it's even better since I will need to fix up the
second patch in that series anyway. Junio?

> Here is what I have applied internally.
>
> -- >8 --
> Subject: Revert "diff: turn --ita-invisible-in-index on by default"
>
> This reverts commit 992118dd95b052bc82a795fd3a8978bea0fe5c0e.  That
> change is promising, since it improves the behavior of "git diff" and
> "git diff --cached" by correctly showing an intent-to-add entry as no
> file.  Unfortunately, though, it breaks "git diff HEAD" in the
> process:
>
> echo hi >new-file
> git add -N new-file
> git diff HEAD
>
> Before: shows addition of the new file
> After: shows no change
>
> Noticed because the new --ita-invisible-in-index default broke a
> script using "git diff --name-only --diff-filter=A master" to get a
> list of added files.  Arguably that script should use diff-index
> instead, which would have sidestepped the issue, but the new behavior
> is unexpected in interactive use as well.
>
> Signed-off-by: Jonathan Nieder 
> ---
>  builtin/diff.c  |  7 ---
>  t/t2203-add-intent.sh   | 40 
>  t/t4011-diff-symlink.sh | 10 --
>  3 files changed, 12 insertions(+), 45 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index b709b6e984..bfefff3a84 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -352,13 +352,6 @@ int cmd_diff(int argc, const char **argv, const char 
> *prefix)
> rev.diffopt.flags.allow_external = 1;
> rev.diffopt.flags.allow_textconv = 1;
>
> -   /*
> -* Default to intent-to-add entries invisible in the
> -* index. This makes them show up as new files in diff-files
> -* and not at all in diff-cached.
> -*/
> -   rev.diffopt.ita_invisible_in_index = 1;
> -
> if (nongit)
> die(_("Not a git repository"));
> argc = setup_revisions(argc, argv, , NULL);
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 1d640a33f0..d9fb151d52 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
> git add -N nitfol &&
> git commit -m second &&
> test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
> -   test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | 
> wc -l) = 1 &&
> -   test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
> -   test $(git diff --name-only -- nitfol | wc -l) = 1
> +   test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
> +   test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | 
> wc -l) = 0 &&
> +   test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc 
> -l) = 1
>  '
>
>  test_expect_success 'can commit with an unrelated i-t-a entry in index' '
> @@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
>
> : >dir/bar &&
> git add -N dir/bar &&
> -   git diff --name-only >actual &&
> +   git diff --cached --name-only >actual &&
> echo dir/bar >expect &&
> test_cmp expect actual &&
>
> git write-tree >/dev/null &&
>
> -   git diff --name-only >actual &&
> +   git diff --cached --name-only >actual &&
> echo dir/bar >expect &&
> test_cmp expect actual
>  '
> @@ -186,19 +186,7 @@ test_expect_success 'rename detection finds the right 
> names' '
> cat >expected.3 <<-EOF &&
> 2 .R N... 100644 100644 100644 $hash $hash R100 third   first
> EOF
> -   test_cmp expected.3 actual.3 &&
> -
> -   git diff --stat >actual.4 &&
> -   cat >expected.4 <<-EOF &&
> -first => third | 0
> -1 file changed, 0 insertions(+), 0 deletions(-)
> -   EOF
> -   test_cmp expected.4 actual.4 &&
> -
> -   git diff --cached --stat >actual.5 &&
> -   : >expected.5 &&
> -   test_cmp expected.5 actual.5
> -
> +   test_cmp expected.3 actual.3
> )
>  '
>
> @@ -234,27 +222,15 @@ 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 &&
> -   git diff 

Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Jonathan Nieder]

Hi,

Duy Nguyen wrote:

> $ echo haha > new; git add -N
> $ git diff
> diff --git a/new b/new
> index e69de29..5ad28e2 100644
> --- a/new
> +++ b/new
> @@ -0,0 +1 @@
> +haha
>
> Notice that the diff does not tell you that 'new' is a new file. The
> diff with this patch gives you this
>
> $ git diff
> diff --git a/new b/new
> new file mode 100644
> index 000..5ad28e2
> --- /dev/null
> +++ b/new
> @@ -0,0 +1 @@
> +haha
[...]
> One consequence of this is you can't apply the diff generated with ita
> entries because the diff expects empty files to be already in the
> worktree. This to me does not make sense.
>
> Of course there's other things that also go along this line. Like if
> "git commit" does not add an ita entry, why should it appear in 'git
> diff --cached', which should show you what's to be committed.

Thankds for this context.  That helps a lot.

[...]
> Since this commit is already in 'next', it's too late to update the
> commit message now.

I think it should be reverted from 'next' because of the unintended
change to the behavior of "git diff HEAD".  Here is what I have
applied internally.

-- >8 -- 
Subject: Revert "diff: turn --ita-invisible-in-index on by default"

This reverts commit 992118dd95b052bc82a795fd3a8978bea0fe5c0e.  That
change is promising, since it improves the behavior of "git diff" and
"git diff --cached" by correctly showing an intent-to-add entry as no
file.  Unfortunately, though, it breaks "git diff HEAD" in the
process:

echo hi >new-file
git add -N new-file
git diff HEAD

Before: shows addition of the new file
After: shows no change

Noticed because the new --ita-invisible-in-index default broke a
script using "git diff --name-only --diff-filter=A master" to get a
list of added files.  Arguably that script should use diff-index
instead, which would have sidestepped the issue, but the new behavior
is unexpected in interactive use as well.

Signed-off-by: Jonathan Nieder 
---
 builtin/diff.c  |  7 ---
 t/t2203-add-intent.sh   | 40 
 t/t4011-diff-symlink.sh | 10 --
 3 files changed, 12 insertions(+), 45 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index b709b6e984..bfefff3a84 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,13 +352,6 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
 
-   /*
-* Default to intent-to-add entries invisible in the
-* index. This makes them show up as new files in diff-files
-* and not at all in diff-cached.
-*/
-   rev.diffopt.ita_invisible_in_index = 1;
-
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, , NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1d640a33f0..d9fb151d52 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-   test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc 
-l) = 1 &&
-   test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
-   test $(git diff --name-only -- nitfol | wc -l) = 1
+   test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+   test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | 
wc -l) = 0 &&
+   test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) 
= 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
: >dir/bar &&
git add -N dir/bar &&
-   git diff --name-only >actual &&
+   git diff --cached --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual &&
 
git write-tree >/dev/null &&
 
-   git diff --name-only >actual &&
+   git diff --cached --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual
 '
@@ -186,19 +186,7 @@ test_expect_success 'rename detection finds the right 
names' '
cat >expected.3 <<-EOF &&
2 .R N... 100644 100644 100644 $hash $hash R100 third   first
EOF
-   test_cmp expected.3 actual.3 &&
-
-   git diff --stat >actual.4 &&
-   cat >expected.4 <<-EOF &&
-first => third | 0
-1 file changed, 0 insertions(+), 0 deletions(-)
-   EOF
-   test_cmp expected.4 actual.4 &&
-
-   git diff --cached --stat >actual.5 &&
-   : >expected.5 &&
-   test_cmp expected.5 actual.5
-
+   test_cmp expected.3 

Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Duy Nguyen]

On Thu, May 24, 2018 at 08:39:42PM -0700, Jonathan Nieder wrote:
> Hi Duy,
> 
> Nguyễn Thái Ngọc Duy wrote:
> 
> > Due to the implementation detail of intent-to-add entries. The current
> > "git diff" (i.e. no treeish or --cached argument) would show the
> > changes in the i-t-a file, but it does not mark the file as new, while
> > "diff --cached" would mark the file as new while showing its content
> > as empty.
> >
> > One evidence of the current output being wrong is that, the output
> > from "git diff" (with ita entries) cannot be applied because it
> > assumes empty files exist before applying.
> >
> > Turning on --ita-invisible-in-index [1] [2] would fix this.
> 
> I'm having a lot of trouble understanding the above.  Part of my
> confusion may be grammatical: for example, the first sentence is a
> sentence fragment.  Another part is that the commit message doesn't tell
> me a story: what does the user try to do and fail that is not possible
> without this?  What is the intention or effect behind the commits
> mentioned that leads to them being cited?
>
> ...

Sorry, this i-t-a thing had been on my mind for too long I mistakenly
thought this problem was common knowledge. Reference [1] in that
commit points to a previous attempt d95d728aba (diff-lib.c: adjust
position of i-t-a entries in diff - 2015-03-16) that was reverted,
which gives more info.

Anyway this is the diff we see today

$ echo haha > new; git add -N
$ git diff
diff --git a/new b/new
index e69de29..5ad28e2 100644
--- a/new
+++ b/new
@@ -0,0 +1 @@
+haha

Notice that the diff does not tell you that 'new' is a new file. The
diff with this patch gives you this

$ git diff
diff --git a/new b/new
new file mode 100644
index 000..5ad28e2
--- /dev/null
+++ b/new
@@ -0,0 +1 @@
+haha

You may argue that an intent-to-add entry is a real entry in the index
and showing "new file mode 100644" here is wrong. I beg to differ.
That just happens to be how you mark an ita entry. If Junio chose to
record ita entries as an index extension, then they would not be
"real" and could still be used to remind users about things to add.

>From the user perspective, as a user I do not care if it's a "real
entry". I want git to _remind_ myself that I need to add that
file. That file should not truly exist in the index because I have not
actually added it. I did not tell git to add anything to the index.

One consequence of this is you can't apply the diff generated with ita
entries because the diff expects empty files to be already in the
worktree. This to me does not make sense.

Of course there's other things that also go along this line. Like if
"git commit" does not add an ita entry, why should it appear in 'git
diff --cached', which should show you what's to be committed. Right
know it shows


$ git diff --cached
diff --git a/new b/new
new file mode 100644
index 000..e69de29

which to me is ridiculous. Why would it show me a diff of a new file
with empty content? I as a user have not mentioned "empty content"
anywhere through the commands I have used.

Since this commit is already in 'next', it's too late to update the
commit message now. Maybe I can elaborate about this more in
git-add.txt if needed (and then I can add more explanation in the
commit message that updates that file).
--
Duy

Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Jonathan Nieder]

Hi Duy,

Nguyễn Thái Ngọc Duy wrote:

> Due to the implementation detail of intent-to-add entries. The current
> "git diff" (i.e. no treeish or --cached argument) would show the
> changes in the i-t-a file, but it does not mark the file as new, while
> "diff --cached" would mark the file as new while showing its content
> as empty.
>
> One evidence of the current output being wrong is that, the output
> from "git diff" (with ita entries) cannot be applied because it
> assumes empty files exist before applying.
>
> Turning on --ita-invisible-in-index [1] [2] would fix this.

I'm having a lot of trouble understanding the above.  Part of my
confusion may be grammatical: for example, the first sentence is a
sentence fragment.  Another part is that the commit message doesn't tell
me a story: what does the user try to do and fail that is not possible
without this?  What is the intention or effect behind the commits
mentioned that leads to them being cited?

To put it another way, the basic aspects I look for in a commit message
are:

 - the motivation behind the change (a wrong behavior, a task that isn't
   possible, some excessive complexity, or whatever).  The reader
   doesn't know your motivation so their default attitude will be to
   assume that nothing should change

 - a little more detail about the why and how behind the current
   behavior, to put the proposed in context.  This makes it easier for
   the reader to understand how the change will affect users of that
   behavior that don't necessarily have the same motivation.

   An example illustrating the behavior can work well here.

 - any interesting details of implementation or alternatives considered
   that can make the patch easier to read now that the motivation is out
   of the way.

 - a word or two on what this makes possible

I'm having trouble pulling apart these pieces in this commit message.
Can you give an example of a command's output before and after this change
so I can understand better why it's a good one?

> This option is on by default in git-status [1] but we need more fixup
> in rename detection code [3]. Luckily we don't need to do anything
> else for the rename detection code in diff.c (wt-status.c uses a
> customized one).
>
> [1] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
> in index" - 2016-10-24)
> [2] b42b451919 (diff: add --ita-[in]visible-in-index - 2016-10-24)
> [3] bc3dca07f4 (Merge branch 'nd/ita-wt-renames-in-status' - 2018-01-23)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/diff.c  |  7 +++
>  t/t2203-add-intent.sh   | 37 ++---
>  t/t4011-diff-symlink.sh | 10 ++
>  3 files changed, 43 insertions(+), 11 deletions(-)

This flips the default as announced but I'm not sure yet whether it's
a good thing.  After all, an intent-to-add entry is a real entry in
the index; why wouldn't it show up in "git diff --cached"?

Is the idea that it shouldn't show up there because "git commit" would
not include the intent-to-add entry?  That makes some sense to me.

What does the endgame look like?  Would we flip the default to
--ita-invisible and then remove the option?

Context is that an internal script does something like

echo 'This file is added!' >added
git add --intent-to-add added
git diff --name-only --no-renames --diff-filter=A master

to list added files and started failing with this patch (in "next").
Arguably the script should use diff-index for a stronger stability
guarantee.  Should the script use --ita-visible as a futureproofing
measure as well?

Actually, why is this "git diff" output changing at all, given that
the script doesn't pass --cached?  I would expect "git diff" to show
the ITA entry because it affects the result of "git commit -a".

Thanks,
Jonathan

[PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Nguyễn Thái Ngọc Duy]

Due to the implementation detail of intent-to-add entries. The current
"git diff" (i.e. no treeish or --cached argument) would show the
changes in the i-t-a file, but it does not mark the file as new, while
"diff --cached" would mark the file as new while showing its content
as empty.

One evidence of the current output being wrong is that, the output
from "git diff" (with ita entries) cannot be applied because it
assumes empty files exist before applying.

Turning on --ita-invisible-in-index [1] [2] would fix this.

This option is on by default in git-status [1] but we need more fixup
in rename detection code [3]. Luckily we don't need to do anything
else for the rename detection code in diff.c (wt-status.c uses a
customized one).

[1] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
in index" - 2016-10-24)
[2] b42b451919 (diff: add --ita-[in]visible-in-index - 2016-10-24)
[3] bc3dca07f4 (Merge branch 'nd/ita-wt-renames-in-status' - 2018-01-23)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/diff.c  |  7 +++
 t/t2203-add-intent.sh   | 37 ++---
 t/t4011-diff-symlink.sh | 10 ++
 3 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 16bfb22f73..00424c296d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,6 +352,13 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
 
+   /*
+* Default to intent-to-add entries invisible in the
+* index. This makes them show up as new files in diff-files
+* and not at all in diff-cached.
+*/
+   rev.diffopt.ita_invisible_in_index = 1;
+
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, , NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 78236dc7d8..31bf50082f 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-   test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
-   test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | 
wc -l) = 0 &&
-   test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) 
= 1
+   test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc 
-l) = 1 &&
+   test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
+   test $(git diff --name-only -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
: >dir/bar &&
git add -N dir/bar &&
-   git diff --cached --name-only >actual &&
+   git diff --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual &&
 
git write-tree >/dev/null &&
 
-   git diff --cached --name-only >actual &&
+   git diff --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual
 '
@@ -186,7 +186,19 @@ test_expect_success 'rename detection finds the right 
names' '
cat >expected.3 <<-EOF &&
2 .R N... 100644 100644 100644 $hash $hash R100 third   first
EOF
-   test_cmp expected.3 actual.3
+   test_cmp expected.3 actual.3 &&
+
+   git diff --stat >actual.4 &&
+   cat >expected.4 <<-EOF &&
+first => third | 0
+1 file changed, 0 insertions(+), 0 deletions(-)
+   EOF
+   test_cmp expected.4 actual.4 &&
+
+   git diff --cached --stat >actual.5 &&
+   : >expected.5 &&
+   test_cmp expected.5 actual.5
+
)
 '
 
@@ -222,5 +234,16 @@ test_expect_success 'double rename detection in status' '
)
 '
 
-test_done
+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 &&
+   git diff --summary >actual &&
+   echo " create mode 100644 new-ita" >expected &&
+   test_cmp expected actual &&
+   git diff --cached --summary >actual2 &&
+   : >expected2 &&
+   test_cmp expected2 actual2
+'
 
+test_done
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index cf0f3a1ee7..108c012a3a 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -139,11 +139,13 @@ test_expect_success SYMLINKS 'setup symlinks with 
attributes' '
 test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' 
'
cat >expect <<-\EOF &&
diff --git a/file.bin b/file.bin
-   index e69de29..d95f3ad 100644
-   Binary files a/file.bin