[PATCH/RFC 0/2] Bug fixes regarding diff and "git add -N"

[PATCH/RFC 0/2] Bug fixes regarding diff and "git add -N"

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

The first one attempts to fix "git status" reporting intent-to-add
files as "changes to be committed". The "new file" report is now moved
to "git diff-files" aka "changes not staged for commit" instead.

I just want to check if I'm going on the right direction as core diff
machinery is not really my area. I know these patches break t2203 and
t4011 but from a quick glance, the tests simply rely on the old behavior,
not real breakages.

Nguyễn Thái Ngọc Duy (2):
  diff --cached: do not report i-t-a entries as "new"
  diff-files: mark i-t-a paths as "new"

 builtin/add.c | 1 +
 diff-lib.c    | 7 +++++++
 2 files changed, 8 insertions(+)

-- 
2.3.0.rc1.137.g477eb31

[PATCH 1/2] diff --cached: do not report i-t-a entries as "new"

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

Paths marked by "git add -N" are simply a reminder to the user that
these files should be staged. If the user does not stage any of them,
"git commit" will not record them.

Align the behavior of "diff --cached" and "git commit". The most
prominent result of this patch is "git status" no longer reports i-t-a
paths as "Changes to be committed".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff-lib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index a85c497..db0e6f8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -400,6 +400,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	 * Something added to the tree?
 	 */
 	if (!tree) {
+		if (idx && (idx->ce_flags & CE_INTENT_TO_ADD))
+			return;
 		show_new_file(revs, idx, cached, match_missing);
 		return;
 	}
-- 
2.3.0.rc1.137.g477eb31

[PATCH 2/2] diff-files: mark i-t-a paths as "new"

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/add.c | 1 +
 diff-lib.c    | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
+		case DIFF_STATUS_ADDED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
 			if (add_file_to_index(&the_index, path, data->flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index db0e6f8..5f1afa4 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       ce->sha1, !is_null_sha1(ce->sha1),
 					       ce->name, 0);
 				continue;
+			} else if (ce->ce_flags & CE_INTENT_TO_ADD) {
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       EMPTY_BLOB_SHA1_BIN, 0,
+					       ce->name, 0);
+				continue;
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-- 
2.3.0.rc1.137.g477eb31

[PATCH] t2203,t4011: adjust to changed intent-to-add treatment

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
For the record, the tests would need to change like this, and it makes
a lot of sense. After the change, "i-t-a" is not a "change staged in
the index" any more - and in fact in never was, as "git commit" shows.

 t/t2203-add-intent.sh   |  7 ++++---
 t/t4011-diff-symlink.sh | 10 ++++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..1a9b3c4 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -43,7 +43,8 @@ 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 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' '
@@ -71,13 +72,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
 '
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 13e7f62..7452fce 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 and b/file.bin differ
+	new file mode 100644
+	index 0000000..d95f3ad
+	Binary files /dev/null and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	index e69de29..dce41ec 120000
-	--- a/link.bin
+	new file mode 120000
+	index 0000000..dce41ec
+	--- /dev/null
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin
-- 
2.3.1.303.g5174db1

Re: [PATCH 1/2] diff --cached: do not report i-t-a entries as "new"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Paths marked by "git add -N" are simply a reminder to the user that
> these files should be staged. If the user does not stage any of them,
> "git commit" will not record them.
>
> Align the behavior of "diff --cached" and "git commit". The most
> prominent result of this patch is "git status" no longer reports i-t-a
> paths as "Changes to be committed".
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  diff-lib.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index a85c497..db0e6f8 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -400,6 +400,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	 * Something added to the tree?
>  	 */
>  	if (!tree) {
> +		if (idx && (idx->ce_flags & CE_INTENT_TO_ADD))
> +			return;
>  		show_new_file(revs, idx, cached, match_missing);
>  		return;
>  	}

This hunk by itself feels like it is going in the right direction.
The HEAD does not have it, and even though there is idx, it merely
is an i-t-a entry.

Don't you need to special case the call to show_modified() at the
end of this function as well, though?  idx is i-t-a, and tree has a
concrete object.  Should it appear as if the path already exists in
the index, or should we pretend as if idx is not yet there?

For example, after this sequence:

    $ git init
    $ >void
    $ git add void
    $ git commit -m void
    $ git rm --cached void
    $ git add -N void

HEAD has "void" with an empty blob, the index has i-t-a.  I am not
sure if we should say something about path "void" when asked:

    $ git diff --cached

Perhaps something like this to cover that case?

	/*
	 * Something removed from the tree?
	 */
