Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7b600d5
sparse-index: design doc and format update
derrickstolee Feb 23, 2021
202253e
t/perf: add performance test for sparse operations
derrickstolee Feb 15, 2021
437a0f1
t1092: clean up script quoting
derrickstolee Feb 15, 2021
b7e1bf5
sparse-index: add guard to ensure full index
derrickstolee Jan 8, 2021
e41d55d
sparse-index: implement ensure_full_index()
derrickstolee Jan 8, 2021
7bfbfbd
t1092: compare sparse-checkout to sparse-index
derrickstolee Jan 15, 2021
a1b8135
test-read-cache: print cache entries with --table
derrickstolee Dec 18, 2020
dd84a2a
test-tool: don't force full index
derrickstolee Jan 7, 2021
b276d2e
unpack-trees: ensure full index
derrickstolee Jan 19, 2021
c3651e2
sparse-checkout: hold pattern list in index
derrickstolee Jan 12, 2021
f926cf8
sparse-index: add 'sdir' index extension
derrickstolee Mar 29, 2021
c870ae5
sparse-index: convert from full to sparse
derrickstolee Jan 8, 2021
bcf0da9
submodule: sparse-index should not collapse links
derrickstolee Jan 11, 2021
7191b48
unpack-trees: allow sparse directories
derrickstolee Jan 5, 2021
57be9b4
sparse-index: check index conversion happens
derrickstolee Jan 8, 2021
c22b411
sparse-index: add index.sparse config option
derrickstolee Mar 29, 2021
75fe9b0
sparse-checkout: toggle sparse index from builtin
derrickstolee Jan 13, 2021
7f55a23
sparse-checkout: disable sparse-index
derrickstolee Feb 18, 2021
3659018
cache-tree: integrate with sparse directory entries
derrickstolee Jan 13, 2021
9b068c4
sparse-index: loose integration with cache_tree_verify()
derrickstolee Feb 16, 2021
6660273
p2000: add sparse-index repos
derrickstolee Feb 15, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Documentation/config/index.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ index.recordOffsetTable::
Defaults to 'true' if index.threads has been explicitly enabled,
'false' otherwise.

index.sparse::
When enabled, write the index using sparse-directory entries. This
has no effect unless `core.sparseCheckout` and
`core.sparseCheckoutCone` are both enabled. Defaults to 'false'.

index.threads::
Specifies the number of threads to spawn when loading the index.
This is meant to reduce index load time on multiprocessor machines.
Expand Down
14 changes: 14 additions & 0 deletions Documentation/git-sparse-checkout.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@ To avoid interfering with other worktrees, it first enables the
When `--cone` is provided, the `core.sparseCheckoutCone` setting is

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Martin Ågren wrote (reply to this):

On Wed, 24 Feb 2021 at 00:57, Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> +that is not completely understood by other tools. Enabling sparse index
> +enables the `extensions.spareseIndex` config value, which might cause

s/sparese/sparse

> +other tools to stop working with your repository. If you have trouble with
> +this compatibility, then run `git sparse-checkout sparse-index disable` to
> +remove this config and rewrite your index to not be sparse.

While I'm commenting on this..:

There are several "layers" here, for lack of a better term. "Enabling foo
enables bar which may cause baz. If you fail due to baz, try dropping
bar by dropping foo." If I remove any mention of the config variable from
your text, I get the following.

 Enabling sparse index might cause other tools to stop working with your
 repository. If you have trouble with this compatibility, then run `git
 sparse-checkout sparse-index disable` to rewrite your index to not be
 sparse.

I'm tempted to suggest such a rewrite to relieve readers of knowing of
the middle step, which you could say is more of an implementation
detail. But if we think that the symptoms / error messages might involve
"extensions.sparseIndex" or, as would be the case with an older Git
installation,

  fatal: unknown repository extensions found:
          sparseindex

maybe there is some value in mentioning the config item by name. Just
thinking out loud, really, and I don't have any strong opinion. I only
came here to point out the typo in the docs.

Martin

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/24/2021 2:11 PM, Martin Ågren wrote:
> On Wed, 24 Feb 2021 at 00:57, Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +that is not completely understood by other tools. Enabling sparse index
>> +enables the `extensions.spareseIndex` config value, which might cause
> 
> s/sparese/sparse

Thanks!

 
>> +other tools to stop working with your repository. If you have trouble with
>> +this compatibility, then run `git sparse-checkout sparse-index disable` to
>> +remove this config and rewrite your index to not be sparse.
> 
> While I'm commenting on this..:
> 
> There are several "layers" here, for lack of a better term. "Enabling foo
> enables bar which may cause baz. If you fail due to baz, try dropping
> bar by dropping foo." If I remove any mention of the config variable from
> your text, I get the following.
> 
>  Enabling sparse index might cause other tools to stop working with your
>  repository. If you have trouble with this compatibility, then run `git
>  sparse-checkout sparse-index disable` to rewrite your index to not be
>  sparse.
> 
> I'm tempted to suggest such a rewrite to relieve readers of knowing of
> the middle step, which you could say is more of an implementation
> detail. But if we think that the symptoms / error messages might involve
> "extensions.sparseIndex" or, as would be the case with an older Git
> installation,
> 
>   fatal: unknown repository extensions found:
>           sparseindex
> 
> maybe there is some value in mentioning the config item by name. Just
> thinking out loud, really, and I don't have any strong opinion. I only
> came here to point out the typo in the docs.
 
I agree that the layers are confusing. We could rearrange and have
a similar flow to what you recommend by mentioning the extension at
the end:

**WARNING:** Using a sparse index requires modifying the index in a way
that is not completely understood by other tools. If you have trouble with
this compatibility, then run `git sparse-checkout sparse-index disable` to
rewrite your index to not be sparse. Older versions of Git will not
understand the `sparseIndex` repository extension and may fail to interact
with your repository until it is disabled.

Thanks,
-Stolee

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Mar 9, 2021 at 12:52 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 2/24/2021 2:11 PM, Martin Ågren wrote:
> > On Wed, 24 Feb 2021 at 00:57, Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +that is not completely understood by other tools. Enabling sparse index
> >> +enables the `extensions.spareseIndex` config value, which might cause
> >
> > s/sparese/sparse
>
> Thanks!
>
>
> >> +other tools to stop working with your repository. If you have trouble with
> >> +this compatibility, then run `git sparse-checkout sparse-index disable` to
> >> +remove this config and rewrite your index to not be sparse.
> >
> > While I'm commenting on this..:
> >
> > There are several "layers" here, for lack of a better term. "Enabling foo
> > enables bar which may cause baz. If you fail due to baz, try dropping
> > bar by dropping foo." If I remove any mention of the config variable from
> > your text, I get the following.
> >
> >  Enabling sparse index might cause other tools to stop working with your
> >  repository. If you have trouble with this compatibility, then run `git
> >  sparse-checkout sparse-index disable` to rewrite your index to not be
> >  sparse.
> >
> > I'm tempted to suggest such a rewrite to relieve readers of knowing of
> > the middle step, which you could say is more of an implementation
> > detail. But if we think that the symptoms / error messages might involve
> > "extensions.sparseIndex" or, as would be the case with an older Git
> > installation,
> >
> >   fatal: unknown repository extensions found:
> >           sparseindex
> >
> > maybe there is some value in mentioning the config item by name. Just
> > thinking out loud, really, and I don't have any strong opinion. I only
> > came here to point out the typo in the docs.
>
> I agree that the layers are confusing. We could rearrange and have
> a similar flow to what you recommend by mentioning the extension at
> the end:
>
> **WARNING:** Using a sparse index requires modifying the index in a way
> that is not completely understood by other tools. If you have trouble with
> this compatibility, then run `git sparse-checkout sparse-index disable` to
> rewrite your index to not be sparse. Older versions of Git will not
> understand the `sparseIndex` repository extension and may fail to interact
> with your repository until it is disabled.
>
> Thanks,
> -Stolee

This looks pretty good to me, but could we change the first sentence
to read "...modifying the index in a way that may not yet be
understood by external tools." ?  I'm worried "other tools" might make
people worry about different builtin commands (e.g. fast-export, log).
I also prefer "may" and "yet" because I suspect most external tools
(e.g. git filter-repo just to name a personal example) won't need to
read an index format and will thus be unaffected, and any tools that
do read the index format will probably eventually learn how to work
with the new format.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 3/9/2021 4:03 PM, Elijah Newren wrote:
> On Tue, Mar 9, 2021 at 12:52 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 2/24/2021 2:11 PM, Martin Ågren wrote:
>>> There are several "layers" here, for lack of a better term. "Enabling foo
>>> enables bar which may cause baz. If you fail due to baz, try dropping
>>> bar by dropping foo." If I remove any mention of the config variable from
>>> your text, I get the following.
>>>
>>>  Enabling sparse index might cause other tools to stop working with your
>>>  repository. If you have trouble with this compatibility, then run `git
>>>  sparse-checkout sparse-index disable` to rewrite your index to not be
>>>  sparse.
>>>
>>> I'm tempted to suggest such a rewrite to relieve readers of knowing of
>>> the middle step, which you could say is more of an implementation
>>> detail. But if we think that the symptoms / error messages might involve
>>> "extensions.sparseIndex" or, as would be the case with an older Git
>>> installation,
>>>
>>>   fatal: unknown repository extensions found:
>>>           sparseindex
>>>
>>> maybe there is some value in mentioning the config item by name. Just
>>> thinking out loud, really, and I don't have any strong opinion. I only
>>> came here to point out the typo in the docs.
>>
>> I agree that the layers are confusing. We could rearrange and have
>> a similar flow to what you recommend by mentioning the extension at
>> the end:
>>
>> **WARNING:** Using a sparse index requires modifying the index in a way
>> that is not completely understood by other tools. If you have trouble with
>> this compatibility, then run `git sparse-checkout sparse-index disable` to
>> rewrite your index to not be sparse. Older versions of Git will not
>> understand the `sparseIndex` repository extension and may fail to interact
>> with your repository until it is disabled.
>>
>> Thanks,
>> -Stolee
> 
> This looks pretty good to me, but could we change the first sentence
> to read "...modifying the index in a way that may not yet be
> understood by external tools." ?  I'm worried "other tools" might make
> people worry about different builtin commands (e.g. fast-export, log).
> I also prefer "may" and "yet" because I suspect most external tools
> (e.g. git filter-repo just to name a personal example) won't need to
> read an index format and will thus be unaffected, and any tools that
> do read the index format will probably eventually learn how to work
> with the new format.

I can make the change, but I do want to point out that the current
use of a repository extension _does_ mean that tools that (correctly)
interact with a Git repository should fail even if they don't try to
access the index file. This is only something to make this work until
we introduce a new index file format version and then can drop the
extension.

"git filter-repo" _should_ be safe because it's really just shelling
to Git, right? I'm more concerned about tools like libgit2.

Thanks,
-Stolee

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Mar 9, 2021 at 1:10 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/9/2021 4:03 PM, Elijah Newren wrote:
> > On Tue, Mar 9, 2021 at 12:52 PM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> On 2/24/2021 2:11 PM, Martin Ågren wrote:
> >>> There are several "layers" here, for lack of a better term. "Enabling foo
> >>> enables bar which may cause baz. If you fail due to baz, try dropping
> >>> bar by dropping foo." If I remove any mention of the config variable from
> >>> your text, I get the following.
> >>>
> >>>  Enabling sparse index might cause other tools to stop working with your
> >>>  repository. If you have trouble with this compatibility, then run `git
> >>>  sparse-checkout sparse-index disable` to rewrite your index to not be
> >>>  sparse.
> >>>
> >>> I'm tempted to suggest such a rewrite to relieve readers of knowing of
> >>> the middle step, which you could say is more of an implementation
> >>> detail. But if we think that the symptoms / error messages might involve
> >>> "extensions.sparseIndex" or, as would be the case with an older Git
> >>> installation,
> >>>
> >>>   fatal: unknown repository extensions found:
> >>>           sparseindex
> >>>
> >>> maybe there is some value in mentioning the config item by name. Just
> >>> thinking out loud, really, and I don't have any strong opinion. I only
> >>> came here to point out the typo in the docs.
> >>
> >> I agree that the layers are confusing. We could rearrange and have
> >> a similar flow to what you recommend by mentioning the extension at
> >> the end:
> >>
> >> **WARNING:** Using a sparse index requires modifying the index in a way
> >> that is not completely understood by other tools. If you have trouble with
> >> this compatibility, then run `git sparse-checkout sparse-index disable` to
> >> rewrite your index to not be sparse. Older versions of Git will not
> >> understand the `sparseIndex` repository extension and may fail to interact
> >> with your repository until it is disabled.
> >>
> >> Thanks,
> >> -Stolee
> >
> > This looks pretty good to me, but could we change the first sentence
> > to read "...modifying the index in a way that may not yet be
> > understood by external tools." ?  I'm worried "other tools" might make
> > people worry about different builtin commands (e.g. fast-export, log).
> > I also prefer "may" and "yet" because I suspect most external tools
> > (e.g. git filter-repo just to name a personal example) won't need to
> > read an index format and will thus be unaffected, and any tools that
> > do read the index format will probably eventually learn how to work
> > with the new format.
>
> I can make the change, but I do want to point out that the current
> use of a repository extension _does_ mean that tools that (correctly)
> interact with a Git repository should fail even if they don't try to
> access the index file. This is only something to make this work until
> we introduce a new index file format version and then can drop the
> extension.

Good point, though...

