[PATCH 1/3] Fix sparse checkout not removing files from index

[PATCH 1/3] Fix sparse checkout not removing files from index

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

When unpack_trees() nearly completes, sparse pattern is applied and
will prevent any updates (CE_UPDATE, CE_WT_REMOVE, CE_REMOVE) outside
sparse checkout area by clearing these flags.

Unfortunately CE_REMOVE also instructs write_index() to remove files from
the index. By clearing this flag, index may keep unwanted files.

This patch turns CE_REMOVE in unpack-trees.c to CE_REMOVE|CE_WT_REMOVE
and only remove files from worktree if CE_WT_REMOVE is set. CE_REMOVE
is now only meaningful to index operations and CE_WT_REMOVE to
worktree ones.

This fault made the test suite itself faulty. With this fault fixed,
two other faults are revealed :(

Reported in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=583699

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h                              |    3 +--
 t/t1011-read-tree-sparse-checkout.sh |    9 ++++++++-
 unpack-trees.c                       |   22 ++++++++++++----------
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index 0d101e4..4e7bb62 100644
--- a/cache.h
+++ b/cache.h
@@ -179,8 +179,7 @@ struct cache_entry {
 #define CE_UNHASHED  (0x200000)
 #define CE_CONFLICTED (0x800000)
 
-/* Only remove in work directory, not index */
-#define CE_WT_REMOVE (0x400000)
+#define CE_WT_REMOVE (0x400000) /* remove in work directory */
 
 #define CE_UNPACKED  (0x1000000)
 
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 62246db..885578a 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -138,7 +138,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '
 	test ! -f sub/added
 '
 
-test_expect_success 'read-tree adds to worktree, dirty case' '
+test_expect_failure 'read-tree adds to worktree, dirty case' '
 	echo init.t > .git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
@@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
 	grep -q dirty sub/added
 '
 
+test_expect_success 'read-tree --reset removes outside worktree' '
+	echo init.t > .git/info/sparse-checkout &&
+	git checkout -f top &&
+	git reset --hard removed &&
+	test -z "$(git ls-files sub/added)"
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index e8f03f5..f2d148c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 
 	clear |= CE_HASHED | CE_UNHASHED;
 
+	if (set & CE_REMOVE)
+		set |= CE_WT_REMOVE;
+
 	memcpy(new, ce, size);
 	new->next = NULL;
 	new->ce_flags = (new->ce_flags & ~clear) | set;
@@ -84,7 +87,7 @@ static int check_updates(struct unpack_trees_options *o)
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
 			struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))
+			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
 				total++;
 		}
 
@@ -104,12 +107,6 @@ static int check_updates(struct unpack_trees_options *o)
 				unlink_entry(ce);
 			continue;
 		}
-
-		if (ce->ce_flags & CE_REMOVE) {
-			display_progress(progress, ++cnt);
-			if (o->update)
-				unlink_entry(ce);
-		}
 	}
 	remove_marked_cache_entries(&o->result);
 	remove_scheduled_dirs();
@@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			/*
 			 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
 			 * area as a result of ce_skip_worktree() shortcuts in
-			 * verify_absent() and verify_uptodate(). Clear them.
+			 * verify_absent() and verify_uptodate().
+			 * Make sure they don't modify worktree.
 			 */
-			if (ce_skip_worktree(ce))
-				ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
+			if (ce_skip_worktree(ce)) {
+				ce->ce_flags &= ~CE_UPDATE;
+
+				if (ce->ce_flags & CE_REMOVE)
+					ce->ce_flags &= ~CE_WT_REMOVE;
+			}
 			else
 				empty_worktree = 0;
 
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 2/3] unpack-trees.c: Do not check ce_stage in will_have_skip_worktree()

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

The idea of sparse checkout is conflicted entries should always stay
in worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,
ce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all
entries have been merged into the index, and identical staged entries
collapsed.

However, will_have_skip_worktree() since f1f523e (unpack-trees():
ignore worktree check outside checkout area) is also used earlier in
verify_* functions, when entries have not been merged to index yet
and ce_stage() may not be zero. Checking ce_stage() then can make
unnecessary verification on entries outside checkout area and error out.

This fixes part of test case "read-tree adds to worktree, dirty case".
The error is changed from

error: Untracked working tree file 'sub/added' would be overwritten by merge.

before this patch to

grep: sub/added: No such file or directory