-	if (!idx) {
+	if (!idx || (ix->ce_flags & CE_INTENT_TO_ADD)) {
		diff_index_show_file(revs, "-", tree, tree->sha1, 1, tree->ce_mode, 0);
		return;
	}

Re: [PATCH 2/2] diff-files: mark i-t-a paths as "new"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/add.c | 1 +
>  diff-lib.c    | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 3390933..ee370b0 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
>  		switch (fix_unmerged_status(p, data)) {
>  		default:
>  			die(_("unexpected diff status %c"), p->status);
> +		case DIFF_STATUS_ADDED:
>  		case DIFF_STATUS_MODIFIED:
>  		case DIFF_STATUS_TYPE_CHANGED:
>  			if (add_file_to_index(&the_index, path, data->flags)) {

Is this related to making "diff-files" show an i-t-a as "new", or
something else?

Ahh, "added" would have never appeared in diff-files output (because
by definition comparing index and working tree for only paths known
to the index would never produce "add"), and the point of this series
is to use that status to signal that the path is marked as i-t-a.

And an i-t-a path is "not yet exist in the index, known to the
system, and exists in the working tree", so catching that new case
here and calling add_file_to_index() would cause such a path to be
truly added to the index (this is "add -u" codepath, right?).

Makes sense.

> diff --git a/diff-lib.c b/diff-lib.c
> index db0e6f8..5f1afa4 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  					       ce->sha1, !is_null_sha1(ce->sha1),
>  					       ce->name, 0);
>  				continue;
> +			} else if (ce->ce_flags & CE_INTENT_TO_ADD) {
> +				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> +					       EMPTY_BLOB_SHA1_BIN, 0,
> +					       ce->name, 0);
> +				continue;
>  			}
>  
>  			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,

And this makes sense, of course.

The way "add -N" entries appear in "git status" has been disturbing
for quite a while to me, too.  Thanks for starting to look into it.

Re: [PATCH] t2203,t4011: adjust to changed intent-to-add treatment

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> For the record, the tests would need to change like this, and it makes
> a lot of sense. After the change, "i-t-a" is not a "change staged in
> the index" any more - and in fact in never was, as "git commit" shows.

Perhaps another follow-up patch can illustrate how these entries
should show "git status", both in the longform and -s output?



>
>  t/t2203-add-intent.sh   |  7 ++++---
>  t/t4011-diff-symlink.sh | 10 ++++++----
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 2a4a749..1a9b3c4 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -43,7 +43,8 @@ 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 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' '
> @@ -71,13 +72,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
>  '
> diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
> index 13e7f62..7452fce 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 and b/file.bin differ
> +	new file mode 100644
> +	index 0000000..d95f3ad
> +	Binary files /dev/null and b/file.bin differ
>  	diff --git a/link.bin b/link.bin
> -	index e69de29..dce41ec 120000
> -	--- a/link.bin
> +	new file mode 120000
> +	index 0000000..dce41ec
> +	--- /dev/null
>  	+++ b/link.bin
>  	@@ -0,0 +1 @@
>  	+file.bin

[PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

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

Entries added by "git add -N" are reminder for the user so that they
don't forget to add them before committing. These entries appear in
the index even though they are not real. Their presence in the index
leads to a confusing "git status" like this:

    On branch master
    Changes to be committed:
            new file:   foo

    Changes not staged for commit:
            modified:   foo

If you do a "git commit", "foo" will not be included even though
"status" reports it as "to be committed". This patch changes the
output to become

    On branch master
    Changes not staged for commit:
            new file:   foo

    no changes added to commit

The two hunks in diff-lib.c adjust "diff-index" and "diff-files" so
that i-t-a entries appear as new files in diff-files and nothing in
diff-index.

Due to this change, diff-files may start to report "new files" for the
first time. "add -u" needs to be told about this or it will die in
denial, screaming "new files can't exist! Reality is wrong." Luckily,
it's the only one among run_diff_files() callers that needs fixing.

The test "cache-tree invalidates i-t-a paths" is marked failure
because I don't think removing "--cached" from "git diff" is the right
fix. This test relies on "diff --cached" behavior but the behavior now
has changed. We may need to revisit this test at some point in future.

Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/add.c           |  1 +
 diff-lib.c              | 12 ++++++++++++
 t/t2203-add-intent.sh   | 21 ++++++++++++++++++---
 t/t4011-diff-symlink.sh | 10 ++++++----
 4 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
+		case DIFF_STATUS_ADDED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
 			if (add_file_to_index(&the_index, path, data->flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index a85c497..714501a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       ce->sha1, !is_null_sha1(ce->sha1),
 					       ce->name, 0);
 				continue;
+			} else if (ce->ce_flags & CE_INTENT_TO_ADD) {
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       EMPTY_BLOB_SHA1_BIN, 0,
+					       ce->name, 0);
+				continue;
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct rev_info *revs = o->unpack_data;
 	int match_missing, cached;
 
+	/* i-t-a entries do not actually exist in the index */
+	if (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {
+		idx = NULL;
+		if (!tree)
+			return;	/* nothing to diff.. */
+	}
+
 	/* if the entry is not checked out, don't examine work tree */
 	cached = o->index_only ||
 		(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..42827b8 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
 . ./test-lib.sh
 
 test_expect_success 'intent to add' '
+	test_commit 1 &&
+	git rm 1.t &&
+	echo hello >1.t &&
 	echo hello >file &&
 	echo hello >elif &&
 	git add -N file &&
-	git add elif
+	git add elif &&
+	git add -N 1.t
+'
+
+test_expect_success 'git status' '
+	git status --porcelain | grep -v actual >actual &&
+	cat >expect <<-\EOF &&
+	DA 1.t
+	A  elif
+	 A file
+	EOF
+	test_cmp expect actual
 '
 
 test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,8 @@ 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 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' '
@@ -62,7 +77,7 @@ test_expect_success 'can "commit -a" with an i-t-a entry' '
 	git commit -a -m all
 '
 
-test_expect_success 'cache-tree invalidates i-t-a paths' '
+test_expect_failure 'cache-tree invalidates i-t-a paths' '
 	git reset --hard &&
 	mkdir dir &&
 	: >dir/foo &&
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 13e7f62..7452fce 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 and b/file.bin differ
+	new file mode 100644
+	index 0000000..d95f3ad
+	Binary files /dev/null and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	index e69de29..dce41ec 120000
-	--- a/link.bin
+	new file mode 120000
+	index 0000000..dce41ec
+	--- /dev/null
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Michael J Gruber]

Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56:
> Entries added by "git add -N" are reminder for the user so that they
> don't forget to add them before committing. These entries appear in
> the index even though they are not real. Their presence in the index
> leads to a confusing "git status" like this:
> 
>     On branch master
>     Changes to be committed:
>             new file:   foo
> 
>     Changes not staged for commit:
>             modified:   foo
> 
> If you do a "git commit", "foo" will not be included even though
> "status" reports it as "to be committed". This patch changes the
> output to become
> 
>     On branch master
>     Changes not staged for commit:
>             new file:   foo
> 
>     no changes added to commit
> 
> The two hunks in diff-lib.c adjust "diff-index" and "diff-files" so
> that i-t-a entries appear as new files in diff-files and nothing in
> diff-index.
> 
> Due to this change, diff-files may start to report "new files" for the
> first time. "add -u" needs to be told about this or it will die in
> denial, screaming "new files can't exist! Reality is wrong." Luckily,
> it's the only one among run_diff_files() callers that needs fixing.
> 
> The test "cache-tree invalidates i-t-a paths" is marked failure
> because I don't think removing "--cached" from "git diff" is the right
> fix. This test relies on "diff --cached" behavior but the behavior now
> has changed. We may need to revisit this test at some point in future.

I can't quite follow that reasoning. If the test describes expected
behavior which the patch breaks, then we should not apply the patch. If
the patch changes behavior in an expected way, then we should change the
test to match.

> Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/add.c           |  1 +
>  diff-lib.c              | 12 ++++++++++++
>  t/t2203-add-intent.sh   | 21 ++++++++++++++++++---
>  t/t4011-diff-symlink.sh | 10 ++++++----
>  4 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 3390933..ee370b0 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
>  		switch (fix_unmerged_status(p, data)) {
>  		default:
>  			die(_("unexpected diff status %c"), p->status);
> +		case DIFF_STATUS_ADDED:
>  		case DIFF_STATUS_MODIFIED:
>  		case DIFF_STATUS_TYPE_CHANGED:
>  			if (add_file_to_index(&the_index, path, data->flags)) {
> diff --git a/diff-lib.c b/diff-lib.c
> index a85c497..714501a 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  					       ce->sha1, !is_null_sha1(ce->sha1),
>  					       ce->name, 0);
>  				continue;
> +			} else if (ce->ce_flags & CE_INTENT_TO_ADD) {
> +				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> +					       EMPTY_BLOB_SHA1_BIN, 0,
> +					       ce->name, 0);
> +				continue;
>  			}
>  
>  			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
> @@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	struct rev_info *revs = o->unpack_data;
>  	int match_missing, cached;
>  
> +	/* i-t-a entries do not actually exist in the index */
> +	if (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {
> +		idx = NULL;
> +		if (!tree)
> +			return;	/* nothing to diff.. */
> +	}
> +
>  	/* if the entry is not checked out, don't examine work tree */
>  	cached = o->index_only ||
>  		(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 2a4a749..42827b8 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -5,10 +5,24 @@ test_description='Intent to add'
>  . ./test-lib.sh
>  
>  test_expect_success 'intent to add' '
> +	test_commit 1 &&
> +	git rm 1.t &&
> +	echo hello >1.t &&
>  	echo hello >file &&
>  	echo hello >elif &&
>  	git add -N file &&
> -	git add elif
> +	git add elif &&
> +	git add -N 1.t
> +'
> +
> +test_expect_success 'git status' '
> +	git status --porcelain | grep -v actual >actual &&
> +	cat >expect <<-\EOF &&
> +	DA 1.t
> +	A  elif
> +	 A file
> +	EOF
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'check result of "add -N"' '
> @@ -43,7 +57,8 @@ 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 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' '
> @@ -62,7 +77,7 @@ test_expect_success 'can "commit -a" with an i-t-a entry' '
>  	git commit -a -m all
>  '
>  
> -test_expect_success 'cache-tree invalidates i-t-a paths' '
> +test_expect_failure 'cache-tree invalidates i-t-a paths' '
>  	git reset --hard &&
>  	mkdir dir &&
>  	: >dir/foo &&
> diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
> index 13e7f62..7452fce 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 and b/file.bin differ
> +	new file mode 100644
> +	index 0000000..d95f3ad
> +	Binary files /dev/null and b/file.bin differ
>  	diff --git a/link.bin b/link.bin
> -	index e69de29..dce41ec 120000
> -	--- a/link.bin
> +	new file mode 120000
> +	index 0000000..dce41ec
> +	--- /dev/null
>  	+++ b/link.bin
>  	@@ -0,0 +1 @@
>  	+file.bin
> 

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56:
>
>> The test "cache-tree invalidates i-t-a paths" is marked failure
>> because I don't think removing "--cached" from "git diff" is the right
>> fix. This test relies on "diff --cached" behavior but the behavior now
>> has changed. We may need to revisit this test at some point in future.
>
> I can't quite follow that reasoning. If the test describes expected
> behavior which the patch breaks, then we should not apply the patch. If
> the patch changes behavior in an expected way, then we should change the
> test to match.

The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a
paths after generating trees, 2012-12-16), which was a fix to an
earlier bug where a cache-tree written out of an index with i-t-a
entries had incorrect information and still claimed it is fully
valid after write-tree rebuilt it.  The test probably should add
another path without i-t-a bit, run the same "diff --cached" with
updated expectation before write-tre, and run the "diff --cached"
again to make sure it produces a result that match the updated
expectation.

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Duy Nguyen]

On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:
> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a
> paths after generating trees, 2012-12-16), which was a fix to an
> earlier bug where a cache-tree written out of an index with i-t-a
> entries had incorrect information and still claimed it is fully
> valid after write-tree rebuilt it.  The test probably should add
> another path without i-t-a bit, run the same "diff --cached" with
> updated expectation before write-tre, and run the "diff --cached"
> again to make sure it produces a result that match the updated
> expectation.

