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

9 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:05:31

Duy Nguyen [off-list ref] writes:

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

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

quoted

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

Agreed.

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

Thanks.

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:15

On Thu, Jun 25, 2015 at 8:07 PM, Junio C Hamano [off-list ref] wrote:

quoted

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

Agreed.

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

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

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

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

-- 
2.3.0.rc1.137.g477eb31

[PATCH 1/8] blame: remove obsolete comment

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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 <redacted>
---
 builtin/blame.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..942f302 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2379,11 +2379,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	ce->ce_mode = create_ce_mode(mode);
 	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 
-	/*
-	 * We are not going to write this out, so this does not matter
-	 * right now, but someday we might optimize diff-index --cached
-	 * with cache-tree information.
-	 */
 	cache_tree_invalidate_path(&the_index, path);
 
 	return commit;

-- 
2.3.0.rc1.137.g477eb31

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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

diff --git a/builtin/rm.c b/builtin/rm.c
index 80b972f..8829b09 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c

@@ -211,7 +211,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		 * "intent to add" entry.
 		 */
 		if (local_changes && staged_changes) {
-			if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))
+			if (!index_only || !ce_intent_to_add(ce))
 				string_list_append(&files_staged, name);
 		}
 		else if (!index_only) {

diff --git a/cache-tree.c b/cache-tree.c
index feace8b..c865bd2 100644
--- a/cache-tree.c
+++ b/cache-tree.c

@@ -377,7 +377,7 @@ static int update_one(struct cache_tree *it,
 		 * they are not part of generated trees. Invalidate up
 		 * to root to force cache-tree users to read elsewhere.
 		 */
-		if (ce->ce_flags & CE_INTENT_TO_ADD) {
+		if (ce_intent_to_add(ce)) {
 			to_invalidate = 1;
 			continue;
 		}

diff --git a/cache.h b/cache.h
index 4e25271..41d811c 100644
--- a/cache.h
+++ b/cache.h

@@ -241,6 +241,7 @@ static inline unsigned create_ce_flags(unsigned stage)
 #define ce_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE)
 #define ce_skip_worktree(ce) ((ce)->ce_flags & CE_SKIP_WORKTREE)
 #define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)
+#define ce_intent_to_add(ce) ((ce)->ce_flags & CE_INTENT_TO_ADD)
 
 #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
 static inline unsigned int create_ce_mode(unsigned int mode)

diff --git a/read-cache.c b/read-cache.c
index 89dbc08..f849cbd 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -327,7 +327,7 @@ int ie_match_stat(const struct index_state *istate,
 	 * by definition never matches what is in the work tree until it
 	 * actually gets added.
 	 */
-	if (ce->ce_flags & CE_INTENT_TO_ADD)
+	if (ce_intent_to_add(ce))
 		return DATA_CHANGED | TYPE_CHANGED | MODE_CHANGED;
 
 	changed = ce_match_stat_basic(ce, st);

@@ -1251,7 +1251,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 
 			if (cache_errno == ENOENT)
 				fmt = deleted_fmt;
-			else if (ce->ce_flags & CE_INTENT_TO_ADD)
+			else if (ce_intent_to_add(ce))
 				fmt = added_fmt; /* must be before other checks */
 			else if (changed & TYPE_CHANGED)
 				fmt = typechange_fmt;

-- 
2.3.0.rc1.137.g477eb31

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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

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

diff --git a/builtin/apply.c b/builtin/apply.c
index 54aba4e..76b58a1 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c

@@ -3553,10 +3553,11 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 {
 	struct stat nst;
 
-	if (check_index &&
-	    cache_name_pos(new_name, strlen(new_name)) >= 0 &&
-	    !ok_if_exists)
-		return EXISTS_IN_INDEX;
+	if (check_index && !ok_if_exists) {
+		int pos = cache_name_pos(new_name, strlen(new_name));
+		if (pos >= 0 && !ce_intent_to_add(active_cache[pos]))
+			return EXISTS_IN_INDEX;
+	}
 	if (cached)
 		return 0;

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..bb5ef2b 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh

@@ -82,5 +82,18 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 	test_cmp expect actual
 '
 
+test_expect_success 'apply adds new file on i-t-a entry' '
+	git init apply &&
+	(
+		cd apply &&
+		echo newcontent >newfile &&
+		git add newfile &&
+		git diff --cached >patch &&
+		rm .git/index &&
+		git add -N newfile &&
+		git apply --cached patch
+	)
+'
+
 test_done

-- 
2.3.0.rc1.137.g477eb31

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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 <redacted>
---
 builtin/apply.c       |  4 ++--
 t/t2203-add-intent.sh | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 76b58a1..b185c97 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c

@@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch)
 		die("BUG: patch to %s is not a creation", patch->old_name);
 
 	pos = cache_name_pos(name, strlen(name));
-	if (pos < 0)
+	if (pos < 0 || ce_intent_to_add(active_cache[pos]))
 		return error(_("%s: does not exist in index"), name);
 	ce = active_cache[pos];
 	if (lstat(name, &st)) {

@@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 
 	if (check_index && !previous) {
 		int pos = cache_name_pos(old_name, strlen(old_name));
-		if (pos < 0) {
+		if (pos < 0 || ce_intent_to_add(active_cache[pos])) {
 			if (patch->is_new < 0)
 				goto is_new;
 			return error(_("%s: does not exist in index"), old_name);

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index bb5ef2b..96c8755 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh

@@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' '
 	)
 '
 
+test_expect_success 'apply:check_preimage() not creating empty file' '
+	git init check-preimage &&
+	(
+		cd check-preimage &&
+		echo oldcontent >newfile &&
+		git add newfile &&
+		echo newcontent >newfile &&
+		git diff >patch &&
+		rm .git/index &&
+		git add -N newfile &&
+		rm newfile &&
+		test_must_fail git apply -3 patch &&
+		! test -f newfile
+	)
+'
+
 test_done

-- 
2.3.0.rc1.137.g477eb31

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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 <redacted>
---
 builtin/checkout-index.c |  5 ++++-
 builtin/checkout.c       |  2 ++
 t/t2203-add-intent.sh    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 8028c37..eca975d 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix)
 	while (pos < active_nr) {
 		struct cache_entry *ce = active_cache[pos];
 		if (ce_namelen(ce) != namelen ||
-		    memcmp(ce->name, name, namelen))
+		    memcmp(ce->name, name, namelen) ||
+		    ce_intent_to_add(ce))
 			break;
 		has_same_name = 1;
 		pos++;

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e1403be..02889d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,
 			 * anything to this entry at all.
 			 */
 			continue;
+		if (ce_intent_to_add(ce))
+			continue;
 		/*
 		 * Either this entry came from the tree-ish we are
 		 * checking the paths out of, or we are checking out

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 96c8755..d0f36a4 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh

@@ -111,5 +111,39 @@ test_expect_success 'apply:check_preimage() not creating empty file' '
 	)
 '
 
+test_expect_success 'checkout ignores i-t-a' '
+	git init checkout &&
+	(
+		cd checkout &&
+		echo data >file &&
+		git add -N file &&
+		test_must_fail git checkout -- file &&
+		echo data >expected &&
+		test_cmp expected file
+	)
+'
+
+test_expect_success 'checkout-index ignores i-t-a' '
+	(
+		cd checkout &&
+		git checkout-index file &&
+		echo data >expected &&
+		test_cmp expected file
+	)
+'
+
+test_expect_success 'checkout-index --all ignores i-t-a' '
+	(
+		cd checkout &&
+		echo data >anotherfile &&
+		git add anotherfile &&
+		rm anotherfile &&
+		git checkout-index --all &&
+		echo data >expected &&
+		test_cmp expected file &&
+		test_cmp expected anotherfile
+	)
+'
+
 test_done

-- 
2.3.0.rc1.137.g477eb31

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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 <redacted>
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d04f440..f508179 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
 
 	for (nr = 0; nr < active_nr; nr++) {
 		const struct cache_entry *ce = active_cache[nr];
-		if (!S_ISREG(ce->ce_mode))
+		if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
 			continue;
 		if (!ce_path_match(ce, pathspec, NULL))
 			continue;

-- 
2.3.0.rc1.137.g477eb31

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

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 23:06:16

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

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 4cbd5ff..95f7296 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -895,7 +895,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			 * submodules which were manually staged, which would
 			 * be really confusing.
 			 */
-			int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+			diff_flags_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 			if (ignore_submodule_arg &&
 			    !strcmp(ignore_submodule_arg, "all"))
 				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;

diff --git a/diff-lib.c b/diff-lib.c
index 241a843..ae09034 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 {
 	int changed = ce_match_stat(ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
-		unsigned orig_flags = diffopt->flags;
+		diff_flags_t orig_flags = diffopt->flags;
 		if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
 			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
 		if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))

@@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags)
+int index_differs_from(const char *def, diff_flags_t diff_flags)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;

diff --git a/diff.c b/diff.c
index 7deac90..2485870 100644
--- a/diff.c
+++ b/diff.c

@@ -4912,7 +4912,7 @@ int diff_can_quit_early(struct diff_options *opt)
 static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
 	int ignored = 0;
-	unsigned orig_flags = options->flags;
+	diff_flags_t orig_flags = options->flags;
 	if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
 		set_diffopt_flags_from_submodule_config(options, path);
 	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))

diff --git a/diff.h b/diff.h
index f7208ad..4241aa5 100644
--- a/diff.h
+++ b/diff.h

@@ -110,13 +110,15 @@ enum diff_words_type {
 	DIFF_WORDS_COLOR
 };
 
+typedef uint64_t diff_flags_t;
+
 struct diff_options {
 	const char *orderfile;
 	const char *pickaxe;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
-	unsigned flags;
-	unsigned touched_flags;
+	diff_flags_t flags;
+	diff_flags_t touched_flags;
 
 	/* diff-filter bits */
 	unsigned int filter;

@@ -347,7 +349,7 @@ extern int diff_result_code(struct diff_options *, int);
 
 extern void diff_no_index(struct rev_info *, int, const char **, const char *);
 
-extern int index_differs_from(const char *def, int diff_flags);
+extern int index_differs_from(const char *def, diff_flags_t diff_flags);
 
 extern size_t fill_textconv(struct userdiff_driver *driver,
 			    struct diff_filespec *df,

-- 
2.3.0.rc1.137.g477eb31

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help