apply --cached --whitespace=fix now failing on items added with "add -N"

apply --cached --whitespace=fix now failing on items added with "add -N"

[Patrick Higgins]

I like to use git to remove trailing whitespace from my files. I use
the following ~/.gitconfig to make this convenient:

[alias]
        wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached
--whitespace=fix;\
                git checkout -- ${1-.} \"$@\"' -"

The wsadd alias doesn't work with new files, so I have to use "git add
-N" on them first. As of a week or two ago, the "apply --cached" step
now fails with the following, assuming a new file named bar containing
"foo " has been added with "add -N":

$ git diff -- "$@" | git apply --cached --whitespace=fix
<stdin>:7: trailing whitespace.
foo
error: bar: already exists in index

The final "git checkout" at the end of wsadd truncates my file. I've
changed the ";" to a "&&" to fix the truncation.

Were there any recent changes to "git apply" that might have caused this?

$ git --version
git version 2.4.3.573.g4eafbef
$ uname -a
Linux <redacted> 3.13.0-53-generic #89-Ubuntu SMP Wed May 20 10:34:39
UTC 2015 x86_64 x86_64 x86_64 GNU/Linux

Re: apply --cached --whitespace=fix now failing on items added with "add -N"

[Duy Nguyen]

On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins <phiggins@google.com> wrote:
> I like to use git to remove trailing whitespace from my files. I use
> the following ~/.gitconfig to make this convenient:
>
> [alias]
>         wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached
> --whitespace=fix;\
>                 git checkout -- ${1-.} \"$@\"' -"
>
> The wsadd alias doesn't work with new files, so I have to use "git add
> -N" on them first. As of a week or two ago, the "apply --cached" step
> now fails with the following, assuming a new file named bar containing
> "foo " has been added with "add -N":
>
> $ git diff -- "$@" | git apply --cached --whitespace=fix
> <stdin>:7: trailing whitespace.
> foo
> error: bar: already exists in index
>
> The final "git checkout" at the end of wsadd truncates my file. I've
> changed the ";" to a "&&" to fix the truncation.
>
> Were there any recent changes to "git apply" that might have caused this?

Probably fallout from this one, merged to 'master' about 5 weeks ago.
I'll have a look at it tomorrow unless somebody does it first

d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16)
-- 
Duy

Re: apply --cached --whitespace=fix now failing on items added with "add -N"

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins <phiggins@google.com> wrote:
>> I like to use git to remove trailing whitespace from my files. I use
>> the following ~/.gitconfig to make this convenient:
>>
>> [alias]
>>         wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached
>> --whitespace=fix;\
>>                 git checkout -- ${1-.} \"$@\"' -"
>>
>> The wsadd alias doesn't work with new files, so I have to use "git add
>> -N" on them first. As of a week or two ago, the "apply --cached" step
>> now fails with the following, assuming a new file named bar containing
>> "foo " has been added with "add -N":
>>
>> $ git diff -- "$@" | git apply --cached --whitespace=fix
>> <stdin>:7: trailing whitespace.
>> foo
>> error: bar: already exists in index
>>
>> The final "git checkout" at the end of wsadd truncates my file. I've
>> changed the ";" to a "&&" to fix the truncation.
>>
>> Were there any recent changes to "git apply" that might have caused this?
>
> Probably fallout from this one, merged to 'master' about 5 weeks ago.
> I'll have a look at it tomorrow unless somebody does it first
>
> d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16)

Yeah, the world order has changed by that commit, and I would expect
to see some more fallouts.

After "add -N", "git diff" used to pretend that an empty blob was in
the index, and showed a modification between an existing "empty" and
the full file contents, and "git apply --cached" was happy to take
that modification.  In the new world order, "git diff" is instructed
to pretend as if the path that was added by "add -N" does not exist
yet in the index at all, but the index still knows about the path,
which is a strange half-born state.  It shows an addition of the
full file contents as a new file.  "git apply --cached" sees an
addition of a new path, which requires that the path does not exist
in the index.  In the new world order, it should be taught to
pretend that these "add -N" paths do not exist in the index, but
that was not done.

Something like the attached (totally untested) may be a good
starting point.

For another potential fallout, try this:

    $ cp COPYING RENAMING
    $ cp COPYING UNTRACKED
    $ >EMPTY
    $ git add -N RENAMING EMPTY
    $ git rm UNTRACKED
    fatal: pathspec 'UNTRACKED' did not match any files
    $ git rm EMPTY RENAMING
    error: the following file has staged content different from both the
    file and the HEAD:
        EMPTY
        RENAMING
    (use -f to force removal)

One could argue that these three should behave the same way, if the
new world order is "path added by 'add -N' does not exist in the
index".

I however think the new world order should be slightly different
from "does not exist in the index", but should be more like "the
index is aware of its presence but has not been told about its
contents yet".  The consequences of this are:

 - "git rm RENAMING" shouldn't say 'did not match any files'.

 - "git rm RENAMING" does not know about 'staged content', so
   complaining about "staged contents different from file and HEAD"
   feels wrong.

Having said that, I do think erroring out and requiring -f is the
right thing when remiving RENAMING and EMPTY.  Unlike contents added
by "git add" without "-N", we do not know what is in the working
tree file at all.  Given that we check and refuse when the contents
are different between the working tree file, the index and the HEAD
even when we know what was in the working tree when "git add"
without "-N" was done, we should keep the safety to prevent
accidental removal of the working tree file, which has the only
existing copy of the user content.

On the other hand, I am also OK if the behaviour was like this:

    $ git rm --cached RENAMING
    ... removed without complaints ...
    $ git rm RENAMING
    error: file 'RENAMING' does not have staged content yet.
    (use -f to force removal)

In any case, here is a "how about this" weather-balloon patch for
"git apply"

 builtin/apply.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 146be97..653191e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3546,12 +3546,23 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 #define EXISTS_IN_INDEX 1
 #define EXISTS_IN_WORKTREE 2
 
+static int exists_in_index(const char *new_name)
+{
+	int pos = cache_name_pos(new_name, strlen(new_name));
+
+	if (pos < 0)
+		return 0;
+	if (active_cache[pos]->ce_flags & CE_INTENT_TO_ADD)
+		return 0;
+	return 1;
+}
+
 static int check_to_create(const char *new_name, int ok_if_exists)
 {
 	struct stat nst;
 
 	if (check_index &&
-	    cache_name_pos(new_name, strlen(new_name)) >= 0 &&
+	    exists_in_index(new_name) &&
 	    !ok_if_exists)
 		return EXISTS_IN_INDEX;
 	if (cached)

[PATCH] apply: fix adding new files on i-t-a entries

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

Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff -
2015-03-16), a normal "git diff" on i-t-a entries would produce a diff
that _adds_ those files, not just adding content to existing and empty
files like before.

This is correct. Unfortunately, applying such a patch on the same
repository that has the same i-t-a entries will fail with message
"already exists in index" because git-apply checks, sees those i-t-a
entries and aborts. git-apply does not realize those are for
bookkeeping only, they do not really exist in the index.

This patch tightens the "exists in index" check, ignoring i-t-a
entries. For fixing the above problem, only the change in
check_to_create() is needed. For other changes,

 - load_current(), reporting "not exists in index" is better than "does
   not match index"

 - check_preimage(), similar to load_current(), but it may also use
   ce_mode from i-t-a entry which is always zero

 - get_current_sha1(), or actually build_fake_ancestor(), we should not
   add i-t-a entries to the temporary index, at least not without also
   adding i-t-a flag back

Since "git add -p" and "git am" use "git apply" underneath, they are
broken too before this patch and fixed now.

