[PATCH 0/3] i-t-a entries in git-status, and git-commit

[PATCH 0/3] i-t-a entries in git-status, and git-commit

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

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

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

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

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"

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7805a0c..e63285c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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].
diff --git a/diff-lib.c b/diff-lib.c
index 3007c85..62d67c8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -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)));
diff --git a/diff.c b/diff.c
index c6da383..4178689 100644
--- a/diff.c
+++ b/diff.c
@@ -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"))
diff --git a/diff.h b/diff.h
index ec76a90..5dd4f9c 100644
--- a/diff.h
+++ b/diff.h
@@ -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
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8f22c43..c6a4648 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
 . ./test-lib.sh
 
 test_expect_success 'intent to add' '
+	test_commit 1 &&
+	git rm 1.t &&
+	echo hello >1.t &&
 	echo hello >file &&
 	echo hello >elif &&
 	git add -N file &&
-	git add elif
+	git add elif &&
+	git add -N 1.t
+'
+
+test_expect_success 'git status' '
+	git status --porcelain | grep -v actual >actual &&
+	cat >expect <<-\EOF &&
+	DA 1.t
+	A  elif
+	 A file
+	EOF
+	test_cmp expect actual
 '
 
 test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,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' '
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..e319fa2 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -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 &&
diff --git a/wt-status.c b/wt-status.c
index 9a14658..5f9b1cd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 2/3] diff-lib.c: enable --shift-ita in index_differs_from()

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

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)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/cache.h b/cache.h
index d0494c8..1ddd515 100644
--- a/cache.h
+++ b/cache.h
@@ -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);
diff --git a/diff-lib.c b/diff-lib.c
index 62d67c8..ea55ee2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -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);
diff --git a/read-cache.c b/read-cache.c
index 31eddec..f6a5f61 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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;
diff --git a/sequencer.c b/sequencer.c
index eec8a60..10cded0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index c6a4648..aa06415 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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

[PATCH 3/3] commit: don't be fooled by ita entries when creating initial commit

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

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.

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

diff --git a/builtin/commit.c b/builtin/commit.c
index bb9f79b..56b24cb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index aa06415..65314fc 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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

Re: [PATCH 0/3] i-t-a entries in git-status, and git-commit

[Duy Nguyen]

On Wed, Sep 28, 2016 at 6:43 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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

Big typo. "between worktree and index".
-- 
Duy

Re: [PATCH 2/3] diff-lib.c: enable --shift-ita in index_differs_from()

[Junio C Hamano]

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

> 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)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

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.

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

[Junio C Hamano]

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

> 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"

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.

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

[Junio C Hamano]

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

> 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:
> ...

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

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

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

[Duy Nguyen]

On Thu, Sep 29, 2016 at 2:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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".

> 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:

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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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,

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

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

[Duy Nguyen]

On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> 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,
>
> 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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> 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,
>>
>> I do not agree with that endgame, and quite honestly I do not want
>> to waste time reviewing such a series.

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.

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

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.

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

[Duy Nguyen]

On Fri, Oct 7, 2016 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Duy Nguyen <pclouds@gmail.com> writes:
>>>
>>>> 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,
>>>
>>> I do not agree with that endgame, and quite honestly I do not want
>>> to waste time reviewing such a series.
>
> 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.

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

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.

> [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

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

> 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".

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

> "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.

--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/1451181092-26054-4-git-send-email-pclouds@gmail.com/
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

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

> [1] https://public-inbox.org/git/1451181092-26054-4-git-send-email-pclouds@gmail.com/

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

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

[PATCH 0/4] nd/ita-empty-commit update

[Nguyễn Thái Ngọc 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

[PATCH 1/4] diff-lib: allow ita entries treated as "not yet exist in index"

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

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/diff-lib.c b/diff-lib.c
index 3007c85..27f1228 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -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)));
diff --git a/diff.h b/diff.h
index ec76a90..68a6618 100644
--- a/diff.h
+++ b/diff.h
@@ -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
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8f22c43..2276e4e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
 . ./test-lib.sh
 
 test_expect_success 'intent to add' '
+	test_commit 1 &&
+	git rm 1.t &&
+	echo hello >1.t &&
 	echo hello >file &&
 	echo hello >elif &&
 	git add -N file &&
-	git add elif
+	git add elif &&
+	git add -N 1.t
+'
+
+test_expect_success 'git status' '
+	git status --porcelain | grep -v actual >actual &&
+	cat >expect <<-\EOF &&
+	DA 1.t
+	A  elif
+	 A file
+	EOF
+	test_cmp expect actual
 '
 
 test_expect_success 'check result of "add -N"' '
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..e319fa2 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -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 &&
diff --git a/wt-status.c b/wt-status.c
index 9a14658..05a7dcb 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 2/4] diff: add --ita-[in]visible-in-index

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

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/diff-options.txt | 8 ++++++++
 diff.c                         | 4 ++++
 t/t2203-add-intent.sh          | 4 +++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7805a0c..0fdd53f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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].
diff --git a/diff.c b/diff.c
index c6da383..e8e73f8 100644
--- a/diff.c
+++ b/diff.c
@@ -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"))
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2276e4e..0e54f63 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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

[PATCH 3/4] commit: fix empty commit creation when there's no changes but ita entries

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

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index bb9f79b..fe8694d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/diff-lib.c b/diff-lib.c
index 27f1228..5244746 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -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);
diff --git a/diff.h b/diff.h
index 68a6618..b171172 100644
--- a/diff.h
+++ b/diff.h
@@ -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
diff --git a/sequencer.c b/sequencer.c
index eec8a60..b082635 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 0e54f63..8652a96 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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

[PATCH 4/4] commit: don't be fooled by ita entries when creating initial commit

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

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.

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

diff --git a/builtin/commit.c b/builtin/commit.c
index fe8694d..42732ba 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8652a96..84a9028 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -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

Re: [PATCH 0/4] nd/ita-empty-commit update

[Junio C Hamano]

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

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

I think that probably is a good move.

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

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.

Re: [PATCH 0/4] nd/ita-empty-commit update

[Duy Nguyen]

On Tue, Oct 25, 2016 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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...
>
> 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.

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..."

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