Re: [PATCH] name-hash: don't add sparse directories in threaded lazy init

From: Derrick Stolee <stolee@gmail.com>
Date: May 21, 2025, 17:17 UTC
Message-ID: <9c26d844-6ac5-449b-a5ff-a842ed6ba8b9@gmail.com>
In-Reply-To: <pull.1970.git.git.1747827645129.gitgitgadget@gmail.com>

On 5/21/2025 7:40 AM, Alex Mironov via GitGitGadget wrote:

> From: Alex Mironov <alexandrfox@gmail.com>
> 
> Similarly to 5f116695864788d1fe45ff06bfad7a71a8d98d0a

nit: we typically use the "reference" style to refer to other commits, use 'git log -1 --pretty=reference <oid>' to get output like this:

  5f116695864 (name-hash: don't add directories to name_hash, 2021-04-12)

Show 29 quoted lines

> make sure to avoid placing sparse directories into the name_hash
> hashtable whenever multithreaded initialization is performed.
> 
> Sparse directory entries represent a directory that is outside the
> sparse-checkout definition. These are not paths to blobs, so should not
> be added to the name_hash table as they must never be queried.
> 
> Signed-off-by: Alex Mironov <alexandrfox@gmail.com>
> ---
>     name-hash: don't add sparse directories in threaded lazy init
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1970%2Falexandrfox%2Ffix-threaded-hash-name-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1970/alexandrfox/fix-threaded-hash-name-v1
> Pull-Request: https://github.com/git/git/pull/1970
> 
>  name-hash.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/name-hash.c b/name-hash.c
> index d66de1cdfd5..03123a8779a 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -492,6 +492,9 @@ static void *lazy_name_thread_proc(void *_data)
>  	for (k = 0; k < d->istate->cache_nr; k++) {
>  		struct cache_entry *ce_k = d->istate->cache[k];
>  		ce_k->ce_flags |= CE_HASHED;
> +		if (S_ISSPARSEDIR(ce_k->ce_mode)) {
> +			continue;
> +		}

nit: for one-line blocks, we usually skip the braces. But I think that it might be better to reverse the logic to get something like:

	if (!S_ISSPARSEDIR(ce_k->ce_mode) {
  		hashmap_entry_init(&ce_k->ent, d->lazy_entries[k].hash_name);
 		hashmap_add(&d->istate->name_hash, &ce_k->ent);
	}

This seems to be a performance-only fix, and it might be interesting to see if there is any impact on p2000-sparse-operations.sh. Those tests don't focus on many sparse-directory entries, so that may not demonstrate any meaningful difference.

Thanks, -Stolee