Reported-by: Patrick Higgins <phiggins@google.com>
Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I think I'm opening a can of worms with d95d728. There's nothing
 wrong with that patch per se, but with this issue popping up, I need
 to go over all {cache,index}_name_pos call sites and see what would be
 the sensible behavior when i-t-a entries are involved.
 
 So far blame, rm and checkout-entry and "checkout <paths>" are on my
 to-think-or-fix list. But this patch can get in early to fix a real
 regression instead of waiting for one big series. A lot more
 discussions will be had before that series gets in good shape.

 builtin/apply.c       |  8 ++++----
 cache.h               |  2 ++
 read-cache.c          | 12 ++++++++++++
 t/t2203-add-intent.sh | 10 ++++++++++
 4 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 146be97..4f813ac 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch)
 	if (!patch->is_new)
 		die("BUG: patch to %s is not a creation", patch->old_name);
 
-	pos = cache_name_pos(name, strlen(name));
+	pos = exists_in_cache(name, strlen(name));
 	if (pos < 0)
 		return error(_("%s: does not exist in index"), name);
 	ce = active_cache[pos];
@@ -3497,7 +3497,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 	}
 
 	if (check_index && !previous) {
-		int pos = cache_name_pos(old_name, strlen(old_name));
+		int pos = exists_in_cache(old_name, strlen(old_name));
 		if (pos < 0) {
 			if (patch->is_new < 0)
 				goto is_new;
@@ -3551,7 +3551,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	struct stat nst;
 
 	if (check_index &&
-	    cache_name_pos(new_name, strlen(new_name)) >= 0 &&
+	    exists_in_cache(new_name, strlen(new_name)) >= 0 &&
 	    !ok_if_exists)
 		return EXISTS_IN_INDEX;
 	if (cached)
@@ -3824,7 +3824,7 @@ static int get_current_sha1(const char *path, unsigned char *sha1)
 
 	if (read_cache() < 0)
 		return -1;
-	pos = cache_name_pos(path, strlen(path));
+	pos = exists_in_cache(path, strlen(path));
 	if (pos < 0)
 		return -1;
 	hashcpy(sha1, active_cache[pos]->sha1);
diff --git a/cache.h b/cache.h
index 571c98f..6a44cb6 100644
--- a/cache.h
+++ b/cache.h
@@ -341,6 +341,7 @@ extern void free_name_hash(struct index_state *istate);
 #define discard_cache() discard_index(&the_index)
 #define unmerged_cache() unmerged_index(&the_index)
 #define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))
+#define exists_in_cache(name, namelen) exists_in_index(&the_index,(name),(namelen))
 #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))
 #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))
 #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))
@@ -512,6 +513,7 @@ extern int verify_path(const char *path);
 extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
 extern int index_name_pos(const struct index_state *, const char *name, int namelen);
+extern int exists_in_index(const struct index_state *, const char *name, int namelen);
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */
diff --git a/read-cache.c b/read-cache.c
index 5dee4e2..d3b50cb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -506,6 +506,18 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel
 	return index_name_stage_pos(istate, name, namelen, 0);
 }
 
+/* This is the same as index_name_pos, except that i-t-a entries are invisible */
+int exists_in_index(const struct index_state *istate, const char *name, int namelen)
+{
+	int pos = index_name_stage_pos(istate, name, namelen, 0);
+
+	if (pos < 0)
+		return pos;
+	if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)
+		return -pos-1;
+	return pos;
+}
+
 /* Remove entry, return true if there are more entries to go.. */
 int remove_index_entry_at(struct index_state *istate, int pos)
 {
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 7c641bf..a10a96a 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -97,5 +97,15 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 	test_cmp expect actual
 '
 
+test_expect_success 'apply on i-t-a entries' '
+	git init apply &&
+	(
+		cd apply &&
+		echo content >file &&
+		git add -N file &&
+		git diff | git apply --cached
+	)
+'
+
 test_done
 
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Junio C Hamano]

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

> Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff -
> 2015-03-16), a normal "git diff" on i-t-a entries would produce a diff
> that _adds_ those files, not just adding content to existing and empty
> files like before.
>
> This is correct. Unfortunately, applying such a patch on the same
> repository that has the same i-t-a entries will fail with message
> "already exists in index" because git-apply checks, sees those i-t-a
> entries and aborts. git-apply does not realize those are for
> bookkeeping only, they do not really exist in the index.

> This patch tightens the "exists in index" check, ignoring i-t-a
> entries. For fixing the above problem, only the change in
> check_to_create() is needed.

And the first thing I noticed and found a bit disturbing was that
this change (which I think is correct, and happens to match what I
sent out earlier) was the only thing necessary to make your new test
pass.  IOW, the other changes in this patch have no test coverage.

> For other changes,
>
>  - load_current(), reporting "not exists in index" is better than "does
>    not match index"

Is that error reporting the only side effect from this change?

This is only used when falling back to three-way merge while
applying a creation patch.

>  - check_preimage(), similar to load_current(), but it may also use
>    ce_mode from i-t-a entry which is always zero

This is for the normal (non three-way) application and the idea is
the same as load_current() as you said above.

>  - get_current_sha1(), or actually build_fake_ancestor(), we should not
>    add i-t-a entries to the temporary index, at least not without also
>    adding i-t-a flag back

This is part of "am" three-way fallback codepath.  I do not think
the merge-recursive three-way merge code knows and cares about, is
capable of handling, or would even want to deal with i-t-a entries
in the first place, so adding an entry as i-t-a bit would not help.
What the ultimate caller wants from us in this codepath is a tree
object, and that is written out from the temporary index---and that
codepath ignores i-t-a entries, so it is correct to omit them from
the temporary index in the first place.  Unlike the previous two
changes, I think this change deserves a new test.

>  I think I'm opening a can of worms with d95d728. There's nothing
>  wrong with that patch per se, but with this issue popping up, I need
>  to go over all {cache,index}_name_pos call sites and see what would be
>  the sensible behavior when i-t-a entries are involved.

Yeah, I agree that d95d728 should have been a part of a larger
series that changes the world order, instead of a single change that
brings inconsistency to the system.

I cannot offhand convince myself that "apply" is the only casualty;
assuming it is, I think a reasonable way forward is to keep d95d728
and adjust "apply" to the new world order.  Otherwise, i.e. if there
are wider fallouts from d95d728, we may instead want to temporarily
revert it off from 'master', deal with fallouts to "apply" and other
things, before resurrecting it.

Anything that internally uses "diff-index" is suspect, I think.

What do others think?  You seem to ...

>  So far blame, rm and checkout-entry and "checkout <paths>" are on my
>  to-think-or-fix list. But this patch can get in early to fix a real
>  regression instead of waiting for one big series. A lot more
>  discussions will be had before that series gets in good shape.

... think that the damage could be quite extensive, so I am inclined
to say that we first revert d95d728 before going forward.

>  builtin/apply.c       |  8 ++++----
>  cache.h               |  2 ++
>  read-cache.c          | 12 ++++++++++++
>  t/t2203-add-intent.sh | 10 ++++++++++
>  4 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 146be97..4f813ac 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch)
>  	if (!patch->is_new)
>  		die("BUG: patch to %s is not a creation", patch->old_name);
>  
> -	pos = cache_name_pos(name, strlen(name));
> +	pos = exists_in_cache(name, strlen(name));

Something that is named as if it would return yes/no that returns a
real value is not a very welcome idea.

> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */
> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)
> +{
> +	int pos = index_name_stage_pos(istate, name, namelen, 0);
> +
> +	if (pos < 0)
> +		return pos;
> +	if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)
> +		return -pos-1;

This is a useless complexity.  Your callers cannot use the returned
value like this:

	pos = exists_in_cache(...);
        if (pos < 0) {
        	if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)
			; /* ah it actually exists but it is i-t-a */
		else
                        ; /* no it does not really exist */
	} else {
        	; /* yes it is really there at pos */
	}