> "git filter-repo" _should_ be safe because it's really just shelling
> to Git, right? I'm more concerned about tools like libgit2.

Yes, libgit2 and jgit and similar tools are clearly going to be
affected and deeply.  Those are of concern, but I suspect most users
when they see "external tools" will be thinking of the large multitude
of scripts out there that just shell out to git under the hood to
provide some higher level wrapper of some sort.  And anything that
operates that way won't be affected directly by the repository
extension.

I'm not sure I'd even mark things that shell out to git as _should_ be
safe.  In general, scripts can make all kinds of assumptions on
interpreting output, and I suspect some of those may become
invalidated by this new feature.  We have a recent guidepost that's
very close to home on that too -- git stash had *3* different bugs in
it once sparse-checkouts were introduced, based on the fact that it
was designed as a just-shell-out-to-low-level-git-commands script and
it made assumptions on how those commands worked together.  See
https://lore.kernel.org/git/ccfedc7140dbf63ba26a15f93bd3885180b26517.1606861519.git.gitgitgadget@gmail.com/.
Sure git-stash is a builtin (supposedly, anyway), but external tools
can make similar logical jumps.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Martin Ågren wrote (reply to this):

On Tue, 9 Mar 2021 at 21:52, Derrick Stolee <stolee@gmail.com> wrote:
>
> I agree that the layers are confusing. We could rearrange and have
> a similar flow to what you recommend by mentioning the extension at
> the end:
>
> **WARNING:** Using a sparse index requires modifying the index in a way
> that is not completely understood by other tools. If you have trouble with
> this compatibility, then run `git sparse-checkout sparse-index disable` to
> rewrite your index to not be sparse. Older versions of Git will not
> understand the `sparseIndex` repository extension and may fail to interact
> with your repository until it is disabled.

I like it. I find this easier to read than the previous version. That
said, is `git sparse-index sparse-checkout disable` really the way to do
this? I don't see a "sparse-index" subcommand of git-sparse-checkout.
... Hmm, no, after building and installing your patches, I get

  $ git sparse-checkout sparse-index disable
  usage: git sparse-checkout (init|list|set|add|reapply|disable) <options>

Should that be `git sparse-checkout init --no-sparse-index`? I just
tried that on a fresh, empty repo. It seems to work in the sense that it
drops the config item. I'm guessing re-initing a sparse checkout is a
safe and sane thing to do?

I don't find any tests for this. If re-initing should be ok and in
particular if it should allow toggling the use of sparse index, it might
be good having a test. At a minimum to see that the command passes and
that the config item goes away? And check that the actual index is
rewritten back to the "old" format? (Sorry if you have that already and
I'm just bad at finding it.)

Martin

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 3/14/2021 4:08 PM, Martin Ågren wrote:
> On Tue, 9 Mar 2021 at 21:52, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> I agree that the layers are confusing. We could rearrange and have
>> a similar flow to what you recommend by mentioning the extension at
>> the end:
>>
>> **WARNING:** Using a sparse index requires modifying the index in a way
>> that is not completely understood by other tools. If you have trouble with
>> this compatibility, then run `git sparse-checkout sparse-index disable` to
>> rewrite your index to not be sparse. Older versions of Git will not
>> understand the `sparseIndex` repository extension and may fail to interact
>> with your repository until it is disabled.
> 
> I like it. I find this easier to read than the previous version. That
> said, is `git sparse-index sparse-checkout disable` really the way to do
> this? I don't see a "sparse-index" subcommand of git-sparse-checkout.
> ... Hmm, no, after building and installing your patches, I get
> 
>   $ git sparse-checkout sparse-index disable
>   usage: git sparse-checkout (init|list|set|add|reapply|disable) <options>
> 
> Should that be `git sparse-checkout init --no-sparse-index`? I just
> tried that on a fresh, empty repo. It seems to work in the sense that it
> drops the config item. I'm guessing re-initing a sparse checkout is a
> safe and sane thing to do?

Yes! Sorry I missed updating this instance when changing the
design. Your suggestion is indeed the proper way to disable the
sparse-index.
 
> I don't find any tests for this. If re-initing should be ok and in
> particular if it should allow toggling the use of sparse index, it might
> be good having a test. At a minimum to see that the command passes and
> that the config item goes away? And check that the actual index is
> rewritten back to the "old" format? (Sorry if you have that already and
> I'm just bad at finding it.)

We have tests already that 'git sparse-checkout init' will preserve
existing sparse-checkout patterns.

I should definitely have a test to ensure that '--no-sparse-index'
rewrites the index to be a full one. Thanks!

-Stolee

also set, allowing for better performance with a limited set of
patterns (see 'CONE PATTERN SET' below).
+
Use the `--[no-]sparse-index` option to toggle the use of the sparse
index format. This reduces the size of the index to be more closely
aligned with your sparse-checkout definition. This can have significant
performance advantages for commands such as `git status` or `git add`.
This feature is still experimental. Some commands might be slower with
a sparse index until they are properly integrated with the feature.
+
**WARNING:** Using a sparse index requires modifying the index in a way
that is not completely understood by external tools. If you have trouble
with this compatibility, then run `git sparse-checkout init --no-sparse-index`
to rewrite your index to not be sparse. Older versions of Git will not
understand the sparse directory entries index extension and may fail to
interact with your repository until it is disabled.

'set'::
Write a set of patterns to the sparse-checkout file, as given as
Expand Down
19 changes: 19 additions & 0 deletions Documentation/technical/index-format.txt
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ Git index format
localization, no special casing of directory separator '/'). Entries

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> This begins a long effort to update the index format to allow sparse
> directory entries. This should result in a significant improvement to
> Git commands when HEAD contains millions of files, but the user has
> selected many fewer files to keep in their sparse-checkout definition.
>
> Currently, the index format is only updated in the presence of
> extensions.sparseIndex instead of increasing a file format version
> number. This is temporary, and index v5 is part of the plan for future
> work in this area.
>
> The design document details many of the reasons for embarking on this
> work, and also the plan for completing it safely.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/technical/index-format.txt |   7 +
>  Documentation/technical/sparse-index.txt | 167 +++++++++++++++++++++++
>  2 files changed, 174 insertions(+)
>  create mode 100644 Documentation/technical/sparse-index.txt
>
> diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
> index b633482b1bdf..387126582556 100644
> --- a/Documentation/technical/index-format.txt
> +++ b/Documentation/technical/index-format.txt
> @@ -44,6 +44,13 @@ Git index format
>    localization, no special casing of directory separator '/'). Entries
>    with the same name are sorted by their stage field.
>
> +  An index entry typically represents a file. However, if sparse-checkout
> +  is enabled in cone mode (`core.sparseCheckoutCone` is enabled) and the
> +  `extensions.sparseIndex` extension is enabled, then the index may
> +  contain entries for directories outside of the sparse-checkout definition.
> +  These entries have mode `0040000`, include the `SKIP_WORKTREE` bit, and
> +  the path ends in a directory separator.
> +
>    32-bit ctime seconds, the last time a file's metadata changed
>      this is stat(2) data
>
> diff --git a/Documentation/technical/sparse-index.txt b/Documentation/technical/sparse-index.txt
> new file mode 100644
> index 000000000000..9070836f0655
> --- /dev/null
> +++ b/Documentation/technical/sparse-index.txt
> @@ -0,0 +1,167 @@
> +Git Sparse-Index Design Document
> +================================
> +
> +The sparse-checkout feature allows users to focus a working directory on
> +a subset of the files at HEAD. The cone mode patterns, enabled by
> +`core.sparseCheckoutCone`, allow for very fast pattern matching to
> +discover which files at HEAD belong in the sparse-checkout cone.
> +
> +Three important scale dimensions for a Git worktree are:
> +
> +* `HEAD`: How many files are present at `HEAD`?
> +
> +* Populated: How many files are within the sparse-checkout cone.
> +
> +* Modified: How many files has the user modified in the working directory?
> +
> +We will use big-O notation -- O(X) -- to denote how expensive certain
> +operations are in terms of these dimensions.
> +
> +These dimensions are ordered by their magnitude: users (typically) modify
> +fewer files than are populated, and we can only populate files at `HEAD`.
> +These dimensions are also ordered by how expensive they are per item: it
> +is expensive to detect a modified file than it is to write one that we
> +know must be populated; changing `HEAD` only really requires updating the
> +index.
> +
> +Problems occur if there is an extreme imbalance in these dimensions. For
> +example, if `HEAD` contains millions of paths but the populated set has
> +only tens of thousands, then commands like `git status` and `git add` can
> +be dominated by operations that require O(`HEAD`) operations instead of
> +O(Populated). Primarily, the cost is in parsing and rewriting the index,
> +which is filled primarily with files at `HEAD` that are marked with the
> +`SKIP_WORKTREE` bit.
> +
> +The sparse-index intends to take these commands that read and modify the
> +index from O(`HEAD`) to O(Populated). To do this, we need to modify the
> +index format in a significant way: add "sparse directory" entries.
> +
> +With cone mode patterns, it is possible to detect when an entire
> +directory will have its contents outside of the sparse-checkout definition.
> +Instead of listing all of the files it contains as individual entries, a
> +sparse-index contains an entry with the directory name, referencing the
> +object ID of the tree at `HEAD` and marked with the `SKIP_WORKTREE` bit.
> +If we need to discover the details for paths within that directory, we
> +can parse trees to find that list.
> +
> +This addition of sparse-directory entries violates expectations about the

Violates current expectations, yes.  Documentation tends to live a
long time, and I suspect that 2-3 years from now reading this sentence
might be jarring since we'll have modified the code to have an updated
set of expectations.  Maybe a simple "As of time of writing, ..." at
the beginning of the sentence here?  Or maybe I'm just being overly
worried...

> +index format and its in-memory data structure. There are many consumers in
> +the codebase that expect to iterate through all of the index entries and
> +see only files. In addition, they expect to see all files at `HEAD`. One
> +way to handle this is to parse trees to replace a sparse-directory entry
> +with all of the files within that tree as the index is loaded. However,
> +parsing trees is slower than parsing the index format, so that is a slower
> +operation than if we left the index alone.
> +
> +The implementation plan below follows four phases to slowly integrate with
> +the sparse-index. The intention is to incrementally update Git commands to
> +interact safely with the sparse-index without significant slowdowns. This
> +may not always be possible, but the hope is that the primary commands that
> +users need in their daily work are dramatically improved.
> +
> +Phase I: Format and initial speedups
> +------------------------------------
> +
> +During this phase, Git learns to enable the sparse-index and safely parse
> +one. Protections are put in place so that every consumer of the in-memory
> +data structure can operate with its current assumption of every file at
> +`HEAD`.
> +
> +At first, every index parse will expand the sparse-directory entries into
> +the full list of paths at `HEAD`. This will be slower in all cases. The
> +only noticable change in behavior will be that the serialized index file

noticeable

> +contains sparse-directory entries.
> +
> +To start, we use a new repository extension, `extensions.sparseIndex`, to
> +allow inserting sparse-directory entries into indexes with file format
> +versions 2, 3, and 4. This prevents Git versions that do not understand
> +the sparse-index from operating on one, but it also prevents other
> +operations that do not use the index at all. A new format, index v5, will
> +be introduced that includes sparse-directory entries by default. It might
> +also introduce other features that have been considered for improving the
> +index, as well.
> +
> +Next, consumers of the index will be guarded against operating on a
> +sparse-index by inserting calls to `ensure_full_index()` or
> +`expand_index_to_path()`. After these guards are in place, we can begin
> +leaving sparse-directory entries in the in-memory index structure.
> +
> +Even after inserting these guards, we will keep expanding sparse-indexes
> +for most Git commands using the `command_requires_full_index` repository
> +setting. This setting will be on by default and disabled one builtin at a
> +time until we have sufficient confidence that all of the index operations
> +are properly guarded.
> +
> +To complete this phase, the commands `git status` and `git add` will be
> +integrated with the sparse-index so that they operate with O(Populated)
> +performance. They will be carefully tested for operations within and
> +outside the sparse-checkout definition.

Good plan so far, but there's something else bugging me a little here.
One thing we noticed with our usage of `sparse-checkout` is that
although unimportant _tracked_ files go away, leftover build files and
other untracked files stick around.  So, although 'git status'
shouldn't have to check the tracked files anymore, it is still going
to have to look at each of the *untracked* files and compare to
.gitignore files in order to correctly classify each file as ignored
or just plain untracked.  Our `sparsify` tool has for a long time
tried to warn about such files when changing the sparsity
patterns/modules and had an --remove-old-ignores option for clearing
out ignored files that are found within directories that are sparse
(Meaning the directories where all files under them are marked
SKIP_WORKTREE.). I was never sure whether a warning was enough, or if
pushing that option more made sense, but about a month ago my
colleagues made the tool just auto-invoke that option from other
sparsify invocations.  To my knowledge, there have been no complaints
about that being automatically turned on; but there were
complaints/confusion before about the directories being left around.
(Of course, non-ignored files are still left around by that option.)

I'm worried that since sparse-checkout doesn't do anything to help
with all these untracked/ignored files, we might not get all the
performance improvements we want from a `git status` with sparse
directories.  We'll be dropping from walking O(2*HEAD) files (1 source
+ 1 object file) down to O(HEAD) files (just the object files) rather
than actually getting down to O(Populated).