Would adding another non-i-t-a entry help? Before this patch
"diff --cached" after write-tree shows the i-t-a entry only when
eec3e7e4 is applied. But with this patch we don't show i-t-a entry any
more, before or after write-tree, eec3e7e4 makes no visible difference.

We could even revert eec3e7e4 and the outcome of "diff --cached" would
be the same because we just sort of move the "invalidation" part from
cache-tree to do_oneway_diff(). Not invalidating would speed up "diff
--cached" when i-t-a entries are present. Still it may be a good idea
to invalidate i-t-a paths to be on the safe side. Perhaps a patch like
this to resurrect the test?

-- 8< --
Subject: t2203: enable 'cache-tree invalidates i-t-a paths' test

This test is disabled in the previous patch because the "diff
--cached" expectation is the same, with or without eec3e7e4 [1] where
this test is added.

But it still is a good idea to invalidate i-t-a paths because the
index does _not_ match the cached-tree exactly.  "diff --cached" may
not care, but other operations might. Update the test to catch this
invalidation instead.

[1] cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 42827b8..1a6c814 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -77,7 +77,7 @@ test_expect_success 'can "commit -a" with an i-t-a entry' '
 	git commit -a -m all
 '
 
-test_expect_failure 'cache-tree invalidates i-t-a paths' '
+test_expect_success 'cache-tree invalidates i-t-a paths' '
 	git reset --hard &&
 	mkdir dir &&
 	: >dir/foo &&