because they cannot tell two cases apart: (1) you do have i-t-a with
the given name, (2) you do not have the entry but the location you
would insert an entry with such a name is occupied by an unrelated
entry (i.e. with a name that sorts adjacent) that happens to be
i-t-a.

> +	return pos;
> +}

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Junio C Hamano]

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

>>  I think I'm opening a can of worms with d95d728....
>
> I cannot offhand convince myself that "apply" is the only casualty;
> assuming it is, I think a reasonable way forward is to keep d95d728
> and adjust "apply" to the new world order.  Otherwise, i.e. if there
> are wider fallouts from d95d728, we may instead want to temporarily
> revert it off from 'master', deal with fallouts to "apply" and other
> things, before resurrecting it.
>
> Anything that internally uses "diff-index" is suspect, I think.
>
> What do others think?  You seem to ...
>
>>  So far blame, rm and checkout-entry and "checkout <paths>" are on my
>>  to-think-or-fix list. But this patch can get in early to fix a real
>>  regression instead of waiting for one big series. A lot more
>>  discussions will be had before that series gets in good shape.
>
> ... think that the damage could be quite extensive, so I am inclined
> to say that we first revert d95d728 before going forward.

Let's do this on 'nd/diff-i-t-a' topic and merge the result
immediately to 'master' for now.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 23 Jun 2015 10:27:47 -0700
Subject: [PATCH] Revert "diff-lib.c: adjust position of i-t-a entries in diff"

This reverts commit d95d728aba06a34394d15466045cbdabdada58a2.

It turns out that many other commands that need to interact with the
result of running diff-files and diff-index, e.g.  "git apply", "git
rm", etc., need to be adjusted to the new world order it brings in.
For example, it would break this sequence to correct a whitespace
breakage in the parts you changed:

	git add -N file
	git diff --cached file | git apply --cached --whitespace=fix
	git checkout file

In the old world order, "diff" showed a patch to modify an existing
empty file by adding its full contents, and "apply" updated the
index by modifying the existing empty blob (which is what an
Intent-to-Add entry records in the index) with that patch.

In the new world order, "diff" shows a patch to create a new file
with its full contents, but because "apply" thinks that the i-t-a
entry already exists in the index, it refused to accept a creation.

Adjusting "apply" to this new world order is easy, but we need to
assess the extent of the damage to the rest of the system the new
world order brought in before going forward and adjust them all,
after which we can resurrect the commit being reverted here.
---
 builtin/add.c           |  1 -
 diff-lib.c              | 12 ------------
 t/t2203-add-intent.sh   | 23 ++++-------------------
 t/t4011-diff-symlink.sh | 10 ++++------
 4 files changed, 8 insertions(+), 38 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ee370b0..3390933 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,7 +63,6 @@ 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 714501a..a85c497 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,11 +212,6 @@ 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,
@@ -381,13 +376,6 @@ 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 7c641bf..2a4a749 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,24 +5,10 @@ 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 -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
+	git add elif
 '
 
 test_expect_success 'check result of "add -N"' '
@@ -57,8 +43,7 @@ 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) = 0 &&
-	test $(git diff --name-only -- nitfol | wc -l) = 1
+	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -87,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 &&
-	>expect &&
+	echo dir/bar >expect &&
 	test_cmp expect actual &&
 
 	git write-tree >/dev/null &&
 
 	git diff --cached --name-only >actual &&
-	>expect &&
+	echo dir/bar >expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 7452fce..13e7f62 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -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.4.4-598-g4ab0ce8

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Junio C Hamano]

On Tue, Jun 23, 2015 at 9:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> -     pos = cache_name_pos(name, strlen(name));
>> +     pos = exists_in_cache(name, strlen(name));
>
> Something that is named as if it would return yes/no that returns a
> real value is not a very welcome idea.
>
>> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */
>> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)
>> +{
>> +     int pos = index_name_stage_pos(istate, name, namelen, 0);
>> +
>> +     if (pos < 0)
>> +             return pos;
>> +     if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)
>> +             return -pos-1;
>
> This is a useless complexity.  Your callers cannot use the returned
> value like this:
>
>         pos = exists_in_cache(...);
>         if (pos < 0) {
>                 if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)
>                         ; /* ah it actually exists but it is i-t-a */
>                 else
>                         ; /* no it does not really exist */
>         } else {
>                 ; /* yes it is really there at pos */
>         }
>
> because they cannot tell two cases apart: (1) you do have i-t-a with
> the given name, (2) you do not have the entry but the location you
> would insert an entry with such a name is occupied by an unrelated
> entry (i.e. with a name that sorts adjacent) that happens to be
> i-t-a.

Also, the callers cannot even use that return value in the usual way they
would use the return value from index_name_pos(), either.

    pos = exists_in_cache(...);
    if (pos < 0) {
        /* ah, it does not exist, so... */
        pos = -1 - pos;
        /*
         * ... it is OK to shift active_cache[pos..] by one and add our
         * entry at active_cache[pos]
         */
   } else {
        /* it exists, so update in place */
        ;
   }

So, returning pos that smells like a return value from index_name_pos()
only has an effect of confusing callers into buggy code, I am afraid. The
callers that care need to be updated to check for ce_flags after finding the
entry with index_name_pos() the usual way if you want to avoid search in
the index_state->cache[] twice, and the callers that are only interested in
knowing if an entry "exists" are better off with an exists_in_cache() that
returns Yes/No and not a confusing and useless "pos", I think.

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Duy Nguyen]

On Tue, Jun 23, 2015 at 11:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This patch tightens the "exists in index" check, ignoring i-t-a
>> entries. For fixing the above problem, only the change in
>> check_to_create() is needed.
>
> And the first thing I noticed and found a bit disturbing was that
> this change (which I think is correct, and happens to match what I
> sent out earlier) was the only thing necessary to make your new test
> pass.  IOW, the other changes in this patch have no test coverage.

Because to be honest I don't understand these code well enough to know
how to trigger it :)

>> For other changes,
>>
>>  - load_current(), reporting "not exists in index" is better than "does
>>    not match index"
>
> Is that error reporting the only side effect from this change?

The only thing that I can see. If an i-t-a entry is returned, it can't
get past verify_index_match because ce_match_stat(). Hmm.. no the
on-disk version is gone, we'll get to checkout_target() where it will
"restore" the entry with an empty file. This is related to checkout
that I will continue later below.

>>  - get_current_sha1(), or actually build_fake_ancestor(), we should not
>>    add i-t-a entries to the temporary index, at least not without also
>>    adding i-t-a flag back
>
> This is part of "am" three-way fallback codepath.  I do not think
> the merge-recursive three-way merge code knows and cares about, is
> capable of handling, or would even want to deal with i-t-a entries
> in the first place, so adding an entry as i-t-a bit would not help.
> What the ultimate caller wants from us in this codepath is a tree
> object, and that is written out from the temporary index---and that
> codepath ignores i-t-a entries, so it is correct to omit them from
> the temporary index in the first place.  Unlike the previous two
> changes, I think this change deserves a new test.

Will do, after I study some more about this apply.c.

>
>>  I think I'm opening a can of worms with d95d728. There's nothing
>>  wrong with that patch per se, but with this issue popping up, I need
>>  to go over all {cache,index}_name_pos call sites and see what would be
>>  the sensible behavior when i-t-a entries are involved.
>
> Yeah, I agree that d95d728 should have been a part of a larger
> series that changes the world order, instead of a single change that
> brings inconsistency to the system.
>
> I cannot offhand convince myself that "apply" is the only casualty;
> assuming it is, I think a reasonable way forward is to keep d95d728
> and adjust "apply" to the new world order.  Otherwise, i.e. if there
> are wider fallouts from d95d728, we may instead want to temporarily
> revert it off from 'master', deal with fallouts to "apply" and other
> things, before resurrecting it.
>
> Anything that internally uses "diff-index" is suspect, I think.