> +
> +Phase II: Careful integrations
> +------------------------------
> +
> +This phase focuses on ensuring that all index extensions and APIs work
> +well with a sparse-index. This requires significant increases to our test
> +coverage, especially for operations that interact with the working
> +directory outside of the sparse-checkout definition. Some of these
> +behaviors may not be the desirable ones, such as some tests already
> +marked for failure in `t1092-sparse-checkout-compatibility.sh`.
> +
> +The index extensions that may require special integrations are:
> +
> +* FS Monitor
> +* Untracked cache
> +
> +While integrating with these features, we should look for patterns that
> +might lead to better APIs for interacting with the index. Coalescing
> +common usage patterns into an API call can reduce the number of places
> +where sparse-directories need to be handled carefully.

Makes sense.

> +Phase III: Important command speedups
> +-------------------------------------
> +
> +At this point, the patterns for testing and implementing sparse-directory
> +logic should be relatively stable. This phase focuses on updating some of
> +the most common builtins that use the index to operate as O(Populated).
> +Here is a potential list of commands that could be valuable to integrate
> +at this point:
> +
> +* `git commit`
> +* `git checkout`
> +* `git merge`
> +* `git rebase`
> +
> +Along with `git status` and `git add`, these commands cover the majority
> +of users' interactions with the working directory.

Sounds like a good plan as well.

I hope we get to make this specific to the merge-ort backend.  It
localizes the index-related code to (a) a call to unpack_trees()
called from checkout-like code (which would probably automatically be
handled by your updates to git checkout), and (b) a single function
named record_conflicted_index_entries().  I feel it should be pretty
easy to update.

In contrast, the idea of attempting to update merge-recursive with
this kind of change sounds overwhelming.

>  In addition, we can
> +integrate with these commands:
> +
> +* `git grep`
> +* `git rm`
> +
> +These have been proposed as some whose behavior could change when in a
> +repo with a sparse-checkout definition. It would be good to include this
> +behavior automatically when using a sparse-index. Some clarity is needed
> +to make the behavior switch clear to the user.

Is this leftover from before recent events?  I think this portion of
the document should just be stricken.

I argued about how these were buggy as-is due SKIP_WORKTREE always
having been an incomplete implementation of an idea at [1], but didn't
hear a further response from you.  I'm curious if you disagreed with
my reasoning, or it just slipped through the cracks in a busy schedule
and this portion of the document was leftover from before.  In my
opinion, both commands are just buggy and should be fixed for general
sparse-checkout usage cases, not just for sparse-index.

As for git grep, it has options for searching the working tree
(default) OR searching the index (--cached) OR searching an old commit
(passing a REVISION).  But never some combination or more than one of
these.  The fact that it combined some in the cases of SKIP_WORKTREE
entries looks entirely like a bug to me.  For the same reasons I
argued that --untracked and --cached are incompatible[2], we shouldn't
be combining results from searching the working tree and searching the
index.  Luckily, this fix has already been submitted[3] and picked up
in mt/grep-sparse-checkout and is marked in the cooking emails as
"Will merge to next".

As for git rm, I'll quote from my email to Matheus:

"""As far as the longer term discussion about making git rm configurable...
_If_ it comes up again in the future, I will argue that if git rm
should have configuration to delete paths outside the sparsity
specification, then git add should have configuration to add paths
outside the sparsity specification that happen to be present despite
being SKIP_WORKTREE, that git diff with no revision arguments (nor
--cached) should have configuration to diff against paths that are
SKIP_WORKTREE but happen to be present, that git status should have
configuration to report on changes to paths that are SKIP_WORKTREE but
happen to be present, that git checkout should have configuration to
write files to the working tree despite matching sparsity paths, etc.
And I'll argue that you do ALL of those or you're being inconsistent.
I hope that people see these are actually all the same request and
that it is horribly inconsistent to do some of these and not others,
and that at least by the time I get to mentioning checkout that they
realize it's a crazy request.  We should just tell users to extend
their sparsity if they want the working copy (and commands that
interact with the working copy) to handle the additional paths.  Maybe
I'm just really biased, but I don't see how this makes sense.  I would
argue more about it, but no one has responded.  My plan was to just
fix the default behavior, and then see if anyone ever actually cared
enough to come back and ask for more configurability."""

Also, for rm, Matheus has already submitted the fix[4], though at
Junio's request he separated out some fixes for git-add as a separate
preliminary series[5] and then will resubmit the other `add` and `rm`
fixes.

[1] https://lore.kernel.org/git/CABPp-BHwNoVnooqDFPAsZxBT9aR5Dwk5D9sDRCvYSb8akxAJgA@mail.gmail.com/
[2] https://lore.kernel.org/git/xmqqtuql0yfp.fsf@gitster.c.googlers.com/
[3] https://lore.kernel.org/git/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br/
[4] https://lore.kernel.org/git/61a77cd5f45ba02c7dff4b7932abdebb17c1667f.1613593946.git.matheus.bernardino@usp.br/
[5] https://lore.kernel.org/git/cover.1614037664.git.matheus.bernardino@usp.br/

Anyway, that's a long way of saying I think this section of your
document is already obsolete.  (Which is a good thing -- less work to
do to get sparse-index working.  Thanks, Matheus!).

> +This phase is the first where parallel work might be possible without too
> +much conflicts between topics.
> +
> +Phase IV: The long tail
> +-----------------------
> +
> +This last phase is less a "phase" and more "the new normal" after all of
> +the previous work.
> +
> +To start, the `command_requires_full_index` option could be removed in
> +favor of expanding only when hitting an API guard.
> +
> +There are many Git commands that could use special attention to operate as
> +O(Populated), while some might be so rare that it is acceptable to leave
> +them with additional overhead when a sparse-index is present.
> +
> +Here are some commands that might be useful to update:
> +
> +* `git sparse-checkout set`
> +* `git am`
> +* `git clean`
> +* `git stash`

Oh, man, git stash is definitely in need of work.  It's still a
minimalistic transliteration of shell to C, complete with lots of
process forking and piping output between various low-level commands.
It might be interesting to rewrite this in terms of the merge
machinery, though its separate stashing of staged stuff, unstaged
stuff, and possibly untracked stuff means that there is a sequence of
two or three merges needed and interesting failure handling to do if
those merges fail, especially if the user uses --index.  But I
digress...


Anyway, overall, very nicely written and planned out.  Thanks for
taking the time to write this all up.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/23/2021 8:13 PM, Elijah Newren wrote:
> On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:>> +This addition of sparse-directory entries violates expectations about the
> 
> Violates current expectations, yes.  Documentation tends to live a
> long time, and I suspect that 2-3 years from now reading this sentence
> might be jarring since we'll have modified the code to have an updated
> set of expectations.  Maybe a simple "As of time of writing, ..." at
> the beginning of the sentence here?  Or maybe I'm just being overly
> worried...

I was hoping that the phrase "this addition of" places this statement in
a moment of time where sparse-directory entries didn't exist, but now they
will. I'm open to alternatives and will give this some thought.

>> +To complete this phase, the commands `git status` and `git add` will be
>> +integrated with the sparse-index so that they operate with O(Populated)
>> +performance. They will be carefully tested for operations within and
>> +outside the sparse-checkout definition.
> 
> Good plan so far, but there's something else bugging me a little here.
> One thing we noticed with our usage of `sparse-checkout` is that
> although unimportant _tracked_ files go away, leftover build files and
> other untracked files stick around.  So, although 'git status'
> shouldn't have to check the tracked files anymore, it is still going
> to have to look at each of the *untracked* files and compare to
> .gitignore files in order to correctly classify each file as ignored
> or just plain untracked.  Our `sparsify` tool has for a long time
> tried to warn about such files when changing the sparsity
> patterns/modules and had an --remove-old-ignores option for clearing
> out ignored files that are found within directories that are sparse
> (Meaning the directories where all files under them are marked
> SKIP_WORKTREE.). I was never sure whether a warning was enough, or if
> pushing that option more made sense, but about a month ago my
> colleagues made the tool just auto-invoke that option from other
> sparsify invocations.  To my knowledge, there have been no complaints
> about that being automatically turned on; but there were
> complaints/confusion before about the directories being left around.
> (Of course, non-ignored files are still left around by that option.)
> 
> I'm worried that since sparse-checkout doesn't do anything to help
> with all these untracked/ignored files, we might not get all the
> performance improvements we want from a `git status` with sparse
> directories.  We'll be dropping from walking O(2*HEAD) files (1 source
> + 1 object file) down to O(HEAD) files (just the object files) rather
> than actually getting down to O(Populated).

This definitely seems like a valuable _enhancement_ to sparse-checkout
that could occur in parallel.

For a workaround in the moment: is "git clean -xdf" sufficient to help
these users?

>> +Phase III: Important command speedups
>> +-------------------------------------
>> +
>> +At this point, the patterns for testing and implementing sparse-directory
>> +logic should be relatively stable. This phase focuses on updating some of
>> +the most common builtins that use the index to operate as O(Populated).
>> +Here is a potential list of commands that could be valuable to integrate
>> +at this point:
>> +
>> +* `git commit`
>> +* `git checkout`
>> +* `git merge`
>> +* `git rebase`
>> +
>> +Along with `git status` and `git add`, these commands cover the majority
>> +of users' interactions with the working directory.
> 
> Sounds like a good plan as well.
> 
> I hope we get to make this specific to the merge-ort backend.  It
> localizes the index-related code to (a) a call to unpack_trees()
> called from checkout-like code (which would probably automatically be
> handled by your updates to git checkout), and (b) a single function
> named record_conflicted_index_entries().  I feel it should be pretty
> easy to update.
> 
> In contrast, the idea of attempting to update merge-recursive with
> this kind of change sounds overwhelming.

Yes, I'm hoping to eventually say "if you are in a sparse-checkout, then
you should use ORT by default." Then, if someone opts to do merge-recursive
instead, then they pay the index expansion cost.

While this is very clear in my head, it might be worth mentioning that
explicitly here.

>>  In addition, we can
>> +integrate with these commands:
>> +
>> +* `git grep`
>> +* `git rm`
>> +
>> +These have been proposed as some whose behavior could change when in a
>> +repo with a sparse-checkout definition. It would be good to include this
>> +behavior automatically when using a sparse-index. Some clarity is needed
>> +to make the behavior switch clear to the user.
> 
> Is this leftover from before recent events?  I think this portion of
> the document should just be stricken.
> 
> I argued about how these were buggy as-is due SKIP_WORKTREE always
> having been an incomplete implementation of an idea at [1], but didn't
> hear a further response from you.  I'm curious if you disagreed with
> my reasoning, or it just slipped through the cracks in a busy schedule
> and this portion of the document was leftover from before.  In my
> opinion, both commands are just buggy and should be fixed for general
> sparse-checkout usage cases, not just for sparse-index.

This is definitely a case of "I've been too busy to read those topics
in detail." I figured that there was something going on that was relevant
to the sparse-checkout and would affect the sparse-index, but I shelved
it in my mind until I had space to think about it directly.

> Anyway, that's a long way of saying I think this section of your
> document is already obsolete.  (Which is a good thing -- less work to
> do to get sparse-index working.  Thanks, Matheus!).

Thank you for your summary, which helps a lot. Thanks, Matheus, too!
If those fixes already include coverage for the behavior, then I'll see
if they also translate to sparse-index tests easily.

I feel like a lot of these later contributions will be more about adding
tests than actually writing a lot of code.

>> +This phase is the first where parallel work might be possible without too
>> +much conflicts between topics.
>> +
>> +Phase IV: The long tail
>> +-----------------------
>> +
>> +This last phase is less a "phase" and more "the new normal" after all of
>> +the previous work.
>> +
>> +To start, the `command_requires_full_index` option could be removed in
>> +favor of expanding only when hitting an API guard.
>> +
>> +There are many Git commands that could use special attention to operate as
>> +O(Populated), while some might be so rare that it is acceptable to leave
>> +them with additional overhead when a sparse-index is present.
>> +
>> +Here are some commands that might be useful to update:
>> +
>> +* `git sparse-checkout set`
>> +* `git am`
>> +* `git clean`
>> +* `git stash`
> 
> Oh, man, git stash is definitely in need of work.  It's still a
> minimalistic transliteration of shell to C, complete with lots of
> process forking and piping output between various low-level commands.
> It might be interesting to rewrite this in terms of the merge
> machinery, though its separate stashing of staged stuff, unstaged
> stuff, and possibly untracked stuff means that there is a sequence of
> two or three merges needed and interesting failure handling to do if
> those merges fail, especially if the user uses --index.  But I
> digress...

I would prefer to leave 'git stash' alone, but it's used by enough
people that I need to care about it eventually.

> Anyway, overall, very nicely written and planned out.  Thanks for
> taking the time to write this all up.

Thanks for your detailed comments!
-Stolee
 

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Thu, Feb 25, 2021 at 7:29 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 2/23/2021 8:13 PM, Elijah Newren wrote:
> > On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:>> +This addition of sparse-directory entries violates expectations about the
> >
> > Violates current expectations, yes.  Documentation tends to live a
> > long time, and I suspect that 2-3 years from now reading this sentence
> > might be jarring since we'll have modified the code to have an updated
> > set of expectations.  Maybe a simple "As of time of writing, ..." at
> > the beginning of the sentence here?  Or maybe I'm just being overly
> > worried...
>
> I was hoping that the phrase "this addition of" places this statement in
> a moment of time where sparse-directory entries didn't exist, but now they
> will. I'm open to alternatives and will give this some thought.

I already listed my only suggestion -- adding a "As of time of
writing," at the beginning.  I'm totally open to other
proposals/suggestions, and it's admittedly a minor point so you can
feel free to just ignore it if we can't come up with wording everyone
likes.

