On 8/27/2021 5:13 PM, Matheus Tavares wrote:
> On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@xxxxxxxxx> 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