which will be addressed in the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 unpack-trees.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index f2d148c..7ea8c5f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -135,9 +135,6 @@ static int will_have_skip_worktree(const struct cache_entry *ce, struct unpack_t
 {
 	const char *basename;
 
-	if (ce_stage(ce))
-		return 0;
-
 	basename = strrchr(ce->name, '/');
 	basename = basename ? basename+1 : ce->name;
 	return excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, o->el) <= 0;
@@ -147,7 +144,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 {
 	int was_skip_worktree = ce_skip_worktree(ce);
 
-	if (will_have_skip_worktree(ce, o))
+	if (!ce_stage(ce) && will_have_skip_worktree(ce, o))
 		ce->ce_flags |= CE_SKIP_WORKTREE;
 	else
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;
-- 
1.7.1.rc1.69.g24c2f7

Re: [PATCH 2/3] unpack-trees.c: Do not check ce_stage in will_have_skip_worktree()

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> The idea of sparse checkout is conflicted entries should always stay
> in worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,
> ce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all
> entries have been merged into the index, and identical staged entries
> collapsed.
> 
> However, will_have_skip_worktree() since f1f523e (unpack-trees():
> ignore worktree check outside checkout area) is also used earlier in
> verify_* functions, when entries have not been merged to index yet
> and ce_stage() may not be zero. Checking ce_stage() then can make
> unnecessary verification on entries outside checkout area and error out.

s/make/provoke/?

So: conflicts from unmerged index entries are supposed to be kept in
the work tree; but unpack_trees() is checking too early and seeing
conflicts where there are none.  Do I understand correctly?

> This fixes part of test case "read-tree adds to worktree, dirty case".

A summary might help people who do not remember the test:

	So attempting to add a file outside the checkout area 
	where there is a file already present (as in
	t1011.13 "read-tree adds to worktree, dirty case")
	will no longer error out with

	error: Untracked working tree file 'sub/added' would be overwritten by merge.

	but instead will remove the file.  (The latter problem will be
	addressed in a later patch.)

> +++ a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -135,9 +135,6 @@ static int will_have_skip_worktree(const struct cache_entry *ce, struct unpack_t
>  {
>  	const char *basename;
>  
> -	if (ce_stage(ce))
> -		return 0;
> -
>  	basename = strrchr(ce->name, '/');
>  	basename = basename ? basename+1 : ce->name;
>  	return excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, o->el) <= 0;
> @@ -147,7 +144,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
>  {
>  	int was_skip_worktree = ce_skip_worktree(ce);
>  
> -	if (will_have_skip_worktree(ce, o))
> +	if (!ce_stage(ce) && will_have_skip_worktree(ce, o))
>  		ce->ce_flags |= CE_SKIP_WORKTREE;
>  	else
>  		ce->ce_flags &= ~CE_SKIP_WORKTREE;

Makes sense.

Thanks,
Jonathan

Re: [PATCH 2/3] unpack-trees.c: Do not check ce_stage in will_have_skip_worktree()

[Nguyen Thai Ngoc Duy]

2010/7/31 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> The idea of sparse checkout is conflicted entries should always stay
>> in worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,
>> ce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all
>> entries have been merged into the index, and identical staged entries
>> collapsed.
>>
>> However, will_have_skip_worktree() since f1f523e (unpack-trees():
>> ignore worktree check outside checkout area) is also used earlier in
>> verify_* functions, when entries have not been merged to index yet
>> and ce_stage() may not be zero. Checking ce_stage() then can make
>> unnecessary verification on entries outside checkout area and error out.
>
> s/make/provoke/?
>
> So: conflicts from unmerged index entries are supposed to be kept in
> the work tree; but unpack_trees() is checking too early and seeing
> conflicts where there are none.  Do I understand correctly?

Correct.
-- 
Duy

[PATCH 3/3] Mark new entries skip-worktree appropriately

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

Sparse checkout updates worktree based on the old and new
skip-worktree status when $GIT_DIR/info/sparse-checkout changes:

old = ce_skip_worktree(ce);           // current skip-worktree
new = will_have_skip_work_tree(ce);   // from $GIT..sparse-checkout
if (old && !new) add_file_back(ce);   // shrink checkout area
if (!old && new) remove_file_out(ce); [1] // enlarge checkout area

New entries after merging will always have skip-worktree unset
(i.e. old = 0). If those files are filtered out by
$GIT_DIR/info/sparse-checkout (i.e. new != 0), then case [1] will
happen. But there is nothing to remove because they're new.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |    2 +-
 unpack-trees.c                       |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 885578a..86a6246 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -138,7 +138,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '
 	test ! -f sub/added
 '
 
-test_expect_failure 'read-tree adds to worktree, dirty case' '
+test_expect_success 'read-tree adds to worktree, dirty case' '
 	echo init.t > .git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7ea8c5f..f98b1cb 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1091,6 +1091,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 	if (!old) {
 		if (verify_absent(merge, "overwritten", o))
 			return -1;
+		if (!o->skip_sparse_checkout && will_have_skip_worktree(merge, o))
+			update |= CE_SKIP_WORKTREE;
 		invalidate_ce_path(merge, o);
 	} else if (!(old->ce_flags & CE_CONFLICTED)) {
 		/*
-- 
1.7.1.rc1.69.g24c2f7

Re: [PATCH 3/3] Mark new entries skip-worktree appropriately

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> Sparse checkout updates worktree based on the old and new
> skip-worktree status when $GIT_DIR/info/sparse-checkout changes:
> 
> old = ce_skip_worktree(ce);           // current skip-worktree
> new = will_have_skip_work_tree(ce);   // from $GIT..sparse-checkout
> if (old && !new) add_file_back(ce);   // shrink checkout area
> if (!old && new) remove_file_out(ce); [1] // enlarge checkout area
> 
> New entries after merging will always have skip-worktree unset
> (i.e. old = 0). If those files are filtered out by
> $GIT_DIR/info/sparse-checkout (i.e. new != 0), then case [1] will
> happen. But there is nothing to remove because they're new.

When using unpack_trees to add a new file, there is no in-file
index entry to grab the previous skip worktree bit from.  If it
is outside the checkout area, we should pretend it was skipped before,
too; otherwise a checkout can cause a file on disk whose name
coincides with a newly added outside-worktree file to be deleted.

Do I understand correctly?

> +++ b/unpack-trees.c
> @@ -1091,6 +1091,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
>  	if (!old) {
>  		if (verify_absent(merge, "overwritten", o))
>  			return -1;
> +		if (!o->skip_sparse_checkout && will_have_skip_worktree(merge, o))
> +			update |= CE_SKIP_WORKTREE;
>  		invalidate_ce_path(merge, o);
>  	} else if (!(old->ce_flags & CE_CONFLICTED)) {
>  		/*

Looks sane.

Thanks for the fixes,
Jonathan

Re: [PATCH 3/3] Mark new entries skip-worktree appropriately

[Nguyen Thai Ngoc Duy]

2010/7/31 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> Sparse checkout updates worktree based on the old and new
>> skip-worktree status when $GIT_DIR/info/sparse-checkout changes:
>>
>> old = ce_skip_worktree(ce);           // current skip-worktree
>> new = will_have_skip_work_tree(ce);   // from $GIT..sparse-checkout
>> if (old && !new) add_file_back(ce);   // shrink checkout area
>> if (!old && new) remove_file_out(ce); [1] // enlarge checkout area
>>
>> New entries after merging will always have skip-worktree unset
>> (i.e. old = 0). If those files are filtered out by
>> $GIT_DIR/info/sparse-checkout (i.e. new != 0), then case [1] will
>> happen. But there is nothing to remove because they're new.
>
> When using unpack_trees to add a new file, there is no in-file
> index entry to grab the previous skip worktree bit from.  If it
> is outside the checkout area, we should pretend it was skipped before,
> too; otherwise a checkout can cause a file on disk whose name
> coincides with a newly added outside-worktree file to be deleted.
>
> Do I understand correctly?

Yes.
-- 
Duy

Re: [PATCH 3/3] Mark new entries skip-worktree appropriately

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:
> 2010/7/31 Jonathan Nieder <jrnieder@gmail.com>:

>> Do I understand correctly?
>
> Yes.

Ah, good.

This is fun code to play with.  Thanks for putting it together.

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

The log message left me confused.  Let’s see if the code is easier.

> +++ b/cache.h
> @@ -179,8 +179,7 @@ struct cache_entry {
>  #define CE_UNHASHED  (0x200000)
>  #define CE_CONFLICTED (0x800000)
>  
> -/* Only remove in work directory, not index */
> -#define CE_WT_REMOVE (0x400000)
> +#define CE_WT_REMOVE (0x400000) /* remove in work directory */

Before[1], CE_REMOVE was used by read-tree et al in core to represent
something to be removed from the index file and work tree (e.g., when
switching branches).

In the new order, one uses CE_REMOVE|CE_WT_REMOVE for that, right?

> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
>  	grep -q dirty sub/added
>  '
>  
> +test_expect_success 'read-tree --reset removes outside worktree' '
> +	echo init.t > .git/info/sparse-checkout &&
> +	git checkout -f top &&
> +	git reset --hard removed &&
> +	test -z "$(git ls-files sub/added)"
> +'
> +

Using reset --hard to remove a file outside the sparse checkout.
A sane, simple test; thanks.  (Nitpick: I would have used

	>empty &&
	git ls-files sub/added >output &&
	test_cmp empty output

even though that does not produce any more helpful output in this
case.)

> +++ b/unpack-trees.c
> @@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>  
>  	clear |= CE_HASHED | CE_UNHASHED;
>  
> +	if (set & CE_REMOVE)
> +		set |= CE_WT_REMOVE;
> +

A bridge between the old and new worlds.

I would have added CE_WT_REMOVE to callers instead (they all pass
constants more or less), but I suppose this way makes the patch
shorter.

[...]
> @@ -84,7 +87,7 @@ static int check_updates(struct unpack_trees_options *o)
>  	if (o->update && o->verbose_update) {
>  		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
>  			struct cache_entry *ce = index->cache[cnt];
> -			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))
> +			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))

Only entries marked CE_WT_REMOVE will be touched by read-tree -u.

> @@ -104,12 +107,6 @@ static int check_updates(struct unpack_trees_options *o)
>  				unlink_entry(ce);
>  			continue;
>  		}
> -
> -		if (ce->ce_flags & CE_REMOVE) {
[...]

and the CE_WT_REMOVE case takes care of that.  Makes sense.

> @@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  			/*
>  			 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
>  			 * area as a result of ce_skip_worktree() shortcuts in
> -			 * verify_absent() and verify_uptodate(). Clear them.
> +			 * verify_absent() and verify_uptodate().
> +			 * Make sure they don't modify worktree.
>  			 */
> -			if (ce_skip_worktree(ce))
> -				ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
> +			if (ce_skip_worktree(ce)) {
> +				ce->ce_flags &= ~CE_UPDATE;
> +
> +				if (ce->ce_flags & CE_REMOVE)
> +					ce->ce_flags &= ~CE_WT_REMOVE;
> +			}
>  			else
>  				empty_worktree = 0;

Ah, and at last we come to the fix. :)

This is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)
represents a narrowing of the checkout and should be preserved,
while CE_WT_REMOVE|CE_REMOVE represents a removed index entry and
should be changed to just CE_REMOVE.

