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 update-index #423

Merged
merged 5 commits into from
Sep 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 15 additions & 3 deletions builtin/update-index.c
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,9 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
if (!verify_path(path, mode))
return error("Invalid path '%s'", path);

if (S_ISSPARSEDIR(mode))
return error("%s: cannot add directory as cache entry", path);

len = strlen(path);
ce = make_empty_cache_entry(&the_index, len);

Expand Down Expand Up @@ -745,17 +748,23 @@ static int do_reupdate(int ac, const char **av,
* commit. Update everything in the index.
*/
has_head = 0;

redo:
/* TODO: audit for interaction with sparse-index. */
ensure_full_index(&the_index);
for (pos = 0; pos < active_nr; pos++) {
const struct cache_entry *ce = active_cache[pos];
struct cache_entry *old = NULL;
int save_nr;
char *path;

if (ce_stage(ce) || !ce_path_match(&the_index, ce, &pathspec, NULL))
/*
* We can safely skip re-updating sparse directories because if there
* were any changes to re-update inside of the sparse directory, it
* would not be sparse.
*/
if (S_ISSPARSEDIR(ce->ce_mode) || ce_stage(ce) ||
!ce_path_match(&the_index, ce, &pathspec, NULL))
continue;

Comment on lines +759 to +767
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have tests for this re-update mode when the file actually exists outside of the sparse-checkout cone? For example, a user might manually write the file to disk. The change you are making here might be a reasonable choice, regardless, but it might also be a behavior change that we'd want to document with a tests.

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've pushed an update to the test that verifies update-index --again - it now explicitly tests:

  • file manually added and modified outside sparse definition (without skip-worktree disabled)
    • result: no-op (update-index --again only operates on staged changes in the index)
  • file manually added outside sparse definition (by disabling skip-worktree on that file) and modified
    • in all checkouts, file is re-updated
  • file manually removed after disabling skip-worktree
    • in all checkouts, file cannot be removed without --remove

if (has_head)
old = read_one_ent(NULL, &head_oid,
ce->name, ce_namelen(ce), 0);
Expand Down Expand Up @@ -1080,6 +1089,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)

git_config(git_default_config, NULL);

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

/* we will diagnose later if it turns out that we need to update it */
newfd = hold_locked_index(&lock_file, 0);
if (newfd < 0)
Expand Down
10 changes: 7 additions & 3 deletions read-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -1324,9 +1324,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
int new_only = option & ADD_CACHE_NEW_ONLY;

if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
cache_tree_invalidate_path(istate, ce->name);

/*
* If this entry's path sorts after the last entry in the index,
* we can avoid searching for it.
Expand All @@ -1337,6 +1334,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
else
pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));

/*
* Cache tree path should be invalidated only after index_name_stage_pos,
* in case it expands a sparse index.
*/
if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
cache_tree_invalidate_path(istate, ce->name);
ldennington marked this conversation as resolved.
Show resolved Hide resolved

/* existing match? Just replace it. */
if (pos >= 0) {
if (!new_only)
Expand Down
165 changes: 165 additions & 0 deletions t/t1092-sparse-checkout-compatibility.sh
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,156 @@ test_expect_success 'reset with wildcard pathspec' '
test_all_match git status --porcelain=v2
'

# NEEDSWORK: although update-index executes without error on files outside
# the sparse checkout definition, it does not actually add the file to the
# index. This is also true when "--no-ignore-skip-worktree-entries" is
# specified.
test_expect_success 'update-index add outside sparse definition' '
init_repos &&

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

run_on_sparse mkdir -p folder1 &&
run_on_sparse cp ../initial-repo/folder1/a folder1/a &&

# Edit the file only in sparse checkouts so that, when checking the status
# of the index, the unmodified full-checkout is compared to the "modified"
# sparse checkouts.
run_on_sparse ../edit-contents folder1/a &&

test_sparse_match git update-index --add folder1/a &&
test_all_match git status --porcelain=v2 &&
test_sparse_match git update-index --add --no-ignore-skip-worktree-entries folder1/a &&
test_all_match git status --porcelain=v2
'

test_expect_success 'update-index remove outside sparse definition' '
init_repos &&

# When --remove is specified, files outside the sparse checkout definition
# are considered "removed".
rm -f full-checkout/folder1/a &&
test_all_match git update-index --remove folder1/a &&
test_all_match git status --porcelain=v2 &&

git reset --hard &&

# When --ignore-skip-worktree-entries is explicitly specified, a file
# outside the sparse definition is not added to the index as "removed"
# (thus matching the unmodified full-checkout).
test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
test_all_match git status --porcelain=v2 &&

git reset --hard &&

# --force-remove supercedes --ignore-skip-worktree-entries and always
# removes the file from the index.
test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
test_all_match git status --porcelain=v2
'

test_expect_success 'update-index folder add/remove' '
init_repos &&

test_all_match test_must_fail git update-index --add --remove deep &&
ldennington marked this conversation as resolved.
Show resolved Hide resolved
test_all_match test_must_fail git update-index --add --remove deep/ &&

# NEEDSWORK: attempting to update-index on an existing folder outside the
# sparse checkout definition does not throw an error (as it does for folders
# inside the definition, or in the full checkout). However, it is a no-op.
test_sparse_match git update-index --add --remove folder1 &&
test_sparse_match git update-index --add --remove folder1/ &&
test_sparse_match git update-index --force-remove folder1/ &&
test_all_match git status --porcelain=v2 &&

# New folders, even in sparse checkouts, throw an error on update-index
run_on_all mkdir folder3 &&
run_on_all cp a folder3/a &&
run_on_all test_must_fail git update-index --add --remove folder3
'

test_expect_success 'update-index with updated flags' '
init_repos &&

# NEEDSWORK: updating flags runs inconsistently on directories, performing no
# operation with warning text specifying the path being ignored if a trailing
# slash is in the path, but throwing an error if there is no trailing slash.
test_all_match test_must_fail git update-index --no-skip-worktree folder1 &&
test_all_match git update-index --no-skip-worktree folder1/ &&
test_all_match git status --porcelain=v2 &&

# Removing the skip-worktree bit from a file outside the sparse checkout
# will cause the file to appear as unstaged and deleted.
test_sparse_match git update-index --no-skip-worktree folder1/a &&
rm -f full-checkout/folder1/a &&
test_all_match git status --porcelain=v2
'

test_expect_success 'update-index --again file outside sparse definition' '
init_repos &&

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

# When a file is manually added and modified outside the checkout
# definition, it will not be changed with `--again` because its changes are
# not tracked in the index.
run_on_sparse mkdir -p folder1 &&
run_on_sparse ../edit-contents folder1/a &&
test_sparse_match git update-index --again &&
test_sparse_match git status --porcelain=v2 &&

# Update the sparse checkouts so that folder1/a is no longer skipped and
# exists on-disk
run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
test_sparse_match git update-index --no-skip-worktree folder1/a &&
test_all_match git status --porcelain=v2 &&

# Stage change for commit
run_on_all ../edit-contents folder1/a &&
test_all_match git update-index folder1/a &&
test_all_match git status --porcelain=v2 &&

# Modify the file
run_on_all ../edit-contents folder1/a &&
test_all_match git status --porcelain=v2 &&

# Run update-index --again, which re-stages the local changes
test_all_match git update-index --again &&
test_all_match git ls-files -s folder1/a &&
test_all_match git status --porcelain=v2 &&

# Running update-index --again with staged changes after manually deleting
# the file on disk will cause it to fail if --remove is not also specified
run_on_all rm -f folder1/a &&
test_all_match test_must_fail git update-index --again folder1 &&
test_all_match git update-index --remove --again &&
test_all_match git status --porcelain=v2
'

test_expect_success 'update-index --cacheinfo' '
init_repos &&

deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&

test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
test_all_match git status --porcelain=v2 &&

# Cannot add sparse directory, even in sparse index case
test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&

# Sparse match only - because folder1/a is outside the sparse checkout
# definition (and thus not on-disk), it will appear as "deleted" in
# unstaged changes.
test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
test_sparse_match git status --porcelain=v2
'

test_expect_success 'merge, cherry-pick, and rebase' '
init_repos &&

Expand Down Expand Up @@ -868,6 +1018,21 @@ test_expect_success 'sparse index is not expanded: sparse-checkout' '
ensure_not_expanded sparse-checkout set
'

test_expect_success 'sparse index is not expanded: update-index' '
init_repos &&

echo "test" >sparse-index/README.md &&
echo "test2" >sparse-index/a &&
rm -f sparse-index/deep/a &&

ensure_not_expanded update-index --add README.md &&
ensure_not_expanded update-index a &&
ensure_not_expanded update-index --remove deep/a &&

rm -f sparse-index/README.md sparse-index/a &&
ensure_not_expanded update-index --add --remove --again
'
derrickstolee marked this conversation as resolved.
Show resolved Hide resolved

# 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
8 changes: 8 additions & 0 deletions t/t2107-update-index-basic.sh
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '
test_cmp expect actual
'

test_expect_success '--cacheinfo does not accept directory mode' '
mkdir folder1 &&
echo content >folder1/content &&
git add folder1 &&
folder1_oid=$(git ls-files -s folder1 | git hash-object --stdin) &&
test_must_fail git update-index --add --cacheinfo 040000 $folder1_oid folder1/
'

test_expect_success '.lock files cleaned up' '
mkdir cleanup &&
(
Expand Down