Matheus Tavares <matheus.bernardino@xxxxxx> writes:
> One of the main uses for a sparse checkout is to allow users to focus on
> the subset of files in a repository in which they are interested. But
> git-grep currently ignores the sparsity patterns and report all matches
> found outside this subset, which kind of goes in the opposite direction.
> Let's fix that, making it honor the sparsity boundaries for every
> grepping case:
>
> - git grep in worktree
> - git grep --cached
> - git grep $REVISION
It makes sense for these to be limited within the "sparse" area.
> - git grep --untracked and git grep --no-index (which already respect
> sparse checkout boundaries)
I can understand the former; those untracked files are what _could_
be brought into attention by "git add", so limiting to the same
"sparse" area may make sense.
I am not sure about the latter, though, as "--no-index" is an
explicit request to pretend that we are dealing with a random
collection of files, not managed in a git repository. But perhaps
there is a similar justification like how "--untracked" is
unjustifiable. I dunno.
> diff --git a/builtin/grep.c b/builtin/grep.c
> index a5056f395a..91ee0b2734 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -410,7 +410,7 @@ static int grep_cache(struct grep_opt *opt,
> const struct pathspec *pathspec, int cached);
> static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> struct tree_desc *tree, struct strbuf *base, int tn_len,
> - int check_attr);
> + int is_root_tree);
>
> static int grep_submodule(struct grep_opt *opt,
> const struct pathspec *pathspec,
> @@ -508,6 +508,10 @@ static int grep_cache(struct grep_opt *opt,
>
> for (nr = 0; nr < repo->index->cache_nr; nr++) {
> const struct cache_entry *ce = repo->index->cache[nr];
> +
> + if (ce_skip_worktree(ce) && !S_ISGITLINK(ce->ce_mode))
> + continue;
Hmph. Why exclude gitlink from this rule? If a submodule sits at a
path that is excluded by the sparse pattern, should we still recurse
into it?
> strbuf_setlen(&name, name_base_len);
> strbuf_addstr(&name, ce->name);
>
> @@ -520,8 +524,7 @@ static int grep_cache(struct grep_opt *opt,
> * cache entry are identical, even if worktree file has
> * been modified, so use cache version instead
> */
> - if (cached || (ce->ce_flags & CE_VALID) ||
> - ce_skip_worktree(ce)) {
> + if (cached || (ce->ce_flags & CE_VALID)) {
> if (ce_stage(ce) || ce_intent_to_add(ce))
> continue;
> hit |= grep_oid(opt, &ce->oid, name.buf,
> @@ -552,9 +555,78 @@ static int grep_cache(struct grep_opt *opt,
> return hit;
> }
>
> -static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> - struct tree_desc *tree, struct strbuf *base, int tn_len,
> - int check_attr)
> +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;
> + }
> +
> + sparse_file = repo_git_path(repo, "info/sparse-checkout");
> + patterns = xcalloc(1, sizeof(*patterns));
> +
> + if (repo_config_get_bool(repo, "core.sparsecheckoutcone", &cone_config))
> + cone_config = 0;
> + patterns->use_cone_patterns = cone_config;
> +
> + if (add_patterns_from_file_to_list(sparse_file, "", 0, patterns, NULL)) {
> + if (file_exists(sparse_file)) {
> + warning(_("failed to load sparse-checkout file: '%s'"),
> + sparse_file);
> + }
> + free(sparse_file);
> + free(patterns);
> + return NULL;
> + }
> +
> + free(sparse_file);
> + return patterns;
> +}
> +
> +static int in_sparse_checkout(struct strbuf *path, int prefix_len,
> + unsigned int entry_mode,
> + struct index_state *istate,
> + struct pattern_list *sparsity,
> + enum pattern_match_result parent_match,
> + enum pattern_match_result *match)
> +{
> + int dtype = DT_UNKNOWN;
> +
> + if (S_ISGITLINK(entry_mode))
> + return 1;
This is consistent with the "we do not care where a gitlink
appears---submodules are always descended into, regardless of the
sparse definition" decision we saw earlier, I think. I am not sure
if that is a good design in the first place, though.
> + if (parent_match == MATCHED_RECURSIVE) {
> + *match = parent_match;
> + return 1;
> + }
> +
> + if (S_ISDIR(entry_mode) && !is_dir_sep(path->buf[path->len - 1]))
> + strbuf_addch(path, '/');
> +
> + *match = path_matches_pattern_list(path->buf, path->len,
> + path->buf + prefix_len, &dtype,
> + sparsity, istate);
> + if (*match == UNDECIDED)
> + *match = parent_match;
> +
> + if (S_ISDIR(entry_mode))
> + strbuf_trim_trailing_dir_sep(path);
> +
> + if (*match == NOT_MATCHED && (S_ISREG(entry_mode) ||
> + (S_ISDIR(entry_mode) && sparsity->use_cone_patterns)))
> + return 0;
> +
> + return 1;
> +}
> +static int do_grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> + struct tree_desc *tree, struct strbuf *base, int tn_len,
> + int check_attr, struct pattern_list *sparsity,
> + enum pattern_match_result default_sparsity_match)
> {
> struct repository *repo = opt->repo;
> int hit = 0;
> @@ -570,6 +642,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>
> while (tree_entry(tree, &entry)) {
> int te_len = tree_entry_len(&entry);
> + enum pattern_match_result sparsity_match = 0;
>
> if (match != all_entries_interesting) {
> strbuf_addstr(&name, base->buf + tn_len);
> @@ -586,6 +659,19 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>
> strbuf_add(base, entry.path, te_len);
>
> + if (sparsity) {
> + struct strbuf path = STRBUF_INIT;
> + strbuf_addstr(&path, base->buf + tn_len);
> +
> + if (!in_sparse_checkout(&path, old_baselen - tn_len,
> + entry.mode, repo->index,
> + sparsity, default_sparsity_match,
> + &sparsity_match)) {
> + strbuf_setlen(base, old_baselen);
> + continue;
> + }
> + }
OK.
> if (S_ISREG(entry.mode)) {
> hit |= grep_oid(opt, &entry.oid, base->buf, tn_len,
> check_attr ? base->buf + tn_len : NULL);
> @@ -602,8 +688,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>
> strbuf_addch(base, '/');
> init_tree_desc(&sub, data, size);
> - hit |= grep_tree(opt, pathspec, &sub, base, tn_len,
> - check_attr);
> + hit |= do_grep_tree(opt, pathspec, &sub, base, tn_len,
> + check_attr, sparsity, sparsity_match);
> free(data);
> } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
> hit |= grep_submodule(opt, pathspec, &entry.oid,
> @@ -621,6 +707,31 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> return hit;
> }
>
> +/*
> + * Note: sparsity patterns and paths' attributes will only be considered if
> + * is_root_tree has true value. (Otherwise, we cannot properly perform pattern
> + * matching on paths.)
> + */
> +static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> + struct tree_desc *tree, struct strbuf *base, int tn_len,
> + int is_root_tree)
> +{
> + struct pattern_list *patterns = NULL;
> + int ret;
> +
> + if (is_root_tree)
> + patterns = get_sparsity_patterns(opt->repo);
> +
> + ret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree,
> + patterns, 0);
> +
> + if (patterns) {
> + clear_pattern_list(patterns);
> + free(patterns);
> + }
OK, it is not like this codepath is driven by "git log" to grep from
top-level tree objects of many commits, so it is OK to grab the
sparsity patterns once before do_grep_tree() and discard it when we
are done.
> + return ret;
> +}
> +
> static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
> struct object *obj, const char *name, const char *path)
> {
> diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
> index 37525cae3a..26852586ac 100755
> --- a/t/t7011-skip-worktree-reading.sh
> +++ b/t/t7011-skip-worktree-reading.sh
> @@ -109,15 +109,6 @@ test_expect_success 'ls-files --modified' '
> test -z "$(git ls-files -m)"
> '
>
> -test_expect_success 'grep with skip-worktree file' '
> - git update-index --no-skip-worktree 1 &&
> - echo test > 1 &&
> - git update-index 1 &&
> - git update-index --skip-worktree 1 &&
> - rm 1 &&
> - test "$(git grep --no-ext-grep test)" = "1:test"
> -'
> -
> echo ":000000 100644 $ZERO_OID $EMPTY_BLOB A 1" > expected
> test_expect_success 'diff-index does not examine skip-worktree absent entries' '
> setup_absent &&
> diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh
> new file mode 100755
> index 0000000000..3bd67082eb
> --- /dev/null
> +++ b/t/t7817-grep-sparse-checkout.sh
> @@ -0,0 +1,140 @@
> +#!/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
> +
> +Where . has non-cone mode sparsity patterns and sub is a submodule with cone
> +mode sparsity patterns. The resulting sparse-checkout should leave the following
> +structure:
> +
> +.
> +|-- a
> +`-- sub
> + `-- B
> + `-- b
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + echo "text" >a &&
> + echo "text" >b &&
> + mkdir dir &&
> + echo "text" >dir/c &&
> +
> + git init sub &&
> + (
> + cd sub &&
> + mkdir A B &&
> + echo "text" >A/a &&
> + echo "text" >B/b &&
> + git add A B &&
> + git commit -m sub &&
> + git sparse-checkout init --cone &&
> + git sparse-checkout set B
> + ) &&
> +
> + git submodule add ./sub &&
> + git add a b dir &&
> + git commit -m super &&
> + git sparse-checkout init --no-cone &&
> + git sparse-checkout set "/*" "!b" "!/*/" &&
> +
> + git tag -am t-commit t-commit HEAD &&
> + tree=$(git rev-parse HEAD^{tree}) &&
> + git tag -am t-tree t-tree $tree &&
> +
> + test_path_is_missing b &&
> + test_path_is_missing dir &&
> + test_path_is_missing sub/A &&
> + test_path_is_file a &&
> + test_path_is_file sub/B/b
> +'
> +
> +test_expect_success 'grep in working tree should honor sparse checkout' '
> + cat >expect <<-EOF &&
> + a:text
> + EOF
> + git grep "text" >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'grep --cached should honor sparse checkout' '
> + cat >expect <<-EOF &&
> + a:text
> + EOF
> + git grep --cached "text" >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'grep <commit-ish> should honor sparse checkout' '
> + commit=$(git rev-parse HEAD) &&
> + cat >expect_commit <<-EOF &&
> + $commit:a:text
> + EOF
> + cat >expect_t-commit <<-EOF &&
> + t-commit:a:text
> + EOF
> + git grep "text" $commit >actual_commit &&
> + test_cmp expect_commit actual_commit &&
> + git grep "text" t-commit >actual_t-commit &&
> + test_cmp expect_t-commit actual_t-commit
> +'
> +
> +test_expect_success 'grep <tree-ish> should ignore sparsity patterns' '
> + commit=$(git rev-parse HEAD) &&
> + tree=$(git rev-parse HEAD^{tree}) &&
> + cat >expect_tree <<-EOF &&
> + $tree:a:text
> + $tree:b:text
> + $tree:dir/c:text
> + EOF
> + cat >expect_t-tree <<-EOF &&
> + t-tree:a:text
> + t-tree:b:text
> + t-tree:dir/c:text
> + EOF
> + git grep "text" $tree >actual_tree &&
> + test_cmp expect_tree actual_tree &&
> + git grep "text" t-tree >actual_t-tree &&
> + test_cmp expect_t-tree actual_t-tree
> +'
> +
> +test_expect_success 'grep --recurse-submodules --cached should honor sparse checkout in submodule' '
> + cat >expect <<-EOF &&
> + a:text
> + sub/B/b:text
> + EOF
> + git grep --recurse-submodules --cached "text" >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'grep --recurse-submodules <commit-ish> should honor sparse checkout in submodule' '
> + commit=$(git rev-parse HEAD) &&
> + cat >expect_commit <<-EOF &&
> + $commit:a:text
> + $commit:sub/B/b:text
> + EOF
> + cat >expect_t-commit <<-EOF &&
> + t-commit:a:text
> + t-commit:sub/B/b:text
> + EOF
> + git grep --recurse-submodules "text" $commit >actual_commit &&
> + test_cmp expect_commit actual_commit &&
> + git grep --recurse-submodules "text" t-commit >actual_t-commit &&
> + test_cmp expect_t-commit actual_t-commit
> +'
> +
> +test_done
