Re: [PATCH 02/12] name-hash: add index_dir_exists2()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2024, 21:43 UTC
Message-ID: <xmqqeddg2g7j.fsf@gitster.g>
In-Reply-To: <3464545fe3feceb08408618c77a70cc95745bfe9.1707857541.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 46 quoted lines

> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Create a new version of index_dir_exists() to return the canonical
> spelling of the matched directory prefix.
>
> The existing index_dir_exists() returns a boolean to indicate if
> there is a case-insensitive match in the directory name-hash, but
> it doesn't tell the caller the exact spelling of that match.
>
> The new version also copies the matched spelling to a provided strbuf.
> This lets the caller, for example, then call index_name_pos() with the
> correct case to search the cache-entry array for the real insertion
> position.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
>  name-hash.c | 16 ++++++++++++++++
>  name-hash.h |  2 ++
>  2 files changed, 18 insertions(+)
>
> diff --git a/name-hash.c b/name-hash.c
> index 251f036eef6..d735c81acb3 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -694,6 +694,22 @@ int index_dir_exists(struct index_state *istate, const char *name, int namelen)
>  	dir = find_dir_entry(istate, name, namelen);
>  	return dir && dir->nr;
>  }
> +int index_dir_exists2(struct index_state *istate, const char *name, int namelen,
> +		      struct strbuf *canonical_path)
> +{
> +	struct dir_entry *dir;
> +
> +	strbuf_init(canonical_path, namelen+1);
> +
> +	lazy_init_name_hash(istate);
> +	expand_to_path(istate, name, namelen, 0);
> +	dir = find_dir_entry(istate, name, namelen);
> +
> +	if (dir && dir->nr)
> +		strbuf_add(canonical_path, dir->name, dir->namelen);
> +
> +	return dir && dir->nr;
> +}
>  
>  void adjust_dirname_case(struct index_state *istate, char *name)

Missing inter-function blank line, before the new function.

I wonder if we can avoid such repetition---the body of index_dir_exists() is 100% shared with this new function.

Isn't it extremely unusual to receive "struct strbuf *" and call strbuf_init() on it? It means that the caller is expected to have a strbuf and pass a pointer to it, but also it is expected to leave the strbuf uninitialized.

I'd understand if it calls strbuf_reset(), but it may not even be necessary, if we make it responsibility of the caller to pass a valid strbuf to be appended into.

	int index_dir_find(struct index_state *istate,
			   const char *name, int namelen,
			   struct strbuf *canonical_path)
	{
                struct dir_entry *dir;

                lazy_init_name_hash(istate);
                expand_to_path(istate, name, namelen, 0);
                dir = find_dir_entry(istate, name, namelen);

                if (canonical_path && dir && dir->nr) {
			// strbuf_reset(canonical_path); ???
                	strbuf_add(canonical_path, dir->name, dir->namelen);
		}
                return dir && dir->nr;
	}

Then we can do

	#define index_dir_exists(i, n, l) index_dir_find((i), (n), (l), NULL)

in the header for existing callers.

Show 13 quoted lines

>  {
> diff --git a/name-hash.h b/name-hash.h
> index b1b4b0fb337..2fcac5c4870 100644
> --- a/name-hash.h
> +++ b/name-hash.h
> @@ -5,6 +5,8 @@ struct cache_entry;
>  struct index_state;
>  
>  int index_dir_exists(struct index_state *istate, const char *name, int namelen);
> +int index_dir_exists2(struct index_state *istate, const char *name, int namelen,
> +		      struct strbuf *canonical_path);
>  void adjust_dirname_case(struct index_state *istate, char *name);
>  struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);