>
> >> +To complete this phase, the commands `git status` and `git add` will be
> >> +integrated with the sparse-index so that they operate with O(Populated)
> >> +performance. They will be carefully tested for operations within and
> >> +outside the sparse-checkout definition.
> >
> > Good plan so far, but there's something else bugging me a little here.
> > One thing we noticed with our usage of `sparse-checkout` is that
> > although unimportant _tracked_ files go away, leftover build files and
> > other untracked files stick around.  So, although 'git status'
> > shouldn't have to check the tracked files anymore, it is still going
> > to have to look at each of the *untracked* files and compare to
> > .gitignore files in order to correctly classify each file as ignored
> > or just plain untracked.  Our `sparsify` tool has for a long time
> > tried to warn about such files when changing the sparsity
> > patterns/modules and had an --remove-old-ignores option for clearing
> > out ignored files that are found within directories that are sparse
> > (Meaning the directories where all files under them are marked
> > SKIP_WORKTREE.). I was never sure whether a warning was enough, or if
> > pushing that option more made sense, but about a month ago my
> > colleagues made the tool just auto-invoke that option from other
> > sparsify invocations.  To my knowledge, there have been no complaints
> > about that being automatically turned on; but there were
> > complaints/confusion before about the directories being left around.
> > (Of course, non-ignored files are still left around by that option.)
> >
> > I'm worried that since sparse-checkout doesn't do anything to help
> > with all these untracked/ignored files, we might not get all the
> > performance improvements we want from a `git status` with sparse
> > directories.  We'll be dropping from walking O(2*HEAD) files (1 source
> > + 1 object file) down to O(HEAD) files (just the object files) rather
> > than actually getting down to O(Populated).
>
> This definitely seems like a valuable _enhancement_ to sparse-checkout
> that could occur in parallel.

Yes, indeed.  Your discussion of performance just reminded me of it,
and since this idea might be important in order to drive the costs
down to O(populated) in practice, I thought I'd mention it.

> For a workaround in the moment: is "git clean -xdf" sufficient to help
> these users?

Not really; that wouldn't remove the ignored stuff (build files) under
sparsified directories which is the point.  (Builds build everything
over here; once you sparsify you have leftover build files from
projects you now don't care about.)  If you convert it to "git clean
-Xdf" then you're closer, but that wouldn't just remove builds info
from sparse projects, it'd force users to rebuild all the stuff
they're interested in.

It's close though; what's wanted is basically a special flag that runs
"git clean -Xf <long list of sparsified directories>", without the
user having to specify 300 directories.

However, for now, since I've got a 'sparsify' script anyway (needed
for determining inter-module dependencies and certain directories that
always need to be present, etc.), it just has a flag for running "git
clean -Xf <long list of sparsified directories>" since it has logic to
compute what all those directories are anyway.

> >> +Phase III: Important command speedups
> >> +-------------------------------------
> >> +
> >> +At this point, the patterns for testing and implementing sparse-directory
> >> +logic should be relatively stable. This phase focuses on updating some of
> >> +the most common builtins that use the index to operate as O(Populated).
> >> +Here is a potential list of commands that could be valuable to integrate
> >> +at this point:
> >> +
> >> +* `git commit`
> >> +* `git checkout`
> >> +* `git merge`
> >> +* `git rebase`
> >> +
> >> +Along with `git status` and `git add`, these commands cover the majority
> >> +of users' interactions with the working directory.
> >
> > Sounds like a good plan as well.
> >
> > I hope we get to make this specific to the merge-ort backend.  It
> > localizes the index-related code to (a) a call to unpack_trees()
> > called from checkout-like code (which would probably automatically be
> > handled by your updates to git checkout), and (b) a single function
> > named record_conflicted_index_entries().  I feel it should be pretty
> > easy to update.
> >
> > In contrast, the idea of attempting to update merge-recursive with
> > this kind of change sounds overwhelming.
>
> Yes, I'm hoping to eventually say "if you are in a sparse-checkout, then
> you should use ORT by default." Then, if someone opts to do merge-recursive
> instead, then they pay the index expansion cost.
>
> While this is very clear in my head, it might be worth mentioning that
> explicitly here.

:+1:

> >>  In addition, we can
> >> +integrate with these commands:
> >> +
> >> +* `git grep`
> >> +* `git rm`
> >> +
> >> +These have been proposed as some whose behavior could change when in a
> >> +repo with a sparse-checkout definition. It would be good to include this
> >> +behavior automatically when using a sparse-index. Some clarity is needed
> >> +to make the behavior switch clear to the user.
> >
> > Is this leftover from before recent events?  I think this portion of
> > the document should just be stricken.
> >
> > I argued about how these were buggy as-is due SKIP_WORKTREE always
> > having been an incomplete implementation of an idea at [1], but didn't
> > hear a further response from you.  I'm curious if you disagreed with
> > my reasoning, or it just slipped through the cracks in a busy schedule
> > and this portion of the document was leftover from before.  In my
> > opinion, both commands are just buggy and should be fixed for general
> > sparse-checkout usage cases, not just for sparse-index.
>
> This is definitely a case of "I've been too busy to read those topics
> in detail." I figured that there was something going on that was relevant
> to the sparse-checkout and would affect the sparse-index, but I shelved
> it in my mind until I had space to think about it directly.
>
> > Anyway, that's a long way of saying I think this section of your
> > document is already obsolete.  (Which is a good thing -- less work to
> > do to get sparse-index working.  Thanks, Matheus!).
>
> Thank you for your summary, which helps a lot. Thanks, Matheus, too!
> If those fixes already include coverage for the behavior, then I'll see
> if they also translate to sparse-index tests easily.
>
> I feel like a lot of these later contributions will be more about adding
> tests than actually writing a lot of code.
>
> >> +This phase is the first where parallel work might be possible without too
> >> +much conflicts between topics.
> >> +
> >> +Phase IV: The long tail
> >> +-----------------------
> >> +
> >> +This last phase is less a "phase" and more "the new normal" after all of
> >> +the previous work.
> >> +
> >> +To start, the `command_requires_full_index` option could be removed in
> >> +favor of expanding only when hitting an API guard.
> >> +
> >> +There are many Git commands that could use special attention to operate as
> >> +O(Populated), while some might be so rare that it is acceptable to leave
> >> +them with additional overhead when a sparse-index is present.
> >> +
> >> +Here are some commands that might be useful to update:
> >> +
> >> +* `git sparse-checkout set`
> >> +* `git am`
> >> +* `git clean`
> >> +* `git stash`
> >
> > Oh, man, git stash is definitely in need of work.  It's still a
> > minimalistic transliteration of shell to C, complete with lots of
> > process forking and piping output between various low-level commands.
> > It might be interesting to rewrite this in terms of the merge
> > machinery, though its separate stashing of staged stuff, unstaged
> > stuff, and possibly untracked stuff means that there is a sequence of
> > two or three merges needed and interesting failure handling to do if
> > those merges fail, especially if the user uses --index.  But I
> > digress...
>
> I would prefer to leave 'git stash' alone, but it's used by enough
> people that I need to care about it eventually.

Oh, it can definitely come later.  And I agree about the desirability
of touching that code; I was avoiding it for a long time, but there
was one important sparse-checkout-related bug recently[1] so I've
already been forced to touch it once.  That might mean I'm
(eventually) on the hook to make it sparse-index friendly, especially
since it might involve using merge-ort to do so...

[1] https://lore.kernel.org/git/pull.919.git.git.1605891222.gitgitgadget@gmail.com/

> > Anyway, overall, very nicely written and planned out.  Thanks for
> > taking the time to write this all up.
>
> Thanks for your detailed comments!
> -Stolee

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Mar 10, 2021 at 11:31 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> This begins a long effort to update the index format to allow sparse
> directory entries. This should result in a significant improvement to
> Git commands when HEAD contains millions of files, but the user has
> selected many fewer files to keep in their sparse-checkout definition.
>
> Currently, the index format is only updated in the presence of
> extensions.sparseIndex instead of increasing a file format version
> number. This is temporary, and index v5 is part of the plan for future
> work in this area.
>
> The design document details many of the reasons for embarking on this
> work, and also the plan for completing it safely.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/technical/index-format.txt |   7 +
>  Documentation/technical/sparse-index.txt | 173 +++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
>  create mode 100644 Documentation/technical/sparse-index.txt
>
> diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
> index b633482b1bdf..387126582556 100644
> --- a/Documentation/technical/index-format.txt
> +++ b/Documentation/technical/index-format.txt
> @@ -44,6 +44,13 @@ Git index format
>    localization, no special casing of directory separator '/'). Entries
>    with the same name are sorted by their stage field.
>
> +  An index entry typically represents a file. However, if sparse-checkout
> +  is enabled in cone mode (`core.sparseCheckoutCone` is enabled) and the
> +  `extensions.sparseIndex` extension is enabled, then the index may
> +  contain entries for directories outside of the sparse-checkout definition.
> +  These entries have mode `0040000`, include the `SKIP_WORKTREE` bit, and
> +  the path ends in a directory separator.
> +
>    32-bit ctime seconds, the last time a file's metadata changed
>      this is stat(2) data
>
> diff --git a/Documentation/technical/sparse-index.txt b/Documentation/technical/sparse-index.txt
> new file mode 100644
> index 000000000000..787a2a0b3b81
> --- /dev/null
> +++ b/Documentation/technical/sparse-index.txt
> @@ -0,0 +1,173 @@
> +Git Sparse-Index Design Document
> +================================
> +
> +The sparse-checkout feature allows users to focus a working directory on
> +a subset of the files at HEAD. The cone mode patterns, enabled by
> +`core.sparseCheckoutCone`, allow for very fast pattern matching to
> +discover which files at HEAD belong in the sparse-checkout cone.
> +
> +Three important scale dimensions for a Git worktree are:
> +
> +* `HEAD`: How many files are present at `HEAD`?
> +
> +* Populated: How many files are within the sparse-checkout cone.
> +
> +* Modified: How many files has the user modified in the working directory?
> +
> +We will use big-O notation -- O(X) -- to denote how expensive certain
> +operations are in terms of these dimensions.
> +
> +These dimensions are ordered by their magnitude: users (typically) modify
> +fewer files than are populated, and we can only populate files at `HEAD`.
> +These dimensions are also ordered by how expensive they are per item: it
> +is expensive to detect a modified file than it is to write one that we
> +know must be populated; changing `HEAD` only really requires updating the
> +index.
> +
> +Problems occur if there is an extreme imbalance in these dimensions. For
> +example, if `HEAD` contains millions of paths but the populated set has
> +only tens of thousands, then commands like `git status` and `git add` can
> +be dominated by operations that require O(`HEAD`) operations instead of
> +O(Populated). Primarily, the cost is in parsing and rewriting the index,
> +which is filled primarily with files at `HEAD` that are marked with the
> +`SKIP_WORKTREE` bit.
> +
> +The sparse-index intends to take these commands that read and modify the
> +index from O(`HEAD`) to O(Populated). To do this, we need to modify the
> +index format in a significant way: add "sparse directory" entries.
> +
> +With cone mode patterns, it is possible to detect when an entire
> +directory will have its contents outside of the sparse-checkout definition.
> +Instead of listing all of the files it contains as individual entries, a
> +sparse-index contains an entry with the directory name, referencing the
> +object ID of the tree at `HEAD` and marked with the `SKIP_WORKTREE` bit.
> +If we need to discover the details for paths within that directory, we
> +can parse trees to find that list.
> +
> +At time of writing, sparse-directory entries violate expectations about the
> +index format and its in-memory data structure. There are many consumers in
> +the codebase that expect to iterate through all of the index entries and
> +see only files. In addition, they expect to see all files at `HEAD`. One
> +way to handle this is to parse trees to replace a sparse-directory entry
> +with all of the files within that tree as the index is loaded. However,
> +parsing trees is slower than parsing the index format, so that is a slower
> +operation than if we left the index alone.
> +
> +The implementation plan below follows four phases to slowly integrate with
> +the sparse-index. The intention is to incrementally update Git commands to
> +interact safely with the sparse-index without significant slowdowns. This
> +may not always be possible, but the hope is that the primary commands that
> +users need in their daily work are dramatically improved.
> +
> +Phase I: Format and initial speedups
> +------------------------------------
> +
> +During this phase, Git learns to enable the sparse-index and safely parse
> +one. Protections are put in place so that every consumer of the in-memory
> +data structure can operate with its current assumption of every file at
> +`HEAD`.
> +
> +At first, every index parse will expand the sparse-directory entries into
> +the full list of paths at `HEAD`. This will be slower in all cases. The
> +only noticable change in behavior will be that the serialized index file
> +contains sparse-directory entries.
> +
> +To start, we use a new repository extension, `extensions.sparseIndex`, to
> +allow inserting sparse-directory entries into indexes with file format
> +versions 2, 3, and 4. This prevents Git versions that do not understand
> +the sparse-index from operating on one, but it also prevents other
> +operations that do not use the index at all. A new format, index v5, will
> +be introduced that includes sparse-directory entries by default. It might
> +also introduce other features that have been considered for improving the
> +index, as well.
> +
> +Next, consumers of the index will be guarded against operating on a
> +sparse-index by inserting calls to `ensure_full_index()` or
> +`expand_index_to_path()`. After these guards are in place, we can begin
> +leaving sparse-directory entries in the in-memory index structure.
> +
> +Even after inserting these guards, we will keep expanding sparse-indexes
> +for most Git commands using the `command_requires_full_index` repository
> +setting. This setting will be on by default and disabled one builtin at a
> +time until we have sufficient confidence that all of the index operations
> +are properly guarded.
> +
> +To complete this phase, the commands `git status` and `git add` will be
> +integrated with the sparse-index so that they operate with O(Populated)
> +performance. They will be carefully tested for operations within and
> +outside the sparse-checkout definition.
> +
> +Phase II: Careful integrations
> +------------------------------
> +
> +This phase focuses on ensuring that all index extensions and APIs work
> +well with a sparse-index. This requires significant increases to our test
> +coverage, especially for operations that interact with the working
> +directory outside of the sparse-checkout definition. Some of these
> +behaviors may not be the desirable ones, such as some tests already
> +marked for failure in `t1092-sparse-checkout-compatibility.sh`.
> +
> +The index extensions that may require special integrations are:
> +
> +* FS Monitor
> +* Untracked cache
> +
> +While integrating with these features, we should look for patterns that
> +might lead to better APIs for interacting with the index. Coalescing
> +common usage patterns into an API call can reduce the number of places
> +where sparse-directories need to be handled carefully.
> +
> +Phase III: Important command speedups
> +-------------------------------------
> +
> +At this point, the patterns for testing and implementing sparse-directory
> +logic should be relatively stable. This phase focuses on updating some of
> +the most common builtins that use the index to operate as O(Populated).
> +Here is a potential list of commands that could be valuable to integrate
> +at this point:
> +
> +* `git commit`
> +* `git checkout`
> +* `git merge`
> +* `git rebase`
> +
> +Hopefully, commands such as `git merge` and `git rebase` can benefit
> +instead from merge algorithms that do not use the index as a data
> +structure, such as the merge-ORT strategy. As these topics mature, we
> +may enalbe the ORT strategy by default for repositories using the

