[PATCH] git add --intent-to-add: fix removal...

[PATCH] git add --intent-to-add: fix removal of cached emptiness

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:42

This uses the extended index flag mechanism introduced earlier to mark
the entries added to the index via "git add -N" with CE_INTENT_TO_ADD.

The logic to detect an "intent to add" entry for the purpose of allowing
"git rm --cached $path" is tightened to check not just for a staged empty
blob, but with the CE_INTENT_TO_ADD bit.  This protects an empty blob that
was explicitly added and then modified in the work tree from being dropped
with this sequence:

	$ >empty
	$ git add empty
	$ echo "non empty" >empty
	$ git rm --cached empty

An index an "intent to add" entry is blocked.  This implies that you
cannot "git commit" from such a state; however "git commit -a" still
works.

Signed-off-by: Junio C Hamano <redacted>
---

 * This applies on top of the result of merging commit 06aaaa0 (Extend
   index to save more flags, 2008-10-01) from nd/narrow topic into 'master'.

 builtin-rm.c               |   11 ++++++-----
 builtin-write-tree.c       |    2 +-
 cache-tree.c               |   10 +++++++---
 cache.h                    |    3 ++-
 read-cache.c               |    2 ++
 t/t3600-rm.sh              |    4 ++--
 t/t3701-add-interactive.sh |   18 ++++++++++++++++++
 7 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/builtin-rm.c b/builtin-rm.c
index b7126e3..8debcec 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c

@@ -73,14 +73,15 @@ static int check_local_mod(unsigned char *head, int index_only)
 		}
 		if (ce_match_stat(ce, &st, 0))
 			local_changes = 1;
-		if (no_head
-		     || get_tree_entry(head, name, sha1, &mode)
-		     || ce->ce_mode != create_ce_mode(mode)
-		     || hashcmp(ce->sha1, sha1))
+		if (no_head ||
+		    ((get_tree_entry(head, name, sha1, &mode)
+		      || ce->ce_mode != create_ce_mode(mode)
+		      || hashcmp(ce->sha1, sha1)) &&
+		     !(ce->ce_flags & CE_INTENT_TO_ADD)))
 			staged_changes = 1;
 
 		if (local_changes && staged_changes &&
-		    !(index_only && is_empty_blob_sha1(ce->sha1)))
+		    !(index_only && (ce->ce_flags & CE_INTENT_TO_ADD)))
 			errs = error("'%s' has staged content different "
 				     "from both the file and the HEAD\n"
 				     "(use -f to force removal)", name);

diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index 52a3c01..9d64050 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c

@@ -42,7 +42,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 		die("%s: error reading the index", me);
 		break;
 	case WRITE_TREE_UNMERGED_INDEX:
-		die("%s: error building trees; the index is unmerged?", me);
+		die("%s: error building trees", me);
 		break;
 	case WRITE_TREE_PREFIX_ERROR:
 		die("%s: prefix %s not found", me, prefix);

diff --git a/cache-tree.c b/cache-tree.c
index 5f8ee87..3d8f218 100644
--- a/cache-tree.c
+++ b/cache-tree.c