But it is a clear improvement over the code from before and should
behave as advertised now.  So aside from the log message,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

[1] v1.6.6.1~23^2~1 (Make on-disk index representation separate from
in-core one, 2008-01-14) and v0.99~295 (git-read-tree: remove deleted
files in the working directory, 2005-06-09).

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Nguyen Thai Ngoc Duy]

2010/7/30 Jonathan Nieder <jrnieder@gmail.com>:
>> +++ b/cache.h
>> @@ -179,8 +179,7 @@ struct cache_entry {
>>  #define CE_UNHASHED  (0x200000)
>>  #define CE_CONFLICTED (0x800000)
>>
>> -/* Only remove in work directory, not index */
>> -#define CE_WT_REMOVE (0x400000)
>> +#define CE_WT_REMOVE (0x400000) /* remove in work directory */
>
> Before[1], CE_REMOVE was used by read-tree et al in core to represent
> something to be removed from the index file and work tree (e.g., when
> switching branches).
>
> In the new order, one uses CE_REMOVE|CE_WT_REMOVE for that, right?

For unpack_trees() only, yes. You made me grep for CE_REMOVE and found
that do_diff_cache() may turn CE_REMOVE on. The function is used by
blame and reset.

Hmm.. It's for removing staged entries. So it's probably fine.

>> +++ b/t/t1011-read-tree-sparse-checkout.sh
>> @@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
>>       grep -q dirty sub/added
>>  '
>>
>> +test_expect_success 'read-tree --reset removes outside worktree' '
>> +     echo init.t > .git/info/sparse-checkout &&
>> +     git checkout -f top &&
>> +     git reset --hard removed &&
>> +     test -z "$(git ls-files sub/added)"
>> +'
>> +
>
> Using reset --hard to remove a file outside the sparse checkout.
> A sane, simple test; thanks.  (Nitpick: I would have used
>
>        >empty &&
>        git ls-files sub/added >output &&
>        test_cmp empty output
>
> even though that does not produce any more helpful output in this
> case.)

Thanks. That looks better.

>
>> +++ b/unpack-trees.c
>> @@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>>
>>       clear |= CE_HASHED | CE_UNHASHED;
>>
>> +     if (set & CE_REMOVE)
>> +             set |= CE_WT_REMOVE;
>> +
>
> A bridge between the old and new worlds.
>
> I would have added CE_WT_REMOVE to callers instead (they all pass
> constants more or less), but I suppose this way makes the patch
> shorter.

It'd better be done in one place. I don't expect anybody to set
CE_REMOVE without CE_WT_REMOVE any time soon. Adding it the the
callers, the new callers might miss it.

>> @@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>                       /*
>>                        * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
>>                        * area as a result of ce_skip_worktree() shortcuts in
>> -                      * verify_absent() and verify_uptodate(). Clear them.
>> +                      * verify_absent() and verify_uptodate().
>> +                      * Make sure they don't modify worktree.
>>                        */
>> -                     if (ce_skip_worktree(ce))
>> -                             ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
>> +                     if (ce_skip_worktree(ce)) {
>> +                             ce->ce_flags &= ~CE_UPDATE;
>> +
>> +                             if (ce->ce_flags & CE_REMOVE)
>> +                                     ce->ce_flags &= ~CE_WT_REMOVE;
>> +                     }
>>                       else
>>                               empty_worktree = 0;
>
> Ah, and at last we come to the fix. :)
>
> This is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)
> represents a narrowing of the checkout and should be preserved,
> while CE_WT_REMOVE|CE_REMOVE represents a removed index entry and
> should be changed to just CE_REMOVE.

