i-t-a entries in git-status, and git-commit

Let's see if I can get the first commit graduated before it turns two years old. Six months to go! This series is about some oddities of intent-to-add entries (aka "git add -N"):

1) An ita entry in git-status shows that it's added in the index (compared to worktree) as a file change, not new file addition. And it shows the entry added to HEAD as an empty version. This is due to an implementation detail that i-t-a entries are registered in the index as empty blobs with a special flag.

2) When you do a "git commit" with no changes whatsoever, but you happen to have some ita entries registered, git-commit does not recognize the resulting commit would be empty and should be caught, unless --allow-empty is given. This has been reported several times.

3) Same symptom as 2) but with initial commit (diff code is not used for detecting empty commits this time). If you only have ita entries in the index by the time you make an initial commit, you'll create an empty commit even if --allow-empty is not specified.

1) and 2) are fixed by changing the position of ita entries in diff code. ita entries should be seen as a new file when compared between worktree and HEAD, and no change when compared between index and HEAD.

Some previous commit made this move globally and was reverted because it could have dangerous unseen side effects, especially in merge code. Now we are moving slowly towards that (first patch), this time we try to handle case by case (e.g. index_differs_from in the second patch).

3) could be handled pretty easily once you know the problem. This is the third patch.

Nguyễn Thái Ngọc Duy (3):
Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" diff-lib.c: enable --shift-ita in index_differs_from()
commit: don't be fooled by ita entries when creating initial commit

 Documentation/diff-options.txt |  7 +++++++
 builtin/commit.c               | 11 ++++++++---
 cache.h                        |  1 +
 diff-lib.c                     | 13 +++++++++++++
 diff.c                         |  2 ++
 diff.h                         |  1 +
 read-cache.c                   | 10 ++++++++++
 sequencer.c                    |  5 +++--
 t/t2203-add-intent.sh          | 41 +++++++++++++++++++++++++++++++++++++++--
 t/t7064-wtstatus-pv2.sh        |  4 ++--
 wt-status.c                    |  7 ++++++-
 11 files changed, 92 insertions(+), 10 deletions(-)

2.8.2.524.g6ff3d78

The original commit d95d728aba06a34394d15466045cbdabdada58a2 was reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we were (and still are) not ready for a new world order. A lot more investigation must be done to see what is impacted. See the 78cc1a5 for details.

This patch takes a smaller and safer step. The new behavior is controlled by shift_ita flag. We can gradually move more diff users to the new behavior after we are sure it's safe to do so. This flag is exposed to outside temporarily as "--shift-ita" for people who prefer "git diff [--cached] --stat" to "git status"

---
 Documentation/diff-options.txt |  7 +++++++
 diff-lib.c                     | 12 ++++++++++++
 diff.c                         |  2 ++
 diff.h                         |  1 +
 t/t2203-add-intent.sh          | 20 ++++++++++++++++++--
 t/t7064-wtstatus-pv2.sh        |  4 ++--
 wt-status.c                    |  7 ++++++-
 7 files changed, 48 insertions(+), 5 deletions(-)

index 7805a0c..e63285c 100644
@@ -575,5 +575,12 @@ endif::git-format-patch[]
 --line-prefix=<prefix>::
 	Prepend an additional prefix to every line of output.
 
+--shift-ita::
+	By default entries added by "git add -N" appear as an existing
+	empty file in "git diff" and a new file in "git diff --cached".
+	This option makes the entry appear as a new file in "git diff"
+	and non-existent in "git diff --cached". Experimental option,
+	could be removed in future.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].

index 3007c85..62d67c8 100644
@@ -214,6 +214,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       !is_null_oid(&ce->oid),
 					       ce->name, 0);
 				continue;
+			} else if (revs->diffopt.shift_ita && ce_intent_to_add(ce)) {
+				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,

@@ -379,6 +384,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 (revs->diffopt.shift_ita && idx && ce_intent_to_add(idx)) {
+		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)));

index c6da383..4178689 100644
@@ -3923,6 +3923,8 @@ int diff_opt_parse(struct diff_options *options,
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 		return parse_ws_error_highlight(options, arg);
+	else if (!strcmp(arg, "--shift-ita"))
+		options->shift_ita = 1;
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))

