diff mbox series

[26/27] pathspec: stop calling ensure_full_index

Message ID 5f53b08225771adc0be12c39e7be169d8620f146.1611596534.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series Sparse Index | expand

Commit Message

Derrick Stolee Jan. 25, 2021, 5:42 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

The add_pathspec_matches_against_index() focuses on matching a pathspec
to file entries in the index. It is possible that this already works
correctly for its only use: checking if untracked files exist in the
index.

It is likely that this causes a behavior issue when adding a directory
that exists at HEAD but is outside the sparse cone. I'm marking this as
a place to pursue with future tests.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 pathspec.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Elijah Newren Feb. 1, 2021, 11:24 p.m. UTC | #1
On Mon, Jan 25, 2021 at 9:42 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The add_pathspec_matches_against_index() focuses on matching a pathspec
> to file entries in the index. It is possible that this already works
> correctly for its only use: checking if untracked files exist in the
> index.
>
> It is likely that this causes a behavior issue when adding a directory
> that exists at HEAD but is outside the sparse cone. I'm marking this as
> a place to pursue with future tests.

Sounds like you're unsure if this patch is good.  Should it be marked
RFC or something?

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  pathspec.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 9b105855483..61dc771aa02 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -36,7 +36,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>                         num_unmatched++;
>         if (!num_unmatched)
>                 return;
> -       ensure_full_index(istate);
>         for (i = 0; i < istate->cache_nr; i++) {
>                 const struct cache_entry *ce = istate->cache[i];
>                 ce_path_match(istate, ce, pathspec, seen);
> --
> gitgitgadget
>
Derrick Stolee Feb. 2, 2021, 2:39 a.m. UTC | #2
On 2/1/2021 6:24 PM, Elijah Newren wrote:
> On Mon, Jan 25, 2021 at 9:42 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The add_pathspec_matches_against_index() focuses on matching a pathspec
>> to file entries in the index. It is possible that this already works
>> correctly for its only use: checking if untracked files exist in the
>> index.
>>
>> It is likely that this causes a behavior issue when adding a directory
>> that exists at HEAD but is outside the sparse cone. I'm marking this as
>> a place to pursue with future tests.
> 
> Sounds like you're unsure if this patch is good.  Should it be marked
> RFC or something?

...isn't the whole series marked as RFC? I only specifically marked the
ensure_full_index() one because I purposefully squashed it.

But in general, everything I'm touching in these areas seems like a
potentially problematic change. So many things are used and re-used
that I'm not sure what is safe or not. More testing is required for
commands to ensure their behavior.

I can enable things like GIT_TEST_SPARSE_INDEX to get other tests
using sparse-checkout to work (but only if they use cone mode). Hence,
I'm relying on what tests I can write to cover the behavior instead of
a robust history of valuable tests.

I hope to gather more confidence as this goes forward. I definitely
work to be confident that I am not making any errors that cause
problems for users who do not enable the sparse-index, but I expect
there to be a long tail of adjustments required as more corner
cases are discovered during the development and testing of the
feature.

The good news is that I have some engaged users who are willing to
test the feature and provide feedback if they hit any snags once
there is a minimal set of functionality.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/pathspec.c b/pathspec.c
index 9b105855483..61dc771aa02 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -36,7 +36,6 @@  void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 			num_unmatched++;
 	if (!num_unmatched)
 		return;
-	ensure_full_index(istate);
 	for (i = 0; i < istate->cache_nr; i++) {
 		const struct cache_entry *ce = istate->cache[i];
 		ce_path_match(istate, ce, pathspec, seen);