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.