index ec76a90..5dd4f9c 100644
@@ -146,6 +146,7 @@ struct diff_options {
 	int dirstat_permille;
 	int setup;
 	int abbrev;
+	int shift_ita;
 /* white-space error highlighting */
 #define WSEH_NEW 1
 #define WSEH_CONTEXT 2

index 8f22c43..c6a4648 100755
@@ -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,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git add -N nitfol &&
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+	test $(git diff --name-only --shift-ita HEAD -- nitfol | wc -l) = 0 &&
+	test $(git diff --name-only --shift-ita -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '

index 3012a4d..e319fa2 100755
@@ -246,8 +246,8 @@ test_expect_success 'verify --intent-to-add output' '
 	git add --intent-to-add intent1.add intent2.add &&
 
 	cat >expect <<-EOF &&
-	1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent1.add
-	1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent2.add
+	1 .A N... 000000 000000 100644 $_z40 $_z40 intent1.add
+	1 .A N... 000000 000000 100644 $_z40 $_z40 intent2.add
 	EOF
 
 	git status --porcelain=v2 >actual &&

index 9a14658..5f9b1cd 100644
@@ -437,7 +437,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:
-			die("BUG: worktree status add???");
+			d->mode_worktree = p->two->mode;
 			break;
 
 		case DIFF_STATUS_DELETED:

@@ -547,6 +547,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
+	rev.diffopt.shift_ita = 1;
 	if (!s->show_untracked_files)
 		DIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	if (s->ignore_submodule_arg) {

@@ -570,6 +571,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, &opt);
 
 	DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+	rev.diffopt.shift_ita = 1;
 	if (s->ignore_submodule_arg) {
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	} else {

@@ -605,6 +607,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 
 		if (!ce_path_match(ce, &s->pathspec, NULL))
 			continue;
+		if (ce_intent_to_add(ce))
+			continue;
 		it = string_list_insert(&s->change, ce->name);
 		d = it->util;
 		if (!d) {

@@ -911,6 +915,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 
 	init_revisions(&rev, NULL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
+	rev.diffopt.shift_ita = 1;
 
 	memset(&opt, 0, sizeof(opt));
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;

2.8.2.524.g6ff3d78

Let's stop advertising this as a resurrection of something else. The original that was unconditional was simply broken.

It is very good to refer to it (and its reversion), when justifying why this version takes the particular approach to introduce a new optional behaviour that can be toggled on selectively, by explaining why doing this unconditionally was a broken idea that needed to be reverted later.

But you would need to explain what problem this patch attempts to solve and how before even going into that. The above two paragraphs are backwards.

As I already said, --shift-ita is not quite descriptive and I think it should be renamed to something else, but I kept that in the following attempt to rewrite:

Subject: diff-lib: allow ita entries treated as "not yet exist in index"

When comparing the index and the working tree to show which paths are new, and comparing the tree recorded in the HEAD and the index to see if committing the contents recorded in the index would result in an empty commit, we would want the former comparison to say "these are new paths" and the latter to say "there is no change" for paths that are marked as intent-to-add.

We made a similar attempt at d95d728a ("diff-lib.c: adjust position of i-t-a entries in diff", 2015-03-16), which redefined the semantics of these two comparison modes globally, which was a disastor and had to be reverted at 78cc1a54 ("Revert "diff-lib.c: adjust position of i-t-a entries in diff"", 2015-06-23). To make sure we do not repeat the same mistake, introduce a new internal diffopt option so that this different semantics can be asked for only by callers that ask it, while making sure other unaudited callers will get the same comparison result. This internal option is also exposed temporarily as "--shift-ita" to help experiment.

After reading the three patches through, however, I do not think we use the command line option anywhere. I'm inclined to say that we shouldn't add it at all. Or at least do so in a separate follow-up patch "now we have an internal mechanism, let's expose it anyway" at the end. Which means that the last sentence in my attempted rewrite should go.

The patch to diff-lib.c machinery looks good.

Thanks.

Please do not use that verbatim; it was full of typo and grammo.

We don't use it internally _yet_. I need to go through all the external diff code and see --shift-ita should be there. The end goal is still changing the default behavior and getting rid of --shift-ita, after making sure we don't break stuff. I do use it though because "git diff" is more often run in my workflow than "git status".

It's meant to be a temporary thing (which could last a year or three, depending on how fast I scan through the code base) so I didn't give much thought on naming.

Umm... after a couple of minutes, I still couldn't think of any better. The one-line summary of this change is "correct the position of intent-to-add entries in diff", or as you put it more precisely (with a bit paraphrasing), "make ita entries not exist in index". I don't see any good way to shorten that to one or two words. --ita-not-in-index good enough? Or maybe --[no-]ita-visible-in-index.

Duy

I do not agree with that endgame, and quite honestly I do not want to waste time reviewing such a series.

I do not believe current "diff" behavior wrt. i-t-a entries is right either. There's no point in pursuing this series then. Feel free to revert 3f6d56d (commit: ignore intent-to-add entries instead of refusing - 2012-02-07) and bring back the old i-t-a behavior. All the problems would go away.

Duy

I definitely shouldn't have said that, especially "waste". Many issues around i-t-a and diff make my head hurt when I think about them [*1*], but not wanting to spend time that gets my head hurt and not wanting to waste time are totally different things. My apologies.

I missed something curious in your statement above, i.e. "external diff". I thought we have pretty much got rid of all the invocation of "git diff" via the run_command() interface and we do not need the command line option (we only need the options->shift_ita so that callers like "git status" can seletively ask for it when making internal calls), and that is why I didn't want to see it.

It is way too late to revert 3f6d56d. Even though I think it was overall a mistake to treat i-t-a as "not exist" while committing, people are now used to the behaviour with that change, and we need to make our progress within the constraint of the real world.

[Footnote]

Here is one of the things around i-t-a and diff. If you make "git diff" (between the index and the working tree) report "new" file, it would imply that "git apply" run without "--index" should create an ita entry in the index for symmetry, wouldn't it? That by itself can be seen as an improvement (we no longer would have to say that "git apply patchfile && git commit -a" that is run in a clean state will forget new files the patchfile creates), but it also means we now need a repository in order to run "git apply" (without "--index"), which is a problem, as "git apply" is often used as a better "patch".

"git apply --cached" may also become "interesting". A patch that would apply cleanly to HEAD should apply cleanly if you did this:

    $ git read-tree HEAD
    $ git apply --cached <patch

no matter what the working tree state is. Should a patch that creates a "new" file add contents to the index, or just an i-t-a entry? I could argue it both ways, but either is quite satisfactory and makes my head hurt.

No problem. I do appreciate a straight shoot down though. Many of my topics have been going on for months (ones not in 'pu') and seeing it rejected near the end is worse than stopping working on them early.

I don't know if we have had any external diff calls in our shell-based commands. I don't read them often. Regardless, people do use "git diff" and it should show the "right thing" (I know it's subjective). Or at least be consistent with both git-commit and git-status.

Off topic. This reminds me of an old patch about apply and ita [1] but that one is not the same here

We could detect "no repo available" and ignore the index, I guess.

--cached tells you to put new contents in the index. I-ta entries, being a reminder to add stuff, don't really fit in here because you want to add contents _now_, i think. After a successful "git apply --cached", a "git commit" should contain exactly what the applied patch has. If new files are i-t-a entries instead, then the new commit would not be the same as the patch.

[1] https://public-inbox.org/git/[email protected]/

Duy

... Yeah, and re-reading that one, I think that sort-of makes sense. I am hesitant to take it out of context, though. I wonder how it would interact with that broken mode of "git apply" where it can take patches to the same path number of times...

Another downside is that "git reset --hard" after "git apply" will suddenly start removing them. Perhaps that can be seen a feature? I dunno.

This function is basically "git diff --cached HEAD", It has three callers:

- One in builtin/commit.c, which uses it to determine if the index is different from HEAD and go ahead making a new commit.

- Two in sequencer.c, which use it to see if the index is dirty.

In the first case, if ita entries are present, index_differs_from() may report "dirty". However at tree creation phase, ita entries are dropped and the result tree may look exactly the same as HEAD (assuming that nothing else is changed in index). This is what we need index_differs_from() for, to catch new empty commits. Enabling shift_ita in index_differs_from() fixes this.

In the second case, the presence of ita entries are enough to say the index is dirty and not continue on. Make an explicit check for that before comparing index against HEAD (whether --shift-ita is present is irrelevant)

---
 cache.h               |  1 +
 diff-lib.c            |  1 +
 read-cache.c          | 10 ++++++++++
 sequencer.c           |  5 +++--
 t/t2203-add-intent.sh | 11 +++++++++++
 5 files changed, 26 insertions(+), 2 deletions(-)

index d0494c8..1ddd515 100644
@@ -561,6 +561,7 @@ extern int do_read_index(struct index_state *istate, const char *path,
 extern int read_index_from(struct index_state *, const char *path);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
+extern int has_ita_entries(struct index_state *);
 #define COMMIT_LOCK		(1 << 0)
 #define CLOSE_LOCK		(1 << 1)
 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);

index 62d67c8..ea55ee2 100644
@@ -545,6 +545,7 @@ int index_differs_from(const char *def, int diff_flags)
 	DIFF_OPT_SET(&rev.diffopt, QUICK);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	rev.diffopt.flags |= diff_flags;
+	rev.diffopt.shift_ita = 1;
 	run_diff_index(&rev, 1);
 	if (rev.pending.alloc)
 		free(rev.pending.objects);

index 31eddec..f6a5f61 100644
@@ -1674,6 +1674,16 @@ int is_index_unborn(struct index_state *istate)
 	return (!istate->cache_nr && !istate->timestamp.sec);
 }
 
+int has_ita_entries(struct index_state *istate)
+{
+	int i;
+
+	for (i = 0; i < istate->cache_nr; i++)
+		if (ce_intent_to_add(istate->cache[i]))
+			return 1;
+	return 0;
+}
+
 int discard_index(struct index_state *istate)
 {
 	int i;

index eec8a60..10cded0 100644
@@ -469,7 +469,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		unborn = get_sha1("HEAD", head);
 		if (unborn)
 			hashcpy(head, EMPTY_TREE_SHA1_BIN);
-		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
+		if (has_ita_entries(&the_index) ||
+		    index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
 			return error_dirty_index(opts);
 	}
 	discard_cache();

@@ -1064,7 +1065,7 @@ static int sequencer_continue(struct replay_opts *opts)
 		if (ret)
 			return ret;
 	}
-	if (index_differs_from("HEAD", 0))
+	if (has_ita_entries(&the_index) || index_differs_from("HEAD", 0))
 		return error_dirty_index(opts);
 	todo_list = todo_list->next;
 	return pick_commits(todo_list, opts);

index c6a4648..aa06415 100755
@@ -129,5 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 	)
 '
 
+test_expect_success 'commit: ita entries ignored in empty commit check' '
+	git init empty-subsequent-commit &&
+	(
+		cd empty-subsequent-commit &&
+		test_commit one &&
+		: >two &&
+		git add -N two &&
+		test_must_fail git commit -m nothing-new-here
+	)
+'
+
 test_done
 

2.8.2.524.g6ff3d78

There are three callers to index_differs_from(), which asks "is the index different from the HEAD". Because you want to change the behaviour of the function for one of these callers while not exposing its undesirable behaviour for the other two callers, you guard the call to it with another call to a new helper function, which needs to scan the entire index one more time.

It somehow sounds like backwards to me.

IOW, I wonder if it makes more sense to add a new interface to tell the index_differs_from() function "I want you to use shift-ita semantics" bit, and pass that when calling it from builtin/commit.c while not toggling that bit on when the other two callers call it, without introducing the has_ita_entries() helper function.

By the way, I think "shift" is a bit unclear name for the diffopt field. The log message of [1/3] is totally unclear (it claims "smaller and safer" without explaining what it exactly does and why that is safer); the documentation update in it is slightly better in that it lets intelligent readers to guess that the option is to declare that ita entries do not yet exist in the index (hence, "git diff" would say "that's a new file", while "git diff --cached" says nothing about it). From that observation, I think a descriptive phrase that is suitable for its name than "shift" needs to be found in a short explanation of what it does: "treat ita as missing in the index", e.g. "rev.diffopt.ita_is_missing = 1", perhaps?

Other than these small implementation details, I think I like the direction these two patches are taking us (I haven't checked 3/3 yet).

Thanks.

ita entries are dropped at tree creation phase. If the entire index consists of just ita entries, the result would be a a commit with no entries, which should be caught unless --allow-empty is specified.

---
 builtin/commit.c      | 11 ++++++++---
 t/t2203-add-intent.sh | 10 ++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

index bb9f79b..56b24cb 100644
@@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (amend)
 			parent = "HEAD^1";
 
-		if (get_sha1(parent, sha1))
-			commitable = !!active_nr;
-		else {
+		if (get_sha1(parent, sha1)) {
+			int i, ita_nr = 0;
+
+			for (i = 0; i < active_nr; i++)
+				if (ce_intent_to_add(active_cache[i]))
+					ita_nr++;
+			commitable = active_nr - ita_nr > 0;
+		} else {
 			/*
 			 * Unless the user did explicitly request a submodule
 			 * ignore mode by passing a command line option we do

index aa06415..65314fc 100755
@@ -129,6 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 	)
 '
 
+test_expect_success 'commit: ita entries ignored in empty intial commit check' '
+	git init empty-intial-commit &&
+	(
+		cd empty-intial-commit &&
+		: >one &&
+		git add -N one &&
+		test_must_fail git commit -m nothing-new-here
+	)
+'
+
 test_expect_success 'commit: ita entries ignored in empty commit check' '
 	git init empty-subsequent-commit &&
 	(

2.8.2.524.g6ff3d78

Big typo. "between worktree and index".

Duy

This version splits the old 1/3 into two, with better description in 1/4. The index_differs_from() also takes a flag to set/clear this new flag instead of relying on has_ita_entries like the old 2/3.

The name ita-invisible-in-index is not perfect but I could not think of any better. Another name could be diff-cached-ignores-ita, but that's just half of what it does. The other half is diff-files-includes-ita...

Nguyễn Thái Ngọc Duy (4):
Subject: diff-lib: allow ita entries treated as "not yet exist in index"
diff: add --ita-[in]visible-in-index
commit: fix empty commit creation when there's no changes but ita entries
commit: don't be fooled by ita entries when creating initial commit

 Documentation/diff-options.txt |  8 ++++++++
 builtin/commit.c               | 13 +++++++++----
 diff-lib.c                     | 18 +++++++++++++++++-
 diff.c                         |  4 ++++
 diff.h                         |  3 ++-
 sequencer.c                    |  4 ++--
 t/t2203-add-intent.sh          | 41 +++++++++++++++++++++++++++++++++++++++--
 t/t7064-wtstatus-pv2.sh        |  4 ++--
 wt-status.c                    |  7 ++++++-
 9 files changed, 89 insertions(+), 13 deletions(-)

2.8.2.524.g6ff3d78

When comparing the index and the working tree to show which paths are new, and comparing the tree recorded in the HEAD and the index to see if committing the contents recorded in the index would result in an empty commit, we would want the former comparison to say "these are new paths" and the latter to say "there is no change" for paths that are marked as intent-to-add.

We made a similar attempt at d95d728a ("diff-lib.c: adjust position of i-t-a entries in diff", 2015-03-16), which redefined the semantics of these two comparison modes globally, which was a disaster and had to be reverted at 78cc1a54 ("Revert "diff-lib.c: adjust position of i-t-a entries in diff"", 2015-06-23).

To make sure we do not repeat the same mistake, introduce a new internal diffopt option so that this different semantics can be asked for only by callers that ask it, while making sure other unaudited callers will get the same comparison result.

---
 diff-lib.c              | 14 ++++++++++++++
 diff.h                  |  1 +
 t/t2203-add-intent.sh   | 16 +++++++++++++++-
 t/t7064-wtstatus-pv2.sh |  4 ++--
 wt-status.c             |  7 ++++++-
 5 files changed, 38 insertions(+), 4 deletions(-)

index 3007c85..27f1228 100644
@@ -214,6 +214,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       !is_null_oid(&ce->oid),
 					       ce->name, 0);
 				continue;
+			} else if (revs->diffopt.ita_invisible_in_index &&
+				   ce_intent_to_add(ce)) {
+				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,

@@ -379,6 +385,14 @@ 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 (revs->diffopt.ita_invisible_in_index &&
+	    idx && ce_intent_to_add(idx)) {
+		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)));

index ec76a90..68a6618 100644
@@ -146,6 +146,7 @@ struct diff_options {
 	int dirstat_permille;
 	int setup;
 	int abbrev;
+	int ita_invisible_in_index;
 /* white-space error highlighting */
 #define WSEH_NEW 1
 #define WSEH_CONTEXT 2

index 8f22c43..2276e4e 100755
@@ -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"' '

index 3012a4d..e319fa2 100755
@@ -246,8 +246,8 @@ test_expect_success 'verify --intent-to-add output' '
 	git add --intent-to-add intent1.add intent2.add &&
 
 	cat >expect <<-EOF &&
-	1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent1.add
-	1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent2.add
+	1 .A N... 000000 000000 100644 $_z40 $_z40 intent1.add
+	1 .A N... 000000 000000 100644 $_z40 $_z40 intent2.add
 	EOF
 
 	git status --porcelain=v2 >actual &&

index 9a14658..05a7dcb 100644
@@ -437,7 +437,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:
-			die("BUG: worktree status add???");
+			d->mode_worktree = p->two->mode;
 			break;
 
 		case DIFF_STATUS_DELETED:

@@ -547,6 +547,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
+	rev.diffopt.ita_invisible_in_index = 1;
 	if (!s->show_untracked_files)
 		DIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	if (s->ignore_submodule_arg) {

@@ -570,6 +571,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, &opt);
 
 	DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+	rev.diffopt.ita_invisible_in_index = 1;
 	if (s->ignore_submodule_arg) {
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	} else {

@@ -605,6 +607,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 
 		if (!ce_path_match(ce, &s->pathspec, NULL))
 			continue;
+		if (ce_intent_to_add(ce))
+			continue;
 		it = string_list_insert(&s->change, ce->name);
 		d = it->util;
 		if (!d) {

@@ -911,6 +915,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 
 	init_revisions(&rev, NULL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
+	rev.diffopt.ita_invisible_in_index = 1;
 
 	memset(&opt, 0, sizeof(opt));
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;

2.8.2.524.g6ff3d78

The option --ita-invisible-in-index exposes the "ita_invisible_in_index" diff flag to outside to allow easier experimentation with this new mode. The "plan" is to make --ita-invisible-in-index default to keep consistent behavior with 'status' and 'commit', but a bunch other commands like 'apply', 'merge', 'reset'.... need to be taken into consideration as well.

---
 Documentation/diff-options.txt | 8 ++++++++
 diff.c                         | 4 ++++
 t/t2203-add-intent.sh          | 4 +++-
 3 files changed, 15 insertions(+), 1 deletion(-)

index 7805a0c..0fdd53f 100644
@@ -575,5 +575,13 @@ endif::git-format-patch[]
 --line-prefix=<prefix>::
 	Prepend an additional prefix to every line of output.
 
+--ita-invisible-in-index::
+	By default entries added by "git add -N" appear as an existing
+	empty file in "git diff" and a new file in "git diff --cached".
+	This option makes the entry appear as a new file in "git diff"
+	and non-existent in "git diff --cached". This option could be
+	reverted with `--ita-visible-in-index`. Both options are
+	experimental and could be removed in future.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].

index c6da383..e8e73f8 100644
@@ -3923,6 +3923,10 @@ int diff_opt_parse(struct diff_options *options,
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 		return parse_ws_error_highlight(options, arg);
+	else if (!strcmp(arg, "--ita-invisible-in-index"))
+		options->ita_invisible_in_index = 1;
+	else if (!strcmp(arg, "--ita-visible-in-index"))
+		options->ita_invisible_in_index = 0;
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))

index 2276e4e..0e54f63 100755
@@ -57,7 +57,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git add -N nitfol &&
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+	test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 0 &&
+	test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '

2.8.2.524.g6ff3d78

If i-t-a entries are present and there is no change between the index and HEAD i-t-a entries, index_differs_from() still returns "dirty, new entries" (aka, the resulting commit is not empty), but cache-tree will skip i-t-a entries and produce the exact same tree of current commit.

index_differs_from() is supposed to catch this so we can abort git-commit (unless --no-empty is specified). Update it to optionally ignore i-t-a entries when doing a diff between the index and HEAD so that it would return "no change" in this case and abort commit.

---
 builtin/commit.c      |  2 +-
 diff-lib.c            |  4 +++-
 diff.h                |  2 +-
 sequencer.c           |  4 ++--
 t/t2203-add-intent.sh | 11 +++++++++++
 5 files changed, 18 insertions(+), 5 deletions(-)

index bb9f79b..fe8694d 100644
@@ -910,7 +910,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			if (ignore_submodule_arg &&
 			    !strcmp(ignore_submodule_arg, "all"))
 				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
-			commitable = index_differs_from(parent, diff_flags);
+			commitable = index_differs_from(parent, diff_flags, 1);
 		}
 	}
 	strbuf_release(&committer_ident);

index 27f1228..5244746 100644
@@ -535,7 +535,8 @@ 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, int diff_flags,
+		       int ita_invisible_in_index)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;

@@ -547,6 +548,7 @@ int index_differs_from(const char *def, int diff_flags)
 	DIFF_OPT_SET(&rev.diffopt, QUICK);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	rev.diffopt.flags |= diff_flags;
+	rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
 	run_diff_index(&rev, 1);
 	if (rev.pending.alloc)
 		free(rev.pending.objects);

index 68a6618..b171172 100644
@@ -356,7 +356,7 @@ extern int diff_result_code(struct diff_options *, int);
 
 extern void diff_no_index(struct rev_info *, int, const char **);
 
-extern int index_differs_from(const char *def, int diff_flags);
+extern int index_differs_from(const char *def, int diff_flags, int ita_invisible_in_index);
 
 /*
  * Fill the contents of the filespec "df", respecting any textconv defined by

index eec8a60..b082635 100644
@@ -469,7 +469,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		unborn = get_sha1("HEAD", head);
 		if (unborn)
 			hashcpy(head, EMPTY_TREE_SHA1_BIN);
-		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
+		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0, 0))
 			return error_dirty_index(opts);
 	}
 	discard_cache();

@@ -1064,7 +1064,7 @@ static int sequencer_continue(struct replay_opts *opts)
 		if (ret)
 			return ret;
 	}
-	if (index_differs_from("HEAD", 0))
+	if (index_differs_from("HEAD", 0, 0))
 		return error_dirty_index(opts);
 	todo_list = todo_list->next;
 	return pick_commits(todo_list, opts);

index 0e54f63..8652a96 100755
@@ -129,5 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 	)
 '
 
+test_expect_success 'commit: ita entries ignored in empty commit check' '
+	git init empty-subsequent-commit &&
+	(
+		cd empty-subsequent-commit &&
+		test_commit one &&
+		: >two &&
+		git add -N two &&
+		test_must_fail git commit -m nothing-new-here
+	)
+'
+
 test_done
 

2.8.2.524.g6ff3d78

ita entries are dropped at tree generation phase. If the entire index consists of just ita entries, the result would be a a commit with no entries, which should be caught unless --allow-empty is specified. The test "!!active_nr" is not sufficient to catch this.

---
 builtin/commit.c      | 11 ++++++++---
 t/t2203-add-intent.sh | 10 ++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

index fe8694d..42732ba 100644
@@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (amend)
 			parent = "HEAD^1";
 
-		if (get_sha1(parent, sha1))
-			commitable = !!active_nr;
-		else {
+		if (get_sha1(parent, sha1)) {
+			int i, ita_nr = 0;
+
+			for (i = 0; i < active_nr; i++)
+				if (ce_intent_to_add(active_cache[i]))
+					ita_nr++;
+			commitable = active_nr - ita_nr > 0;
+		} else {
 			/*
 			 * Unless the user did explicitly request a submodule
 			 * ignore mode by passing a command line option we do

index 8652a96..84a9028 100755
@@ -129,6 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 	)
 '
 
+test_expect_success 'commit: ita entries ignored in empty intial commit check' '
+	git init empty-intial-commit &&
+	(
+		cd empty-intial-commit &&
+		: >one &&
+		git add -N one &&
+		test_must_fail git commit -m nothing-new-here
+	)
+'
+
 test_expect_success 'commit: ita entries ignored in empty commit check' '
 	git init empty-subsequent-commit &&
 	(

2.8.2.524.g6ff3d78

I think that probably is a good move.

I can't either, and it is one of the reasons why I am reluctant. Not being able to be named with a short-and-sweet name often is a sign that the thing to be named is conceptually not well thought out.

But as we need to give it some name to the flat to ease experimenting, let's take that name as-is.

It's implementation detail leak, and probably why naming it for "normal" people is so hard. Whatever the name is must somehow imply "so these i-t-a markers actually live in the index and considered real index entries, associated to empty blob, most of the time..."

Duy