@@ -155,13 +155,17 @@ static int verify_cache(struct cache_entry **cache,
 	funny = 0;
 	for (i = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
-		if (ce_stage(ce)) {
+		if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
 			if (10 < ++funny) {
 				fprintf(stderr, "...\n");
 				break;
 			}
-			fprintf(stderr, "%s: unmerged (%s)\n",
-				ce->name, sha1_to_hex(ce->sha1));
+			if (ce_stage(ce))
+				fprintf(stderr, "%s: unmerged (%s)\n",
+					ce->name, sha1_to_hex(ce->sha1));
+			else
+				fprintf(stderr, "%s: not added yet\n",
+					ce->name);
 		}
 	}
 	if (funny)

diff --git a/cache.h b/cache.h
index ef2e7f9..f15b3fc 100644
--- a/cache.h
+++ b/cache.h

@@ -176,10 +176,11 @@ struct cache_entry {
 /*
  * Extended on-disk flags
  */
+#define CE_INTENT_TO_ADD 0x20000000
 /* CE_EXTENDED2 is for future extension */
 #define CE_EXTENDED2 0x80000000
 
-#define CE_EXTENDED_FLAGS (0)
+#define CE_EXTENDED_FLAGS (CE_INTENT_TO_ADD)
 
 /*
  * Safeguard to avoid saving wrong flags:

diff --git a/read-cache.c b/read-cache.c
index abc627b..fa30a0f 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -546,6 +546,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	ce->ce_flags = namelen;
 	if (!intent_only)
 		fill_stat_cache_info(ce, st);
+	else
+		ce->ce_flags |= CE_INTENT_TO_ADD;
 
 	if (trust_executable_bit && has_symlinks)
 		ce->ce_mode = create_ce_mode(st_mode);

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5b4d6f7..b7d46e5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -187,8 +187,8 @@ test_expect_success 'but with -f it should work.' '
 	test_must_fail git ls-files --error-unmatch baz
 '
 
-test_expect_failure 'refuse to remove cached empty file with modifications' '
-	touch empty &&
+test_expect_success 'refuse to remove cached empty file with modifications' '
+	>empty &&
 	git add empty &&
 	echo content >empty &&
 	test_must_fail git rm --cached empty

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index e95663d..473ef85 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh

@@ -133,6 +133,24 @@ test_expect_success 'real edit works' '
 	test_cmp expected output
 '
 
+test_expect_success 'cannot commit with i-t-a entry' '
+	git reset --hard &&
+	echo xyzzy >rezrov &&
+	echo frotz >nitfol &&
+	git add rezrov &&
+	git add -N nitfol &&
+	test_must_fail git commit
+'
+
+test_expect_success 'can commit with an unrelated i-t-a entry in index' '
+	git reset --hard &&
+	echo xyzzy >rezrov &&
+	echo frotz >nitfol &&
+	git add rezrov &&
+	git add -N nitfol &&
+	git commit -m partial rezrov
+'
+
 if test "$(git config --bool core.filemode)" = false
 then
     say 'skipping filemode tests (filesystem does not properly support modes)'

-- 
1.6.0.4.850.g6bd829

[PATCH 1/3] builtin-rm.c: explain and clarify the "local change" logic

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:42

Explain the logic to check local modification a bit more in the comment,
especially because the existing comment that talks about "git rm --cached"
was placed in a part that was not about "--cached" at all.

Also clarify "if .. else if .." structure.

Signed-off-by: Junio C Hamano <redacted>
---

 * This is the first patch of a re-rolled sesries of the previous one, and
   again applies to the master plus the first commit from nd/narrow.

 builtin-rm.c |   53 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/builtin-rm.c b/builtin-rm.c
index b7126e3..3d03da0 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c

@@ -31,7 +31,8 @@ static void add_list(const char *name)
 
 static int check_local_mod(unsigned char *head, int index_only)
 {
-	/* items in list are already sorted in the cache order,
+	/*
+	 * Items in list are already sorted in the cache order,
 	 * so we could do this a lot more efficiently by using
 	 * tree_desc based traversal if we wanted to, but I am
 	 * lazy, and who cares if removal of files is a tad

@@ -71,25 +72,55 @@ static int check_local_mod(unsigned char *head, int index_only)
 			 */
 			continue;
 		}
+
+		/*
+		 * "rm" of a path that has changes need to be treated
+		 * carefully not to allow losing local changes
+		 * accidentally.  A local change could be (1) file in
+		 * work tree is different since the index; and/or (2)
+		 * the user staged a content that is different from
+		 * the current commit in the index.
+		 *
+		 * In such a case, you would need to --force the
+		 * removal.  However, "rm --cached" (remove only from
+		 * the index) is safe if the index matches the file in
+		 * the work tree or the HEAD commit, as it means that
+		 * the content being removed is available elsewhere.
+		 */
+
+		/*
+		 * Is the index different from the file in the work tree?
+		 */
 		if (ce_match_stat(ce, &st, 0))
 			local_changes = 1;
+
+		/*
+		 * Is the index different from the HEAD commit?  By
+		 * definition, before the very initial commit,
+		 * anything staged in the index is treated by the same
+		 * way as changed from the HEAD.
+		 */
 		if (no_head
 		     || get_tree_entry(head, name, sha1, &mode)
 		     || ce->ce_mode != create_ce_mode(mode)
 		     || hashcmp(ce->sha1, sha1))
 			staged_changes = 1;
 
-		if (local_changes && staged_changes &&
-		    !(index_only && is_empty_blob_sha1(ce->sha1)))
-			errs = error("'%s' has staged content different "
-				     "from both the file and the HEAD\n"
-				     "(use -f to force removal)", name);
+		/*
+		 * If the index does not match the file in the work
+		 * tree and if it does not match the HEAD commit
+		 * either, (1) "git rm" without --cached definitely
+		 * will lose information; (2) "git rm --cached" will
+		 * lose information unless it is about removing an
+		 * "intent to add" entry.
+		 */
+		if (local_changes && staged_changes) {
+			if (!index_only || !is_empty_blob_sha1(ce->sha1))
+				errs = error("'%s' has staged content different "
+					     "from both the file and the HEAD\n"
+					     "(use -f to force removal)", name);
+		}
 		else if (!index_only) {
-			/* It's not dangerous to "git rm --cached" a
-			 * file if the index matches the file or the
-			 * HEAD, since it means the deleted content is
-			 * still available somewhere.
-			 */
 			if (staged_changes)
 				errs = error("'%s' has changes staged in the index\n"
 					     "(use --cached to keep the file, "

-- 
1.6.0.4.850.g6bd829

`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