diff: turn --ita-invisible-in-index on by default

Similar to 'git reset -N', this option makes 'git apply' automatically mark new files as intent-to-add so they are visible in the following 'git diff' command and could also be committed with 'git commit -a'.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/git-apply.txt |  9 ++++++++-
 apply.c                     | 38 +++++++++++++++++++++++++++++++------
 apply.h                     |  1 +
 t/t2203-add-intent.sh       | 12 ++++++++++++
 4 files changed, 53 insertions(+), 7 deletions(-)

Documentation/git-apply.txt +1 -1

index 4ebc3d3271..2374f64b51 100644
@@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 --------
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
 	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]

apply.c +10 -0

@@ -4305,6 +4304,27 @@ static int add_index_file(struct apply_state *state,
 	return 0;
 }
 
+static int add_ita_file(struct apply_state *state,
+			const char *path, unsigned mode)
+{
+	struct cache_entry *ce;
+	int namelen = strlen(path);
+	unsigned ce_size = cache_entry_size(namelen);
+
+	ce = xcalloc(1, ce_size);
+	memcpy(ce->name, path, namelen);
+	ce->ce_mode = create_ce_mode(mode);

Quoted 17 lines

Documentation/git-apply.txt +1 -1

index 4ebc3d3271..2374f64b51 100644
@@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 --------
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
 	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]

@@ -74,6 +74,13 @@ OPTIONS
 	cached data, apply the patch, and store the result in the index
 	without using the working tree. This implies `--index`.
 

Documentation/git-apply.txt OPTIONS · line 82

Quoted 6 lines

Documentation/git-apply.txt +6 -0

+--intent-to-add::
+	When applying the patch only to the working tree, mark new
+	files to be added to the index later (see `--intent-to-add`
+	option in linkgit:git-add[1]). This option is ignored if
+	`--index` is present or the command is not run in a Git
+	repository.

It may make sense to make it incompatible with "--index" like you did, but how does this interact with "--cached" or "--3way"? It is unclear from the above documentation.

Quoted 6 lines

apply.c +0 -0

index 7e5792c996..31d3e50401 100644
@@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
 		state->apply = 0;

apply.c int check_apply_state(struct apply_state *state, int force_apply) · line 140

Quoted 4 lines

apply.c +2 -0

 	if (state->check_index && is_not_gitdir)
 		return error(_("--index outside a repository"));
+	if (state->set_ita && is_not_gitdir)
+		state->set_ita = 0;

I think this should error out, just like one line above does. "I-t-a" is impossible without having the index, just like "--index" is impossible without having the index.

Quoted 8 lines

apply.c int check_apply_state(struct apply_state *state, int force_apply) · line 140

Quoted 4 lines

apply.c +2 -0

 	if (state->check_index && is_not_gitdir)
 		return error(_("--index outside a repository"));
+	if (state->set_ita && is_not_gitdir)
+		state->set_ita = 0;

I think this should error out, just like one line above does. "I-t-a" is impossible without having the index, just like "--index" is impossible without having the index.

I was hoping to put this in an alias that works both with or without a repository. Do you feel strongly about this?

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 <[email protected]>

---
 builtin/diff.c          |  7 +++++++
 t/t2203-add-intent.sh   | 37 ++++++++++++++++++++++++++++++-------
 t/t4011-diff-symlink.sh | 10 ++++++----
 3 files changed, 43 insertions(+), 11 deletions(-)

Hi Duy,

Quoted 11 lines · 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.

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?

...

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

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 <[email protected]>

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

builtin/diff.c +0 -7

index b709b6e984..bfefff3a84 100644
@@ -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, &rev, NULL);

t/t2203-add-intent.sh +8 -32

index 1d640a33f0..d9fb151d52 100755
@@ -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 --summary >actual &&
-       echo " create mode 100644 new-ita" >expected &&
-       test_cmp expected actual &&
-       git diff --cached --summary >actual2 &&
-       : >expected2 &&
-       test_cmp expected2 actual2
-'
-
 test_expect_success 'apply --intent-to-add' '
        git reset --hard &&
        echo new >new-ita &&
        git add -N new-ita &&
-       git diff >expected &&
+       git diff --ita-invisible-in-index >expected &&
        grep "new file" expected &&
        git reset --hard &&
        git apply --intent-to-add expected &&
-       git diff >actual &&
+       git diff --ita-invisible-in-index >actual &&
        test_cmp expected actual
 '
 

t/t4011-diff-symlink.sh +4 -7

index 108c012a3a..cf0f3a1ee7 100755
@@ -139,13 +139,11 @@ 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
-       new file mode 100644
-       index 0000000..d95f3ad
-       Binary files /dev/null and b/file.bin differ
+       index e69de29..d95f3ad 100644
+       Binary files a/file.bin and b/file.bin differ
        diff --git a/link.bin b/link.bin
-       new file mode 120000
-       index 0000000..dce41ec
-       --- /dev/null
+       index e69de29..dce41ec 120000
+       --- a/link.bin
        +++ b/link.bin
        @@ -0,0 +1 @@
        +file.bin
--

2.17.0.921.gf22659ad46

This option is supposed to fix the diff of "diff-files" (not reporting ita entries as new files) and "diff-index --cached <tree>" ( showing

ita entries as present in the index with empty content) but not "diff-index <tree>".

When --ita-invisible-in-index is set on "git diff-index <tree>", unpack_trees() will eventually call oneway_diff() on the ita entry with the same code flow as "diff-index --cached <tree>". We want to ignore the ita entry for "diff-index --cached <tree>" but not "diff-index <tree>" since the latter will examine and produce a diff based on worktree entry's (real) content, not ita index entry's (empty) content.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>