Re: [RFC PATCH v2 3/4] grep: honor sparse checkout patterns

On Fri, May 22, 2020 at 1:54 PM Matheus Tavares Bernardino
[off-list ref] wrote:

Hi, Elijah

On Fri, May 22, 2020 at 12:36 PM Elijah Newren [off-list ref] wrote:

quoted

On Fri, May 22, 2020 at 7:26 AM Elijah Newren [off-list ref] wrote:

quoted

Hi Matheus,

On Thu, May 21, 2020 at 10:49 PM Matheus Tavares Bernardino [off-list ref] wrote:

quoted

On Thu, May 21, 2020 at 2:52 PM Elijah Newren [off-list ref] wrote:

quoted

<snip>

quoted

quoted

Does this seem like a good approach? Or is there another solution that
I have not considered? Or even further, should we choose to skip the
submodules in excluded paths? My only concern in this case is that it
would be contrary to the design in git-sparse-checkout.txt. And the
working tree grep and cached grep would differ even on a clean working
tree.

<snip>

quoted

Anyway, the wording in that file seems to be really important, so
let's fix it.

Let me also try to give a concrete proposal for grep behavior for the
edge cases we've discussed:

Thank you for this proposal and for the previous comments as well.

quoted

git -c sparse.restrictCmds=true grep --recurse-submodules $PATTERN

This goes through all the files in the index (i.e. all tracked files)
which do not have the SKIP_WORKTREE bit set.  For each of these: If
the file is a symlink, ignore it (like grep currently does).  If the
file is a regular file and is present in the working copy, search it.
If the file is a submodule and it is initialized, recurse into it.

Sounds good. And when sparse.restrictCmds=false, we also search the
present files and present initialized submodules that have the
SKIP_WORKTREE set, right?

You're really pushing those corner cases, I love it.  :-)
SKIP_WORKTREE is supposed to mean we have removed it from the working
tree, i.e. it shouldn't be present (if we decide we're not going to
remove it from the working tree, e.g. because the file is unmerged or
something, then we don't mark it as SKIP_WORKTREE even if it doesn't
match sparsity patterns).  Therefore, the set of files that satisfy
this condition you have given should generally be empty.

But presuming we hit this corner case, I'd say you are right.
sparse.restrictCmds=false means we ignore the SKIP_WORKTREE bit
entirely (and in the case of grepping a $REVISION, we ignore the
sparsity patterns entirely).

quoted

git -c sparse.restrictCmds=true grep --recurse-submodules --cached $PATTERN

This goes through all the files in the index (i.e. all tracked files)
which do not have the SKIP_WORKTREE bit set.  For each of these: Skip
symlinks.  Search regular files.  Recurse into submodules if they are
initialized.

OK.

quoted

git -c sparse.restrictCmds=true grep --recurse-submodules $REVISION $PATTERN

This goes through all the files in the given revision (i.e. all
tracked files) which match the sparsity patterns (i.e. that would not
have the SKIP_WORKTREE bit set if were we to checkout that commit).
For each of these: Skip symlinks.  Search regular files.  Recurse into
submodules if they are initialized.

OK.

quoted

Further, for any of these, when recursing into submodules, make sure
to load that submodules' core.sparseCheckout setting (and related
settings) and the submodules' sparsity patterns, if any.

Sound good?

I think this addresses the edge cases we've discussed so far:
interaction between submodules and sparsity patterns, and handling of
files that are still present despite not matching the sparsity
patterns. (Also note that files which are present-despite-the-rules
are prone to be removed by the next `git sparse-checkout reapply` or
anything that triggers a call to unpack_trees(); there's already
multiple things that do and Stolee's proposed patches would add more).
If I've missed edge cases, let me know.

Sounds great. This addresses all the edge cases we've mentioned
before. Thanks again for the detailed proposal, and for considering
case by case.

And thank you for working on this.  :-)