Yeah. I did wonder if it's worth a comment to explain. I forget why I
did not add that comment now.
-- 
Duy

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> For unpack_trees() only, yes.

Ah, that is what I was missing.

So would the following be correct?

	Subject: unpack-trees: let read-tree -u remove index entries outside sparse area

	To avoid touching the worktree outside a sparse checkout,
	when the update flag is enabled unpack_trees() clears the
	CE_UPDATE and CE_REMOVE flags on entries that do not match the
	sparse pattern before actually committing any updates to the
	index file or worktree.

	The effect on the index was unintentional; sparse checkout was
	never meant to prevent index updates outside the area checked
	out.  And the result is very confusing: for example, after a
	failed merge, currently "git reset --hard" does not reset the
	state completely but an additional "git reset --mixed" will.

	So stop clearing the CE_REMOVE flag.  Instead, maintain a
	CE_WT_REMOVE flag to separately track whether a particular
	file removal should apply to the worktree in addition to the
	index or not.

	The CE_WT_REMOVE flag is used already to mark files that
	should be removed because of a narrowing checkout area.  That
	usage will still apply; do not clear the CE_WT_REMOVE flag
	in that case (detectable because the CE_REMOVE flag is not
	set).

	This bug masked some other bugs illustrated by the test
	suite, which will be addressed by later patches.

	Reported-by: Frédéric Brière <fbriere@fbriere.net>
	Fixes: http://bugs.debian.org/583699

