Re: [PATCH v3 3/3] builtin/grep.c: walking tree instead of expanding index with --sparse

[Junio C Hamano <gitster@pobox.com>]

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:

> Before this patch, whenever --sparse is used, `git-grep` utilizes the
> ensure_full_index() method to expand the index and search all the
> entries. Because this method requires walking all the trees and
> constructing the index, it is the slow part within the whole command.
>
> To achieve better performance, this patch uses grep_tree() to search the
> sparse directory entries and get rid of the ensure_full_index() method.

When you encounter a "sparsedir" (i.e. a tree recorded in index),
you should know the path leading to that directory. Even though I no
longer remember the details of the implementations of grep_$where()
which I did long time ago, I think grep_tree() should know how to
pass the leading path down, as that is the most natural way to
implement the recursive behaviour.  This patch should be able to
piggyback on that.

> @@ -537,8 +534,26 @@ static int grep_cache(struct grep_opt *opt,
>  
>  		strbuf_setlen(&name, name_base_len);
>  		strbuf_addstr(&name, ce->name);
> +		if (S_ISSPARSEDIR(ce->ce_mode)) {
> +			enum object_type type;
> +			struct tree_desc tree;
> +			void *data;
> +			unsigned long size;
> +			struct strbuf base = STRBUF_INIT;
> +
> +			strbuf_addstr(&base, ce->name);
> +
> +			data = read_object_file(&ce->oid, &type, &size);
> +			init_tree_desc(&tree, data, size);
>  
> +			/*
> +			 * sneak in the ce_mode using check_attr parameter
> +			 */
> +			hit |= grep_tree(opt, pathspec, &tree, &base,
> +					 base.len, ce->ce_mode);

OK.  Instead of inventing a new "base" strbuf, we could reuse
existing name while running the grep_tree() and restore it after it
returns, and I suspect that the end result would be more in line
with how grep_cache() uses that "name" buffer for all the cache
entries.  But that is not a correctness issue (it is move about
preventing from making the code worse).

> @@ -598,7 +613,14 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>  		int te_len = tree_entry_len(&entry);
>  
>  		if (match != all_entries_interesting) {
> -			strbuf_addstr(&name, base->buf + tn_len);
> +			if (S_ISSPARSEDIR(check_attr)) {
> +				// object is a sparse directory entry

No // comments, please.