git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 21/27] sparse-index: expand_to_path no-op if path exists

From
Elijah Newren <newren@gmail.com>
Date
Feb 1, 2021, 22:34 UTC
Message-ID
<CABPp-BG5+cjN3OPRUPSKdDaJWmtagn1NhRJ9sYXg_0UeCLUOPQ@mail.gmail.com>
In-Reply-To
<dfbafbde3f54333dc27a18e46b5b79573f015e60.1611596534.git.gitgitgadget@gmail.com>

On Mon, Jan 25, 2021 at 9:42 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 52 quoted lines
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> We need to check the file hashmap first, then look to see if the
> directory signals a non-sparse directory entry. In such a case, we can
> rely on the contents of the sparse-index.
>
> We still use ensure_full_index() in the case that we hit a path that is
> within a sparse-directory entry.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  name-hash.c    |  6 ++++++
>  sparse-index.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>
> diff --git a/name-hash.c b/name-hash.c
> index 641f6900a7c..cb0f316f652 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -110,6 +110,12 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
>         if (ce->ce_flags & CE_HASHED)
>                 return;
>         ce->ce_flags |= CE_HASHED;
> +
> +       if (ce->ce_mode == CE_MODE_SPARSE_DIRECTORY) {
> +               add_dir_entry(istate, ce);
> +               return;
> +       }
> +
>         hashmap_entry_init(&ce->ent, memihash(ce->name, ce_namelen(ce)));
>         hashmap_add(&istate->name_hash, &ce->ent);
>
> diff --git a/sparse-index.c b/sparse-index.c
> index dd1a06dfdd3..bf8dce9a09b 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -281,9 +281,62 @@ void ensure_full_index(struct index_state *istate)
>         trace2_region_leave("index", "ensure_full_index", istate->repo);
>  }
>
> +static int in_expand_to_path = 0;
> +
>  void expand_to_path(struct index_state *istate,
>                     const char *path, size_t pathlen, int icase)
>  {
> +       struct strbuf path_as_dir = STRBUF_INIT;
> +       int pos;
> +
> +       /* prevent extra recursion */
> +       if (in_expand_to_path)
> +               return;

Maybe "prevent extra expand_to_path() <-> index_file_exists() recursion", just to be extra explicit?

Show 6 quoted lines
> +
> +       if (!istate || !istate->sparse_index)
> +               return;
> +
> +       if (!istate->repo)
> +               istate->repo = the_repository;

So, we assume the_repository if istate->repo isn't set. I guess given the number of the_repository assumptions we have in the code, this isn't a big deal. And instead of a USE_THE_REPOSITORY_COMPATIBILITY_MACROS we have a NO_THE_REPOSITORY_COMPATIBILITY_MACROS, so there's nothing to mark this either.

Show 17 quoted lines
> +
> +       in_expand_to_path = 1;
> +
> +       /*
> +        * We only need to actually expand a region if the
> +        * following are both true:
> +        *
> +        * 1. 'path' is not already in the index.
> +        * 2. Some parent directory of 'path' is a sparse directory.
> +        */
> +
> +       strbuf_add(&path_as_dir, path, pathlen);
> +       strbuf_addch(&path_as_dir, '/');
> +
> +       /* in_expand_to_path prevents infinite recursion here */
> +       if (index_file_exists(istate, path, pathlen, icase))
> +               goto cleanup;

Shouldn't the editing of path_as_dir be done after the index_file_exists() call? In the case that the entry already exists, writing to path_as_dir is wasted work.

Show 12 quoted lines
> +       pos = index_name_pos(istate, path_as_dir.buf, path_as_dir.len);
> +
> +       if (pos < 0)
> +               pos = -pos - 1;
> +
> +       /*
> +        * Even if the path doesn't exist, if the value isn't exactly a
> +        * sparse-directory entry, then there is no need to expand the
> +        * index.
> +        */
> +       if (istate->cache[pos]->ce_mode != CE_MODE_SPARSE_DIRECTORY)
> +               goto cleanup;

This looked wrong to me until I tried to come up with a counter-example. Here you are relying on the fact that before the comment, pos is going to be the index of a sparse directory entry -- either for path_as_dir or some ancestor directory. It would be nice if the comment mentioned that.

Show 14 quoted lines
> +
> +       trace2_region_enter("index", "expand_to_path", istate->repo);
> +
>         /* for now, do the obviously-correct, slow thing */
>         ensure_full_index(istate);
> +
> +       trace2_region_leave("index", "expand_to_path", istate->repo);
> +
> +cleanup:
> +       strbuf_release(&path_as_dir);
> +       in_expand_to_path = 0;
>  }
> --
> gitgitgadget
Looks good otherwise.
Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee via GitGitGadget
Message 44 of 61 in “[RFC] Sparse Index”
  1. 00/27 [RFC] Sparse IndexDerrick Stolee via GitGitGadget, Jan 25, 2021
  2. 02/27 sparse-index: implement ensure_full_index()Derrick Stolee via GitGitGadget, Jan 25, 2021
  3. Elijah NewrenJan 27, 2021
  4. Derrick StoleeJan 27, 2021
  5. Elijah NewrenJan 27, 2021
  6. Junio C HamanoJan 28, 2021
  7. 01/27 sparse-index: add guard to ensure full indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  8. 09/27 sparse-index: convert from full to sparseDerrick Stolee via GitGitGadget, Jan 25, 2021
  9. Elijah NewrenJan 27, 2021
  10. 08/27 sparse-checkout: hold pattern list in indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  11. Elijah NewrenJan 27, 2021
  12. Derrick StoleeJan 28, 2021
  13. 06/27 test-tool: don't force full indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  14. 12/27 sparse-index: check index conversion happensDerrick Stolee via GitGitGadget, Jan 25, 2021
  15. Elijah NewrenJan 27, 2021
  16. 15/27 [RFC-VERSION] *: ensure full indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  17. Elijah NewrenFeb 1, 2021
  18. Derrick StoleeFeb 1, 2021
  19. 26/27 pathspec: stop calling ensure_full_indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  20. Elijah NewrenFeb 1, 2021
  21. Derrick StoleeFeb 2, 2021
  22. 14/27 sparse-checkout: toggle sparse index from builtinDerrick Stolee via GitGitGadget, Jan 25, 2021
  23. Elijah NewrenJan 27, 2021
  24. Derrick StoleeJan 28, 2021
  25. 19/27 status: skip sparse-checkout percentage with sparse-indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  26. 04/27 test-read-cache: print cache entries with --tableDerrick Stolee via GitGitGadget, Jan 25, 2021
  27. Elijah NewrenJan 27, 2021
  28. 16/27 unpack-trees: make sparse awareDerrick Stolee via GitGitGadget, Jan 25, 2021
  29. Elijah NewrenFeb 1, 2021
  30. Derrick StoleeFeb 9, 2021
  31. 03/27 t1092: compare sparse-checkout to sparse-indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  32. Elijah NewrenJan 27, 2021
  33. Derrick StoleeJan 27, 2021
  34. Elijah NewrenJan 27, 2021
  35. 27/27 cache-tree: integrate with sparse directory entriesDerrick Stolee via GitGitGadget, Jan 25, 2021
  36. Elijah NewrenFeb 1, 2021
  37. Derrick StoleeFeb 2, 2021
  38. Elijah NewrenFeb 2, 2021
  39. 18/27 status: use sparse-index throughoutDerrick Stolee via GitGitGadget, Jan 25, 2021
  40. 22/27 add: allow operating on a sparse-only indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  41. Elijah NewrenFeb 1, 2021
  42. 20/27 sparse-index: expand_to_path() trivial implementationDerrick Stolee via GitGitGadget, Jan 25, 2021
  43. 21/27 sparse-index: expand_to_path no-op if path existsDerrick Stolee via GitGitGadget, Jan 25, 2021
  44. Elijah NewrenFeb 1, 2021
  45. 10/27 submodule: sparse-index should not collapse linksDerrick Stolee via GitGitGadget, Jan 25, 2021
  46. 17/27 dir.c: accept a directory as part of cone-mode patternsDerrick Stolee via GitGitGadget, Jan 25, 2021
  47. Elijah NewrenFeb 1, 2021
  48. 25/27 fsmonitor: disable if index is sparseDerrick Stolee via GitGitGadget, Jan 25, 2021
  49. 13/27 sparse-index: create extension for compatibilityDerrick Stolee via GitGitGadget, Jan 25, 2021
  50. Elijah NewrenJan 27, 2021
  51. 07/27 unpack-trees: ensure full indexDerrick Stolee via GitGitGadget, Jan 25, 2021
  52. Elijah NewrenJan 27, 2021
  53. 24/27 dir: use expand_to_path in add_patterns()Derrick Stolee via GitGitGadget, Jan 25, 2021
  54. Elijah NewrenFeb 1, 2021
  55. 05/27 test-tool: read-cache --table --no-statDerrick Stolee via GitGitGadget, Jan 25, 2021
  56. 11/27 unpack-trees: allow sparse directoriesDerrick Stolee via GitGitGadget, Jan 25, 2021
  57. Elijah NewrenJan 27, 2021
  58. 23/27 submodule: die_path_inside_submodule is sparse awareDerrick Stolee via GitGitGadget, Jan 25, 2021
  59. Junio C HamanoJan 25, 2021
  60. Derrick StoleeJan 25, 2021
  61. Elijah NewrenFeb 2, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.