On Tue, Oct 5, 2021 at 6:21 AM Victoria Dye via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Victoria Dye <vdye@xxxxxxxxxx>
>
> In order to accurately reconstruct the cache tree in `prime_cache_tree_rec`,
> the function must determine whether the currently-processing directory in
> the tree is sparse or not. If it is not sparse, the tree is parsed and
> subtree recursively constructed. If it is sparse, no subtrees are added to
> the tree and the entry count is set to 1 (representing the sparse directory
> itself).
>
> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx>
> ---
> cache-tree.c | 44 +++++++++++++++++++++---
> cache.h | 10 ++++++
> read-cache.c | 22 ++++++++----
> t/t1092-sparse-checkout-compatibility.sh | 15 ++++++--
> 4 files changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 9be19c85b66..9021669d682 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -740,15 +740,29 @@ out:
> return ret;
> }
>
> +static void prime_cache_tree_sparse_dir(struct repository *r,
> + struct cache_tree *it,
> + struct tree *tree,
> + struct strbuf *tree_path)
> +{
> +
> + oidcpy(&it->oid, &tree->object.oid);
> + it->entry_count = 1;
> + return;
Why are 'r' and 'tree_path' passed to this function?
> +}
> +
> static void prime_cache_tree_rec(struct repository *r,
> struct cache_tree *it,
> - struct tree *tree)
> + struct tree *tree,
> + struct strbuf *tree_path)
> {
> + struct strbuf subtree_path = STRBUF_INIT;
> struct tree_desc desc;
> struct name_entry entry;
> int cnt;
>
> oidcpy(&it->oid, &tree->object.oid);
> +
Why the blank line addition here?
> init_tree_desc(&desc, tree->buffer, tree->size);
> cnt = 0;
> while (tree_entry(&desc, &entry)) {
> @@ -757,27 +771,49 @@ static void prime_cache_tree_rec(struct repository *r,
> else {
> struct cache_tree_sub *sub;
> struct tree *subtree = lookup_tree(r, &entry.oid);
> +
> if (!subtree->object.parsed)
> parse_tree(subtree);
> sub = cache_tree_sub(it, entry.path);
> sub->cache_tree = cache_tree();
> - prime_cache_tree_rec(r, sub->cache_tree, subtree);
> + strbuf_reset(&subtree_path);
> + strbuf_grow(&subtree_path, tree_path->len + entry.pathlen + 1);
> + strbuf_addbuf(&subtree_path, tree_path);
> + strbuf_add(&subtree_path, entry.path, entry.pathlen);
> + strbuf_addch(&subtree_path, '/');
Reconstructing the full path each time? And despite only being useful
for the sparse-index case?
Would it be better to drop subtree_path from this function, then
append entry.path + '/' here to tree_path, and then after the if-block
below, call strbuf_setlen to remove the part that this function call
added? That way, we don't need subtree_path, and don't have to copy
the leading path every time.
Also, maybe it'd be better to only do this strbuf manipulation if
r->index->sparse_index, since it's not ever used otherwise?
> +
> + /*
> + * If a sparse index is in use, the directory being processed may be
> + * sparse. To confirm that, we can check whether an entry with that
> + * exact name exists in the index. If it does, the created subtree
> + * should be sparse. Otherwise, cache tree expansion should continue
> + * as normal.
> + */
> + if (r->index->sparse_index &&
> + index_entry_exists(r->index, subtree_path.buf, subtree_path.len))
> + prime_cache_tree_sparse_dir(r, sub->cache_tree, subtree, &subtree_path);
> + else
> + prime_cache_tree_rec(r, sub->cache_tree, subtree, &subtree_path);
> cnt += sub->cache_tree->entry_count;
> }
> }
> it->entry_count = cnt;
> +
> + strbuf_release(&subtree_path);
> }
>
> void prime_cache_tree(struct repository *r,
> struct index_state *istate,
> struct tree *tree)
> {
> + struct strbuf tree_path = STRBUF_INIT;
> +
> trace2_region_enter("cache-tree", "prime_cache_tree", the_repository);
> cache_tree_free(&istate->cache_tree);
> istate->cache_tree = cache_tree();
>
> - ensure_full_index(istate);
> - prime_cache_tree_rec(r, istate->cache_tree, tree);
> + prime_cache_tree_rec(r, istate->cache_tree, tree, &tree_path);
> + strbuf_release(&tree_path);
> istate->cache_changed |= CACHE_TREE_CHANGED;
> trace2_region_leave("cache-tree", "prime_cache_tree", the_repository);
> }
> diff --git a/cache.h b/cache.h
> index f6295f3b048..1d3e4665562 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -816,6 +816,16 @@ 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);
> +
> /*
> * 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/read-cache.c b/read-cache.c
> index f5d4385c408..ea1166895f8 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -551,7 +551,10 @@ int cache_name_stage_compare(const char *name1, int len1, int stage1, const char
> return 0;
> }
>
> -static int index_name_stage_pos(struct index_state *istate, const char *name, int namelen, int stage)
> +static int index_name_stage_pos(struct index_state *istate,
> + const char *name, int namelen,
> + int stage,
> + int search_sparse)
It'd be nicer to make search_sparse an enum defined within this file, so that...
> {
> int first, last;
>
> @@ -570,7 +573,7 @@ static int index_name_stage_pos(struct index_state *istate, const char *name, in
> first = next+1;
> }
>
> - if (istate->sparse_index &&
> + if (search_sparse && istate->sparse_index &&
> first > 0) {
> /* Note: first <= istate->cache_nr */
> struct cache_entry *ce = istate->cache[first - 1];
> @@ -586,7 +589,7 @@ static int index_name_stage_pos(struct index_state *istate, const char *name, in
> ce_namelen(ce) < namelen &&
> !strncmp(name, ce->name, ce_namelen(ce))) {
> ensure_full_index(istate);
> - return index_name_stage_pos(istate, name, namelen, stage);
> + return index_name_stage_pos(istate, name, namelen, stage, search_sparse);
> }
> }
>
> @@ -595,7 +598,12 @@ static int index_name_stage_pos(struct index_state *istate, const char *name, in
>
> int index_name_pos(struct index_state *istate, const char *name, int namelen)
> {
> - return index_name_stage_pos(istate, name, namelen, 0);
> + return index_name_stage_pos(istate, name, namelen, 0, 1);
...this could use SEARCH_SPARSE or some name like that which is more
meaningful than "1" here.
> +}
> +
> +int index_entry_exists(struct index_state *istate, const char *name, int namelen)
> +{
> + return index_name_stage_pos(istate, name, namelen, 0, 0) >= 0;
...and likewise this spot could use SEARCH_FULL or some name like
that, which is more meaningful than the second "0".
Similarly for multiple call sites below...
> }
>
> int remove_index_entry_at(struct index_state *istate, int pos)
> @@ -1222,7 +1230,7 @@ static int has_dir_name(struct index_state *istate,
> */
> }
>
> - pos = index_name_stage_pos(istate, name, len, stage);
> + pos = index_name_stage_pos(istate, name, len, stage, 1);
> if (pos >= 0) {
> /*
> * Found one, but not so fast. This could
> @@ -1322,7 +1330,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
> strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
> pos = index_pos_to_insert_pos(istate->cache_nr);
> else
> - pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
> + pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), 1);
>
> /* existing match? Just replace it. */
> if (pos >= 0) {
> @@ -1357,7 +1365,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
> if (!ok_to_replace)
> return error(_("'%s' appears as both a file and as a directory"),
> ce->name);
> - pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
> + pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce), 1);
> pos = -pos-1;
> }
> return pos + 1;
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index f0723a6ac97..e301ef5633a 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -786,9 +786,9 @@ test_expect_success 'sparse-index is not expanded' '
> ensure_not_expanded checkout - &&
> ensure_not_expanded switch rename-out-to-out &&
> ensure_not_expanded switch - &&
> - git -C sparse-index reset --hard &&
> + ensure_not_expanded reset --hard &&
> ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&
> - git -C sparse-index reset --hard &&
> + ensure_not_expanded reset --hard &&
> ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 &&
>
> echo >>sparse-index/README.md &&
> @@ -798,6 +798,17 @@ test_expect_success 'sparse-index is not expanded' '
> echo >>sparse-index/untracked.txt &&
> ensure_not_expanded add . &&
>
> + for ref in update-deep update-folder1 update-folder2 update-deep
> + do
> + echo >>sparse-index/README.md &&
> + ensure_not_expanded reset --hard $ref || return 1
> + done &&
> +
> + ensure_not_expanded reset --hard update-deep &&
> + ensure_not_expanded reset --keep base &&
> + ensure_not_expanded reset --merge update-deep &&
> + ensure_not_expanded reset --hard &&
> +
> ensure_not_expanded checkout -f update-deep &&
> test_config -C sparse-index pull.twohead ort &&
> (
> --
> gitgitgadget
