[PATCH 0/6] nd/ita-cleanup updates

[PATCH 0/6] nd/ita-cleanup updates

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

Most of the updates are in commit message (see the old thread [1]). I
give up on adding new tests for git-apply, finally admitting I don't
know that command that well. Code change from 'pu' version is entirely
in 5/6:

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 687c82e..d9fe8f4 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -48,6 +48,7 @@ static int checkout_file(const char *name, const char *prefix)
 	int pos = cache_name_pos(name, namelen);
 	int has_same_name = 0;
 	int did_checkout = 0;
+	int has_intent_to_add = 0;
 	int errs = 0;
 
 	if (pos < 0)
@@ -56,9 +57,11 @@ 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) ||
-		    ce_intent_to_add(ce))
+		    memcmp(ce->name, name, namelen)) {
+			if (ce_intent_to_add(ce))
+				has_intent_to_add = 1;
 			break;
+		}
 		has_same_name = 1;
 		pos++;
 		if (ce_stage(ce) != checkout_stage
@@ -78,7 +81,9 @@ static int checkout_file(const char *name, const char *prefix)
 
 	if (!state.quiet) {
 		fprintf(stderr, "git checkout-index: %s ", name);
-		if (!has_same_name)
+		if (has_intent_to_add)
+			fprintf(stderr, "is not yet in the cache");
+		else if (!has_same_name)
 			fprintf(stderr, "is not in the cache");
 		else if (checkout_stage)
 			fprintf(stderr, "does not exist at stage %d",
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6d198b3..ac37d92 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -300,8 +300,6 @@ 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
@@ -330,12 +328,15 @@ 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];
+		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))
+					ce->ce_flags &= ~CE_MATCHED;
 				continue;
+			}
+			/* Unmerged paths */
 			if (opts->force) {
 				warning(_("path '%s' is unmerged"), ce->name);
 			} else if (opts->writeout_stage) {
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index d0f36a4..52e9f7f 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -117,7 +117,7 @@ test_expect_success 'checkout ignores i-t-a' '
 		cd checkout &&
 		echo data >file &&
 		git add -N file &&
-		test_must_fail git checkout -- file &&
+		git checkout -- file &&
 		echo data >expected &&
 		test_cmp expected file
 	)

[1] http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276352

Nguyễn Thái Ngọc Duy (6):
  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

 builtin/apply.c          | 13 +++++-----
 builtin/blame.c          |  5 ----
 builtin/checkout-index.c | 12 +++++++--
 builtin/checkout.c       |  9 ++++---
 builtin/grep.c           |  2 +-
 builtin/rm.c             |  2 +-
 cache-tree.c             |  2 +-
 cache.h                  |  1 +
 read-cache.c             |  4 +--
 t/t2203-add-intent.sh    | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 92 insertions(+), 21 deletions(-)

-- 
2.3.0.rc1.137.g477eb31

[PATCH 1/6] 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 06484c2..9a0df92 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/6] 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 3304bff..74a7a43 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -212,7 +212,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 32772b9..e590346 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 3d3244b..b1f0531 100644
--- a/cache.h
+++ b/cache.h
@@ -233,6 +233,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 36ff89f..fb80c47 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -311,7 +311,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);
@@ -1231,7 +1231,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/6] apply: fix adding new files on i-t-a entries

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

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, i-t-a entries are ignored and 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.

(commit message mostly from Junio)

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 0769b09..315fce8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3550,10 +3550,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

[PATCH 4/6] 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 315fce8..11f04fd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3345,7 +3345,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)) {
@@ -3498,7 +3498,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

[PATCH 5/6] 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 | 12 ++++++++++--
 builtin/checkout.c       |  9 ++++++---
 t/t2203-add-intent.sh    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..d9fe8f4 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -48,6 +48,7 @@ static int checkout_file(const char *name, const char *prefix)
 	int pos = cache_name_pos(name, namelen);
 	int has_same_name = 0;
 	int did_checkout = 0;
+	int has_intent_to_add = 0;
 	int errs = 0;
 
 	if (pos < 0)
@@ -56,8 +57,11 @@ 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)) {
+			if (ce_intent_to_add(ce))
+				has_intent_to_add = 1;
 			break;
+		}
 		has_same_name = 1;
 		pos++;
 		if (ce_stage(ce) != checkout_stage
@@ -77,7 +81,9 @@ static int checkout_file(const char *name, const char *prefix)
 
 	if (!state.quiet) {
 		fprintf(stderr, "git checkout-index: %s ", name);
-		if (!has_same_name)
+		if (has_intent_to_add)
+			fprintf(stderr, "is not yet in the cache");
+		else if (!has_same_name)
 			fprintf(stderr, "is not in the cache");
 		else if (checkout_stage)
 			fprintf(stderr, "does not exist at stage %d",
@@ -99,6 +105,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 3e141fc..ac37d92 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -328,12 +328,15 @@ 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];
+		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))
+					ce->ce_flags &= ~CE_MATCHED;
 				continue;
