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

[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?

> 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