> It'd better be done in one place.

I think I understand better now, and now I agree.  (This patch is not
a change in the semantics of CE_REMOVE, but just some new unpack-trees
bookkeeping.)

> 2010/7/30 Jonathan Nieder <jrnieder@gmail.com>:

>> This is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)
>> represents a narrowing of the checkout and should be preserved,
>> while CE_WT_REMOVE|CE_REMOVE represents a removed index entry and
>> should be changed to just CE_REMOVE.
>
> Yeah. I did wonder if it's worth a comment to explain. I forget why I
> did not add that comment now.

What happens if an index entry is removed at the same time as the
checkout is narrowed?

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Jonathan Nieder]

Jonathan Nieder wrote:

> What happens if an index entry is removed at the same time as the
> checkout is narrowed?

Hm, maybe something like this would help.

Jonathan Nieder (2):
  t1011 (sparse checkout): style nitpicks
  read-tree -m -u: always remove relevant files when narrowing checkout

 cache.h                              |    3 +-
 t/t1011-read-tree-sparse-checkout.sh |  129 +++++++++++++++++++++-------------
 unpack-trees.c                       |   42 +++--------
 3 files changed, 94 insertions(+), 80 deletions(-)

[PATCH 1/2] t1011 (sparse checkout): style nitpicks

[Jonathan Nieder]

The exit status of "git ls-files" in

	test -z "$(git ls-files sub/added)"

is ignored.  So redirect output to a file and compare to an
empty file with test_cmp instead.

While changing that, tweak the rest of the script to more closely
follow the test style guide.  Guarding setup commands with
test_expect_success makes it easy to see the scope in which some
particular data is used; removal of whitespace after >redirection
operators is just for consistency.

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |  108 ++++++++++++++++++---------------
 1 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 885578a..2955071 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -1,16 +1,30 @@
 #!/bin/sh
 
-test_description='sparse checkout tests'
+test_description='sparse checkout tests
+
+* (tag: removed, master) removed
+| D	sub/added
+* (HEAD, tag: top) modified and added
+| M	init.t
+| A	sub/added
+* (tag: init) init
+  A	init.t
+'
 
 . ./test-lib.sh
 
-cat >expected <<EOF
-100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
-EOF
 test_expect_success 'setup' '
+	cat >expected <<-\EOF &&
+	100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
+	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
+	EOF
+	cat >expected.swt <<-\EOF &&
+	H init.t
+	H sub/added
+	EOF
+
 	test_commit init &&
-	echo modified >> init.t &&
+	echo modified >>init.t &&
 	mkdir sub &&
 	touch sub/added &&
 	git add init.t sub/added &&
@@ -20,26 +34,22 @@ test_expect_success 'setup' '
 	git commit -m removed &&
 	git tag removed &&
 	git checkout top &&
-	git ls-files --stage > result &&
+	git ls-files --stage >result &&
 	test_cmp expected result
 '
 