+			}
+			/* Unmerged paths */
 			if (opts->force) {
 				warning(_("path '%s' is unmerged"), ce->name);
 			} else if (opts->writeout_stage) {
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 96c8755..52e9f7f 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 &&
+		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

[PATCH 6/6] 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

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

[Junio C Hamano]

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

> 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, i-t-a entries are ignored and 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.
>
> (commit message mostly from Junio)

Hmm, I do not quite recall.  The above looks under-explained anyway;
we should stress the fact that it is a designed behaviour of this
change to allow only "apply --cached" and continue rejecting other
forms of "apply", but the above makes it sound like it is merely a
coincidence.

It might make sense, from purely mechanistic point of view, to allow
"git apply --cached" to create in the above scenario, but it does
not make any sense to allow "git apply" or "git apply --index", both
of which modifies the working tree (and I do not think the patch
allows the former; I think the latter would fail correctly, but it
may leave the index in a weird state--I didn't check).

"git add -N F" is like saying "I am telling you that F _exists_; I
am just not telling you what its exact contents are yet".  It's like
reserving a table in a restaurant.  The diner might not have arrived
yet, but that does not mean you can give the table to somebody else.

You need to wait for the reservation to be canceled (which you would
do by "git rm --cached F") before you give the table to somebody
else (i.e. creation by the patch).

So from that point of view, I am not convinced "git apply --cached"
should be allowed in such a case, though.

"I thought I told you to that I'll add to this path, but you chose
not to notice that the patch I tried to apply would create the path
with totally different contents and now I am getting from 'git diff'
nonsensical comparison" would be a plausible complaint if we took
this patch.  What is the practical benefit of automatically and
silently canceling the reservation made by an earlier "add -N"?
What workflow benefits from this change, and is this the best
solution to help that workflow?


> 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 0769b09..315fce8 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3550,10 +3550,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

Re: [PATCH 1/6] blame: remove obsolete comment

[Junio C Hamano]

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

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

Thanks.

>  builtin/blame.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 06484c2..9a0df92 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;

Re: [PATCH 2/6] Add and use convenient macro ce_intent_to_add()

[Junio C Hamano]

Looks good; thanks.

Re: [PATCH 4/6] 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:

> 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
> +	)
> +'

Unlike "apply --index" (and "apply" without any option to work on
working tree), "apply -3" is allowed to write to the working tree
even when it fails, if the failure comes from a merge conflict and
that is by design.  In this case, the patch expects that there is an
existing file with 'oldcontent' so it should try to do a three-way
merge to go from an 'unspecified' content (i.e. the current state in
the index) in a way similar to going from 'oldcontent' to
'newcontent', and because we _have_ to make sure that the path that
holds the 'unspecified' content is clean between the index and the
working tree--but an earlier "add -N" said that "the index knows
about the path, I am not telling what its contents are yet", by
definition the index and the working tree cannot match so the
application _must_ fail, before even attempting to do a three-way
merge, hence it should not touch the working tree.

So expecting a failure from "apply -3" sounds like the right thing
to do, and obviously it should not leave any "newfile".  The test
also must check that the index has not been modified since the last
"git add -N" (most notably, we want to see 'newfile' still has the
I-T-A bit on).

More interesting cases to test would be:

 - Instead of "rm newfile", have ">newfile" to empty the contents
   before applying the patch with "apply -3".  We should expect the
   same "apply -3 fails", but we should see an empty 'newfile' in
   the working tree.  The index must be the same as the last "git
   add -N".

 - Instead of "rm newfile", have "echo oldcontent >newfile".  What
   should "apply -3" do?  The patch would be applicable with "apply"
   without any argument (hence without involving the index), but
   "-3" requires 'check_index', and again any entry with I-T-A bit
   on by definition does not match between the index and the working
   tree, so the application must fail, I think.

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

[Junio C Hamano]

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

> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..d9fe8f4 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -48,6 +48,7 @@ static int checkout_file(const char *name, const char *prefix)
>  	int pos = cache_name_pos(name, namelen);
>  	int has_same_name = 0;
>  	int did_checkout = 0;
> +	int has_intent_to_add = 0;
>  	int errs = 0;
>  
>  	if (pos < 0)
> @@ -56,8 +57,11 @@ 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)) {
> +			if (ce_intent_to_add(ce))
> +				has_intent_to_add = 1;
>  			break;
> +		}
>  		has_same_name = 1;
>  		pos++;
>  		if (ce_stage(ce) != checkout_stage

Sorry, but I cannot see what this hunk is trying to do.

Earlier, 'pos' is set to where the 'name' would be inserted (or
would replace, if there is an existing entry with the same name).
We iterate over the index starting from that 'pos' until we see a
different name (i.e. the length is different, or the name does not
match) and then break out, because that entry is not something we
want to muck with, because the entry here you are checking with
ce_intent_to_add() is an entry that is entirely unrelated to the
path you are checking out.

So you do not check it out, just as before, which is correct, but
then this 'has_intent_to_add' flag is used to say something
nonsensical below ...

> @@ -77,7 +81,9 @@ static int checkout_file(const char *name, const char *prefix)
>  
>  	if (!state.quiet) {
>  		fprintf(stderr, "git checkout-index: %s ", name);
> -		if (!has_same_name)
> +		if (has_intent_to_add)
> +			fprintf(stderr, "is not yet in the cache");

... the reason why you are here does not have anything to do with the
entry that happened to come after the path you wanted to check out
was marked with the I-T-A bit.  The reason why you are here is
because 'did_checkout' was unset, which in turn was because there
wasn't a path that match 'name' you are checking out in the index,
with or without the I-T-A bit.

    $ rm .git/index
    $ >file
    $ git add -N file
    $ git checkout-index fild

would hit this codepath, I think, and the code would give a message
that says "fild is not yet in the cache", which may be technically
correct but it is the same as !has_same_name's "fild is not in the
cache".

Probably the I-T-A check must go after the code decides that 'ce' is
an entry for the 'name' the caller asked us to check out (and of
course we shouldn't check it out in that case).

> @@ -99,6 +105,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 probably correct, I didn't check the context to fully
see if it is valid, though.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3e141fc..ac37d92 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -328,12 +328,15 @@ 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];
> +		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))
> +					ce->ce_flags &= ~CE_MATCHED;
>  				continue;
> +			}
> +			/* Unmerged paths */
>  			if (opts->force) {
>  				warning(_("path '%s' is unmerged"), ce->name);
>  			} else if (opts->writeout_stage) {

This loop has become dual-purpose; it is drops CE_MATCHED bit from
cache entries with the I-T-A bit, and it continues to handle errors
for an attempt to checkout unmerged paths.

    $ rm .git/index
    $ >file
    $ git add -N file
    $ git checkout file

would hit this codepath, and it will force-skip "file".

There is a small source of confusion for new people here, though.

    $ git checkout filo

would complain that there is no 'filo'.  But

    $ git checkout file

would silently turn itself into a no-op.  Don't we want to at least
give some indication that we are not checking out "file" because its
contents do not yet exist in the index, or something?

> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 96c8755..52e9f7f 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 &&
> +		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

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

[Junio C Hamano]

This one looks correct.

I'll pick the obviously correct pieces (1, 2, and 6) from this
series, as they are pretty-much independent, and discard the rest
for now.

Thanks.

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

[Duy Nguyen]

On Mon, Dec 28, 2015 at 10:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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, i-t-a entries are ignored and 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.
>>
>> (commit message mostly from Junio)
>
> Hmm, I do not quite recall.

For reference ;)

https://www.mail-archive.com/git@vger.kernel.org/msg76461.html

> The above looks under-explained anyway;
> ...

Not enough energy to go through this. Will do later and post new
proposed commit message.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Not enough energy to go through this. Will do later and post new
> proposed commit message.

I do not have as much issue with the log message as what the patch
does, actually; this change does not look like a good one, which is
a more serious problem.