Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Sparse Index] Enable index.sparse by default #388

Conversation

derrickstolee
Copy link
Collaborator

This is the last thing to do before cutting an experimental release: enable index.sparse by default. It can be disabled in a few ways:

  1. Don't use cone mode.
  2. git config index.sparse false
  3. git sparse-checkout init --cone --no-sparse-index

The third item is the one that will update the index to be full on-disk.

@derrickstolee derrickstolee self-assigned this Jun 21, 2021
There is some strangeness when expanding a sparse-index that exists
within a submodule. We will need to resolve that later, but for now,
let's do a better job of explicitly disabling the sparse-index when
requested, and do so in t7817.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
This commit is necessary becaue of the previous change that turned on
the sparse-index by default. Some cases in t1091 now enable the sparse
index and hit different corner cases around cone mode patterns.

Also silently return if there are no cache entries, which is a simple
case: there are no paths to make sparse!

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
This was added last-minute to an earlier pull request. This fixup will
apply to the upstream submission.

Using a size_t is more robust to future changes, so let's do that
now.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
This took WAY TOO LONG to figure out, but when we disable the sparse
index for a given command we can get in a state where the in-memory
cache tree is actually equal to the sparse index's cache tree, resulting
in incorrect entry_count values. By clearing the cache tree before
converting to sparse, we avoid this issue.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to microsoft/scalar that referenced this pull request Jun 23, 2021
…e merge

Something strange happens with the MergeChangesOutOfTheConeWithConflict
when run in sequence with the other tests in this class. Some edited
files from previous tests cause confusion when running 'git merge'.

For some reason, this is causing a behavior change with the
sparse-index, but it seems that either behavior would be acceptable
here.

Another thing we could do is remove the `ShouldContain("Merge conflict")`
line and the test passes that way, too. This seems like a more robust
solution.

This will resolve the last failing test in microsoft/git#388 for Linux and macOS.

There is one more strange error happening on Windows. Will investigate.
If cache_tree_update() returns a non-zero value, then it could not
create the cache tree likely due to a merge conflict. If we remove our
dependence on the cache tree within convert_to_sparse(), then we could
still recover from this scenario and have a sparse index. Since we are
already returning early, let's return silently to avoid making it seem
like we failed to write the index at all.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee marked this pull request as ready for review June 25, 2021 01:17
@derrickstolee
Copy link
Collaborator Author

@dscho @jeffhostetler I fixed the last broken test on Windows. I'm only battling the FS Monitor flakes on macOS to get green builds. Please review this on Friday so we can cut an experimental release on Monday.

dir.c Outdated Show resolved Hide resolved
Copy link

@jeffhostetler jeffhostetler left a comment

Choose a reason for hiding this comment

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

Nicely done!

@derrickstolee derrickstolee merged commit be0f3ff into microsoft:features/sparse-index Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants