Re: [PATCH v3 5/5] config: add setting to ignore sparsity patterns in some cmds

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Sat, May 30, 2020 at 1:18 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, May 27, 2020 at 6:14 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
> > 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:

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

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.

> > 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!

>
> > 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/

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