-cat >expected.swt <<EOF
-H init.t
-H sub/added
-EOF
 test_expect_success 'read-tree without .git/info/sparse-checkout' '
 	git read-tree -m -u HEAD &&
-	git ls-files --stage > result &&
+	git ls-files --stage >result &&
 	test_cmp expected result &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result
 '
 
 test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '
-	echo > .git/info/sparse-checkout
+	echo >.git/info/sparse-checkout
 	git read-tree -m -u HEAD &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test -f init.t &&
 	test -f sub/added
@@ -47,9 +57,9 @@ test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '
 
 test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-checkout and enabled' '
 	git config core.sparsecheckout true &&
-	echo > .git/info/sparse-checkout &&
+	echo >.git/info/sparse-checkout &&
 	git read-tree --no-sparse-checkout -m -u HEAD &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test -f init.t &&
 	test -f sub/added
@@ -57,101 +67,101 @@ test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-
 
 test_expect_success 'read-tree with empty .git/info/sparse-checkout' '
 	git config core.sparsecheckout true &&
-	echo > .git/info/sparse-checkout &&
+	echo >.git/info/sparse-checkout &&
 	test_must_fail git read-tree -m -u HEAD &&
-	git ls-files --stage > result &&
+	git ls-files --stage >result &&
 	test_cmp expected result &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test -f init.t &&
 	test -f sub/added
 '
 
-cat >expected.swt <<EOF
-S init.t
-H sub/added
-EOF
 test_expect_success 'match directories with trailing slash' '
+	cat >expected.swt-noinit <<-\EOF &&
+	S init.t
+	H sub/added
+	EOF
+
 	echo sub/ > .git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
 	git ls-files -t > result &&
-	test_cmp expected.swt result &&
+	test_cmp expected.swt-noinit result &&
 	test ! -f init.t &&
 	test -f sub/added
 '
 
-cat >expected.swt <<EOF
-H init.t
-H sub/added
-EOF
 test_expect_failure 'match directories without trailing slash' '
-	echo init.t > .git/info/sparse-checkout &&
-	echo sub >> .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
+	echo sub >>.git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test ! -f init.t &&
 	test -f sub/added
 '
 
-cat >expected.swt <<EOF
-H init.t
-S sub/added
-EOF
 test_expect_success 'checkout area changes' '
-	echo init.t > .git/info/sparse-checkout &&
+	cat >expected.swt-nosub <<-\EOF &&
+	H init.t
+	S sub/added
+	EOF
+
+	echo init.t >.git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
-	git ls-files -t > result &&
-	test_cmp expected.swt result &&
+	git ls-files -t >result &&
+	test_cmp expected.swt-nosub result &&
 	test -f init.t &&
 	test ! -f sub/added
 '
 
 test_expect_success 'read-tree updates worktree, absent case' '
-	echo sub/added > .git/info/sparse-checkout &&
+	echo sub/added >.git/info/sparse-checkout &&
 	git checkout -f top &&
 	git read-tree -m -u HEAD^ &&
 	test ! -f init.t
 '
 
 test_expect_success 'read-tree updates worktree, dirty case' '
-	echo sub/added > .git/info/sparse-checkout &&
+	echo sub/added >.git/info/sparse-checkout &&
 	git checkout -f top &&
-	echo dirty > init.t &&
+	echo dirty >init.t &&
 	git read-tree -m -u HEAD^ &&
 	grep -q dirty init.t &&
 	rm init.t
 '
 
 test_expect_success 'read-tree removes worktree, dirty case' '
-	echo init.t > .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f top &&
-	echo dirty > added &&
+	echo dirty >added &&
 	git read-tree -m -u HEAD^ &&
 	grep -q dirty added
 '
 
 test_expect_success 'read-tree adds to worktree, absent case' '
-	echo init.t > .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	git read-tree -u -m HEAD^ &&
 	test ! -f sub/added
 '
 
 test_expect_failure 'read-tree adds to worktree, dirty case' '
-	echo init.t > .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
-	echo dirty > sub/added &&
+	echo dirty >sub/added &&
 	git read-tree -u -m HEAD^ &&
 	grep -q dirty sub/added
 '
 
 test_expect_success 'read-tree --reset removes outside worktree' '
-	echo init.t > .git/info/sparse-checkout &&
+	>empty &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f top &&
 	git reset --hard removed &&
-	test -z "$(git ls-files sub/added)"
+	git ls-files sub/added >result &&
+	test_cmp empty result
 '
 
 test_done
-- 
1.7.2.1.544.ga752d.dirty

[PATCH 2/2] read-tree -m -u: always remove relevant files when narrowing checkout

[Jonathan Nieder]

