threads / patch / 41079

patch, 6 partsnd/ita-cleanup updates

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

## tl;dr

15 messages between Dec 27, 2015 and Dec 29, 2015. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · lore

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:

Show changes to 3 files +15 −9

builtin/checkout-index.c, builtin/checkout.c, t/t2203-add-intent.sh

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
Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/6] blame: remove obsolete comment

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(-)
Show changes to builtin/blame.c +0 −5
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
Junio C Hamano· Dec 28, 2015, 17:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 19 quoted lines
>  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;
Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 4 files +5 −4

builtin/rm.c, cache-tree.c, cache.h, read-cache.c

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
Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

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

With this patch, 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(-)
Show changes to 2 files +18 −4

builtin/apply.c, t/t2203-add-intent.sh

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
Junio C Hamano· Dec 28, 2015, 03:01 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 16 quoted lines
> 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?

Show 50 quoted lines
> 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
Duy Nguyen· Dec 29, 2015, 14:02 UTC · re: Junio C Hamano · lore

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

On Mon, Dec 28, 2015 at 10:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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
Junio C Hamano· Dec 29, 2015, 17:40 UTC · re: Duy Nguyen · lore

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

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.

Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +18 −2

builtin/apply.c, t/t2203-add-intent.sh

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
Junio C Hamano· Dec 28, 2015, 18:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 23 quoted lines
> 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.
Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 3 files +50 −5

builtin/checkout-index.c, builtin/checkout.c, t/t2203-add-intent.sh

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
Junio C Hamano· Dec 28, 2015, 19:56 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 25 quoted lines
> 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 ...

Show 7 quoted lines
> @@ -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).

Show 6 quoted lines
> @@ -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.

Show 23 quoted lines
> 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?

Show 43 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Dec 27, 2015, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to builtin/grep.c +1 −1
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

← back to recent threads