@@ -86,14 +86,14 @@ test_expect_failure 'cache-tree invalidates i-t-a paths' '
 
 	: >dir/bar &&
 	git add -N dir/bar &&
-	git diff --cached --name-only >actual &&
-	echo dir/bar >expect &&
-	test_cmp expect actual &&
-
 	git write-tree >/dev/null &&
-
-	git diff --cached --name-only >actual &&
-	echo dir/bar >expect &&
+	test-dump-cache-tree >actual &&
+	cat >expect <<-\EOF &&
+	invalid                                   (1 subtrees)
+	invalid                                  #(ref)  (1 subtrees)
+	invalid                                  dir/ (0 subtrees)
+	invalid                                  #(ref) dir/ (0 subtrees)
+	EOF
 	test_cmp expect actual
 ' 
-- 8< --

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:
>> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a
>> paths after generating trees, 2012-12-16), which was a fix to an
>> earlier bug where a cache-tree written out of an index with i-t-a
>> entries had incorrect information and still claimed it is fully
>> valid after write-tree rebuilt it.  The test probably should add
>> another path without i-t-a bit, run the same "diff --cached" with
>> updated expectation before write-tre, and run the "diff --cached"
>> again to make sure it produces a result that match the updated
>> expectation.
>
> Would adding another non-i-t-a entry help? Before this patch
> "diff --cached" after write-tree shows the i-t-a entry only when
> eec3e7e4 is applied. But with this patch we don't show i-t-a entry any
> more, before or after write-tree, eec3e7e4 makes no visible difference.
>
> We could even revert eec3e7e4 and the outcome of "diff --cached" would
> be the same because we just sort of move the "invalidation" part from
> cache-tree to do_oneway_diff(). Not invalidating would speed up "diff
> --cached" when i-t-a entries are present. Still it may be a good idea
> to invalidate i-t-a paths to be on the safe side. Perhaps a patch like
> this to resurrect the test?

My unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths
after generating trees, 2012-12-16) fixed was that in this sequence:

    - You prepare an index.

    - You write-tree out of the index, which involves:

      - updating the cache-tree to match the shape of the resulting
        from writing the index out.

      - create tree objects matching all levels of the cache-tree as
        needed on disk.

      - report the top-level tree object name

   - run "diff-index --cached", which can and will take advantage of
     the fact that everything in a subtree below a known-to-be-valid
     cache-tree entry does not have to be checked one-by-one.  If a
     cache-tree says "everything under D/ in the index would hash to
     tree object T" and the HEAD has tree object T at D/, then the
     diff machinery will bypass the entire section in the index
     under D/, which is a valid optimization.

     However, when there is an i-t-a entry, we excluded that entry
     from the tree object computation, its presence did not
     contribute to the tree object name, but still marked the
     cache-tree entries that contain it as valid by mistake.  This
     old bug was what the commit fixed, so an invocation of "diff
     --cached" after a write-tree, even if the index contains an
     i-t-a entry, will not see cache-tree entries that are marked
     valid when they are not.  Instead, "diff --cached" will bypass
     the optimization and makes comparison one-by-one for the index
     entries.

So reverting the fix obviously is not the right thing to do.  If the
tests show different results from two invocations of "diff --cached"
with your patch applied, there is something that is broken by your
patch, because the index and the HEAD does not change across
write-tree in that test.

If on the other hand the tests show the same result from these two
"diff --cached" and the result is different from what the test
expects, that means your patch changed the world order, i.e. an
i-t-a entry used to be treated as if it were adding an empty blob to
the index but it is now treated as non-existent, then that is a good
thing and the only thing we need to update is what the test expects.
I am guessing that instead of expecting dir/bar to be shown, it now
should expect no output?

Does adding an non-i-t-a entry help?  It does not hurt, and it makes
the test uses a non-empty output, making its effect more visible,
which may or may not count as helping.


     

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Duy Nguyen]

