Re: [PATCH v3 4/5] grep: honor sparse checkout patterns

On Sun, May 31, 2020 at 9:44 PM Matheus Tavares Bernardino
[off-list ref] wrote:

On Sat, May 30, 2020 at 12:48 PM Elijah Newren [off-list ref] wrote:

quoted

On Wed, May 27, 2020 at 6:13 PM Matheus Tavares
[off-list ref] wrote:

quoted

[...]

quoted

quoted

+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.

quoted

quoted

+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:

quoted

quoted

+       *match = path_matches_pattern_list(path->buf, path->len,
+                                          path->buf + prefix_len, &dtype,
+                                          sparsity, istate);
+       if (*match == UNDECIDED)
+               *match = parent_match;

quoted

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.

quoted

quoted

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!

quoted

quoted

+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.