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

From: Matheus Tavares <matheus.bernardino@usp.br>
Date: Aug 27, 2021, 21:13 UTC
Message-ID: <20210827211310.108999-1-matheus.bernardino@usp.br>
In-Reply-To: <eeba97ad492307302637faf33f6bf6ae8965faa3.1629842085.git.gitgitgadget@gmail.com>

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).

Show 11 quoted lines

> 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):

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);

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.

>                         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.