On Wed, Mar 18, 2015 at 12:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:
>>> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a
>>> paths after generating trees, 2012-12-16), which was a fix to an
>>> earlier bug where a cache-tree written out of an index with i-t-a
>>> entries had incorrect information and still claimed it is fully
>>> valid after write-tree rebuilt it.  The test probably should add
>>> another path without i-t-a bit, run the same "diff --cached" with
>>> updated expectation before write-tre, and run the "diff --cached"
>>> again to make sure it produces a result that match the updated
>>> expectation.
>>
>> Would adding another non-i-t-a entry help? Before this patch
>> "diff --cached" after write-tree shows the i-t-a entry only when
>> eec3e7e4 is applied. But with this patch we don't show i-t-a entry any
>> more, before or after write-tree, eec3e7e4 makes no visible difference.
>>
>> We could even revert eec3e7e4 and the outcome of "diff --cached" would
>> be the same because we just sort of move the "invalidation" part from
>> cache-tree to do_oneway_diff(). Not invalidating would speed up "diff
>> --cached" when i-t-a entries are present. Still it may be a good idea
>> to invalidate i-t-a paths to be on the safe side. Perhaps a patch like
>> this to resurrect the test?
>
> My unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths
> after generating trees, 2012-12-16) fixed was that in this sequence:
>
>    ...
>
> So reverting the fix obviously is not the right thing to do.  If the
> tests show different results from two invocations of "diff --cached"
> with your patch applied, there is something that is broken by your
> patch, because the index and the HEAD does not change across
> write-tree in that test.

Right. I missed this but I think this is a less important test because
I added a new test to make sure "diff --cached" ("git status" to be
exact) outputs the right thing when i-t-a entries are present.

> If on the other hand the tests show the same result from these two
> "diff --cached" and the result is different from what the test
> expects, that means your patch changed the world order, i.e. an
> i-t-a entry used to be treated as if it were adding an empty blob to
> the index but it is now treated as non-existent, then that is a good
> thing and the only thing we need to update is what the test expects.
> I am guessing that instead of expecting dir/bar to be shown, it now
> should expect no output?

Yes, no output.

> Does adding an non-i-t-a entry help?  It does not hurt, and it makes
> the test uses a non-empty output, making its effect more visible,
> which may or may not count as helping.

But still, if I revert the change in cache-tree.c and force write-tree
to produce valid cache-tree when i-t-a entries are present, this test
still passes incorrectly. This is why I changed the test.
-- 
Duy

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Right. I missed this but I think this is a less important test
> because I added a new test to make sure "diff --cached" ("git
> status" to be exact) outputs the right thing when i-t-a entries
> are present.

OK.

>> If on the other hand the tests show the same result from these two
>> "diff --cached" and the result is different from what the test
>> expects, that means your patch changed the world order, i.e. an
>> i-t-a entry used to be treated as if it were adding an empty blob to
>> the index but it is now treated as non-existent, then that is a good
>> thing and the only thing we need to update is what the test expects.
>> I am guessing that instead of expecting dir/bar to be shown, it now
>> should expect no output?
>
> Yes, no output.

Good, as that means your changes did not break things, right?

> But still, if I revert the change in cache-tree.c and force write-tree
> to produce valid cache-tree when i-t-a entries are present, this test
> still passes incorrectly.

This is worrysome.

Doesn't that suggest that the diff-cache optimization is disabled
and cache-tree that is marked as valid (because you "revert" the
fix) is not looked at?  That is the only explanation that makes
sense to me---you write out a tree omitting i-t-a entries in the
index and update the index without your earlier fix eec3e7e4
(cache-tree: invalidate i-t-a paths after generating trees,
2012-12-16), i.e. marking the cache-tree entries valid.  A later
invocation of diff-cache *should* trust that validity and just say
"the tree and the index match" without even digging into the
individual entries, at which point you should see a bogus result.

If that is the case, it would be a performance regression, which may
be already there before your patches or something your patches may
have introduced.

Ah, wait.

I suspect that it all cancels out.

 * You "add -N dir/bar".  You write-tree.  The tree is written as if
   dir/bar is not there.  cache-tree is updated and it is marked as
   valid (because you deliberately broke the earlier fix you made at
   eec3e7e4).

 * You run "diff-cache".  It walks the HEAD and the cache-tree and
   finds that HEAD:dir and the cache-tree entry for "dir" records
   the same tree object name, and the cache-tree entry is marked
   "valid".  Hence you declare "no differences".

But for the updated definition of "diff-cache", there indeed is no
difference.  The HEAD and the index matches, because dir/bar does
not yet exist.

OK, so I think I can see how the result could work without
invalidating the cache-tree entry that contains i-t-a entries.

It might be even the right thing to do in general.  We can view that
invalidation a workaround to achieve the old behaviour of diff-cache,
which used to report that dir/ are different between HEAD and index.

We can even argue that it is the right thing to mark the cache-tree
entry for dir/ as valid, no matter how many i-t-a entries are in
there, as long as writing out the tree for dir/ out of index results
in the same tree as the tree that is stored as HEAD:dir/.  And from
that viewpoint, keeping the earlier fix (invalidation code) when
these patches are applied _is_ introducing a performance bug.

Now, as you mentioned, there _may_ be codepaths that uses the same
definition of "what's in the index" as what diff-cache used to take
before your patches, and they may be broken by removing the
invalidation.  If we find such codepaths, we should treat their old
behaviour as buggy and fix them, instead of reintroducing the
invalidation and keep their current behaviour, as the new world
order is "i-t-a entries in the index does not yet exist."

Thanks for straightening my confusion out.

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Junio C Hamano]

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

