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] Integrate with git status #374

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2a4a725
sparse-index: skip indexes with unmerged entries
derrickstolee Apr 29, 2021
f5bae86
sparse-index: include EXTENDED flag when expanding
derrickstolee May 4, 2021
d965669
t1092: replace incorrect 'echo' with 'cat'
derrickstolee May 26, 2021
44a9402
t1092: expand repository data shape
derrickstolee May 14, 2021
701ac0e
t1092: add tests for status/add and sparse files
derrickstolee Apr 12, 2021
587333f
unpack-trees: preserve cache_bottom
derrickstolee Apr 21, 2021
6fc898a
unpack-trees: compare sparse directories correctly
derrickstolee Apr 21, 2021
b676ef4
unpack-trees: unpack sparse directory entries
derrickstolee May 27, 2021
d693f00
dir.c: accept a directory as part of cone-mode patterns
derrickstolee Jan 11, 2021
ed11cfc
diff-lib: handle index diffs with sparse dirs
derrickstolee Jun 1, 2021
48fd25a
status: skip sparse-checkout percentage with sparse-index
derrickstolee Jan 15, 2021
3499105
status: use sparse-index throughout
derrickstolee Jan 8, 2021
60a6706
wt-status: expand added sparse directory entries
derrickstolee May 4, 2021
76bd8ec
fsmonitor: integrate with sparse index
derrickstolee Jan 15, 2021
a1a570a
Merge sparse-aware 'git status' into vfs-2.32.0
derrickstolee Jun 15, 2021
093a832
t1092: cleanups
derrickstolee Jun 17, 2021
722e7cd
fixup! unpack-trees: unpack sparse directory entries
derrickstolee Jun 17, 2021
9edbebf
fixup! unpack-trees: unpack sparse directory entries
derrickstolee Jun 17, 2021
0fda21d
fixup! t1092: expand repository data shape
derrickstolee Jun 17, 2021
610518c
fixup! unpack-trees: unpack sparse directory entries
derrickstolee Jun 17, 2021
320586f
fixup! dir.c: accept a directory as part of cone-mode patterns
derrickstolee Jun 17, 2021
ddaebb7
fixup! wt-status: expand added sparse directory entries
derrickstolee Jun 17, 2021
44bfb50
fixup! sparse-index: implement ensure_full_index()
derrickstolee Jun 17, 2021
0c20b94
fixup! fsmonitor: integrate with sparse index
derrickstolee Jun 17, 2021
f462956
fixup! fixup! t1092: expand repository data shape
derrickstolee Jun 18, 2021
87a3a29
fixup! wt-status: expand added sparse directory entries
derrickstolee Jun 21, 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
3 changes: 3 additions & 0 deletions builtin/commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -1669,6 +1669,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_status_usage, builtin_status_options);

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

