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

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

quoted

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

quoted

diff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt
new file mode 100644
index 0000000000..2a25b4b8ef
--- /dev/null
+++ b/Documentation/config/sparse.txt

@@ -0,0 +1,24 @@
+sparse.restrictCmds::
+       Only meaningful in conjunction with core.sparseCheckout. This option
+       extends sparse checkouts (which limit which paths are written to the
+       working tree), so that output and operations are also limited to the
+       sparsity paths where possible and implemented. The purpose of this
+       option is to (1) focus output for the user on the portion of the
+       repository that is of interest to them, and (2) enable potentially
+       dramatic performance improvements, especially in conjunction with
+       partial clones.
++
+When this option is true (default), some git commands may limit their behavior
+to the paths specified by the sparsity patterns, or to the intersection of
+those paths and any (like `*.c`) that the user might also specify on the
+command line. When false, the affected commands will work on full trees,
+ignoring the sparsity patterns. For now, only git-grep honors this setting. In
+this command, the restriction takes effect in three cases: with --cached; when
+a commit-ish is given; when searching a working tree where some paths excluded
+by the sparsity patterns are present (e.g. manually created paths or not
+removed submodules).

I think "In this command, the restriction takes effect..." to the end
of the paragraph should be removed.  I don't want every subcommand's
behavior to be specified here; it'll grow unreadably long and be more
likely to eventually go stale.

Yeah, I was also concerned about that. But wouldn't it be important to
inform the users how the setting takes place in grep (specially with
the corner cases)? And maybe others, in the future?

What if we move the information that is only relevant to a single
command into its own man page? I.e. git-grep.txt would have something
like:

Moving it to grep's manpage seems ideal to me.  grep's behavior should
be defined in grep's manual.

sparse.restrictCmds::
See complete definition in linkgit:git-config[1]. In grep, the
restriction takes effect in three cases: with --cached; when a
commit-ish is given; when searching a working tree where some paths
excluded by the sparsity patterns are present (e.g. manually created
paths or not removed submodules).

That looks more than a little confusing.  Could this definition be
something more like "See base definition in linkgit:git-config[1].
grep honors sparse.restrictCmds by limiting searches to the sparsity
paths in three cases: when searching the working tree, when searching
the index with --cached, or when searching a specified commit"

The only problem then is that the information would be a little
scattered... But I think it shouldn't be a big deal, as a person
interested in knowing how foo behaves with sparse.restrictCmds would
only need to look into foo's man page, anyway.

quoted

quoted

diff --git a/git.c b/git.c
index a2d337eed7..6db1382ae4 100644
--- a/git.c
+++ b/git.c

@@ -319,6 +324,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
                (*argv)++;
                (*argc)--;
        }
+
        return (*argv) - orig_argv;
 }

Why the stray whitespace change?

Oops, that shouldn't be there. Thanks!

quoted

quoted

diff --git a/sparse-checkout.c b/sparse-checkout.c
new file mode 100644
index 0000000000..9a9e50fd29
--- /dev/null
+++ b/sparse-checkout.c

@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "config.h"
+#include "sparse-checkout.h"
+
+int restrict_to_sparse_paths(struct repository *repo)
+{
+       int ret;
+
+       if (opt_restrict_to_sparse_paths >= 0)
+               return opt_restrict_to_sparse_paths;
+
+       if (repo_config_get_bool(repo, "sparse.restrictcmds", &ret))
+               ret = 1;
+
+       return ret;
+}

Do we want to considering renaming this file to sparse.c, since it's
for sparse grep and sparse diff and etc., not just for the checkout
piece?  It would also go along well with our toplevel related config
being in the "sparse" namespace.

Makes sense. But since Stolee is already working on
"sparse-checkout.c" [1], if we use "sparse.c" in this series we will
end up with two extra files. And as "sparse.c" is quite small, I think
we could unify into the "sparse-checkout.c".

[1]: https://lore.kernel.org/git/0181a134bfb6986dc0e54ae624c478446a1324a9.1588857462.git.gitgitgadget@gmail.com/ (local)

Or we could just suggest he use sparse.c too.  :-)

Stolee?

quoted

quoted

diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh
index ce080cf572..1aef084186 100755
--- a/t/t7817-grep-sparse-checkout.sh
+++ b/t/t7817-grep-sparse-checkout.sh

All these testcases look great (modulo the small typo I pointed out
earlier); I kept thinking "but what about case <x>?" and then I kept
reading and saw you covered it.  You even added some I wasn't thinking
about and might have overlooked but seem important.

Thanks :)