s/enalbe/enable/

> +sparse-index feature.
> +
> +Along with `git status` and `git add`, these commands cover the majority
> +of users' interactions with the working directory. In addition, we can
> +integrate with these commands:
> +
> +* `git grep`
> +* `git rm`
> +
> +These have been proposed as some whose behavior could change when in a
> +repo with a sparse-checkout definition. It would be good to include this
> +behavior automatically when using a sparse-index. Some clarity is needed
> +to make the behavior switch clear to the user.
> +
> +This phase is the first where parallel work might be possible without too
> +much conflicts between topics.
> +
> +Phase IV: The long tail
> +-----------------------
> +
> +This last phase is less a "phase" and more "the new normal" after all of
> +the previous work.
> +
> +To start, the `command_requires_full_index` option could be removed in
> +favor of expanding only when hitting an API guard.
> +
> +There are many Git commands that could use special attention to operate as
> +O(Populated), while some might be so rare that it is acceptable to leave
> +them with additional overhead when a sparse-index is present.
> +
> +Here are some commands that might be useful to update:
> +
> +* `git sparse-checkout set`
> +* `git am`
> +* `git clean`
> +* `git stash`
> --
> gitgitgadget
>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> This begins a long effort to update the index format to allow sparse
> directory entries. This should result in a significant improvement to
> Git commands when HEAD contains millions of files, but the user has
> selected many fewer files to keep in their sparse-checkout definition.

This compromise makes sense.

In the past, we often dreamed of recording trees in the index
(instead of using a bolted on extension like cache-tree, treating
trees as first-class citizens) and lazily expanding it only when the
user starts modifying the paths within the subdirectory.

But such an optimization never materialized, as the dual and
conflicting nature of the index to keep track of the contents for
the "next" commit (for which it is sufficient to just record trees
for parts that have not been modified) and to cache stat information
to detect which working tree paths may possibly have modifications
(for which, we used the one-entry-per-path nature of the cache
entries so far) was never resolved.

But if we limit the use of trees-in-index for sparse/cone checkout
case, we do not even have to worry about having to cache the stat
information for those paths that we are not going to populate in the
working tree at all.  It is a great simplification of the problem.

> +  These entries have mode `0040000`, include the `SKIP_WORKTREE` bit, and
> +  the path ends in a directory separator.
> +

Why leading two 0's?  At the tree object level, we do not 0-pad blob
mode word, and if you are writing for C programmers, you need only
one '0' prefix to signal that it is in octal (in the on-disk index
file, the blob mode word is stored in a be16 word).

> diff --git a/Documentation/technical/sparse-index.txt b/Documentation/technical/sparse-index.txt
> new file mode 100644
> index 000000000000..aa116406a016
> --- /dev/null
> +++ b/Documentation/technical/sparse-index.txt
> @@ -0,0 +1,173 @@
> +Git Sparse-Index Design Document
> +================================
> +
> +The sparse-checkout feature allows users to focus a working directory on
> +a subset of the files at HEAD. The cone mode patterns, enabled by
> +`core.sparseCheckoutCone`, allow for very fast pattern matching to
> +discover which files at HEAD belong in the sparse-checkout cone.
> +
> +Three important scale dimensions for a Git worktree are:

s/worktree/working tree/; The former is the thing the "git worktree"
command deals with.  The latter is relevant even when "git worktree"
is not used (the traditional "git clone and you get a working tree
to work in").

> +* `HEAD`: How many files are present at `HEAD`?
> +
> +* Populated: How many files are within the sparse-checkout cone.
> +
> +* Modified: How many files has the user modified in the working directory?
> +
> +We will use big-O notation -- O(X) -- to denote how expensive certain
> +operations are in terms of these dimensions.
> +
> +These dimensions are ordered by their magnitude: users (typically) modify
> +fewer files than are populated, and we can only populate files at `HEAD`.

OK.

> +These dimensions are also ordered by how expensive they are per item: it
> +is expensive to detect a modified file than it is to write one that we
> +know must be populated; changing `HEAD` only really requires updating the
> +index.

This is a bit too dense to grok.  Among Populated, there are some
Modified but it takes lstat(2) per path or fsmonitor listening to
inotify to know which ones are in the Modified set.  Is that the
"expensive" you are referring to here?  I am not sure how you
compared the cost to know if a path is modified or merely populated
with the cost of "write one that we know must be populated" (which I
take as "given a populated file, make modification to it").  Also it
is unclear what you mean by "changing HEAD only require updating the
index".  Certainly when "git switch" flips HEAD from one commit to
another, you'd update the index and update the files in the working
tree (in the Populated part that is in the sparse-checkout cone) to
match, no?

> +Problems occur if there is an extreme imbalance in these dimensions. For
> +example, if `HEAD` contains millions of paths but the populated set has
> +only tens of thousands, then commands like `git status` and `git add` can
> +be dominated by operations that require O(`HEAD`) operations instead of
> +O(Populated). Primarily, the cost is in parsing and rewriting the index,
> +which is filled primarily with files at `HEAD` that are marked with the
> +`SKIP_WORKTREE` bit.
> +
> +The sparse-index intends to take these commands that read and modify the
> +index from O(`HEAD`) to O(Populated). To do this, we need to modify the
> +index format in a significant way: add "sparse directory" entries.

OK.

> +With cone mode patterns, it is possible to detect when an entire
> +directory will have its contents outside of the sparse-checkout definition.
> +Instead of listing all of the files it contains as individual entries, a
> +sparse-index contains an entry with the directory name, referencing the
> +object ID of the tree at `HEAD` and marked with the `SKIP_WORKTREE` bit.
> +If we need to discover the details for paths within that directory, we
> +can parse trees to find that list.

;-)

> +At time of writing, sparse-directory entries violate expectations about the
> +index format and its in-memory data structure. There are many consumers in
> +the codebase that expect to iterate through all of the index entries and
> +see only files.

True.

> In addition, they expect to see all files at `HEAD`.

It is not clear to me what this means.  After "git add", "git
ls-files" would expect to see a file that may not even in HEAD.
After "git rm", it would expect to see some file missing from the
set of paths in HEAD.  While I do not think that is what you meant
here, it is hard to guess what you wanted to say.

> One
> +way to handle this is to parse trees to replace a sparse-directory entry
> +with all of the files within that tree as the index is loaded. However,
> +parsing trees is slower than parsing the index format, so that is a slower
> +operation than if we left the index alone.

Besides, that would leave in-core index fully populated, so I would
suspect that you'd lose a lot of benefit that comes from having to
keep much fewer entries in the in-core index than what is in HEAD.
It would be nice for "git diff-index --cached" (which is part of
"git status") to be able to skip a single "tree" entry in the sparse
index as "known to be untouched", than skipping thousands of paths
in that single subdirectory (in a mega monorepo project) as "these
are marked with SKIP_WORKTREE so ignore what is in the working tree".

> +The implementation plan below follows four phases to slowly integrate with
> +the sparse-index. The intention is to incrementally update Git commands to
> +interact safely with the sparse-index without significant slowdowns. This
> +may not always be possible, but the hope is that the primary commands that
> +users need in their daily work are dramatically improved.

OK.

> +Phase I: Format and initial speedups
> +------------------------------------
> +
> +During this phase, Git learns to enable the sparse-index and safely parse
> +one. Protections are put in place so that every consumer of the in-memory
> +data structure can operate with its current assumption of every file at
> +`HEAD`.

IOW, before they iterate over the in-core index, tree entries are expanded
into bunch of individual entries with SKIP_WORKTREE bit?  Makes sense.

> +At first, every index parse will expand the sparse-directory entries into
> +the full list of paths at `HEAD`. This will be slower in all cases. The
> +only noticable change in behavior will be that the serialized index file
> +contains sparse-directory entries.

Hmph, do you mean that the expansion is done by not replacing each
"tree" entry with blob entries for the contents of the directory,
but the original "tree" entry is still left in the in-core index?
It is not immediately clear what we are trying to gain by leaving it
in, but let's read on.  Perhaps we can get rid of cache-tree
extension and replace its use with these "tree" entries whose
content paths are populated in the index?

> +To start, we use a new repository extension, `extensions.sparseIndex`, to
> +allow inserting sparse-directory entries into indexes with file format
> +versions 2, 3, and 4. This prevents Git versions that do not understand
> +the sparse-index from operating on one, but it also prevents other
> +operations that do not use the index at all. A new format, index v5, will
> +be introduced that includes sparse-directory entries by default. It might
> +also introduce other features that have been considered for improving the
> +index, as well.

OK.

> +Next, consumers of the index will be guarded against operating on a
> +sparse-index by inserting calls to `ensure_full_index()` or
> +`expand_index_to_path()`. After these guards are in place, we can begin
> +leaving sparse-directory entries in the in-memory index structure.

It is unclear why "we can begin leaving"; an iterator that only
expects to see blobs would need to be updated to skip them, too, no?
They would probably be already skipping blob entries that are marked
with the SKIP_WORKTREE bit, so it may be just a matter of skipping
more things than the current code.

Or did I misread the design presented earlier, and when a directory
that is outside the cone is expanded into the paths of blobs in the
directory, the "tree" entry is removed from the in-core index?

> +Even after inserting these guards, we will keep expanding sparse-indexes
> +for most Git commands using the `command_requires_full_index` repository
> +setting. This setting will be on by default and disabled one builtin at a
> +time until we have sufficient confidence that all of the index operations
> +are properly guarded.

OK.

> +To complete this phase, the commands `git status` and `git add` will be
> +integrated with the sparse-index so that they operate with O(Populated)
> +performance. They will be carefully tested for operations within and
> +outside the sparse-checkout definition.

;-)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 3/19/2021 7:43 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> This begins a long effort to update the index format to allow sparse
>> directory entries. This should result in a significant improvement to
>> Git commands when HEAD contains millions of files, but the user has
>> selected many fewer files to keep in their sparse-checkout definition.
> 
> This compromise makes sense.
> 
> In the past, we often dreamed of recording trees in the index
> (instead of using a bolted on extension like cache-tree, treating
> trees as first-class citizens) and lazily expanding it only when the
> user starts modifying the paths within the subdirectory.
> 
> But such an optimization never materialized, as the dual and
> conflicting nature of the index to keep track of the contents for
> the "next" commit (for which it is sufficient to just record trees
> for parts that have not been modified) and to cache stat information
> to detect which working tree paths may possibly have modifications
> (for which, we used the one-entry-per-path nature of the cache
> entries so far) was never resolved.
> 
> But if we limit the use of trees-in-index for sparse/cone checkout
> case, we do not even have to worry about having to cache the stat
> information for those paths that we are not going to populate in the
> working tree at all.  It is a great simplification of the problem.

Thanks. I appreciate your input here.
 
>> +  These entries have mode `0040000`, include the `SKIP_WORKTREE` bit, and
>> +  the path ends in a directory separator.
>> +
> 
> Why leading two 0's?  At the tree object level, we do not 0-pad blob
> mode word, and if you are writing for C programmers, you need only
> one '0' prefix to signal that it is in octal (in the on-disk index
> file, the blob mode word is stored in a be16 word).

Fixed.