Yeah that's one or two more grep runs and more reading.

> What do others think?  You seem to ...
>
>>  So far blame, rm and checkout-entry and "checkout <paths>" are on my
>>  to-think-or-fix list. But this patch can get in early to fix a real
>>  regression instead of waiting for one big series. A lot more
>>  discussions will be had before that series gets in good shape.
>
> ... think that the damage could be quite extensive, so I am inclined
> to say that we first revert d95d728 before going forward.

I'm not opposed to reverting if you think it's the safest option and I
will report back soon after grepping diff-index. But those I mentioned
above have more to do with the fact that an i-t-a entry does exist in
the index in a normal way, so reverting does not help.

Take checkout for example, when you do "git checkout -- foo" where foo
is i-t-a, the file foo on disk will be emptied because the SHA-1 in
the i-t-a entry is an empty blob, mostly to help "git diff". I think
it should behave as if foo is not i-t-a: checkout should error out
about not matching pathspec, or at least not destroy "foo" on disk. To
me, when "ce" is an i-t-a entry, only i-t-a flag and ce_name are
valid, the rest of "ce" should never be accessed.

blame.c's situation is close to check_preimage() where it may read
zero from ce_mode. It may be ok for check_preimage() to take zero as
mode, but I think this is like fixed size buffer vs strbuf again. It
works now, but if the code is reorganized or refactored, then it may
or may not work. Better be safe than sorry and avoid reading something
we should not read in the first place.

>>  builtin/apply.c       |  8 ++++----
>>  cache.h               |  2 ++
>>  read-cache.c          | 12 ++++++++++++
>>  t/t2203-add-intent.sh | 10 ++++++++++
>>  4 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 146be97..4f813ac 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch)
>>       if (!patch->is_new)
>>               die("BUG: patch to %s is not a creation", patch->old_name);
>>
>> -     pos = cache_name_pos(name, strlen(name));
>> +     pos = exists_in_cache(name, strlen(name));
>
> Something that is named as if it would return yes/no that returns a
> real value is not a very welcome idea.

Yeah. But I don't want the caller to call cache_name_pos again to get
"pos". If I can't find a better name, I'll probably go with
cache_name_pos_without_ita().

>> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */
>> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)
>> +{
>> +     int pos = index_name_stage_pos(istate, name, namelen, 0);
>> +
>> +     if (pos < 0)
>> +             return pos;
>> +     if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)
>> +             return -pos-1;
>
> This is a useless complexity.  Your callers cannot use the returned
> value like this:
>
>         pos = exists_in_cache(...);
>         if (pos < 0) {
>                 if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)
>                         ; /* ah it actually exists but it is i-t-a */
>                 else
>                         ; /* no it does not really exist */
>         } else {
>                 ; /* yes it is really there at pos */
>         }
>
> because they cannot tell two cases apart: (1) you do have i-t-a with
> the given name, (2) you do not have the entry but the location you
> would insert an entry with such a name is occupied by an unrelated
> entry (i.e. with a name that sorts adjacent) that happens to be
> i-t-a.

OK so either return -1 when the entry does not exist or is i-t-a and
non-negative otherwise.
-- 
Duy

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Take checkout for example, when you do "git checkout -- foo" where foo
> is i-t-a, the file foo on disk will be emptied because the SHA-1 in
> the i-t-a entry is an empty blob, mostly to help "git diff". I think
> it should behave as if foo is not i-t-a: checkout should error out
> about not matching pathspec, or at least not destroy "foo" on disk. To
> me, when "ce" is an i-t-a entry, only i-t-a flag and ce_name are
> valid, the rest of "ce" should never be accessed.
>
> blame.c's situation is close to check_preimage() where it may read
> zero from ce_mode. It may be ok for check_preimage() to take zero as
> mode, but I think this is like fixed size buffer vs strbuf again. It
> works now, but if the code is reorganized or refactored, then it may
> or may not work. Better be safe than sorry and avoid reading something
> we should not read in the first place.

All of the above say there _are_ some codepaths that want to treat
the existence of a path in the index not as <exists, does not exist>
boolean but as <exists, i-t-a, does not exist> tristate.  I do not
think we disagree on that.

But that is different from saying that it is always OK to treat
i-t-a entries the same way as "does not exist" non-entries.

Internal "diff-index --cached" is used for another reason you did
not mention (and scripted Porcelains literally use that externally
for the same reason).  When we start a mergy operation, we say it is
OK if the working tree has local changes relative to the index, but
we require the index does not have any modifications since the HEAD
was read.

	Side note: some codepaths insist that "diff-index --cached"
        and "diff-files" are both clean, so d95d728a is harmless;
        the former may say "clean" on i-t-a entries more than
        before, but the latter will still catch the difference
        between the index and the working tree and stop the caller
        from going forward.

With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff,
2015-03-16)'s world view, an empty output from "diff-index --cached"
no longer means that.  Entries added with any "git add -N" are not
reported, so we would go ahead to record the merge result on top of
that "half-dirty" index.

	Side note: a merge based on unpack-trees has an extra layer
	of safety that unpack_trees() does not ignore i-t-a entry as
	"not exist (yet)" and notices that the path does exist in
	the index but not in HEAD.  But that just shows that some
	parts of the world do need to consider that having an i-t-a
	in the index makes it different from HEAD.

If the mergy operation goes without any conflict, the next thing we
do typically is to write the index out as a tree (to record in a new
commit, etc.) and we are OK only in that particular case, because
i-t-a entries are ignored.  But what would happen when the mergy
operation conflicts?  I haven't thought about that fully, but I
doubt it is a safe thing to do in general.

But that is just one example that there are also other codepaths
that do not want to be fooled into thinking that i-t-a entries do
not exist in the index at all.

All we learned from the above discussion is that unconditionally
declaring that adding i-t-a entries to the index without doing
anything else should keep the index compare the same to HEAD.

If d95d728a were only about what wt_status.c sees (and gets reported
in "git status" output), which was what the change wanted to address
anyway, we didn't have to have this discussion.  Without realizing
that two kinds of callers want different view out of "diff-index
--cached" and "diff-files", we broke half the world by changing the
world order unconditionally for everybody, I am afraid.

Perhaps a good and safe way forward to resurrect what d95d728a
wanted to do is to first add an option to tell run_diff_index() and
run_diff_files() which behaviour the caller wants to see, add that
only to the caller in wt-status.c?  Then incrementally pass that
option from more callsites that we are absolutely certain that want
this different worldview with respect to i-t-a?

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Duy Nguyen]

On Thu, Jun 25, 2015 at 12:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Internal "diff-index --cached" is used for another reason you did
> not mention (and scripted Porcelains literally use that externally
> for the same reason).  When we start a mergy operation, we say it is
> OK if the working tree has local changes relative to the index, but
> we require the index does not have any modifications since the HEAD
> was read.
>
>         Side note: some codepaths insist that "diff-index --cached"
>         and "diff-files" are both clean, so d95d728a is harmless;
>         the former may say "clean" on i-t-a entries more than
>         before, but the latter will still catch the difference
>         between the index and the working tree and stop the caller
>         from going forward.
>
> With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff,
> 2015-03-16)'s world view, an empty output from "diff-index --cached"
> no longer means that.  Entries added with any "git add -N" are not
> reported, so we would go ahead to record the merge result on top of
> that "half-dirty" index.
>
>         Side note: a merge based on unpack-trees has an extra layer
>         of safety that unpack_trees() does not ignore i-t-a entry as
>         "not exist (yet)" and notices that the path does exist in
>         the index but not in HEAD.  But that just shows that some
>         parts of the world do need to consider that having an i-t-a
>         in the index makes it different from HEAD.
>
> If the mergy operation goes without any conflict, the next thing we
> do typically is to write the index out as a tree (to record in a new
> commit, etc.) and we are OK only in that particular case, because
> i-t-a entries are ignored.  But what would happen when the mergy
> operation conflicts?  I haven't thought about that fully, but I
> doubt it is a safe thing to do in general.
>
> But that is just one example that there are also other codepaths
> that do not want to be fooled into thinking that i-t-a entries do
> not exist in the index at all.

