git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 23, 2015, 17:37 UTC
Message-ID
<xmqqh9pye378.fsf@gitster.dls.corp.google.com>
In-Reply-To
<xmqqoak6e5dx.fsf@gitster.dls.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 20 quoted lines
>>  I think I'm opening a can of worms with d95d728....
>
> I cannot offhand convince myself that "apply" is the only casualty;
> assuming it is, I think a reasonable way forward is to keep d95d728
> and adjust "apply" to the new world order.  Otherwise, i.e. if there
> are wider fallouts from d95d728, we may instead want to temporarily
> revert it off from 'master', deal with fallouts to "apply" and other
> things, before resurrecting it.
>
> Anything that internally uses "diff-index" is suspect, I think.
>
> What do others think?  You seem to ...
>
>>  So far blame, rm and checkout-entry and "checkout <paths>" are on my
>>  to-think-or-fix list. But this patch can get in early to fix a real
>>  regression instead of waiting for one big series. A lot more
>>  discussions will be had before that series gets in good shape.
>
> ... think that the damage could be quite extensive, so I am inclined
> to say that we first revert d95d728 before going forward.

Let's do this on 'nd/diff-i-t-a' topic and merge the result immediately to 'master' for now.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 23 Jun 2015 10:27:47 -0700
Subject: [PATCH] Revert "diff-lib.c: adjust position of i-t-a entries in diff"
This reverts commit d95d728aba06a34394d15466045cbdabdada58a2.

It turns out that many other commands that need to interact with the result of running diff-files and diff-index, e.g. "git apply", "git rm", etc., need to be adjusted to the new world order it brings in. For example, it would break this sequence to correct a whitespace breakage in the parts you changed:

	git add -N file
	git diff --cached file | git apply --cached --whitespace=fix
	git checkout file

In the old world order, "diff" showed a patch to modify an existing empty file by adding its full contents, and "apply" updated the index by modifying the existing empty blob (which is what an Intent-to-Add entry records in the index) with that patch.

In the new world order, "diff" shows a patch to create a new file with its full contents, but because "apply" thinks that the i-t-a entry already exists in the index, it refused to accept a creation.

Adjusting "apply" to this new world order is easy, but we need to
assess the extent of the damage to the rest of the system the new
world order brought in before going forward and adjust them all,
after which we can resurrect the commit being reverted here.
---
 builtin/add.c           |  1 -
 diff-lib.c              | 12 ------------
 t/t2203-add-intent.sh   | 23 ++++-------------------
 t/t4011-diff-symlink.sh | 10 ++++------
 4 files changed, 8 insertions(+), 38 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ee370b0..3390933 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,7 +63,6 @@ static void update_callback(struct diff_queue_struct *q,
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
-		case DIFF_STATUS_ADDED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
 			if (add_file_to_index(&the_index, path, data->flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index 714501a..a85c497 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,11 +212,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       ce->sha1, !is_null_sha1(ce->sha1),
 					       ce->name, 0);
 				continue;
-			} else if (ce->ce_flags & CE_INTENT_TO_ADD) {
-				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
-					       EMPTY_BLOB_SHA1_BIN, 0,
-					       ce->name, 0);
-				continue;
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -381,13 +376,6 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct rev_info *revs = o->unpack_data;
 	int match_missing, cached;
 
-	/* i-t-a entries do not actually exist in the index */
-	if (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {
-		idx = NULL;
-		if (!tree)
-			return;	/* nothing to diff.. */
-	}
-
 	/* if the entry is not checked out, don't examine work tree */
 	cached = o->index_only ||
 		(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 7c641bf..2a4a749 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,24 +5,10 @@ test_description='Intent to add'
 . ./test-lib.sh
 
 test_expect_success 'intent to add' '
-	test_commit 1 &&
-	git rm 1.t &&
-	echo hello >1.t &&
 	echo hello >file &&
 	echo hello >elif &&
 	git add -N file &&
-	git add elif &&
-	git add -N 1.t
-'
-
-test_expect_success 'git status' '
-	git status --porcelain | grep -v actual >actual &&
-	cat >expect <<-\EOF &&
-	DA 1.t
-	A  elif
-	 A file
-	EOF
-	test_cmp expect actual
+	git add elif
 '
 
 test_expect_success 'check result of "add -N"' '
@@ -57,8 +43,7 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git add -N nitfol &&
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only -- nitfol | wc -l) = 1
+	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -87,13 +72,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 	: >dir/bar &&
 	git add -N dir/bar &&
 	git diff --cached --name-only >actual &&
-	>expect &&
+	echo dir/bar >expect &&
 	test_cmp expect actual &&
 
 	git write-tree >/dev/null &&
 
 	git diff --cached --name-only >actual &&
-	>expect &&
+	echo dir/bar >expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 7452fce..13e7f62 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -139,13 +139,11 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '
 test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
 	cat >expect <<-\EOF &&
 	diff --git a/file.bin b/file.bin
-	new file mode 100644
-	index 0000000..d95f3ad
-	Binary files /dev/null and b/file.bin differ
+	index e69de29..d95f3ad 100644
+	Binary files a/file.bin and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	new file mode 120000
-	index 0000000..dce41ec
-	--- /dev/null
+	index e69de29..dce41ec 120000
+	--- a/link.bin
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin

-- 
2.4.4-598-g4ab0ce8
Previous: Junio C HamanoNext: Junio C Hamano
Message 6 of 28 in “apply --cached --whitespace=fix now failing on items added with "add -N"”
  1. Patrick HigginsJun 22, 2015
  2. Duy NguyenJun 22, 2015
  3. Junio C HamanoJun 22, 2015
  4. apply: fix adding new files on i-t-a entriesNguyễn Thái Ngọc Duy, Jun 23, 2015
  5. Junio C HamanoJun 23, 2015
  6. Junio C HamanoJun 23, 2015
  7. Junio C HamanoJun 24, 2015
  8. Duy NguyenJun 24, 2015
  9. Junio C HamanoJun 24, 2015
  10. Duy NguyenJun 25, 2015
  11. Junio C HamanoJun 25, 2015
  12. 0/8 Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"Nguyễn Thái Ngọc Duy, Aug 22, 2015
  13. 1/8 blame: remove obsolete commentNguyễn Thái Ngọc Duy, Aug 22, 2015
  14. 2/8 Add and use convenient macro ce_intent_to_add()Nguyễn Thái Ngọc Duy, Aug 22, 2015
  15. 3/8 apply: fix adding new files on i-t-a entriesNguyễn Thái Ngọc Duy, Aug 22, 2015
  16. Junio C HamanoAug 25, 2015
  17. 4/8 apply: make sure check_preimage() does not leave empty file on errorNguyễn Thái Ngọc Duy, Aug 22, 2015
  18. Junio C HamanoAug 25, 2015
  19. 5/8 checkout(-index): do not checkout i-t-a entriesNguyễn Thái Ngọc Duy, Aug 22, 2015
  20. Junio C HamanoAug 25, 2015
  21. Duy NguyenNov 29, 2015
  22. Junio C HamanoNov 30, 2015
  23. 6/8 grep: make it clear i-t-a entries are ignoredNguyễn Thái Ngọc Duy, Aug 22, 2015
  24. 7/8 diff.h: extend "flags" field to 64 bits because we're out of bitsNguyễn Thái Ngọc Duy, Aug 22, 2015
  25. Junio C HamanoAug 25, 2015
  26. Duy NguyenAug 31, 2015
  27. Junio C HamanoAug 25, 2015
  28. Junio C HamanoAug 25, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.