status_init_config(&s, git_status_config);
argc = parse_options(argc, argv, prefix,
builtin_status_options,
Expand Down
16 changes: 16 additions & 0 deletions diff-lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,11 @@ static void show_new_file(struct rev_info *revs,
unsigned dirty_submodule = 0;
struct index_state *istate = revs->diffopt.repo->index;

derrickstolee marked this conversation as resolved.
Show resolved Hide resolved
if (new_file && S_ISSPARSEDIR(new_file->ce_mode)) {
diff_tree_oid(NULL, &new_file->oid, new_file->name, &revs->diffopt);
return;
}

/*
* New file in the index: it might actually be different in
* the working tree.
Expand All @@ -347,6 +352,17 @@ static int show_modified(struct rev_info *revs,
unsigned dirty_submodule = 0;
struct index_state *istate = revs->diffopt.repo->index;

/*
* If both are sparse directory entries, then expand the
* modifications to the file level.
*/
if (old_entry && new_entry &&
S_ISSPARSEDIR(old_entry->ce_mode) &&
S_ISSPARSEDIR(new_entry->ce_mode)) {
diff_tree_oid(&old_entry->oid, &new_entry->oid, new_entry->name, &revs->diffopt);
return 0;
}

if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,
&dirty_submodule, &revs->diffopt) < 0) {
if (report_missing)
Expand Down
17 changes: 15 additions & 2 deletions dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -1452,14 +1452,27 @@ enum pattern_match_result path_matches_pattern_list(
strbuf_addch(&parent_pathname, '/');
strbuf_add(&parent_pathname, pathname, pathlen);

/*
* Directory entries are matched if and only if a file
* contained immediately within them is matched. For the
* case of a directory entry, modify the path to create
* a fake filename within this directory, allowing us to
* use the file-base matching logic in an equivalent way.
*/
if (parent_pathname.len > 0 &&
parent_pathname.buf[parent_pathname.len - 1] == '/') {
slash_pos = parent_pathname.buf + parent_pathname.len - 1;
strbuf_add(&parent_pathname, "-", 1);
Copy link
Member

Choose a reason for hiding this comment

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

Would . make sense here instead of -?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason having a filename as . seems stranger to me than -. Perhaps _ would seem less odd?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I talked with @dscho offline here and here is the conclusion:

  • The recommendation here is clever: path/to/directory/. is semantically identical to path/to/directory, so . is a natural option here.
  • This relies on the cone-mode path-checking logic to be ignorant of this meaning of the trailing /.. While it is dumb enough to ignore that, we would be adding an expectation that it remains dumb and otherwise doesn't check that the input path is a valid name for a file.

The conclusion is that this is a clever idea, but perhaps too clever.

Choose a reason for hiding this comment

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

I'm not sure there is a correct answer here. But it makes me want to say that we want to treat the ce's as a {type,path} tuple with an accessor and all that and get away from the pattern of just iterating over a dumb list of strings (that all callers know is a dumb list of strings).

Is there existing code to detect file -vs- symlink collisions? or file -vs- submodule collisions? How are they handled?
I mean we're adding the concept of (sparse) directory nodes into the index; it would be nice if they collided the same way. (I'm not trying to cause problems, just asking dumb questions.)

} else {
slash_pos = strrchr(parent_pathname.buf, '/');
}

derrickstolee marked this conversation as resolved.
Show resolved Hide resolved
if (hashmap_contains_path(&pl->recursive_hashmap,
&parent_pathname)) {
result = MATCHED_RECURSIVE;
goto done;
}

slash_pos = strrchr(parent_pathname.buf, '/');

if (slash_pos == parent_pathname.buf) {
/* include every file in root */
result = MATCHED;
Expand Down
10 changes: 8 additions & 2 deletions read-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -1598,8 +1598,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
*/
preload_index(istate, pathspec, 0);
trace2_region_enter("index", "refresh", NULL);
/* TODO: audit for interaction with sparse-index. */
ensure_full_index(istate);

for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce, *new_entry;
int cache_errno = 0;
Expand All @@ -1614,6 +1613,13 @@ int refresh_index(struct index_state *istate, unsigned int flags,
if (ignore_skip_worktree && ce_skip_worktree(ce))
continue;

/*
* If this entry is a sparse directory, then there isn't
* any stat() information to update. Ignore the entry.
*/
if (S_ISSPARSEDIR(ce->ce_mode))
continue;

if (pathspec && !ce_path_match(istate, ce, pathspec, seen))
filtered = 1;

Expand Down
29 changes: 27 additions & 2 deletions sparse-index.c
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,17 @@ int set_sparse_index_config(struct repository *repo, int enable)
return res;
}

static int index_has_unmerged_entries(struct index_state *istate)
{
int i;
for (i = 0; i < istate->cache_nr; i++) {
if (ce_stage(istate->cache[i]))
return 1;
}

return 0;
}

int convert_to_sparse(struct index_state *istate)
{
int test_env;
Expand Down Expand Up @@ -152,6 +163,13 @@ int convert_to_sparse(struct index_state *istate)
return -1;
}

/*
* NEEDSWORK: If we have unmerged entries, then stay full.
* Unmerged entries prevent the cache-tree extension from working.
*/
if (index_has_unmerged_entries(istate))
return 0;

if (cache_tree_update(istate, 0)) {
warning(_("unable to update cache-tree, staying full"));
return -1;
Expand All @@ -168,6 +186,10 @@ int convert_to_sparse(struct index_state *istate)
cache_tree_free(&istate->cache_tree);
cache_tree_update(istate, 0);

istate->fsmonitor_has_run_once = 0;
FREE_AND_NULL(istate->fsmonitor_dirty);
FREE_AND_NULL(istate->fsmonitor_last_update);

Copy link
Member

Choose a reason for hiding this comment

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

An alternative would be to map the old extension data (which is essentially a compressed version of the fsmonitor_dirty bitmap, which we should be able to map relatively easily by iterating over the full and the sparse index simultaneously).

I do not suggest to implement this here, but maybe mention it as a potential future improvement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you recommending a translation of the old bitmap into a new bitmap based on the new path positions? Yes, that would work, but I don't think it is worth pursuing much at all. We are better off investing in the sparse-index work such that we only change the sparse directory entries during git sparse-checkout set, which is rare and likely is on a clean working directory.

istate->sparse_index = 1;
trace2_region_leave("index", "convert_to_sparse", istate->repo);
return 0;
Expand Down Expand Up @@ -195,7 +217,7 @@ static int add_path_to_index(const struct object_id *oid,
strbuf_addstr(base, path);

ce = make_cache_entry(istate, mode, oid, base->buf, 0, 0);
ce->ce_flags |= CE_SKIP_WORKTREE;
ce->ce_flags |= CE_SKIP_WORKTREE | CE_EXTENDED;
set_index_entry(istate, istate->cache_nr++, ce);

strbuf_setlen(base, len);
Expand Down Expand Up @@ -239,7 +261,7 @@ void ensure_full_index(struct index_state *istate)
warning(_("index entry is a directory, but not sparse (%08x)"),
ce->ce_flags);

/* recursively walk into cd->name */
/* recursively walk into ce->name */
derrickstolee marked this conversation as resolved.
Show resolved Hide resolved
tree = lookup_tree(istate->repo, &ce->oid);

memset(&ps, 0, sizeof(ps));
Expand All @@ -264,6 +286,9 @@ void ensure_full_index(struct index_state *istate)
istate->cache = full->cache;
istate->cache_nr = full->cache_nr;
istate->cache_alloc = full->cache_alloc;
istate->fsmonitor_has_run_once = 0;
FREE_AND_NULL(istate->fsmonitor_dirty);
FREE_AND_NULL(istate->fsmonitor_last_update);
derrickstolee marked this conversation as resolved.
Show resolved Hide resolved

strbuf_release(&base);
free(full);
Expand Down
Loading