Skip to content

rm/sparse-index-integration-v1.1 #7

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

Open
ffyuanda wants to merge 5 commits into
base: rm/sparse-index-integration
Choose a base branch
from
Open

rm/sparse-index-integration-v1.1 #7

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a50c772
t1092: add tests for `git-rm`
ffyuanda Aug 2, 2022
f5534dd
pathspec.h: move pathspec_needs_expanded_index() from reset.c to here
ffyuanda Jul 31, 2022
5921ac2
rm: expand the index only when necessary
ffyuanda Jul 31, 2022
8d6fc06
t1092: normalize a behavioral difference of `git-rm` under sparse-index
ffyuanda Aug 1, 2022
e6ec4ca
rm: integrate with sparse-index
ffyuanda Jul 31, 2022
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
4 changes: 4 additions & 0 deletions builtin/rm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
char *seen;

git_config(git_default_config, NULL);
if (the_repository->gitdir) {
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
}
Comment on lines +265 to +268

@vdye vdye Aug 3, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can avoid the need for if (the_repository->gitdir) - which is when the index is needed before parse_options() is called (link) - by putting the prepare_repo_settings() immediately before the index is accessed. In this case, I think that's right before the hold_locked_index() call (line 294).

@ffyuanda ffyuanda Aug 3, 2022

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I'm slightly suspicious here.

You can avoid the need for if (the_repository->gitdir) - which is when the index is needed before parse_options() is called (link)

In this commit they actually use if (the_repository->gitdir) as the way to go:

However, it is unfortunately not that simple. In cmd_pack_objects(),
for example, the repo settings need to be fully populated so that the
command-line options --sparse/--no-sparse can override them, not the
other way round.

Therefore, we choose to imitate the strategy taken in cmd_diff(),
where we simply do not bother to prepare and initialize the repo
settings unless we have a gitdir.

Though I'm following your advice right now because it does make sense to me ;-)


argc = parse_options(argc, argv, prefix, builtin_rm_options,
builtin_rm_usage, 0);
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 @@ -123,5 +123,6 @@ test_perf_on_all git blame $SPARSE_CONE/f3/a
test_perf_on_all git read-tree -mu HEAD
test_perf_on_all git checkout-index -f --all
test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
test_perf_on_all git rm -f $SPARSE_CONE/a

test_done
4 changes: 2 additions & 2 deletions t/t1092-sparse-checkout-compatibility.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1870,7 +1870,7 @@ test_expect_success 'rm pathspec inside sparse definition' '
test_all_match git status --porcelain=v2
'

test_expect_failure 'rm pathspec outside sparse definition' '
test_expect_success 'rm pathspec outside sparse definition' '
init_repos &&

for file in folder1/a folder1/0/1
Expand Down Expand Up @@ -1910,7 +1910,7 @@ test_expect_failure 'rm pathspec outside sparse definition' '
test_sparse_match git status --porcelain=v2
'

test_expect_failure 'sparse index is not expanded: rm' '
test_expect_success 'sparse index is not expanded: rm' '
init_repos &&

ensure_not_expanded rm deep/a &&
Expand Down