Sparse index: checkout-index

[vdye]

Changes

• Expand testing in t1092 to cover checkout-index
• Modify the behavior of checkout-index --all to make more sense with sparse checkouts (sparse index or not)
  • Introduce --sparse flag maintaining "old" behavior
• Integrate sparse index with checkout-index, including corresponding ensure_not_expanded tests

[ldennington]

This feels a little odd. From what I understand, we generally use the --sparse flag to enable sparse behavior, but here we seem to be circumventing it. We may want to consider something more like --ignore-sparse. Let me know if I'm missing something, though!

[derrickstolee]

Check out gitgitgadget#1018 which restricts how Git operates outside of the sparse-checkout. The --sparse option allows overriding these protections. It's really new.

[vdye]

@ldennington I definitely see where you're coming from (it does seem somewhat backwards that --sparse would expand the index/do things outside the sparse checkout). I did base it on @derrickstolee's linked PR (+ls-trees), and I think this line conveys it best:

a new '--sparse' option is added that ignores the sparse-checkout patterns and the SKIP_WORKTREE bit

The option here is pretty much treating --sparse as "does things in parts of the repository that should be sparse", rather than ignoring those parts of the repository. @derrickstolee please correct me if I'm using it wrong, though - I'm mostly looking for this to be consistent with the other already-implemented options (and offer a way to use checkout-index --all without always expanding the full index).

[derrickstolee]

@vdye: I think you have the right approach. We should make Git ignore files outside of the sparse-checkout cone as much as possible, and allow users to override that behavior by specifying --sparse. We do not expect users to have a legitimate reason to care about files outside of the sparse-checkout, but Git needs to care unless we make these changes.

[derrickstolee]

Good progress. I have a few questions.

[derrickstolee]

Good catch! Scary that this could completely abolish all performance benefits for a sparse-checkout!

[derrickstolee]

Is it possible to make both implementations say "file is outside the sparse-checkout definition"?

• The "file does not appear in the index" is not entirely correct for full indexes.
• The "file is a sparse directory" has a bit too much internal information.

I wonder if using if (!path_in_sparse_checkout(name, &the_index)) somewhere would help here.

It's perhaps a bit tricky how this loop is structured, since it is doing a prefix match on a range of index entries.

In this test script, what happens with this example?

git sparse-checkout set deep/deeper1
git checkout-index -f -- deep/

Note that deep/deeper2/ is a sparse directory within the deep/ directory, but we would still want the other entries to succeed, silently ignoring the deep/deeper2/ entry. This would only be an error if the user ran git checkout-index -f -- deep/deeper2/, right?

[vdye]

I think it's just comparing the names, not performing a prefix match (the first condition in this check makes sure the names are the same length):

if (ce_namelen(ce) != namelen ||
		    memcmp(ce->name, name, namelen))

The result is that, from your example, git checkout-index -f -- deep/ (or any path that doesn't refer to a file entry in the index) returns git checkout-index: <name> is not in the cache. It seems like this command expects exact names of cache entries (except for the subcommands using a pathspec, but they handle things differently anyway), in which case <name> is not in the cache makes some amount of sense? Agreed that it's misleading, though - would that be something that should be clarified in the command doc, or in this message itself?

As for checking whether something is in the sparse checkout, this doesn't actually block checking out files outside the sparse definition (you can successfully run git checkout-index -- folder1/a without any other repo setup). The issue is that sparse directories do appear as standalone cache entries, so without the S_ISSPARSEDIR check the entry would be passed along to checkout_entry (which then fails because it doesn't handle directories). Since there's no support checkout-index-ing directories otherwise, this probably needs to be blocked rather than worked around. Maybe the message should be the same as whenever anyone tries to checkout-index a directory in general? I.e., "lie" and say <name> is not in the cache, or some other error message that's the same between them?

[derrickstolee]

Why shouldn't these be combined? What if I want to poke at a file outside of my sparse-checkout? It certainly seems like git checkout-index --sparse -- deep/deeper2/a would be a good way to get that file on-disk.

[vdye]

The reason I didn't include --sparse for individual files is because, as it is right now, individually-specified files will always expand the index if that specified file is not already in the index - I wasn't really sure what the "right" way to handle it was, though. On one hand, not requiring a --sparse flag to checkout-index individual files (even those outside the checkout definition) is the most consistent with existing behavior. On the other hand, it is kind of weird that we need a special --sparse option for files in sparse directories for --all, but not when you list out individual files.

What do you think would be better (especially based on how you've handled similar situations in other commands)?

[derrickstolee]

Since this is a plumbing command, let's be cautious and avoid the behavior break by requiring --sparse for individual files. (This is a change of opinion from my earlier comment.)

[derrickstolee]

All of my prior concerns were misconceptions about how git checkout-index works. Thanks for clearing them up!

[vdye]

@derrickstolee + @ldennington I added one last change (performance test for git checkout-index -f --all), which had these results (tip of vfs-2.33.0 vs the tip of this branch):

Test                                                   ms/vfs-2.33.0       HEAD                  
-------------------------------------------------------------------------------------------------
2000.30: git checkout-index -f --all (full-v3)         76.79(9.89+64.59)   0.52(0.22+0.26) -99.3%
2000.31: git checkout-index -f --all (full-v4)         80.96(9.65+63.89)   0.52(0.21+0.26) -99.4%
2000.32: git checkout-index -f --all (sparse-v3)       89.43(9.83+64.99)   0.34(0.05+0.24) -99.6%
2000.33: git checkout-index -f --all (sparse-v4)       93.67(9.65+63.46)   0.34(0.05+0.25) -99.6%

The huge speedup seems to mostly come from having checkout-index --all respect the sparse checkout definition, but there is also improvement from full index to sparse index (which is nice to see)!

[derrickstolee]

@derrickstolee + @ldennington I added one last change (performance test for git checkout-index -f --all), which had these results (tip of vfs-2.33.0 vs the tip of this branch):

Test                                                   ms/vfs-2.33.0       HEAD                  
-------------------------------------------------------------------------------------------------
2000.30: git checkout-index -f --all (full-v3)         76.79(9.89+64.59)   0.52(0.22+0.26) -99.3%
2000.31: git checkout-index -f --all (full-v4)         80.96(9.65+63.89)   0.52(0.21+0.26) -99.4%
2000.32: git checkout-index -f --all (sparse-v3)       89.43(9.83+64.99)   0.34(0.05+0.24) -99.6%
2000.33: git checkout-index -f --all (sparse-v4)       93.67(9.65+63.46)   0.34(0.05+0.25) -99.6%

The huge speedup seems to mostly come from having checkout-index --all respect the sparse checkout definition, but there is also improvement from full index to sparse index (which is nice to see)!

Yeah! Those 75-90s times are definitely from writing too much data to the working tree. It's nice to see exactly why this is such an important change to include and why users would not like that old behavior!