>> diff --git a/Documentation/technical/sparse-index.txt b/Documentation/technical/sparse-index.txt
>> new file mode 100644
>> index 000000000000..aa116406a016
>> --- /dev/null
>> +++ b/Documentation/technical/sparse-index.txt
>> @@ -0,0 +1,173 @@
>> +Git Sparse-Index Design Document
>> +================================
>> +
>> +The sparse-checkout feature allows users to focus a working directory on
>> +a subset of the files at HEAD. The cone mode patterns, enabled by
>> +`core.sparseCheckoutCone`, allow for very fast pattern matching to
>> +discover which files at HEAD belong in the sparse-checkout cone.
>> +
>> +Three important scale dimensions for a Git worktree are:
> 
> s/worktree/working tree/; The former is the thing the "git worktree"
> command deals with.  The latter is relevant even when "git worktree"
> is not used (the traditional "git clone and you get a working tree
> to work in").

I guess I'm distracted by using SKIP_WORKTREE a lot, but "working
directory" is more specific and hence better.

>> +These dimensions are also ordered by how expensive they are per item: it
>> +is expensive to detect a modified file than it is to write one that we
>> +know must be populated; changing `HEAD` only really requires updating the
>> +index.
> 
> This is a bit too dense to grok.  Among Populated, there are some
> Modified but it takes lstat(2) per path or fsmonitor listening to
> inotify to know which ones are in the Modified set.  Is that the
> "expensive" you are referring to here?  I am not sure how you
> compared the cost to know if a path is modified or merely populated
> with the cost of "write one that we know must be populated" (which I
> take as "given a populated file, make modification to it"). 

I could rearrange things here. The important things to note are:

1. Updating index entries is very fast, but adds up at large scale.

2. It is faster to write a file to disk from Git's object database
   than it is to compare a file on disk to the copy in the database,
   which is frequently necessary when the mtime on disk doesn't match
   the mtime in the index.

> Also it
> is unclear what you mean by "changing HEAD only require updating the
> index".  Certainly when "git switch" flips HEAD from one commit to
> another, you'd update the index and update the files in the working
> tree (in the Populated part that is in the sparse-checkout cone) to
> match, no?

This is unclear of me. I was thinking more on the lines of "git reset"
(soft mode) which updates HEAD without changing the files on disk.

After all of this postulating, I think that the offending sentences
are better off deleted. They don't add clarity over what can be
inferred by an interested reader.

>> In addition, they expect to see all files at `HEAD`.
> 
> It is not clear to me what this means.  After "git add", "git
> ls-files" would expect to see a file that may not even in HEAD.
> After "git rm", it would expect to see some file missing from the
> set of paths in HEAD.  While I do not think that is what you meant
> here, it is hard to guess what you wanted to say.

I'm mixing terms incorrectly. I think what I really mean is

  In fact, these loops expect to see a reference to every
  staged file.

>> One
>> +way to handle this is to parse trees to replace a sparse-directory entry
>> +with all of the files within that tree as the index is loaded. However,
>> +parsing trees is slower than parsing the index format, so that is a slower
>> +operation than if we left the index alone.
> 
> Besides, that would leave in-core index fully populated, so I would
> suspect that you'd lose a lot of benefit that comes from having to
> keep much fewer entries in the in-core index than what is in HEAD.
> It would be nice for "git diff-index --cached" (which is part of
> "git status") to be able to skip a single "tree" entry in the sparse
> index as "known to be untouched", than skipping thousands of paths
> in that single subdirectory (in a mega monorepo project) as "these
> are marked with SKIP_WORKTREE so ignore what is in the working tree".

Absolutely! I'm burying the lead here, so I should get to the real
point by adding this to the end:

 The plan is to make all of these integrations "sparse aware" so
 this expansion through tree parsing is unnecessary and they use
 fewer resources than when using a full index.

>> +Phase I: Format and initial speedups
>> +------------------------------------
>> +
>> +During this phase, Git learns to enable the sparse-index and safely parse
>> +one. Protections are put in place so that every consumer of the in-memory
>> +data structure can operate with its current assumption of every file at
>> +`HEAD`.
> 
> IOW, before they iterate over the in-core index, tree entries are expanded
> into bunch of individual entries with SKIP_WORKTREE bit?  Makes sense.
> 
>> +At first, every index parse will expand the sparse-directory entries into
>> +the full list of paths at `HEAD`. This will be slower in all cases. The
>> +only noticable change in behavior will be that the serialized index file
>> +contains sparse-directory entries.
> 
> Hmph, do you mean that the expansion is done by not replacing each
> "tree" entry with blob entries for the contents of the directory,
> but the original "tree" entry is still left in the in-core index?

I meant by "serialized index file" is that the file written to disk has
the sparse directory entries, but the in-core copy will not (except for
a very brief moment in time, during do_read_index()).

The intention at this point in time is that all code behaves identically
to the full index case, except that the index file itself is smaller due
to these sparse directory entries.

> It is not immediately clear what we are trying to gain by leaving it
> in, but let's read on.  Perhaps we can get rid of cache-tree
> extension and replace its use with these "tree" entries whose
> content paths are populated in the index?

This is an interesting idea, but not one I plan to pursue with this work.

>> +Next, consumers of the index will be guarded against operating on a
>> +sparse-index by inserting calls to `ensure_full_index()` or
>> +`expand_index_to_path()`. After these guards are in place, we can begin
>> +leaving sparse-directory entries in the in-memory index structure.
> 
> It is unclear why "we can begin leaving"; an iterator that only
> expects to see blobs would need to be updated to skip them, too, no?
> They would probably be already skipping blob entries that are marked
> with the SKIP_WORKTREE bit, so it may be just a matter of skipping
> more things than the current code.
> 
> Or did I misread the design presented earlier, and when a directory
> that is outside the cone is expanded into the paths of blobs in the
> directory, the "tree" entry is removed from the in-core index?

I will make this more explicit.
 
Thanks for your help improving this doc! Hopefully the plan is a
little more clear, now.

-Stolee

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

>>> +Three important scale dimensions for a Git worktree are:
>> 
>> s/worktree/working tree/; The former is the thing the "git worktree"
>> command deals with.  The latter is relevant even when "git worktree"
>> is not used (the traditional "git clone and you get a working tree
>> to work in").
>
> I guess I'm distracted by using SKIP_WORKTREE a lot, but "working
> directory" is more specific and hence better.

Since the user's current working directory can be outside any
working tree that is governed by any git repository, "working
directory" is a term I try to avoid when describing the directory
where a checkout of a revision lives.

Documentation/glossary-content.txt is where the suggestion for
"working tree" comes from.

> I could rearrange things here. The important things to note are:
>
> 1. Updating index entries is very fast, but adds up at large scale.

This is the "checkout to match the index to the tree of HEAD" part,
ignoring the cost of writing working tree files out?

> 2. It is faster to write a file to disk from Git's object database
>    than it is to compare a file on disk to the copy in the database,
>    which is frequently necessary when the mtime on disk doesn't match
>    the mtime in the index.

True.  But of course, not having to do either (i.e. having a fresh
cached stat info) would be even faster ;-).

>> Also it
>> is unclear what you mean by "changing HEAD only require updating the
>> index".  Certainly when "git switch" flips HEAD from one commit to
>> another, you'd update the index and update the files in the working
>> tree (in the Populated part that is in the sparse-checkout cone) to
>> match, no?
>
> This is unclear of me. I was thinking more on the lines of "git reset"
> (soft mode) which updates HEAD without changing the files on disk.

OK, and that is in line with your "updating index entries is very
fast (but adds up)".

> After all of this postulating, I think that the offending sentences
> are better off deleted. They don't add clarity over what can be
> inferred by an interested reader.

OK.

> I'm mixing terms incorrectly. I think what I really mean is
>
>   In fact, these loops expect to see a reference to every
>   staged file.

OK.

>  The plan is to make all of these integrations "sparse aware" so
>  this expansion through tree parsing is unnecessary and they use
>  fewer resources than when using a full index.

;-)

> I meant by "serialized index file" is that the file written to disk has
> the sparse directory entries, but the in-core copy will not (except for
> a very brief moment in time, during do_read_index()).

Nice.  That would probably mean cache-tree extension on-disk can go
away, because we can populate in-core cache-tree from these entries.
I've always hated the on-disk encoding of that extension.

Or we are not doing this "extra tree" everywhere (i.e. limited only
to the parts that are marked for "sparse checkout")?