> Ah, wait.
>
> I suspect that it all cancels out.
> ...
> Now, as you mentioned, there _may_ be codepaths that uses the same
> definition of "what's in the index" as what diff-cache used to take
> before your patches, and they may be broken by removing the
> invalidation.  If we find such codepaths, we should treat their old
> behaviour as buggy and fix them, instead of reintroducing the
> invalidation and keep their current behaviour, as the new world
> order is "i-t-a entries in the index does not yet exist."

One potential negative consequence of the new world order I can
immediately think of is this.  In many operations, we try to be
lenient to changes in the working tree as long as the index is
clean.  "read-tree -m" is the most visible one, where we require
that the index and HEAD matches while allowing changes to working
tree paths as long as they do not interfere with the paths that are
involved in the merge.  We need to make sure that the path dir/bar
added by "add -N dir/bar", which in the new world order does not
count as "index is not clean and there is a difference from HEAD",
(1) does not interfere with the mergy operation that wants to touch
dir (which _could_ even be expected to be a file) or dir/bar, and
(2) is not lost because the mergy operation wants to touch dir or
dir/bar, for example.

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Junio C Hamano]

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

> Ah, wait.
>
> I suspect that it all cancels out.
> ...
> OK, so I think I can see how the result could work without
> invalidating the cache-tree entry that contains i-t-a entries.
>
> It might be even the right thing to do in general.  We can view that
> invalidation a workaround to achieve the old behaviour of diff-cache,
> which used to report that dir/ are different between HEAD and index.

So, we somehow left this hanging for a week without progress.  Here
is the version I am going to queue.

Thanks.

-- >8 --
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date: Mon, 16 Mar 2015 20:56:46 +0700
Subject: [PATCH] diff-lib.c: adjust position of i-t-a entries in diff

Entries added by "git add -N" are reminder for the user so that they
don't forget to add them before committing. These entries appear in
the index even though they are not real. Their presence in the index
leads to a confusing "git status" like this:

    On branch master
    Changes to be committed:
            new file:   foo

    Changes not staged for commit:
            modified:   foo

If you do a "git commit", "foo" will not be included even though
"status" reports it as "to be committed". This patch changes the
output to become

    On branch master
    Changes not staged for commit:
            new file:   foo

    no changes added to commit

The two hunks in diff-lib.c adjust "diff-index" and "diff-files" so
that i-t-a entries appear as new files in diff-files and nothing in
diff-index.

Due to this change, diff-files may start to report "new files" for the
first time. "add -u" needs to be told about this or it will die in
denial, screaming "new files can't exist! Reality is wrong." Luckily,
it's the only one among run_diff_files() callers that needs fixing.

Now in the new world order, a hierarchy in the index that contain
i-t-a paths is written out as a tree object as if these i-t-a
entries do not exist, and comparing the index with such a tree
object that would result from writing out the hierarchy will result
in no difference.  Update a test in t2203 that expected the i-t-a
entries to appear as "added to the index" in the comparison to
instead expect no output.

An earlier change eec3e7e4 (cache-tree: invalidate i-t-a paths after
generating trees, 2012-12-16) becomes an unnecessary pessimization
in the new world order---a cache-tree in the index that corresponds
to a hierarchy with i-t-a paths can now be marked as valid and
record the object name of the tree that results from writing a tree
object out of that hierarchy, as it will compare equal to that tree.

Reverting the commit is left for the future, though, as it is purely
a performance issue and no longer affects correctness.

Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c           |  1 +
 diff-lib.c              | 12 ++++++++++++
 t/t2203-add-intent.sh   | 23 +++++++++++++++++++----
 t/t4011-diff-symlink.sh | 10 ++++++----
 4 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
+		case DIFF_STATUS_ADDED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
 			if (add_file_to_index(&the_index, path, data->flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index a85c497..714501a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       ce->sha1, !is_null_sha1(ce->sha1),
 					       ce->name, 0);
 				continue;
+			} else if (ce->ce_flags & CE_INTENT_TO_ADD) {
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       EMPTY_BLOB_SHA1_BIN, 0,
+					       ce->name, 0);
+				continue;
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct rev_info *revs = o->unpack_data;
 	int match_missing, cached;
 
