Re: [PATCH v2 01/16] name-hash: add index_dir_find()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 23, 2024, 06:37 UTC
- Message-ID
- <xmqqy1bb1y9l.fsf@gitster.g>
- In-Reply-To
- <03b07d9c25edb951db4de518212f8a3e9e184e49.1708658300.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 16 quoted lines
> From: Jeff Hostetler <jeffhostetler@github.com> > > Replace the index_dir_exists() function with index_dir_find() and > change the API to take an optional strbuf to return the canonical > spelling of the matched directory prefix. > > Create an index_dir_exists() wrapper macro for existing callers. > > 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.
The usual way to compose a log message of this project is to
- Give an observation on how the current system work in the present tense (so no need to say "Currently X is Y", just "X is Y"), and discuss what you perceive as a problem in it.
- Propose a solution (optional---often, problem description trivially leads to an obvious solution in reader's minds).
- Give commands to the codebase to "become like so".
in this order.
I think the third paragraph you wrote should come at the beginning, then the first (now second) paragraph should describe more clearly that index_dir_find() is a new function and what it does (perhaps by reusing what is in the "The new version also..." paragraph), without mentioning index_dir_exists().
The second (now third) paragraph then can talk about reimplementing index_dir_exists() in terms of index_dir_find().
The patch text looks good.
Thanks.
Show 35 quoted lines
> -int index_dir_exists(struct index_state *istate, const char *name, int namelen)
> +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;
> }
>
> diff --git a/name-hash.h b/name-hash.h
> index b1b4b0fb337..0cbfc428631 100644
> --- a/name-hash.h
> +++ b/name-hash.h
> @@ -4,7 +4,12 @@
> struct cache_entry;
> struct index_state;
>
> -int index_dir_exists(struct index_state *istate, const char *name, int namelen);
> +
> +int index_dir_find(struct index_state *istate, const char *name, int namelen,
> + struct strbuf *canonical_path);
> +
> +#define index_dir_exists(i, n, l) index_dir_find((i), (n), (l), NULL)
> +
> 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);