Thanks.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 3/23/2021 4:10 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>>> +Three important scale dimensions for a Git worktree are:
>>>
>>> s/worktree/working tree/; The former is the thing the "git worktree"
>>> command deals with.  The latter is relevant even when "git worktree"
>>> is not used (the traditional "git clone and you get a working tree
>>> to work in").
>>
>> I guess I'm distracted by using SKIP_WORKTREE a lot, but "working
>> directory" is more specific and hence better.
> 
> Since the user's current working directory can be outside any
> working tree that is governed by any git repository, "working
> directory" is a term I try to avoid when describing the directory
> where a checkout of a revision lives.
> 
> Documentation/glossary-content.txt is where the suggestion for
> "working tree" comes from.

Whoops. Somehow I read that wrong. Thanks for pointing out my error.

>> I meant by "serialized index file" is that the file written to disk has
>> the sparse directory entries, but the in-core copy will not (except for
>> a very brief moment in time, during do_read_index()).
> 
> Nice.  That would probably mean cache-tree extension on-disk can go
> away, because we can populate in-core cache-tree from these entries.
> I've always hated the on-disk encoding of that extension.
> 
> Or we are not doing this "extra tree" everywhere (i.e. limited only
> to the parts that are marked for "sparse checkout")?

The current design is to only have these entries when all paths
within the directory are marked with SKIP_WORKTREE. This pairs
with the cache-tree extension, which has these directories as
nodes, but only consuming one cache entry (for itself).

I haven't considered the idea of inserting trees for other
reasons. Seems like a valuable experiment.

Thanks,
-Stolee

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Tue, Mar 23, 2021 at 01:44:09PM +0000, Derrick Stolee via GitGitGadget wrote:
> Currently, the index format is only updated in the presence of
> extensions.sparseIndex instead of increasing a file format version
> number. This is temporary, and index v5 is part of the plan for future
> work in this area.

> diff --git a/Documentation/technical/sparse-index.txt b/Documentation/technical/sparse-index.txt
> new file mode 100644
> index 000000000000..62f6dc225a44
> --- /dev/null
> +++ b/Documentation/technical/sparse-index.txt

> +To start, we use a new repository extension, `extensions.sparseIndex`, to
> +allow inserting sparse-directory entries into indexes with file format
> +versions 2, 3, and 4. This prevents Git versions that do not understand
> +the sparse-index from operating on one, but it also prevents other
> +operations that do not use the index at all.

Why is this not a non-optional index extension?  That would allow
older Git versions and other implementations not understanding sparse
directory entries to still perform any operation that doesn't involve
the index.  More importantly, that would prevent older Git versions
and other implementations not understanding repository extensions from
potentially wreaking havoc when the index contains sparse directory
entries.  Notably JGit's current version (5.11.0.202103091610-r) does
still completely ignore repository extensions, and e.g. happily
attempts any operations on a SHA256 repository, so it would do the
same in the presence of 'extensions.sparseIndex' as well.  JGit does
respect non-optional index extensions, see e.g. 87a6bb701a
(t5310-pack-bitmaps: make JGit tests work with GIT_TEST_SPLIT_INDEX,
2018-05-10).

This really should be a non-optional index extension.

> A new format, index v5, will
> +be introduced that includes sparse-directory entries by default. It might
> +also introduce other features that have been considered for improving the
> +index, as well.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> +To start, we use a new repository extension, `extensions.sparseIndex`, to
>> +allow inserting sparse-directory entries into indexes with file format
>> +versions 2, 3, and 4. This prevents Git versions that do not understand
>> +the sparse-index from operating on one, but it also prevents other
>> +operations that do not use the index at all.
>
> Why is this not a non-optional index extension?  ...
> This really should be a non-optional index extension.

Yeah, the index extension mechanism was designed with optional and
required kinds because we wanted to support exactly a use case like
this one.

Thanks for pointing it out.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 3/27/2021 9:47 PM, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
>>> +To start, we use a new repository extension, `extensions.sparseIndex`, to
>>> +allow inserting sparse-directory entries into indexes with file format
>>> +versions 2, 3, and 4. This prevents Git versions that do not understand
>>> +the sparse-index from operating on one, but it also prevents other
>>> +operations that do not use the index at all.
>>
>> Why is this not a non-optional index extension?  ...
>> This really should be a non-optional index extension.
> 
> Yeah, the index extension mechanism was designed with optional and
> required kinds because we wanted to support exactly a use case like
> this one.
> 
> Thanks for pointing it out.

Ok, so let me be sure I understand the request, as I believe it is
a very good one:

Using a REQUIRED index extension that says "this index has
sparse-directory entries" will allow tools that don't touch
the index to be compatible with repos using the sparse-index,
while also avoiding a new index version.

I'll work on this right away. Thanks!
-Stolee

with the same name are sorted by their stage field.

An index entry typically represents a file. However, if sparse-checkout
is enabled in cone mode (`core.sparseCheckoutCone` is enabled) and the
`extensions.sparseIndex` extension is enabled, then the index may
contain entries for directories outside of the sparse-checkout definition.
These entries have mode `040000`, include the `SKIP_WORKTREE` bit, and
the path ends in a directory separator.

32-bit ctime seconds, the last time a file's metadata changed
this is stat(2) data

Expand Down Expand Up @@ -385,3 +392,15 @@ The remaining data of each directory block is grouped by type:
in this block of entries.

- 32-bit count of cache entries in this block

== Sparse Directory Entries

When using sparse-checkout in cone mode, some entire directories within
the index can be summarized by pointing to a tree object instead of the
entire expanded list of paths within that tree. An index containing such
entries is a "sparse index". Index format versions 4 and less were not
implemented with such entries in mind. Thus, for these versions, an
index containing sparse directory entries will include this extension
with signature { 's', 'd', 'i', 'r' }. Like the split-index extension,
tools should avoid interacting with a sparse index unless they understand
this extension.
175 changes: 175 additions & 0 deletions Documentation/technical/sparse-index.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
Git Sparse-Index Design Document
================================

The sparse-checkout feature allows users to focus a working directory on
a subset of the files at HEAD. The cone mode patterns, enabled by
`core.sparseCheckoutCone`, allow for very fast pattern matching to
discover which files at HEAD belong in the sparse-checkout cone.

Three important scale dimensions for a Git working directory are:

* `HEAD`: How many files are present at `HEAD`?

* Populated: How many files are within the sparse-checkout cone.

* Modified: How many files has the user modified in the working directory?

We will use big-O notation -- O(X) -- to denote how expensive certain
operations are in terms of these dimensions.

These dimensions are ordered by their magnitude: users (typically) modify
fewer files than are populated, and we can only populate files at `HEAD`.

Problems occur if there is an extreme imbalance in these dimensions. For
example, if `HEAD` contains millions of paths but the populated set has
only tens of thousands, then commands like `git status` and `git add` can
be dominated by operations that require O(`HEAD`) operations instead of
O(Populated). Primarily, the cost is in parsing and rewriting the index,
which is filled primarily with files at `HEAD` that are marked with the
`SKIP_WORKTREE` bit.

The sparse-index intends to take these commands that read and modify the
index from O(`HEAD`) to O(Populated). To do this, we need to modify the
index format in a significant way: add "sparse directory" entries.

With cone mode patterns, it is possible to detect when an entire
directory will have its contents outside of the sparse-checkout definition.
Instead of listing all of the files it contains as individual entries, a
sparse-index contains an entry with the directory name, referencing the
object ID of the tree at `HEAD` and marked with the `SKIP_WORKTREE` bit.
If we need to discover the details for paths within that directory, we
can parse trees to find that list.

At time of writing, sparse-directory entries violate expectations about the
index format and its in-memory data structure. There are many consumers in
the codebase that expect to iterate through all of the index entries and
see only files. In fact, these loops expect to see a reference to every
staged file. One way to handle this is to parse trees to replace a
sparse-directory entry with all of the files within that tree as the index
is loaded. However, parsing trees is slower than parsing the index format,
so that is a slower operation than if we left the index alone. The plan is
to make all of these integrations "sparse aware" so this expansion through
tree parsing is unnecessary and they use fewer resources than when using a
full index.

The implementation plan below follows four phases to slowly integrate with
the sparse-index. The intention is to incrementally update Git commands to
interact safely with the sparse-index without significant slowdowns. This
may not always be possible, but the hope is that the primary commands that
users need in their daily work are dramatically improved.

Phase I: Format and initial speedups
------------------------------------

During this phase, Git learns to enable the sparse-index and safely parse
one. Protections are put in place so that every consumer of the in-memory
data structure can operate with its current assumption of every file at
`HEAD`.

At first, every index parse will call a helper method,
`ensure_full_index()`, which scans the index for sparse-directory entries
(pointing to trees) and replaces them with the full list of paths (with
blob contents) by parsing tree objects. This will be slower in all cases.
The only noticeable change in behavior will be that the serialized index
file contains sparse-directory entries.

To start, we use a new required index extension, `sdir`, to allow
inserting sparse-directory entries into indexes with file format
versions 2, 3, and 4. This prevents Git versions that do not understand
the sparse-index from operating on one, while allowing tools that do not
understand the sparse-index to operate on repositories as long as they do
not interact with the index. A new format, index v5, will be introduced
that includes sparse-directory entries by default. It might also
introduce other features that have been considered for improving the
index, as well.

Next, consumers of the index will be guarded against operating on a
sparse-index by inserting calls to `ensure_full_index()` or
`expand_index_to_path()`. After these guards are in place, we can begin
leaving sparse-directory entries in the in-memory index structure.

Even after inserting these guards, we will keep expanding sparse-indexes
for most Git commands using the `command_requires_full_index` repository
setting. This setting will be on by default and disabled one builtin at a
time until we have sufficient confidence that all of the index operations
are properly guarded.

To complete this phase, the commands `git status` and `git add` will be
integrated with the sparse-index so that they operate with O(Populated)
performance. They will be carefully tested for operations within and
outside the sparse-checkout definition.

Phase II: Careful integrations
------------------------------

This phase focuses on ensuring that all index extensions and APIs work
well with a sparse-index. This requires significant increases to our test
coverage, especially for operations that interact with the working
directory outside of the sparse-checkout definition. Some of these
behaviors may not be the desirable ones, such as some tests already
marked for failure in `t1092-sparse-checkout-compatibility.sh`.

The index extensions that may require special integrations are:

* FS Monitor
* Untracked cache

While integrating with these features, we should look for patterns that
might lead to better APIs for interacting with the index. Coalescing
common usage patterns into an API call can reduce the number of places
where sparse-directories need to be handled carefully.

Phase III: Important command speedups
-------------------------------------

At this point, the patterns for testing and implementing sparse-directory
logic should be relatively stable. This phase focuses on updating some of
the most common builtins that use the index to operate as O(Populated).
Here is a potential list of commands that could be valuable to integrate
at this point:

* `git commit`
* `git checkout`
* `git merge`
* `git rebase`

Hopefully, commands such as `git merge` and `git rebase` can benefit
instead from merge algorithms that do not use the index as a data
structure, such as the merge-ORT strategy. As these topics mature, we
may enable the ORT strategy by default for repositories using the
sparse-index feature.

Along with `git status` and `git add`, these commands cover the majority
of users' interactions with the working directory. In addition, we can
integrate with these commands:

* `git grep`
* `git rm`

These have been proposed as some whose behavior could change when in a
repo with a sparse-checkout definition. It would be good to include this
behavior automatically when using a sparse-index. Some clarity is needed
to make the behavior switch clear to the user.

This phase is the first where parallel work might be possible without too
much conflicts between topics.

Phase IV: The long tail
-----------------------

This last phase is less a "phase" and more "the new normal" after all of
the previous work.

To start, the `command_requires_full_index` option could be removed in
favor of expanding only when hitting an API guard.

There are many Git commands that could use special attention to operate as
O(Populated), while some might be so rare that it is acceptable to leave
them with additional overhead when a sparse-index is present.

Here are some commands that might be useful to update:

* `git sparse-checkout set`
* `git am`
* `git clean`
* `git stash`
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,7 @@ LIB_OBJS += setup.o
LIB_OBJS += shallow.o

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Upcoming changes will introduce modifications to the index format that
> allow sparse directories. It will be useful to have a mechanism for
> converting those sparse index files into full indexes by walking the
> tree at those sparse directories. Name this method ensure_full_index()
> as it will guarantee that the index is fully expanded.
>
> This method is not implemented yet, and instead we focus on the
> scaffolding to declare it and call it at the appropriate time.
>
> Add a 'command_requires_full_index' member to struct repo_settings. This
> will be an indicator that we need the index in full mode to do certain
> index operations. This starts as being true for every command, then we
> will set it to false as some commands integrate with sparse indexes.
>
> If 'command_requires_full_index' is true, then we will immediately
> expand a sparse index to a full one upon reading from disk. This
> suffices for now, but we will want to add more callers to
> ensure_full_index() later.

Same as 01/27 of your RFC series; looks good.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Makefile        |  1 +
>  repo-settings.c |  8 ++++++++
>  repository.c    | 11 ++++++++++-
>  repository.h    |  2 ++
>  sparse-index.c  |  8 ++++++++
>  sparse-index.h  |  7 +++++++
>  6 files changed, 36 insertions(+), 1 deletion(-)
>  create mode 100644 sparse-index.c
>  create mode 100644 sparse-index.h
>
> diff --git a/Makefile b/Makefile
> index 5a239cac20e3..3bf61699238d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -980,6 +980,7 @@ LIB_OBJS += setup.o
>  LIB_OBJS += shallow.o
>  LIB_OBJS += sideband.o
>  LIB_OBJS += sigchain.o
> +LIB_OBJS += sparse-index.o
>  LIB_OBJS += split-index.o
>  LIB_OBJS += stable-qsort.o
>  LIB_OBJS += strbuf.o
> diff --git a/repo-settings.c b/repo-settings.c
> index f7fff0f5ab83..d63569e4041e 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -77,4 +77,12 @@ void prepare_repo_settings(struct repository *r)
>                 UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);
>
>         UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
> +
> +       /*
> +        * 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;
>  }
> diff --git a/repository.c b/repository.c
> index c98298acd017..a8acae002f71 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -10,6 +10,7 @@
>  #include "object.h"
>  #include "lockfile.h"
>  #include "submodule-config.h"
> +#include "sparse-index.h"
>
>  /* The main repository */
>  static struct repository the_repo;
> @@ -261,6 +262,8 @@ void repo_clear(struct repository *repo)
>
>  int repo_read_index(struct repository *repo)
>  {
> +       int res;
> +
>         if (!repo->index)
>                 repo->index = xcalloc(1, sizeof(*repo->index));
>
> @@ -270,7 +273,13 @@ int repo_read_index(struct repository *repo)
>         else if (repo->index->repo != repo)
>                 BUG("repo's index should point back at itself");
>
> -       return read_index_from(repo->index, repo->index_file, repo->gitdir);
> +       res = read_index_from(repo->index, repo->index_file, repo->gitdir);
> +
> +       prepare_repo_settings(repo);
> +       if (repo->settings.command_requires_full_index)
> +               ensure_full_index(repo->index);
> +
> +       return res;
>  }
>
>  int repo_hold_locked_index(struct repository *repo,
> diff --git a/repository.h b/repository.h
> index b385ca3c94b6..e06a23015697 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -41,6 +41,8 @@ struct repo_settings {
>         enum fetch_negotiation_setting fetch_negotiation_algorithm;
>
>         int core_multi_pack_index;
> +
> +       unsigned command_requires_full_index:1;
>  };
>
>  struct repository {
> diff --git a/sparse-index.c b/sparse-index.c
> new file mode 100644
> index 000000000000..82183ead563b
> --- /dev/null
> +++ b/sparse-index.c
> @@ -0,0 +1,8 @@
> +#include "cache.h"
> +#include "repository.h"
> +#include "sparse-index.h"
> +
> +void ensure_full_index(struct index_state *istate)
> +{
> +       /* intentionally left blank */
> +}
> diff --git a/sparse-index.h b/sparse-index.h
> new file mode 100644
> index 000000000000..09a20d036c46
> --- /dev/null
> +++ b/sparse-index.h
> @@ -0,0 +1,7 @@
> +#ifndef SPARSE_INDEX_H__
> +#define SPARSE_INDEX_H__
> +
> +struct index_state;
> +void ensure_full_index(struct index_state *istate);
> +
> +#endif
> --
> gitgitgadget
>

LIB_OBJS += sideband.o
LIB_OBJS += sigchain.o
LIB_OBJS += sparse-index.o
LIB_OBJS += split-index.o
LIB_OBJS += stable-qsort.o
LIB_OBJS += strbuf.o
Expand Down
44 changes: 35 additions & 9 deletions builtin/sparse-checkout.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "unpack-trees.h"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Tue, Feb 23, 2021 at 08:14:26PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> We use 'git sparse-checkout init --cone --sparse-index' to toggle the
> sparse-index feature. It makes sense to also disable it when running
> 'git sparse-checkout disable'. This is particularly important because it
> removes the extensions.sparseIndex config option, allowing other tools
> to use this Git repository again.
> 
> This does mean that 'git sparse-checkout init' will not re-enable the
> sparse-index feature, even if it was previously enabled.
> 
> While testing this feature, I noticed that the sparse-index was not
> being written on the first run, but by a second. This was caught by the
> call to 'test-tool read-cache --table'. This requires adjusting some
> assignments to core_apply_sparse_checkout and pl.use_cone_patterns in
> the sparse_checkout_init() logic.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/sparse-checkout.c          | 10 +++++++++-
>  t/t1091-sparse-checkout-builtin.sh | 13 +++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index ca63e2c64e95..585343fa1972 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -280,6 +280,9 @@ static int set_config(enum sparse_checkout_mode mode)
>  				      "core.sparseCheckoutCone",
>  				      mode == MODE_CONE_PATTERNS ? "true" : NULL);
>  
> +	if (mode == MODE_NO_PATTERNS)
> +		set_sparse_index_config(the_repository, 0);
> +
>  	return 0;
>  }
>  
> @@ -341,10 +344,11 @@ static int sparse_checkout_init(int argc, const char **argv)
>  		the_repository->index->updated_workdir = 1;
>  	}
>  
> +	core_apply_sparse_checkout = 1;
> +
>  	/* If we already have a sparse-checkout file, use it. */
>  	if (res >= 0) {
>  		free(sparse_filename);
> -		core_apply_sparse_checkout = 1;
>  		return update_working_directory(NULL);
>  	}
>  
> @@ -366,6 +370,7 @@ static int sparse_checkout_init(int argc, const char **argv)
>  	add_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);
>  	strbuf_addstr(&pattern, "!/*/");
>  	add_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);
> +	pl.use_cone_patterns = init_opts.cone_mode;
>  
>  	return write_patterns_and_update(&pl);
>  }
> @@ -632,6 +637,9 @@ static int sparse_checkout_disable(int argc, const char **argv)
>  	strbuf_addstr(&match_all, "/*");
>  	add_pattern(strbuf_detach(&match_all, NULL), empty_base, 0, &pl, 0);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.sparse_index = 0;
> +
>  	if (update_working_directory(&pl))
>  		die(_("error while refreshing working directory"));
>  
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index fc64e9ed99f4..ff1ad570a255 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -205,6 +205,19 @@ test_expect_success 'sparse-checkout disable' '
>  	check_files repo a deep folder1 folder2
>  '
>  
> +test_expect_success 'sparse-index enabled and disabled' '
> +	git -C repo sparse-checkout init --cone --sparse-index &&
> +	test_cmp_config -C repo true extensions.sparseIndex &&
> +	test-tool -C repo read-cache --table >cache &&
> +	grep " tree " cache &&
> +
> +	git -C repo sparse-checkout disable &&
> +	test-tool -C repo read-cache --table >cache &&
> +	! grep " tree " cache &&
> +	git -C repo config --list >config &&
> +	! grep extensions.sparseindex config
> +'