When narrowing the checkout pattern to exclude a file that the merge
was going to delete, read-tree decides two wrongs make a right and
leaves the file behind.

Or that’s what it looks like.  Actually, for some reason
apply_sparse_checkout() is not paying proper attention to entries
removed by the merge, so they look like ordinary deletions outside
the sparse area.

While fixing that, clarify the semantics of the unpack-trees flags:

 - CE_UPDATE marks an entry for which the worktree might need
   to be updated to match.  If CE_UPDATE is not set, changes
   only apply to the index file.

 - CE_REMOVE marks an entry removed by the merge (just like
   it did before).

 - CE_WT_REMOVE marks an entry that no longer matches the sparse
   pattern.

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sorry for the lack of clarity before.  This is just to give the
idea; I have no confidence that I am not breaking something,
especially because I have not checked why the "if" I am removing
in apply_sparse_checkout was added.

 cache.h                              |    3 +-
 t/t1011-read-tree-sparse-checkout.sh |   21 +++++++++++++++++
 unpack-trees.c                       |   42 +++++++++------------------------
 3 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/cache.h b/cache.h
index 3c7fb6f..c9fa3df 100644
--- a/cache.h
+++ b/cache.h
@@ -179,7 +179,8 @@ struct cache_entry {
 #define CE_UNHASHED  (0x200000)
 #define CE_CONFLICTED (0x800000)
 
-#define CE_WT_REMOVE (0x400000) /* remove in work directory */
+/* Only remove in work directory, not index */
+#define CE_WT_REMOVE (0x400000)
 
 #define CE_UNPACKED  (0x1000000)
 
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 2955071..247f295 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -155,6 +155,27 @@ test_expect_failure 'read-tree adds to worktree, dirty case' '
 	grep -q dirty sub/added
 '
 
+test_expect_success 'narrow checkout while removing file' '
+	echo "H init.t" >expected.swt-after &&
+	printf "%s\n" init.t sub/added | sort >expected.ls-before
+	echo init.t >expected.ls-after &&
+
+	printf "%s\n" init.t sub/added >.git/info/sparse-checkout &&
+	git checkout -f top &&
+	git ls-files -t >index.before &&
+	ls init.t sub/added | sort >worktree.before &&
+
+	echo init.t >.git/info/sparse-checkout &&
+	git read-tree -m -u HEAD^ &&
+	git ls-files -t >index.after &&
+	ls init.t sub/added | sort >worktree.after &&
+
+	test_cmp expected.swt index.before &&
+	test_cmp expected.swt-after index.after &&
+	test_cmp expected.ls-before worktree.before &&
+	test_cmp expected.ls-after worktree.after
+'
+
 test_expect_success 'read-tree --reset removes outside worktree' '
 	>empty &&
 	echo init.t >.git/info/sparse-checkout &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 523e1de..deabfd9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -53,9 +53,6 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 
 	clear |= CE_HASHED | CE_UNHASHED;
 
-	if (set & CE_REMOVE)
-		set |= CE_WT_REMOVE;
-
 	memcpy(new, ce, size);
 	new->next = NULL;
 	new->ce_flags = (new->ce_flags & ~clear) | set;
@@ -87,7 +84,7 @@ static int check_updates(struct unpack_trees_options *o)
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
 			struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+			if (ce->ce_flags & CE_UPDATE)
 				total++;
 		}
 
@@ -101,11 +98,12 @@ static int check_updates(struct unpack_trees_options *o)
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
-		if (ce->ce_flags & CE_WT_REMOVE) {
+		if (ce->ce_flags & CE_UPDATE &&
+		    ce->ce_flags & (CE_REMOVE | CE_WT_REMOVE)) {
 			display_progress(progress, ++cnt);
+			ce->ce_flags &= ~CE_UPDATE & ~CE_WT_REMOVE;
 			if (o->update)
 				unlink_entry(ce);
-			continue;
 		}
 	}
 	remove_marked_cache_entries(&o->result);
@@ -152,15 +150,6 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 	else
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;
 
-	/*
-	 * We only care about files getting into the checkout area
-	 * If merge strategies want to remove some, go ahead, this
-	 * flag will be removed eventually in unpack_trees() if it's
-	 * outside checkout area.
-	 */
-	if (ce->ce_flags & CE_REMOVE)
-		return 0;
-
 	if (!was_skip_worktree && ce_skip_worktree(ce)) {
 		/*
 		 * If CE_UPDATE is set, verify_uptodate() must be called already
@@ -169,7 +158,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 		 */
 		if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
 			return -1;
-		ce->ce_flags |= CE_WT_REMOVE;
+		ce->ce_flags |= CE_WT_REMOVE | CE_UPDATE;
 	}
 	if (was_skip_worktree && !ce_skip_worktree(ce)) {
 		if (verify_absent_sparse(ce, "overwritten", o))
@@ -796,20 +785,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 				goto done;
 			}
 			/*
-			 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
-			 * area as a result of ce_skip_worktree() shortcuts in
-			 * verify_absent() and verify_uptodate().
-			 * Make sure they don't modify worktree.
+			 * Do not modify worktree outside of sparse checkout area,
+			 * except to make the checkout more narrow.
 			 */
