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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2022, 17:17 UTC
Message-ID: <xmqqpmgf9fpr.fsf@gitster.g>
In-Reply-To: <20220901045736.523371-4-shaoxuan.yuan02@gmail.com>

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

Show 7 quoted lines

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

Show 21 quoted lines

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

Show 7 quoted lines

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