On Fri, Dec 6, 2024 at 2:13 PM Shubham Kanodia <skanodia@xxxxxxxxxxxxx> wrote:
>
> On Fri, Dec 6, 2024 at 9:53 AM Manoraj K <mkenchugonde@xxxxxxxxxxxxx> wrote:
> >
> > ++ CC
> >
> > On Fri, Dec 6, 2024 at 9:16 AM Manoraj K <mkenchugonde@xxxxxxxxxxxxx> wrote:
> > >
> > > Hi team,
> > >
> > > I am currently on a sparse checkout repository, and it looks like the
> > > `git add—p` command is much slower than in a full checkout repository.
> > >
> > > When run, the `git add—p` command expands to the full index and takes
> > > more time to start showing the patch.
> > >
> > > Also, we have git trace2 enabled, and the timings of the subcommands
> > > gave us an indication that the `git apply—-cached` subcommand is
> > > taking way too long, with an average P50 of 3000ms against 300ms in
> > > the full checkout repository. The other notable spikes are in `git
> > > diff-files --color` (180ms vs 80ms) and `git diff-files --no-color`
> > > (180ms vs 80ms).
> > >
> > > I am really interested in understanding why `git add -p` expands to a
> > > full index, which I believe is the issue with start-up regression.
> > > Also, would be great to understand the difference in performance of
> > > the subcommands.
>
> I started tracing execution of `git add -p`, it seems that `git add
> -p` seems to require a
> full index whereas a regular `git add` does not, because `git add -p`
> ends up calling
> `do_read_index` in `read_cache.c`.
>
>
> `do_read_index` seems to then call `prepare_repo_settings` that resets
> the value of
> `istate->repo.settings.command_requires_full_index` to `1`, even though
> `istate->repo.settings.command_requires_full_index` passed to
> `do_read_index` was
> initially `0` for `git add -p`.
>
> ```
> int do_read_index(struct index_state *istate, const char *path, int must_exist)
> {
> ....
> /*
> * If the command explicitly requires a full index, force it
> * to be full. Otherwise, correct the sparsity based on repository
> * settings and other properties of the index (if necessary).
> */
> prepare_repo_settings(istate->repo);
> ```
>
> The comment here seems confusing to me — it seems like the intent here
> was to preserve
> sparseness when as possible, but `prepare_repo_settings` seemsto do a
> blanket reset —
>
> ```
> void prepare_repo_settings(struct repository *r)
> {
> ....
> /*
> * This setting guards all index reads to require a full index
> * over a sparse index. After suitable guards are placed in the
> * codebase around uses of the index, this setting will be
> * removed.
> */
> r->settings.command_requires_full_index = 1;
> ```
>
> Retaining the original setting seems to fix the problem and makes `git
> add -p` fast,
> however I don't know if there's a reason why it wasn't already done this way.
>
> ```
> read-cache.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 01d0b3ad22..92f7561a88 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2339,7 +2339,12 @@ int do_read_index(struct index_state *istate,
> const char *path, int must_exist)
> * to be full. Otherwise, correct the sparsity based on repository
> * settings and other properties of the index (if necessary).
> */
> + int previous_full_index_setting =
> + istate->repo->settings.command_requires_full_index;
> +
> prepare_repo_settings(istate->repo);
> + istate->repo->settings.command_requires_full_index =
> previous_full_index_setting;
> +
> if (istate->repo->settings.command_requires_full_index)
> ensure_full_index(istate);
> else
> ```
Adding Derick — in case you know if indeed a non-sparse index is
necessary for `git add -p` or if this could just have been an
oversight given support for sparse checkouts was added progressively
to git.
--
Regards,
Shubham Kanodia
