| Message ID | 5f53b08225771adc0be12c39e7be169d8620f146.1611596534.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sparse Index | expand |
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 >
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 --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);