+	/* i-t-a entries do not actually exist in the index */
+	if (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {
+		idx = NULL;
+		if (!tree)
+			return;	/* nothing to diff.. */
+	}
+
 	/* if the entry is not checked out, don't examine work tree */
 	cached = o->index_only ||
 		(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..7c641bf 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
 . ./test-lib.sh
 
 test_expect_success 'intent to add' '
+	test_commit 1 &&
+	git rm 1.t &&
+	echo hello >1.t &&
 	echo hello >file &&
 	echo hello >elif &&
 	git add -N file &&
-	git add elif
+	git add elif &&
+	git add -N 1.t
+'
+
+test_expect_success 'git status' '
+	git status --porcelain | grep -v actual >actual &&
+	cat >expect <<-\EOF &&
+	DA 1.t
+	A  elif
+	 A file
+	EOF
+	test_cmp expect actual
 '
 
 test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,8 @@ 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 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' '
@@ -72,13 +87,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 	: >dir/bar &&
 	git add -N dir/bar &&
 	git diff --cached --name-only >actual &&
-	echo dir/bar >expect &&
+	>expect &&
 	test_cmp expect actual &&
 
 	git write-tree >/dev/null &&
 
 	git diff --cached --name-only >actual &&
-	echo dir/bar >expect &&
+	>expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 13e7f62..7452fce 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 and b/file.bin differ
+	new file mode 100644
+	index 0000000..d95f3ad
+	Binary files /dev/null and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	index e69de29..dce41ec 120000
-	--- a/link.bin
+	new file mode 120000
+	index 0000000..dce41ec
+	--- /dev/null
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin
-- 
2.3.4-437-g9384a34

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Duy Nguyen]

On Thu, Mar 19, 2015 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ah, wait.
>>
>> I suspect that it all cancels out.
>> ...
>> Now, as you mentioned, there _may_ be codepaths that uses the same
>> definition of "what's in the index" as what diff-cache used to take
>> before your patches, and they may be broken by removing the
>> invalidation.  If we find such codepaths, we should treat their old
>> behaviour as buggy and fix them, instead of reintroducing the
>> invalidation and keep their current behaviour, as the new world
>> order is "i-t-a entries in the index does not yet exist."
>
> One potential negative consequence of the new world order I can
> immediately think of is this.  In many operations, we try to be
> lenient to changes in the working tree as long as the index is
> clean.  "read-tree -m" is the most visible one, where we require
> that the index and HEAD matches while allowing changes to working
> tree paths as long as they do not interfere with the paths that are
> involved in the merge.  We need to make sure that the path dir/bar
> added by "add -N dir/bar", which in the new world order does not
> count as "index is not clean and there is a difference from HEAD",
> (1) does not interfere with the mergy operation that wants to touch
> dir (which _could_ even be expected to be a file) or dir/bar, and
> (2) is not lost because the mergy operation wants to touch dir or
> dir/bar, for example.

"read-tree -m" does not invoke diff, does it? If I went with my
previous approach (modifying unpack-trees to ignore i-t-a entries)
then this could be a problem, but because unpack-trees is untouched,
merge operations should not be impacted by this patch. Even if some
other command does "diff --cached" first to abort early, if "diff
--cached" fails to report "HEAD and index are different" as you
described, I would expect unpack-trees to be able to deal with it
anyway.

PS. Sorry for the late response, busy fighting the evil last weekend.
I blame Steam on Linux.
-- 
Duy

Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> "read-tree -m" does not invoke diff, does it? If I went with my
> previous approach (modifying unpack-trees to ignore i-t-a entries)
> then this could be a problem, but because unpack-trees is untouched,
> merge operations should not be impacted by this patch.

Theoretically yes, but not quite.

I wouldn't be surprised if an enterprising soul saw an optimization
opportunity in the "read-tree -m A B" codepath.  When it finds that
a tree in A and a valid cache-tree entry that corresponds to the
tree matches, it could blow away all index entries covered by the
cache-tree entry and replace them with B, either

 (1) unconditionally when "-u" is not given; or

 (2) as long as the working tree matches the index inside that
     directory when running with "-u".

And such an optimization used to be a valid thing to do in the old
world; but (1) will break in the new world, if we drop that
invalidation---the i-t-a entries will be discarded from the index.

As i-t-a is not a norm but an abberration, I'd rather keep the
pessimizing invalidation to keep the door open for such an
optimization for a more common case, and there may be other cases
in which our correctness around i-t-a depends on.