I think it's clear that you need to revert that commit. I didn't see
this at all when I made the commit.

> All we learned from the above discussion is that unconditionally
> declaring that adding i-t-a entries to the index without doing
> anything else should keep the index compare the same to HEAD.
>
> If d95d728a were only about what wt_status.c sees (and gets reported
> in "git status" output), which was what the change wanted to address
> anyway, we didn't have to have this discussion.  Without realizing
> that two kinds of callers want different view out of "diff-index
> --cached" and "diff-files", we broke half the world by changing the
> world order unconditionally for everybody, I am afraid.
>
> Perhaps a good and safe way forward to resurrect what d95d728a
> wanted to do is to first add an option to tell run_diff_index() and
> run_diff_files() which behaviour the caller wants to see, add that
> only to the caller in wt-status.c?  Then incrementally pass that
> option from more callsites that we are absolutely certain that want
> this different worldview with respect to i-t-a?

Agreed.
-- 
Duy

Re: [PATCH] apply: fix adding new files on i-t-a entries

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> I think it's clear that you need to revert that commit. I didn't see
> this at all when I made the commit.

I didn't either, and no other reviewers did. But now we know it was
not sufficient, so let's see...

>> Perhaps a good and safe way forward to resurrect what d95d728a
>> wanted to do is to first add an option to tell run_diff_index() and
>> run_diff_files() which behaviour the caller wants to see, add that
>> only to the caller in wt-status.c?  Then incrementally pass that
>> option from more callsites that we are absolutely certain that want
>> this different worldview with respect to i-t-a?
>
> Agreed.

OK.  Perhaps then first I should do that revert and we'll
incrementally rebuild on top.

Thanks.

[PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

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

On Thu, Jun 25, 2015 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Perhaps a good and safe way forward to resurrect what d95d728a
>>> wanted to do is to first add an option to tell run_diff_index() and
>>> run_diff_files() which behaviour the caller wants to see, add that
>>> only to the caller in wt-status.c?  Then incrementally pass that
>>> option from more callsites that we are absolutely certain that want
>>> this different worldview with respect to i-t-a?
>>
>> Agreed.
>
> OK.  Perhaps then first I should do that revert and we'll
> incrementally rebuild on top.

Here comes the rebuild. This adds --shift-ita option (and an internal
flag) to enable this behavior. Those only take the last two patches.
The remaining is to make sure we handle i-t-a entries correctly in
some commands.

Nguyễn Thái Ngọc Duy (8):
  blame: remove obsolete comment
  Add and use convenient macro ce_intent_to_add()
  apply: fix adding new files on i-t-a entries
  apply: make sure check_preimage() does not leave empty file on error
  checkout(-index): do not checkout i-t-a entries
  grep: make it clear i-t-a entries are ignored
  diff.h: extend "flags" field to 64 bits because we're out of bits
  Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

 Documentation/diff-options.txt |  6 +++
 builtin/apply.c                | 13 ++++---
 builtin/blame.c                |  5 ---
 builtin/checkout-index.c       |  5 ++-
 builtin/checkout.c             |  2 +
 builtin/commit.c               |  2 +-
 builtin/grep.c                 |  2 +-
 builtin/rm.c                   |  2 +-
 cache-tree.c                   |  2 +-
 cache.h                        |  1 +
 diff-lib.c                     | 18 ++++++++-
 diff.c                         |  4 +-
 diff.h                         |  9 +++--
 read-cache.c                   |  4 +-
 t/t2203-add-intent.sh          | 83 +++++++++++++++++++++++++++++++++++++++++-
 wt-status.c                    |  2 +
 16 files changed, 134 insertions(+), 26 deletions(-)

-- 
2.3.0.rc1.137.g477eb31

[PATCH 1/8] blame: remove obsolete comment

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

That "someday" in the comment happened two years later in
b65982b (Optimize "diff-index --cached" using cache-tree - 2009-05-20)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/blame.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..942f302 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2379,11 +2379,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	ce->ce_mode = create_ce_mode(mode);
 	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 
-	/*
-	 * We are not going to write this out, so this does not matter
-	 * right now, but someday we might optimize diff-index --cached
-	 * with cache-tree information.
-	 */
 	cache_tree_invalidate_path(&the_index, path);
 
 	return commit;
-- 
2.3.0.rc1.137.g477eb31

[PATCH 2/8] Add and use convenient macro ce_intent_to_add()

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/rm.c | 2 +-
 cache-tree.c | 2 +-
 cache.h      | 1 +
 read-cache.c | 4 ++--
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 80b972f..8829b09 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -211,7 +211,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		 * "intent to add" entry.
 		 */
 		if (local_changes && staged_changes) {
-			if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))
+			if (!index_only || !ce_intent_to_add(ce))
 				string_list_append(&files_staged, name);
 		}
 		else if (!index_only) {
diff --git a/cache-tree.c b/cache-tree.c
index feace8b..c865bd2 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -377,7 +377,7 @@ static int update_one(struct cache_tree *it,
 		 * they are not part of generated trees. Invalidate up
 		 * to root to force cache-tree users to read elsewhere.
 		 */
-		if (ce->ce_flags & CE_INTENT_TO_ADD) {
+		if (ce_intent_to_add(ce)) {
 			to_invalidate = 1;
 			continue;
 		}
diff --git a/cache.h b/cache.h
index 4e25271..41d811c 100644
--- a/cache.h
+++ b/cache.h
@@ -241,6 +241,7 @@ static inline unsigned create_ce_flags(unsigned stage)
 #define ce_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE)
 #define ce_skip_worktree(ce) ((ce)->ce_flags & CE_SKIP_WORKTREE)
 #define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)
+#define ce_intent_to_add(ce) ((ce)->ce_flags & CE_INTENT_TO_ADD)
 
 #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
 static inline unsigned int create_ce_mode(unsigned int mode)
diff --git a/read-cache.c b/read-cache.c
index 89dbc08..f849cbd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -327,7 +327,7 @@ int ie_match_stat(const struct index_state *istate,
 	 * by definition never matches what is in the work tree until it
 	 * actually gets added.
 	 */
-	if (ce->ce_flags & CE_INTENT_TO_ADD)
+	if (ce_intent_to_add(ce))
 		return DATA_CHANGED | TYPE_CHANGED | MODE_CHANGED;
 
 	changed = ce_match_stat_basic(ce, st);
@@ -1251,7 +1251,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 
 			if (cache_errno == ENOENT)
 				fmt = deleted_fmt;
-			else if (ce->ce_flags & CE_INTENT_TO_ADD)
+			else if (ce_intent_to_add(ce))
 				fmt = added_fmt; /* must be before other checks */
 			else if (changed & TYPE_CHANGED)
 				fmt = typechange_fmt;
-- 
2.3.0.rc1.137.g477eb31

[PATCH 3/8] apply: fix adding new files on i-t-a entries

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

Applying a patch that adds a file when that file is registered with "git
add -N" will fail with message "already exists in index" because
git-apply checks, sees those i-t-a entries and aborts. git-apply does
not realize those are for bookkeeping only, they do not really exist in
the index. This patch tightens the "exists in index" check, ignoring
i-t-a entries.

Reported-by: Patrick Higgins <phiggins@google.com>
Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/apply.c       |  9 +++++----
 t/t2203-add-intent.sh | 13 +++++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 54aba4e..76b58a1 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3553,10 +3553,11 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 {
 	struct stat nst;
 
-	if (check_index &&
-	    cache_name_pos(new_name, strlen(new_name)) >= 0 &&
-	    !ok_if_exists)
-		return EXISTS_IN_INDEX;
+	if (check_index && !ok_if_exists) {
+		int pos = cache_name_pos(new_name, strlen(new_name));
+		if (pos >= 0 && !ce_intent_to_add(active_cache[pos]))
+			return EXISTS_IN_INDEX;
+	}
 	if (cached)
 		return 0;
 
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..bb5ef2b 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -82,5 +82,18 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 	test_cmp expect actual
 '
 
+test_expect_success 'apply adds new file on i-t-a entry' '
+	git init apply &&
+	(
+		cd apply &&
+		echo newcontent >newfile &&
+		git add newfile &&
+		git diff --cached >patch &&
+		rm .git/index &&
+		git add -N newfile &&
+		git apply --cached patch
+	)
+'
+
 test_done
 
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH 3/8] apply: fix adding new files on i-t-a entries

[Junio C Hamano]

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

> Applying a patch that adds a file when that file is registered with "git
> add -N" will fail with message "already exists in index" because
> git-apply checks, sees those i-t-a entries and aborts. git-apply does
> not realize those are for bookkeeping only, they do not really exist in
> the index. This patch tightens the "exists in index" check, ignoring
> i-t-a entries.

Suppose that you came up with some contents to register at path F in
your working tree, told Git about your intention with "add -N F",
and then tried to apply a patch that wants to _create_ F:

Without this patch, we would say "F already exists so a patch to
create is incompatible with our current state".

With this patch, we do not say that, but instead we'll hopefully
trigger "does it exist in the working tree" check, unless you are
running under "--cached".

Which means that this change will not lead to data loss in the
"untracked" file F in the working tree that was merely added to the
index with i-t-a bit.

As you did not say the equivalent of the last paragraph above in the
log message, my knee-jerk reaction was "why could this possibly be a
good idea?"

Please try again with a better log message.

Thanks.

[PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error

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

The test case probably describes the test scenario the best. We have a
patch to modify some file but the base file is gone. Because
check_preimage() finds an index entry with the same old_name, it tries
to restore the on-disk base file with cached content with
checkout_target() and move on.

If this old_name is i-t-a, before this patch "apply -3" will call
checkout_target() which will write an empty file (i-t-a entries always
have "empty blob" SHA-1), then it'll fail at verify_index_match()
(i-t-a entries are always "modified") and the operation is aborted.

This empty file should not be created. Compare to the case where
old_name does not exist in index, "apply -3" fails with a different
error message "...: does not exist" but it does not touch
worktree. This patch makes sure the same happens to i-t-a entries.

load_current() shares the same code pattern (look up an index entry,
if on-disk version is not found, restore using the cached
version). Fix it too (even though it's not exercised in any test case)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/apply.c       |  4 ++--
 t/t2203-add-intent.sh | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 76b58a1..b185c97 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch)
 		die("BUG: patch to %s is not a creation", patch->old_name);
 
 	pos = cache_name_pos(name, strlen(name));
-	if (pos < 0)
+	if (pos < 0 || ce_intent_to_add(active_cache[pos]))
 		return error(_("%s: does not exist in index"), name);
 	ce = active_cache[pos];
 	if (lstat(name, &st)) {
@@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 
 	if (check_index && !previous) {
 		int pos = cache_name_pos(old_name, strlen(old_name));
-		if (pos < 0) {
+		if (pos < 0 || ce_intent_to_add(active_cache[pos])) {
 			if (patch->is_new < 0)
 				goto is_new;
 			return error(_("%s: does not exist in index"), old_name);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index bb5ef2b..96c8755 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' '
 	)
 '
 
+test_expect_success 'apply:check_preimage() not creating empty file' '
+	git init check-preimage &&
+	(
+		cd check-preimage &&
+		echo oldcontent >newfile &&
+		git add newfile &&
+		echo newcontent >newfile &&
+		git diff >patch &&
+		rm .git/index &&
+		git add -N newfile &&
+		rm newfile &&
+		test_must_fail git apply -3 patch &&
+		! test -f newfile
+	)
+'
+
 test_done
 
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error

[Junio C Hamano]

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

> The test case probably describes the test scenario the best. We have a
> patch to modify some file but the base file is gone. Because
> check_preimage() finds an index entry with the same old_name, it tries
> to restore the on-disk base file with cached content with
> checkout_target() and move on.
>
> If this old_name is i-t-a, before this patch "apply -3" will call
> checkout_target() which will write an empty file (i-t-a entries always
> have "empty blob" SHA-1), then it'll fail at verify_index_match()
> (i-t-a entries are always "modified") and the operation is aborted.
>
> This empty file should not be created. Compare to the case where
> old_name does not exist in index, "apply -3" fails with a different
> error message "...: does not exist" but it does not touch
> worktree. This patch makes sure the same happens to i-t-a entries.

This part (unlike 3/8) is very well explained, and makes sense to
me.

> load_current() shares the same code pattern (look up an index entry,
> if on-disk version is not found, restore using the cached
> version).

The "fall-back to the cached one" is very much deliberate to allow
us to work with an empty working tree as long as the index is
correct, so this perhaps makes sense.  If the user said "add -N F",
and F is involved in the patch application, we do not want to
consider F exists in the index and we would instead want to pretend
that we do not have F ourselves.  It does not even make sense to do
"apply -3" for such a path, and rejecting with "does not exist" is a
good thing to do.

Good.

> Fix it too (even though it's not exercised in any test case)

Hmm, as this is adding new test for the other case, perhaps this
case is covered by another one, too?

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/apply.c       |  4 ++--
>  t/t2203-add-intent.sh | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 76b58a1..b185c97 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch)
>  		die("BUG: patch to %s is not a creation", patch->old_name);
>  
>  	pos = cache_name_pos(name, strlen(name));
> -	if (pos < 0)
> +	if (pos < 0 || ce_intent_to_add(active_cache[pos]))
>  		return error(_("%s: does not exist in index"), name);
>  	ce = active_cache[pos];
>  	if (lstat(name, &st)) {
> @@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>  
>  	if (check_index && !previous) {
>  		int pos = cache_name_pos(old_name, strlen(old_name));
> -		if (pos < 0) {
> +		if (pos < 0 || ce_intent_to_add(active_cache[pos])) {
>  			if (patch->is_new < 0)
>  				goto is_new;
>  			return error(_("%s: does not exist in index"), old_name);
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index bb5ef2b..96c8755 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' '
>  	)
>  '
>  
> +test_expect_success 'apply:check_preimage() not creating empty file' '
> +	git init check-preimage &&
> +	(
> +		cd check-preimage &&
> +		echo oldcontent >newfile &&
> +		git add newfile &&
> +		echo newcontent >newfile &&
> +		git diff >patch &&
> +		rm .git/index &&
> +		git add -N newfile &&
> +		rm newfile &&
> +		test_must_fail git apply -3 patch &&
> +		! test -f newfile
> +	)
> +'
> +
>  test_done

[PATCH 5/8] checkout(-index): do not checkout i-t-a entries

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

The cached blob of i-t-a entries are empty blob. By checkout, we delete
the content we have. Don't do it.

This is done higher up instead of inside checkout_entry() because we
would have limited options in there: silently ignore, loudly ignore,
die. At higher level we can do better reporting. For example, "git
checkout -- foo" will complain that "foo" does not match pathspec, just
like when "foo" is not registered with "git add -N"

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout-index.c |  5 ++++-
 builtin/checkout.c       |  2 ++
 t/t2203-add-intent.sh    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 8028c37..eca975d 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix)
 	while (pos < active_nr) {
 		struct cache_entry *ce = active_cache[pos];
 		if (ce_namelen(ce) != namelen ||
-		    memcmp(ce->name, name, namelen))
+		    memcmp(ce->name, name, namelen) ||
+		    ce_intent_to_add(ce))
 			break;
 		has_same_name = 1;
 		pos++;
@@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length)
 		if (ce_stage(ce) != checkout_stage
 		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
 			continue;
+		if (ce_intent_to_add(ce))
+			continue;
 		if (prefix && *prefix &&
 		    (ce_namelen(ce) <= prefix_length ||
 		     memcmp(prefix, ce->name, prefix_length)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index e1403be..02889d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,
 			 * anything to this entry at all.
 			 */
 			continue;
+		if (ce_intent_to_add(ce))
+			continue;
 		/*
 		 * Either this entry came from the tree-ish we are
 		 * checking the paths out of, or we are checking out
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 96c8755..d0f36a4 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -111,5 +111,39 @@ test_expect_success 'apply:check_preimage() not creating empty file' '
 	)
 '
 
+test_expect_success 'checkout ignores i-t-a' '
+	git init checkout &&
+	(
+		cd checkout &&
+		echo data >file &&
+		git add -N file &&
+		test_must_fail git checkout -- file &&
+		echo data >expected &&
+		test_cmp expected file
+	)
+'
+
+test_expect_success 'checkout-index ignores i-t-a' '
+	(
+		cd checkout &&
+		git checkout-index file &&
+		echo data >expected &&
+		test_cmp expected file
+	)
+'
+
+test_expect_success 'checkout-index --all ignores i-t-a' '
+	(
+		cd checkout &&
+		echo data >anotherfile &&
+		git add anotherfile &&
+		rm anotherfile &&
+		git checkout-index --all &&
+		echo data >expected &&
+		test_cmp expected file &&
+		test_cmp expected anotherfile
+	)
+'
+
 test_done
 
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries

[Junio C Hamano]

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

> The cached blob of i-t-a entries are empty blob. By checkout, we delete
> the content we have. Don't do it.
>
> This is done higher up instead of inside checkout_entry() because we
> would have limited options in there: silently ignore, loudly ignore,
> die. At higher level we can do better reporting. For example, "git
> checkout -- foo" will complain that "foo" does not match pathspec, just
> like when "foo" is not registered with "git add -N"

Hmm, I am not sure about the example, though.  The user _told_ Git
that 'foo' is a path she cares about so "does not match pathspec" is
a very unfriendly thing to say.  "foo does not exist in the index
hence we cannot check it out" is a very good thing to say, though
(and of course, I agree that not touching the working tree is a good
thing to do).

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/checkout-index.c |  5 ++++-
>  builtin/checkout.c       |  2 ++
>  t/t2203-add-intent.sh    | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 8028c37..eca975d 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix)
>  	while (pos < active_nr) {
>  		struct cache_entry *ce = active_cache[pos];
>  		if (ce_namelen(ce) != namelen ||
> -		    memcmp(ce->name, name, namelen))
> +		    memcmp(ce->name, name, namelen) ||
> +		    ce_intent_to_add(ce))
>  			break;
>  		has_same_name = 1;
>  		pos++;

'pos' here comes from cache_name_pos(), and it is either the
location an entry with the same name exists, or the location a new
entry with the given name would be inserted at.

When we detect that the entry at pos is different from the given
name, we break out of the loop, because we _know_ that an entry with
the same name cannot exist.  Does the same hold for an i-t-a entry
that exactly records the given name?

The answer is yes, as you cannot have an unmerged i-t-a entry.  If a
path marked as i-t-a is in the index, no other entry with the same
name would exist.

So this change is good.  I would actually have introduced another
bool has_i_t_a to make the reporting richer, as it is your primary
reason why you are touching this part of the code instead of
checkout_entry().  The reporting part then would become

	fprintf(stderr, "git checkout-index: %s ", name);
-	if (!has_same_name)
+	if (has_i_t_a)
+       	fprintf(stderr, "is not yet in the cache);
	else if (!has_same_name)
		fprintf(stderr, "is not in the cache");

;-)

> @@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length)
>  		if (ce_stage(ce) != checkout_stage
>  		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
>  			continue;
> +		if (ce_intent_to_add(ce))
> +			continue;

This one is obviously good ;-)

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e1403be..02889d4 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,
>  			 * anything to this entry at all.
>  			 */
>  			continue;
> +		if (ce_intent_to_add(ce))
> +			continue;
>  		/*
>  		 * Either this entry came from the tree-ish we are
>  		 * checking the paths out of, or we are checking out

Hmm, while this does prevent the later code from checking it out, I
am not sure how well this interacts with ps_matched[] logic here.
If the user told Git that 'foo' is a path that she cares about with
"add -N foo", and said "git checkout -- foo", should we be somehow
saying that 'foo' did match but there is nothing to check out, or
something?

Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries

[Duy Nguyen]

Sorry for this waaay too late response, everything (of the series
nd/ita-cleanup) is addressed so far except this..

On Tue, Aug 25, 2015 at 10:36:52AM -0700, Junio C Hamano wrote:
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index e1403be..02889d4 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,
> >  			 * anything to this entry at all.
> >  			 */
> >  			continue;
> > +		if (ce_intent_to_add(ce))
> > +			continue;
> >  		/*
> >  		 * Either this entry came from the tree-ish we are
> >  		 * checking the paths out of, or we are checking out
> 
> Hmm, while this does prevent the later code from checking it out, I
> am not sure how well this interacts with ps_matched[] logic here.
> If the user told Git that 'foo' is a path that she cares about with
> "add -N foo", and said "git checkout -- foo", should we be somehow
> saying that 'foo' did match but there is nothing to check out, or
> something?

How about this? It does not mess with ps_matched logic. But it does
not say "nothing to checkout" at the end either. While we could do
that (in general case, not just because all we are checking out is ita
entries), I'm not sure if such verbosity helps anyone. I'm thinking of
dropping the new warning I added here too..

-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3e141fc..c11fe71 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -328,12 +328,17 @@ static int checkout_paths(const struct checkout_opts *opts,
 	if (opts->merge)
 		unmerge_marked_index(&the_index);
 
-	/* Any unmerged paths? */
 	for (pos = 0; pos < active_nr; pos++) {
 		const struct cache_entry *ce = active_cache[pos];
 		if (ce->ce_flags & CE_MATCHED) {
-			if (!ce_stage(ce))
+			if (!ce_stage(ce)) {
+				if (ce_intent_to_add(ce)) {
+					warning(_("path '%s' is only intended to add"), ce->name);
+					ce->ce_flags &= ~CE_MATCHED;
+				}
 				continue;
+			}
+			/* Any unmerged paths? */
 			if (opts->force) {
 				warning(_("path '%s' is unmerged"), ce->name);
 			} else if (opts->writeout_stage) {
-- 8< --
--
Duy

Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Sorry for this waaay too late response, everything (of the series
> nd/ita-cleanup) is addressed so far except this..
>
> On Tue, Aug 25, 2015 at 10:36:52AM -0700, Junio C Hamano wrote:
>> > diff --git a/builtin/checkout.c b/builtin/checkout.c
>> > index e1403be..02889d4 100644
>> > --- a/builtin/checkout.c
>> > +++ b/builtin/checkout.c
>> > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,
>> >  			 * anything to this entry at all.
>> >  			 */
>> >  			continue;
>> > +		if (ce_intent_to_add(ce))
>> > +			continue;
>> >  		/*
>> >  		 * Either this entry came from the tree-ish we are
>> >  		 * checking the paths out of, or we are checking out
>> 
>> Hmm, while this does prevent the later code from checking it out, I
>> am not sure how well this interacts with ps_matched[] logic here.
>> If the user told Git that 'foo' is a path that she cares about with
>> "add -N foo", and said "git checkout -- foo", should we be somehow
>> saying that 'foo' did match but there is nothing to check out, or
>> something?
>
> How about this? It does not mess with ps_matched logic. But it does
> not say "nothing to checkout" at the end either. While we could do
> that (in general case, not just because all we are checking out is ita
> entries), I'm not sure if such verbosity helps anyone. I'm thinking of
> dropping the new warning I added here too..

I agree that these warnings are unwanted when you run "checkout ."
in a repository with tons of i-t-a paths (but on the other hand,
having tons of i-t-a paths is unusual so the user might want to be
reminded of them--I dunno).

With or without the new warning(), this one looks an improvement
over the previous one to me.

Thanks.

> -- 8< --
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3e141fc..c11fe71 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -328,12 +328,17 @@ static int checkout_paths(const struct checkout_opts *opts,
>  	if (opts->merge)
>  		unmerge_marked_index(&the_index);
>  
> -	/* Any unmerged paths? */
>  	for (pos = 0; pos < active_nr; pos++) {
>  		const struct cache_entry *ce = active_cache[pos];
>  		if (ce->ce_flags & CE_MATCHED) {
> -			if (!ce_stage(ce))
> +			if (!ce_stage(ce)) {
> +				if (ce_intent_to_add(ce)) {
> +					warning(_("path '%s' is only intended to add"), ce->name);
> +					ce->ce_flags &= ~CE_MATCHED;
> +				}
>  				continue;
> +			}
> +			/* Any unmerged paths? */
>  			if (opts->force) {
>  				warning(_("path '%s' is unmerged"), ce->name);
>  			} else if (opts->writeout_stage) {
> -- 8< --
> --
> Duy

[PATCH 6/8] grep: make it clear i-t-a entries are ignored

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

The expression "!S_ISREG(ce)" covers i-t-a entries as well because
ce->ce_mode would be zero then. I could make a comment saying that, but
it's probably better just to comment with code, in case i-t-a entry
content changes in future.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d04f440..f508179 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
 
 	for (nr = 0; nr < active_nr; nr++) {
 		const struct cache_entry *ce = active_cache[nr];
-		if (!S_ISREG(ce->ce_mode))
+		if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
 			continue;
 		if (!ce_path_match(ce, pathspec, NULL))
 			continue;
-- 
2.3.0.rc1.137.g477eb31

[PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits

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

I renamed both "flags" and "touched_flags" fields while making this
patch to make sure I was aware of how these flags were manipulated
(besides DIFF_OPT* macros). So hopefully I didn't miss anything.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c | 2 +-
 diff-lib.c       | 4 ++--
 diff.c           | 2 +-
 diff.h           | 8 +++++---
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 4cbd5ff..95f7296 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -895,7 +895,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			 * submodules which were manually staged, which would
 			 * be really confusing.
 			 */
-			int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+			diff_flags_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 			if (ignore_submodule_arg &&
 			    !strcmp(ignore_submodule_arg, "all"))
 				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
diff --git a/diff-lib.c b/diff-lib.c
index 241a843..ae09034 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 {
 	int changed = ce_match_stat(ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
-		unsigned orig_flags = diffopt->flags;
+		diff_flags_t orig_flags = diffopt->flags;
 		if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
 			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
 		if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
@@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags)
+int index_differs_from(const char *def, diff_flags_t diff_flags)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
diff --git a/diff.c b/diff.c
index 7deac90..2485870 100644
--- a/diff.c
+++ b/diff.c
@@ -4912,7 +4912,7 @@ int diff_can_quit_early(struct diff_options *opt)
 static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
 	int ignored = 0;
-	unsigned orig_flags = options->flags;
+	diff_flags_t orig_flags = options->flags;
 	if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
 		set_diffopt_flags_from_submodule_config(options, path);
 	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
diff --git a/diff.h b/diff.h
index f7208ad..4241aa5 100644
--- a/diff.h
+++ b/diff.h
@@ -110,13 +110,15 @@ enum diff_words_type {
 	DIFF_WORDS_COLOR
 };
 
+typedef uint64_t diff_flags_t;
+
 struct diff_options {
 	const char *orderfile;
 	const char *pickaxe;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
-	unsigned flags;
-	unsigned touched_flags;
+	diff_flags_t flags;
+	diff_flags_t touched_flags;
 
 	/* diff-filter bits */
 	unsigned int filter;
@@ -347,7 +349,7 @@ extern int diff_result_code(struct diff_options *, int);
 
 extern void diff_no_index(struct rev_info *, int, const char **, const char *);
 
-extern int index_differs_from(const char *def, int diff_flags);
+extern int index_differs_from(const char *def, diff_flags_t diff_flags);
 
 extern size_t fill_textconv(struct userdiff_driver *driver,
 			    struct diff_filespec *df,
-- 
2.3.0.rc1.137.g477eb31

Re: [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits

[Junio C Hamano]

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

> I renamed both "flags" and "touched_flags" fields while making this
> patch to make sure I was aware of how these flags were manipulated
> (besides DIFF_OPT* macros). So hopefully I didn't miss anything.

It is a bad taste to use user_defined_t typedef (I think it actually
is a standard violation), isn't it?

The diff-struct is not like objects where we need million copies of
in-core while running.  What do you need many more flags for?

Re: [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits

[Duy Nguyen]

On Wed, Aug 26, 2015 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> I renamed both "flags" and "touched_flags" fields while making this
>> patch to make sure I was aware of how these flags were manipulated
>> (besides DIFF_OPT* macros). So hopefully I didn't miss anything.
>
> It is a bad taste to use user_defined_t typedef (I think it actually
> is a standard violation), isn't it?

Yeah I think you posted a patch somewhere updating CodingGuidelines about this..

> The diff-struct is not like objects where we need million copies of
> in-core while running.  What do you need many more flags for?

We already use all 32 bit flags and I need one more flag. I guess I go
with flags because it's how we add features in diff struct. Adding a
new field instead of extending flags could be dangerous: elsewhere
people copy flags out to a temporary place, do something then restore.
If it's a separate field, it's left in place and bad things could
happen.
-- 
Duy

Re: [PATCH 6/8] grep: make it clear i-t-a entries are ignored

[Junio C Hamano]

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

> The expression "!S_ISREG(ce)" covers i-t-a entries as well because
> ce->ce_mode would be zero then. I could make a comment saying that, but
> it's probably better just to comment with code, in case i-t-a entry
> content changes in future.

OK.  Thansk.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/grep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index d04f440..f508179 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
>  
>  	for (nr = 0; nr < active_nr; nr++) {
>  		const struct cache_entry *ce = active_cache[nr];
> -		if (!S_ISREG(ce->ce_mode))
> +		if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
>  			continue;
>  		if (!ce_path_match(ce, pathspec, NULL))
>  			continue;

Re: [PATCH 6/8] grep: make it clear i-t-a entries are ignored

[Junio C Hamano]

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

> The expression "!S_ISREG(ce)" covers i-t-a entries as well because
> ce->ce_mode would be zero then. I could make a comment saying that, but
> it's probably better just to comment with code, in case i-t-a entry
> content changes in future.

OK.  Thanks.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/grep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index d04f440..f508179 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
>  
>  	for (nr = 0; nr < active_nr; nr++) {
>  		const struct cache_entry *ce = active_cache[nr];
> -		if (!S_ISREG(ce->ce_mode))
> +		if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
>  			continue;
>  		if (!ce_path_match(ce, pathspec, NULL))
>  			continue;