-			if (ce_skip_worktree(ce)) {
-				ce->ce_flags &= ~CE_UPDATE;
-
-				if (ce->ce_flags & CE_REMOVE)
-					ce->ce_flags &= ~CE_WT_REMOVE;
-			}
-			else
+			if (!ce_skip_worktree(ce))
 				empty_worktree = 0;
-
+			else if (!(ce->ce_flags & CE_WT_REMOVE))
+				ce->ce_flags &= ~CE_UPDATE;
 		}
 		if (o->result.cache_nr && empty_worktree) {
 			ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
@@ -968,7 +950,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 		if (!ce_stage(ce2)) {
 			if (verify_uptodate(ce2, o))
 				return -1;
-			add_entry(o, ce2, CE_REMOVE, 0);
+			add_entry(o, ce2, CE_REMOVE|CE_UPDATE, 0);
 			mark_ce_used(ce2, o);
 		}
 		cnt++;
@@ -1138,7 +1120,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 	}
 	if (!(old->ce_flags & CE_CONFLICTED) && verify_uptodate(old, o))
 		return -1;
-	add_entry(o, ce, CE_REMOVE, 0);
+	add_entry(o, ce, CE_REMOVE|CE_UPDATE, 0);
 	invalidate_ce_path(ce, o);
 	return 1;
 }
-- 
1.7.2.1.544.ga752d.dirty

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Nguyen Thai Ngoc Duy]

On Sat, Jul 31, 2010 at 11:04 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> What happens if an index entry is removed at the same time as the
>> checkout is narrowed?
>
> Hm, maybe something like this would help.

Or maybe we should move the CE_UPDATE/CE_WT_REMOVE removal code into
apply_sparse_checkout(). That function knows when checkout area is
narrowed and CE_WT_REMOVE should not be removed in that case,
regardless CE_REMOVE|CE_WT_REMOVE combination.
-- 
Duy

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> maybe we should move the CE_UPDATE/CE_WT_REMOVE removal code into
> apply_sparse_checkout().

Not a bad idea regardless.

Re: [PATCH 1/3] Fix sparse checkout not removing files from index

[Nguyen Thai Ngoc Duy]

On Sat, Jul 31, 2010 at 1:33 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
>
>> maybe we should move the CE_UPDATE/CE_WT_REMOVE removal code into
>> apply_sparse_checkout().
>
> Not a bad idea regardless.

Yeah, something like this, perhaps. It's not tested. I'll repost the
patch later, incorporating all your suggestions.

diff --git a/unpack-trees.c b/unpack-trees.c
index f2d148c..48654b3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -153,6 +153,26 @@ static int apply_sparse_checkout(struct
cache_entry *ce, struct unpack_trees_opt
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;

 	/*
+	 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
+	 * area as a result of ce_skip_worktree() shortcuts in
+	 * verify_absent() and verify_uptodate().
+	 * Make sure they don't modify worktree if they are already
+	 * outside checkout area
+	 */
+	if (was_skip_worktree && ce_skip_worktree(ce)) {
+		ce->ce_flags &= ~CE_UPDATE;
+
+		/*
+		 * By default, when CE_REMOVE is on, CE_WT_REMOVE is also
+		 * on to get that file removed from both index and worktree.
+		 * If that file is already outside worktree area, don't
+		 * bother remove it.
+		 */
+		if (ce->ce_flags & CE_REMOVE)
+			ce->ce_flags &= ~CE_WT_REMOVE;
+	}
+
+	/*
 	 * We only care about files getting into the checkout area
 	 * If merge strategies want to remove some, go ahead, this
 	 * flag will be removed eventually in unpack_trees() if it's
@@ -793,19 +813,7 @@ int unpack_trees(unsigned len, struct tree_desc
*t, struct unpack_trees_options
 				ret = -1;
 				goto done;
 			}
-			/*
-			 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
-			 * area as a result of ce_skip_worktree() shortcuts in
-			 * verify_absent() and verify_uptodate().
-			 * Make sure they don't modify worktree.
-			 */
-			if (ce_skip_worktree(ce)) {
-				ce->ce_flags &= ~CE_UPDATE;
-
-				if (ce->ce_flags & CE_REMOVE)
-					ce->ce_flags &= ~CE_WT_REMOVE;
-			}
-			else
+			if (!ce_skip_worktree(ce))
 				empty_worktree = 0;

 		}
-- 
Duy