On Sun, May 31, 2020 at 9:44 PM Matheus Tavares Bernardino <matheus.bernardino@xxxxxx> wrote: > > On Sat, May 30, 2020 at 12:48 PM Elijah Newren <newren@xxxxxxxxx> wrote: > > > > On Wed, May 27, 2020 at 6:13 PM Matheus Tavares > > <matheus.bernardino@xxxxxx> wrote: > > > [...] > > > +static struct pattern_list *get_sparsity_patterns(struct repository *repo) > > > +{ > > > + struct pattern_list *patterns; > > > + char *sparse_file; > > > + int sparse_config, cone_config; > > > + > > > + if (repo_config_get_bool(repo, "core.sparsecheckout", &sparse_config) || > > > + !sparse_config) { > > > + return NULL; > > > + } > > > > Is core_apply_sparse_checkout not initialized for some reason? > > It should be already initialized, yes. But we cannot rely on that as > `repo` might be a submodule, and core_apply_sparse_checkout holds the > configuration's value for `the_repository`. Ah, gotcha. Thanks for straightening me out. > > > +static int in_sparse_checkout(struct strbuf *path, int prefix_len, > > > > This function name in_sparse_checkout() makes me think "Does the > > working tree represent a sparse checkout?" Perhaps we could rename it > > to path_matches_sparsity_patterns() ? > > > > Also, is there a reason we can't use dir.c's > > path_matches_pattern_list() here? > > Oh, we do use path_matches_pattern_list() inside: > > > > + *match = path_matches_pattern_list(path->buf, path->len, > > > + path->buf + prefix_len, &dtype, > > > + sparsity, istate); > > > + if (*match == UNDECIDED) > > > + *match = parent_match; > > > How does this new function differ > > in behavior from that function? > > The idea of in_sparse_checkout() is to implement a logic closer to > what we have in clear_ce_flags_1(). Here, it is effectively a wrapper > to path_matches_pattern_list() but with some extra logic to decide > whether grep should search in a given entry, based on its mode, the > match result against the sparsity patterns, and the result from the > parent dir. I've had this response and one to 5/5 sitting in my draft folder for over a day because I was hoping to go read clear_ce_flags_1() and find out what it is. I have no idea, so your answer doesn't answer my question... ;-) I'll try to find some time and maybe respond further after I do. > > > > diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh > > > new file mode 100755 > > > index 0000000000..ce080cf572 > > > --- /dev/null > > > +++ b/t/t7817-grep-sparse-checkout.sh > > > @@ -0,0 +1,174 @@ > > > +#!/bin/sh > > > + > > > +test_description='grep in sparse checkout > > > + > > > +This test creates a repo with the following structure: > > > + > > > +. > > > +|-- a > > > +|-- b > > > +|-- dir > > > +| `-- c > > > +|-- sub > > > +| |-- A > > > +| | `-- a > > > +| `-- B > > > +| `-- b > > > +`-- sub2 > > > + `-- a > > > + > > > +Where . has non-cone mode sparsity patterns, sub is a submodule with cone mode > > > > Maybe "Where the outer repository has non-code mode..."? The use of > > '.' threw me for a bit. > > Sure! > > > > +test_done > > > -- > > > 2.26.2 > > > > Looks good. Do we want to add a testcase where a file is unmerged and > > present in the working copy despite not matching the sparsity patterns > > (i.e. to emulate being in the middle of a merge/rebase/cherry-pick)? > > Sure, I can add that. But after a quick test here, it seems that the > unmerged path doesn't have the SKIP_WORKTREE bit set. Is this how it > should be? Right, the merge machinery will clear the SKIP_WORKTREE bit when it writes out conflicted files. Also, any future 'git sparse-checkout' commands will see the unmerged entry and avoid marking it as SKIP_WORKTREE even though it doesn't match the sparsity patterns. Thus, grep doesn't have to do any special checking for whether the files are merged or not, and from your current implementation probably doesn't look like a special case at all -- you just check the SKIP_WORKTREE bit. However, I think the test still has value because the test enforces that other areas of the code (merge, sparse-checkout) don't break the invariants that grep is relying on. (I could see someone making a merge change that keeps the SKIP_WORKTREE bit accidentally set even though it writes the file out to the working tree, for example.) Sure, merge has some tests around that, so it might be viewed as slightly duplicative, but I see it as an interesting edge case that exercises whether the SKIP_WORKTREE bit should really be set and since grep expects a certain invariant about how that is handled, the testcase will help make sure our expectations aren't violated.