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.

Sign up for GitHub

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 clean and stash -u #430

Merged
merged 3 commits into from
Sep 23, 2021
Merged

Sparse index: integrate with clean and stash -u #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43d600e
fixup! checkout-index: add `--sparse` option
vdye Sep 22, 2021
626751a
clean: integrate with sparse index
vdye Sep 22, 2021
0c06339
stash: expand testing for `git stash -u`
vdye Sep 22, 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/clean.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
dir.flags |= DIR_KEEP_UNTRACKED_CONTENTS;
}

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

if (read_cache() < 0)
die(_("index file corrupt"));
enable_fscache(active_nr);
Expand Down
2 changes: 1 addition & 1 deletion builtin/stash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ static int restore_untracked(struct object_id *u_tree)

child_process_init(&cp);
cp.git_cmd = 1;
strvec_pushl(&cp.args, "checkout-index", "--all", NULL);
strvec_pushl(&cp.args, "checkout-index", "--all", "--sparse", NULL);
strvec_pushf(&cp.env_array, "GIT_INDEX_FILE=%s",
stash_index_path.buf);

Expand Down
1 change: 1 addition & 0 deletions t/perf/p2000-sparse-operations.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ test_perf_on_all () {

test_perf_on_all git status
test_perf_on_all 'git stash && git stash pop'
test_perf_on_all 'echo >>new && git stash -u && git stash pop'
test_perf_on_all git add -A
test_perf_on_all git add .
test_perf_on_all git commit -a -m A
Expand Down
61 changes: 61 additions & 0 deletions t/t1092-sparse-checkout-compatibility.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,23 +1068,42 @@ test_expect_success 'clean' '
test_all_match git commit -m "ignore bogus files" &&

run_on_sparse mkdir folder1 &&
run_on_all mkdir -p deep/untracked-deep &&
run_on_all touch folder1/bogus &&
run_on_all touch folder1/untracked &&
run_on_all touch deep/untracked-deep/bogus &&
run_on_all touch deep/untracked-deep/untracked &&

test_all_match git status --porcelain=v2 &&
test_all_match git clean -f &&
test_all_match git status --porcelain=v2 &&
test_sparse_match ls &&
test_sparse_match ls folder1 &&
run_on_all test_path_exists folder1/bogus &&
run_on_all test_path_is_missing folder1/untracked &&
run_on_all test_path_exists deep/untracked-deep/bogus &&
run_on_all test_path_exists deep/untracked-deep/untracked &&

test_all_match git clean -fd &&
test_all_match git status --porcelain=v2 &&
test_sparse_match ls &&
test_sparse_match ls folder1 &&
run_on_all test_path_exists folder1/bogus &&
run_on_all test_path_exists deep/untracked-deep/bogus &&
run_on_all test_path_is_missing deep/untracked-deep/untracked &&

test_all_match git clean -xf &&
test_all_match git status --porcelain=v2 &&
test_sparse_match ls &&
test_sparse_match ls folder1 &&
run_on_all test_path_is_missing folder1/bogus &&
run_on_all test_path_exists deep/untracked-deep/bogus &&

test_all_match git clean -xdf &&
test_all_match git status --porcelain=v2 &&
test_sparse_match ls &&
test_sparse_match ls folder1 &&
run_on_all test_path_is_missing deep/untracked-deep/bogus &&

test_sparse_match test_path_is_dir folder1
'
Expand Down Expand Up @@ -1202,6 +1221,8 @@ test_expect_success 'sparse-index is not expanded' '
git -C sparse-index add README.md &&
ensure_not_expanded diff --staged &&

ensure_not_expanded clean -fd &&

Comment on lines +1224 to +1225
Copy link

Choose a reason for hiding this comment

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

Could you add a test that checks that git clean -fd correctly removes untracked files in sparse directories?

something like:

git sparse-checkout set deep/deeper1 &&
mkdir deep/deeper2 folder1 &&
touch deep/deeper2/untracked &&
touch folder1/untracked &&
git clean -fd &&
test_path_is_missing deep/deeper2/untracked &&
test_path_is_missing folder1/untracked

I expect that the index will be expanded to do this operation, but let's make sure it happens.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the existing git clean tests to check those cases and explicitly verify paths are present or missing. Also, I verified that the index is expanded when untracked files are inside a sparse directory, so no tests were added to the ensure_not_expanded test.

ensure_not_expanded checkout -f update-deep &&
(
sane_unset GIT_TEST_MERGE_ALGORITHM &&
Expand Down Expand Up @@ -1280,6 +1301,46 @@ test_expect_success 'sparse index is not expanded: read-tree' '
ensure_not_expanded read-tree --prefix=deep/deeper2 -u deepest
'

# NEEDSWORK: although the full repository's index is _not_ expanded as part of
# stash, a temporary index, which is _not_ sparse, is created when stashing and
# applying a stash of untracked files. As a result, the test reports that it
# finds an instance of `ensure_full_index`, but it does not carry with it the
# performance implications of expanding the full repository index.
Comment on lines +1304 to +1308
Copy link

Choose a reason for hiding this comment

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

This is a good explanation of the problem. I wonder if there is something we can do to make creating this temporary index as a sparse one be better. Do you have a pointer to the place in the code that creates this index?

Copy link
Author

@vdye vdye Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It comes from setting the GIT_INDEX_FILE in update-index (1, 2) and read-tree/checkout-index (1, 2) to temporary filename while either constructing the stash or applying it.

When creating the stash (with untracked entries included) update-index will be invoked while the temporary index file does not exist, causing it to enter this block in do_read_index - because it doesn't ever read the existing index, the sparse_index is never set and it's treated as a full index. I tried adding a check to set sparse_index based on the sparse_index repo setting and/or command_requires_full_index, but I hit some segfaults when testing, so it needed a closer look.

The more complicated case is on the unstashing side - read-tree also gets a non-existent index file, but it never actually reads in the index before writing out the result of unpack_trees (so I tried adding in do_read_index is never reached). In that case, the sparse_index flag is only ever set if a sparse directory is written to it, which (I think) won't happen when unpacking a tree without comparing to the index.

All that to say - I agree that there's more that can be done to preserve "sparseness" of the temp index. I think complications come up when dealing with untracked files in sparse directories (likely part of what caused the segfaults in my earlier testing), but I don't see it being impossible to handle.

test_expect_success 'sparse index is not expanded: stash -u' '
init_repos &&

mkdir -p sparse-index/folder1 &&
echo >>sparse-index/README.md &&
echo >>sparse-index/a &&
echo >>sparse-index/folder1/new &&

GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
git -C sparse-index stash -u &&
test_region index ensure_full_index trace2.txt &&

GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
git -C sparse-index stash pop &&
test_region index ensure_full_index trace2.txt
'

# NEEDSWORK: similar to `git add`, untracked files outside of the sparse
# checkout definition are successfully stashed and unstashed.
test_expect_success 'stash -u outside sparse checkout definition' '
init_repos &&

write_script edit-contents <<-\EOF &&
echo text >>$1
EOF

run_on_sparse mkdir -p folder1 &&
run_on_all ../edit-contents folder1/new &&
test_all_match git stash -u &&
test_all_match git status --porcelain=v2 &&

test_all_match git stash pop -q &&
test_all_match git status --porcelain=v2
'

# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
# in this scenario, but it shouldn't.
test_expect_success 'reset mixed and checkout orphan' '
Expand Down