Re: [PATCH 06/13] add: skip paths that are outside sparse-checkout cone

[Derrick Stolee <stolee@gmail.com>]

On 8/27/2021 5:13 PM, Matheus Tavares wrote:
> On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
>>
>> Subject: [PATCH 06/13] add: skip paths that are outside sparse-checkout cone
> 
> Perhaps this could be "skip _tracked_ paths that ..." (to help
> differentiate the end goal of this patch from the previous one).
> 
>> diff --git a/pathspec.c b/pathspec.c
>> index 44306fdaca2..0e6e60fdc5a 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -39,7 +39,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>>                 return;
>>         for (i = 0; i < istate->cache_nr; i++) {
>>                 const struct cache_entry *ce = istate->cache[i];
>> -               if (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce))
>> +               if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>> +                   (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
> 
> Hmm, even though we skip the sparse paths here, cmd_add() will call
> add_files_to_cache() at the end and still update these paths in the
> index. I think there are two ways to fix this. We could either change
> run_diff_files() to skip these paths (but I don't know how other callers
> of this functions want to handle this, so maybe this needs to hide
> behind an option flag):

Ok, this sent me off on a tangent (see other replies) trying to show
that 'git add <sparse-path>' is still modifying index entries. I finally
added enough checks that this fails in my merge/cherry-pick/rebase tests
for conflicts outside of the sparse-checkout cone. Here is a test that I
can add to t3705 to get a failure:

test_expect_success 'git add fails outside of sparse-checkout definition' '
	test_when_finished git sparse-checkout disable &&
	test_commit a &&
	git sparse-checkout init &&
	git sparse-checkout set a &&

	git update-index --no-skip-worktree sparse_entry &&
	test_must_fail git add sparse_entry &&
	test_sparse_entry_unstaged
'
 
> diff --git a/diff-lib.c b/diff-lib.c
> index f9eadc4fc1..4245d7ead5 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -198,7 +198,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  				continue;
>  		}
>  
> -		if (ce_uptodate(ce) || ce_skip_worktree(ce))
> +		if (ce_uptodate(ce) || ce_skip_worktree(ce) ||
> +		    !path_in_sparse_checkout(ce->name, istate))
>  			continue;
>  
>  		/*
> 
> Or we could change the callback in add itself:
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index f675bdeae4..3d7762aac2 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -94,6 +94,10 @@ static void update_callback(struct diff_queue_struct *q,
>  	for (i = 0; i < q->nr; i++) {
>  		struct diff_filepair *p = q->queue[i];
>  		const char *path = p->one->path;
> +
> +		if (!path_in_sparse_checkout(path, &the_index))
> +			continue;
> +
>  		switch (fix_unmerged_status(p, data)) {
>  		default:
>  			die(_("unexpected diff status %c"), p->status);

If I use this second callback, then I have access to 'include_sparse'
in the later change that adds the --sparse option.

> I believe we also need to update a few other places to use the
> `(ce_skip_worktree(ce) || !path_in_sparse_checkout())` logic in order to
> avoid updating tracked sparse paths: chmod_pathspec() for add's --chmod
> option, renormalize_tracked_files() for --renormalize, and
> read-cache.c:refresh_index() for --refresh.

OK, I'll make sure to include and test these cases.
 
>>                         continue;
>>                 ce_path_match(istate, ce, pathspec, seen);
>>         }
> 
> Hmm, don't we also want to update
> find_pathspecs_matching_skip_worktree() in this patch to use
> path_in_sparse_checkout()? I see you did that in patch 8, but I think
> this should be together with this current patch as, without it, we stop
> adding tracked sparse paths but we print no error/advice message about
> it.

I'll merge the patches to avoid confusion.

Thanks,
-Stolee