| Message ID | f7978d223fe86b6d0d43e905ee3abdc02fef8b7d.1659645967.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset/checkout: fix miscellaneous sparse index bugs | expand |
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Victoria Dye <vdye@github.com> > > Replace 'index_entry_exists()' (which returns a binary '1' or '0' depending > on whether a specified entry exists in the index) with > 'index_name_pos_sparse()' (which behaves the same as 'index_name_pos()', > except that it does not expand a sparse index to search for an entry inside > a sparse directory). > > 'index_entry_exists()' was original implemented in 20ec2d034c (reset: make "original" -> "originally"? > sparse-aware (except --mixed), 2021-11-29) to allow callers to search for an > index entry without expanding a sparse index. That particular case only > required knowing whether the requested entry existed. This patch expands the > amount of information returned by indicating both 1) whether the entry > exists, and 2) its position (or potential position) in the index. "position or potential position" is good, but the latter stalled my reading briefly, wondering if "potential" refers to "the position that the entry would be at when the sparse entries are all expanded", which is not what the patch and the description intend to say. I am not sure how to rephrase the proposed log message to alleviate the confusion, though. > diff --git a/cache.h b/cache.h > index 4aa1bd079d5..6bfd1d80b24 100644 > --- a/cache.h > +++ b/cache.h > @@ -831,14 +831,13 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na > int index_name_pos(struct index_state *, const char *name, int namelen); > > /* > + * Like index_name_pos, returns the position of an entry of the given name in > + * the index if one exists, otherwise returns a negative value where the negated > + * value minus 1 is the position where the index entry would be inserted. Unlike > + * index_name_pos, however, a sparse index is not expanded to find an entry > + * inside a sparse directory. > + */ This description is perfectly clear. > +int index_name_pos_sparse(struct index_state *, const char *name, int namelen); > > /* > * Some functions return the negative complement of an insert position when a > diff --git a/read-cache.c b/read-cache.c > index 4de207752dc..a85b922a3bc 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -620,9 +620,9 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen) > return index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE); > } > > -int index_entry_exists(struct index_state *istate, const char *name, int namelen) > +int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen) > { > - return index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0; > + return index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE); > } Nice. Thanks.
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > 'index_entry_exists()' was original implemented in 20ec2d034c (reset: make > sparse-aware (except --mixed), 2021-11-29) to allow callers to search for an > index entry without expanding a sparse index. That particular case only > required knowing whether the requested entry existed. This patch expands the > amount of information returned by indicating both 1) whether the entry > exists, and 2) its position (or potential position) in the index. This patch probably should keep index_entry_exists() to potential new callers that may be introduced by contemporary topics in flight, by doing something like the following squashed in. We know that Shaoxuan's series does add a new callsite, which is of a lessor concern as that series may want to be rebased on top of these fixes anyway. But there may be other topics that may want to add new calls for this helper that is more trivially and obviously correct than these four patches, in which case such topics want to proceed independent of these four patches. cache.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git c/cache.h w/cache.h index ba85435fee..039a32a317 100644 --- c/cache.h +++ w/cache.h @@ -839,6 +839,17 @@ int index_name_pos(struct index_state *, const char *name, int namelen); */ int index_name_pos_sparse(struct index_state *, const char *name, int namelen); +/* + * This helper function is only kept to help possible + * contemporary topics in flight. Do not use it in new + * code; use index_name_pos_sparse() instead. + */ +static inline int index_entry_exists(struct index_state *istate, + const char *name, int namelen) +{ + return 0 <= index_name_pos_sparse(istate, name, namelen); +} + /* * Some functions return the negative complement of an insert position when a * precise match was not found but a position was found where the entry would
diff --git a/cache-tree.c b/cache-tree.c index 56db0b5026b..ff0c1733356 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -798,7 +798,7 @@ static void prime_cache_tree_rec(struct repository *r, * as normal. */ if (r->index->sparse_index && - index_entry_exists(r->index, tree_path->buf, tree_path->len)) + index_name_pos_sparse(r->index, tree_path->buf, tree_path->len) >= 0) prime_cache_tree_sparse_dir(sub->cache_tree, subtree); else prime_cache_tree_rec(r, sub->cache_tree, subtree, tree_path); diff --git a/cache.h b/cache.h index 4aa1bd079d5..6bfd1d80b24 100644 --- a/cache.h +++ b/cache.h @@ -831,14 +831,13 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na int index_name_pos(struct index_state *, const char *name, int namelen); /* - * Determines whether an entry with the given name exists within the - * given index. The return value is 1 if an exact match is found, otherwise - * it is 0. Note that, unlike index_name_pos, this function does not expand - * the index if it is sparse. If an item exists within the full index but it - * is contained within a sparse directory (and not in the sparse index), 0 is - * returned. - */ -int index_entry_exists(struct index_state *, const char *name, int namelen); + * Like index_name_pos, returns the position of an entry of the given name in + * the index if one exists, otherwise returns a negative value where the negated + * value minus 1 is the position where the index entry would be inserted. Unlike + * index_name_pos, however, a sparse index is not expanded to find an entry + * inside a sparse directory. + */ +int index_name_pos_sparse(struct index_state *, const char *name, int namelen); /* * Some functions return the negative complement of an insert position when a diff --git a/read-cache.c b/read-cache.c index 4de207752dc..a85b922a3bc 100644 --- a/read-cache.c +++ b/read-cache.c @@ -620,9 +620,9 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen) return index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE); } -int index_entry_exists(struct index_state *istate, const char *name, int namelen) +int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen) { - return index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0; + return index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE); } int remove_index_entry_at(struct index_state *istate, int pos)