[PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced

[PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced

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

Intent-to-add entries are basically "I may want to commit these files
later, but for now they are untracked". As such, when the user does "git
reset --hard <tree>", which removes i-t-a entries from the index, i-t-a
entries in worktree should be kept as untracked.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Lost two files today, luckily I had a backup.

 t/t2203-add-intent.sh | 15 +++++++++++++++
 unpack-trees.c        |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..63086bf 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -82,5 +82,20 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reset --hard leaves on-disk ita entries alone' '
+	git init keep-ita &&
+	(
+	cd keep-ita &&
+	echo abc >abc &&
+	echo def >def &&
+	git add abc &&
+	git commit -m abc &&
+	git add -N def &&
+	git reset --hard HEAD &&
+	echo def >expected &&
+	test_cmp expected def
+	)
+'
+
 test_done
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 9f55cc2..1a2271b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -104,7 +104,7 @@ static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 {
 	clear |= CE_HASHED;
 
-	if (set & CE_REMOVE)
+	if ((set & CE_REMOVE) && !ce_intent_to_add(ce))
 		set |= CE_WT_REMOVE;
 
 	ce->ce_flags = (ce->ce_flags & ~clear) | set;
-- 
2.7.2.531.gc9e018c

Re: [PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced

[Junio C Hamano]

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

> Intent-to-add entries are basically "I may want to commit these files
> later, but for now they are untracked". As such, when the user does "git
> reset --hard <tree>", which removes i-t-a entries from the index, i-t-a
> entries in worktree should be kept as untracked.

Hmm, I can see that the control flow of "reset --hard" for an i-t-a
path does pass through this function, but it is not very obvious
to see how this will not negatively affect other uses of the
unpack-trees machinery (e.g. "checkout" and "merge", especially when
such a path needs to turn into a directory by getting removed).

>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Lost two files today, luckily I had a backup.
>
>  t/t2203-add-intent.sh | 15 +++++++++++++++
>  unpack-trees.c        |  2 +-
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 2a4a749..63086bf 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -82,5 +82,20 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'reset --hard leaves on-disk ita entries alone' '
> +	git init keep-ita &&
> +	(
> +	cd keep-ita &&
> +	echo abc >abc &&
> +	echo def >def &&
> +	git add abc &&
> +	git commit -m abc &&
> +	git add -N def &&
> +	git reset --hard HEAD &&
> +	echo def >expected &&
> +	test_cmp expected def
> +	)
> +'
> +
>  test_done
>  
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 9f55cc2..1a2271b 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -104,7 +104,7 @@ static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>  {
>  	clear |= CE_HASHED;
>  
> -	if (set & CE_REMOVE)
> +	if ((set & CE_REMOVE) && !ce_intent_to_add(ce))
>  		set |= CE_WT_REMOVE;
>  
>  	ce->ce_flags = (ce->ce_flags & ~clear) | set;

Re: [PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Intent-to-add entries are basically "I may want to commit these files
>> later, but for now they are untracked". As such, when the user does "git
>> reset --hard <tree>", which removes i-t-a entries from the index, i-t-a
>> entries in worktree should be kept as untracked.
>
> Hmm, I can see that the control flow of "reset --hard" for an i-t-a
> path does pass through this function, but it is not very obvious
> to see how this will not negatively affect other uses of the
> unpack-trees machinery (e.g. "checkout" and "merge", especially when
> such a path needs to turn into a directory by getting removed).
>

Thinking about it more, I have to say that I do not agree with the
basic premise of this patch.  I-T-A is not "may want to commit, but
they are untracked" at all.  It is "I know I want to add, I just
cannot yet decide the exact contents".

That is why "git add -N newfile && git grep string" would find the
string from newfile, and "git add -N newfile && git diff HEAD newfile"
would show the addition.

Sane people would expect that "git reset --hard HEAD" would behave
as "git diff HEAD | git apply --index -R" when your index is fully
merged, but this change will break the expectation.

Earlier we changed "git commit" to pretend as if an I-T-A entry does
not exist in "git add -N newfile && git commit", but I think that
was a mistake that was caused by the same fuzzy thinking.

3f6d56de (commit: ignore intent-to-add entries instead of refusing,
2012-02-07) does talk about the use of "git add -N" in conjunction
with "git status" and "git diff", but somehow nobody realized that
it was introducing inconsistency in the semantics.

Re: [PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced

[Duy Nguyen]

On Thu, Feb 25, 2016 at 6:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thinking about it more, I have to say that I do not agree with the
> basic premise of this patch.  I-T-A is not "may want to commit, but
> they are untracked" at all.  It is "I know I want to add, I just
> cannot yet decide the exact contents".
>
> That is why "git add -N newfile && git grep string" would find the
> string from newfile, and "git add -N newfile && git diff HEAD newfile"
> would show the addition.

We can agree to disagree. To me i-t-a is still a reminder, not a real
tracked entry (with unknown content).

> Sane people would expect that "git reset --hard HEAD" would behave
> as "git diff HEAD | git apply --index -R" when your index is fully
> merged, but this change will break the expectation.

$ echo abc >abc
$ git add -N abc
$ git diff HEAD|LANG=C git apply --index -R
error: abc: does not match index
$ cat abc
abc

OK this is just nit-picking. We could probably make it work, though
I'm not interested in doing that.

> Earlier we changed "git commit" to pretend as if an I-T-A entry does
> not exist in "git add -N newfile && git commit", but I think that
> was a mistake that was caused by the same fuzzy thinking.
>
> 3f6d56de (commit: ignore intent-to-add entries instead of refusing,
> 2012-02-07) does talk about the use of "git add -N" in conjunction
> with "git status" and "git diff", but somehow nobody realized that
> it was introducing inconsistency in the semantics.
-- 
Duy