This test passes with GIT_TEST_SPLIT_INDEX=1 at the moment, because,
unfortunately, GIT_TEST_SPLIT_INDEX has been broken for the past two
years.  However, if I run it with my WIP fixes for that issue [1],
then it will fail:

  +git -C repo sparse-checkout init --cone --sparse-index
  +test_cmp_config -C repo true extensions.sparseIndex
  +test-tool -C repo read-cache --table
  +grep  tree  cache
  error: last command exited with $?=1
  not ok 16 - sparse-index enabled and disabled

https://travis-ci.com/github/szeder/git-cooking-topics-for-travis-ci/jobs/486702444#L2594

[1] Try to run it with:

      https://github.com/szeder/git split-index-fixes

    The code is, I believe, close to final, the commit messages,
    however, are far from being finished.


> +
>  test_expect_success 'cone mode: init and set' '
>  	git -C repo sparse-checkout init --cone &&
>  	git -C repo config --list >config &&
> -- 
> gitgitgadget
> 

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/27/2021 7:32 AM, SZEDER Gábor wrote:
> On Tue, Feb 23, 2021 at 08:14:26PM +0000, Derrick Stolee via GitGitGadget wrote:
>> +test_expect_success 'sparse-index enabled and disabled' '
>> +	git -C repo sparse-checkout init --cone --sparse-index &&
>> +	test_cmp_config -C repo true extensions.sparseIndex &&
>> +	test-tool -C repo read-cache --table >cache &&
>> +	grep " tree " cache &&
>> +
>> +	git -C repo sparse-checkout disable &&
>> +	test-tool -C repo read-cache --table >cache &&
>> +	! grep " tree " cache &&
>> +	git -C repo config --list >config &&
>> +	! grep extensions.sparseindex config
>> +'
> 
> This test passes with GIT_TEST_SPLIT_INDEX=1 at the moment, because,
> unfortunately, GIT_TEST_SPLIT_INDEX has been broken for the past two
> years.  However, if I run it with my WIP fixes for that issue [1],
> then it will fail:
> 
>   +git -C repo sparse-checkout init --cone --sparse-index
>   +test_cmp_config -C repo true extensions.sparseIndex
>   +test-tool -C repo read-cache --table
>   +grep  tree  cache
>   error: last command exited with $?=1
>   not ok 16 - sparse-index enabled and disabled
> 
> https://travis-ci.com/github/szeder/git-cooking-topics-for-travis-ci/jobs/486702444#L2594
> 
> [1] Try to run it with:
> 
>       https://github.com/szeder/git split-index-fixes
> 
>     The code is, I believe, close to final, the commit messages,
>     however, are far from being finished.

I'll keep that in mind. I should have added a variable
that disables GIT_TEST_SPLIT_INDEX for this test script,
since the sparse-index is (currently) incompatible with
the split-index. I bet that the test is failing because
it isn't actually writing the sparse-directory entry due
to that short-circuit check.

Thanks,
-Stolee

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 3/9/2021 3:20 PM, Derrick Stolee wrote:
> On 2/27/2021 7:32 AM, SZEDER Gábor wrote:
>> On Tue, Feb 23, 2021 at 08:14:26PM +0000, Derrick Stolee via GitGitGadget wrote:
>>> +test_expect_success 'sparse-index enabled and disabled' '
>>> +	git -C repo sparse-checkout init --cone --sparse-index &&
>>> +	test_cmp_config -C repo true extensions.sparseIndex &&
>>> +	test-tool -C repo read-cache --table >cache &&
>>> +	grep " tree " cache &&
>>> +
>>> +	git -C repo sparse-checkout disable &&
>>> +	test-tool -C repo read-cache --table >cache &&
>>> +	! grep " tree " cache &&
>>> +	git -C repo config --list >config &&
>>> +	! grep extensions.sparseindex config
>>> +'
>>
>> This test passes with GIT_TEST_SPLIT_INDEX=1 at the moment, because,
>> unfortunately, GIT_TEST_SPLIT_INDEX has been broken for the past two
>> years.  However, if I run it with my WIP fixes for that issue [1],
>> then it will fail:
>>
>>   +git -C repo sparse-checkout init --cone --sparse-index
>>   +test_cmp_config -C repo true extensions.sparseIndex
>>   +test-tool -C repo read-cache --table
>>   +grep  tree  cache
>>   error: last command exited with $?=1
>>   not ok 16 - sparse-index enabled and disabled
>>
>> https://travis-ci.com/github/szeder/git-cooking-topics-for-travis-ci/jobs/486702444#L2594
>>
>> [1] Try to run it with:
>>
>>       https://github.com/szeder/git split-index-fixes
>>
>>     The code is, I believe, close to final, the commit messages,
>>     however, are far from being finished.
> 
> I'll keep that in mind. I should have added a variable
> that disables GIT_TEST_SPLIT_INDEX for this test script,
> since the sparse-index is (currently) incompatible with
> the split-index. I bet that the test is failing because
> it isn't actually writing the sparse-directory entry due
> to that short-circuit check.

The next version will include GIT_TEST_SPLIT_INDEX=0 at
the start and that will make it work with your branch.

Thanks,
-Stolee

#include "wt-status.h"
#include "quote.h"
#include "sparse-index.h"

static const char *empty_base = "";

Expand Down Expand Up @@ -110,6 +111,8 @@ static int update_working_directory(struct pattern_list *pl)
if (is_index_unborn(r->index))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> As we modify the sparse-checkout definition, we perform index operations
> on a pattern_list that only exists in-memory. This allows easy backing
> out in case the index update fails.
>
> However, if the index write itself cares about the sparse-checkout
> pattern set, we need access to that in-memory copy. Place a pointer to
> a 'struct pattern_list' in the index so we can access this on-demand.
> This will be used in the next change which uses the sparse-checkout
> definition to filter out directories that are outsie the sparse cone.

Looks like you still have the "outsie" typo.  ;-)

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/sparse-checkout.c | 17 ++++++++++-------
>  cache.h                   |  2 ++
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 2306a9ad98e0..e00b82af727b 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -110,6 +110,8 @@ static int update_working_directory(struct pattern_list *pl)
>         if (is_index_unborn(r->index))
>                 return UPDATE_SPARSITY_SUCCESS;
>
> +       r->index->sparse_checkout_patterns = pl;
> +
>         memset(&o, 0, sizeof(o));
>         o.verbose_update = isatty(2);
>         o.update = 1;
> @@ -138,6 +140,7 @@ static int update_working_directory(struct pattern_list *pl)
>         else
>                 rollback_lock_file(&lock_file);
>
> +       r->index->sparse_checkout_patterns = NULL;
>         return result;
>  }
>
> @@ -517,19 +520,18 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
>  {
>         int result;
>         int changed_config = 0;
> -       struct pattern_list pl;
> -       memset(&pl, 0, sizeof(pl));
> +       struct pattern_list *pl = xcalloc(1, sizeof(*pl));
>
>         switch (m) {
>         case ADD:
>                 if (core_sparse_checkout_cone)
> -                       add_patterns_cone_mode(argc, argv, &pl);
> +                       add_patterns_cone_mode(argc, argv, pl);
>                 else
> -                       add_patterns_literal(argc, argv, &pl);
> +                       add_patterns_literal(argc, argv, pl);
>                 break;
>
>         case REPLACE:
> -               add_patterns_from_input(&pl, argc, argv);
> +               add_patterns_from_input(pl, argc, argv);
>                 break;
>         }
>
> @@ -539,12 +541,13 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
>                 changed_config = 1;
>         }
>
> -       result = write_patterns_and_update(&pl);
> +       result = write_patterns_and_update(pl);
>
>         if (result && changed_config)
>                 set_config(MODE_NO_PATTERNS);
>
> -       clear_pattern_list(&pl);
> +       clear_pattern_list(pl);
> +       free(pl);
>         return result;
>  }
>
> diff --git a/cache.h b/cache.h
> index 1336c8d7435e..d75b352f38d3 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -307,6 +307,7 @@ static inline unsigned int canon_mode(unsigned int mode)
>  struct split_index;
>  struct untracked_cache;
>  struct progress;
> +struct pattern_list;
>
>  struct index_state {
>         struct cache_entry **cache;
> @@ -332,6 +333,7 @@ struct index_state {
>         struct mem_pool *ce_mem_pool;
>         struct progress *progress;
>         struct repository *repo;
> +       struct pattern_list *sparse_checkout_patterns;
>  };
>
>  /* Name hashing */
> --
> gitgitgadget
>

return UPDATE_SPARSITY_SUCCESS;

r->index->sparse_checkout_patterns = pl;

memset(&o, 0, sizeof(o));
o.verbose_update = isatty(2);
o.update = 1;
Expand Down Expand Up @@ -138,6 +141,7 @@ static int update_working_directory(struct pattern_list *pl)
else
rollback_lock_file(&lock_file);

r->index->sparse_checkout_patterns = NULL;
return result;
}

Expand Down Expand Up @@ -276,16 +280,20 @@ static int set_config(enum sparse_checkout_mode mode)
"core.sparseCheckoutCone",
mode == MODE_CONE_PATTERNS ? "true" : NULL);

if (mode == MODE_NO_PATTERNS)
set_sparse_index_config(the_repository, 0);

return 0;
}

static char const * const builtin_sparse_checkout_init_usage[] = {
N_("git sparse-checkout init [--cone]"),
N_("git sparse-checkout init [--cone] [--[no-]sparse-index]"),
NULL
};

static struct sparse_checkout_init_opts {
int cone_mode;
int sparse_index;
} init_opts;

static int sparse_checkout_init(int argc, const char **argv)
Expand All @@ -300,11 +308,15 @@ static int sparse_checkout_init(int argc, const char **argv)
static struct option builtin_sparse_checkout_init_options[] = {
OPT_BOOL(0, "cone", &init_opts.cone_mode,
N_("initialize the sparse-checkout in cone mode")),
OPT_BOOL(0, "sparse-index", &init_opts.sparse_index,
N_("toggle the use of a sparse index")),
OPT_END(),
};

repo_read_index(the_repository);

init_opts.sparse_index = -1;

argc = parse_options(argc, argv, NULL,
builtin_sparse_checkout_init_options,
builtin_sparse_checkout_init_usage, 0);
Expand All @@ -323,10 +335,20 @@ static int sparse_checkout_init(int argc, const char **argv)
sparse_filename = get_sparse_checkout_filename();
res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL);

if (init_opts.sparse_index >= 0) {
if (set_sparse_index_config(the_repository, init_opts.sparse_index) < 0)
die(_("failed to modify sparse-index config"));

/* force an index rewrite */
repo_read_index(the_repository);
the_repository->index->updated_workdir = 1;
}

core_apply_sparse_checkout = 1;

/* If we already have a sparse-checkout file, use it. */
if (res >= 0) {
free(sparse_filename);
core_apply_sparse_checkout = 1;
return update_working_directory(NULL);
}

Expand All @@ -348,6 +370,7 @@ static int sparse_checkout_init(int argc, const char **argv)
add_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);
strbuf_addstr(&pattern, "!/*/");
add_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);
pl.use_cone_patterns = init_opts.cone_mode;

return write_patterns_and_update(&pl);
}
Expand Down Expand Up @@ -517,19 +540,18 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
{
int result;
int changed_config = 0;
struct pattern_list pl;
memset(&pl, 0, sizeof(pl));
struct pattern_list *pl = xcalloc(1, sizeof(*pl));

switch (m) {
case ADD:
if (core_sparse_checkout_cone)
add_patterns_cone_mode(argc, argv, &pl);
add_patterns_cone_mode(argc, argv, pl);
else
add_patterns_literal(argc, argv, &pl);
add_patterns_literal(argc, argv, pl);
break;

case REPLACE:
add_patterns_from_input(&pl, argc, argv);
add_patterns_from_input(pl, argc, argv);
break;
}

Expand All @@ -539,12 +561,13 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
changed_config = 1;
}

result = write_patterns_and_update(&pl);
result = write_patterns_and_update(pl);

if (result && changed_config)
set_config(MODE_NO_PATTERNS);

clear_pattern_list(&pl);
clear_pattern_list(pl);
free(pl);
return result;
}

Expand Down Expand Up @@ -614,6 +637,9 @@ static int sparse_checkout_disable(int argc, const char **argv)
strbuf_addstr(&match_all, "/*");
add_pattern(strbuf_detach(&match_all, NULL), empty_base, 0, &pl, 0);

prepare_repo_settings(the_repository);
the_repository->settings.sparse_index = 0;

if (update_working_directory(&pl))
die(_("error while refreshing working directory"));

Expand Down
Loading