| Message ID | 02f2c7b63982cfbafc300e1cd901473d5b9b7297.1623069253.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse-index: integrate with status | expand |
On Mon, Jun 7, 2021 at 5:34 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > It is difficult, but possible, to get into a state where we intend to > add a directory that is outside of the sparse-checkout definition. Add a > test to t1092-sparse-checkout-compatibility.sh that demonstrates this > using a combination of 'git reset --mixed' and 'git checkout --orphan'. > > This test failed before because the output of 'git status > --porcelain=v2' would not match on the lines for folder1/: > > * The sparse-checkout repo (with a full index) would output each path > name that is intended to be added. > > * The sparse-index repo would only output that "folder1/" is staged for > addition. > > The status should report the full list of files to be added, and so this > sparse-directory entry should be expanded to a full list when reaching > it inside the wt_status_collect_changes_initial() method. Use > read_tree_at() to assist. > > Somehow, this loop over the cache entries was not guarded by > ensure_full_index() as intended. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > t/t1092-sparse-checkout-compatibility.sh | 36 +++++++++++++++++ > wt-status.c | 50 ++++++++++++++++++++++++ > 2 files changed, 86 insertions(+) > > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 099dc2bf440f..39b86fbe2be6 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -521,4 +521,40 @@ test_expect_success 'sparse-index is not expanded' ' > test_region ! index ensure_full_index trace2.txt > ' > > +test_expect_success 'reset mixed and checkout orphan' ' > + init_repos && > + > + test_all_match git checkout rename-out-to-in && > + > + # Sparse checkouts do not agree with full checkouts about > + # how to report a directory/file conflict during a reset. > + # This command would fail with test_all_match because the > + # full checkout reports "T folder1/0/1" while a sparse > + # checkout reports "D folder1/0/1". This matches because > + # the sparse checkouts skip "adding" the other side of > + # the conflict. > + test_sparse_match git reset --mixed HEAD~1 && Ooh! I think you found a sparse-checkout bug here. I agree that sparse-checkouts and full-checkouts should give different output in this case, but I don't think the current difference is the correct one. Digging in a little closer, before running `git reset --mixed HEAD~1` I see: $ git ls-files -t | grep folder S folder1/0/0/0 S folder1/0/1 S folder2/0/0/0 S folder2/0/1/1 S folder2/a S folder2/larger-content and after running git reset --mixed HEAD~1, I see: S folder1/0/0/0 S folder1/0/1 H folder1/a H folder1/larger-content S folder2/0/0/0 H folder2/0/1 S folder2/a S folder2/larger-content meaning that the reset of the index failed. It thinks some entries are present in the working copy, though it didn't actually check any of them out, leaving them to be marked as deleted. This leaves the sparse-checkout in a messed up state. To correct it, I need to run either of the following: git diff --diff-filter=D --name-only | xargs git update-index --skip-worktree or git sparse-checkout reapply (Though one could ask whether sparse-checkout reapply should take a missing file that isn't SKIP_WORKTREE and determine it's okay to just mark it as SKIP_WORKTREE rather than treating it as dirty. I'm not sure the answer to that...) I really think that `git reset --mixed ...` should have been getting the sparsity right on its own without the manual fixup afterwards that I needed to add. > + test_sparse_match test-tool read-cache --table --expand && If both the full and the sparse checkouts do a reset --mixed, I would think that this step should be able to use a test_all_match...at least if reset --mixed weren't broken. > + test_sparse_match git status --porcelain=v2 && > + test_sparse_match git status --porcelain=v2 && Why is this test run twice? > + > + # At this point, sparse-checkouts behave differently > + # from the full-checkout. > + test_sparse_match git checkout --orphan new-branch && > + test_sparse_match test-tool read-cache --table --expand && > + test_sparse_match git status --porcelain=v2 && > + test_sparse_match git status --porcelain=v2 And again, you run the status twice...why? > +' > + > +test_expect_success 'add everything with deep new file' ' > + init_repos && > + > + run_on_sparse git sparse-checkout set deep/deeper1/deepest && > + > + run_on_all touch deep/deeper1/x && > + test_all_match git add . && > + test_all_match git status --porcelain=v2 && > + test_all_match git status --porcelain=v2 same question. > +' > + > test_done > diff --git a/wt-status.c b/wt-status.c > index 0425169c1895..90db8bd659fa 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -654,6 +654,34 @@ static void wt_status_collect_changes_index(struct wt_status *s) > run_diff_index(&rev, 1); > } > > +static int add_file_to_list(const struct object_id *oid, > + struct strbuf *base, const char *path, > + unsigned int mode, void *context) > +{ > + struct string_list_item *it; > + struct wt_status_change_data *d; > + struct wt_status *s = context; > + char *full_name; > + > + if (S_ISDIR(mode)) > + return READ_TREE_RECURSIVE; > + > + full_name = xstrfmt("%s%s", base->buf, path); > + it = string_list_insert(&s->change, full_name); > + d = it->util; > + if (!d) { > + CALLOC_ARRAY(d, 1); > + it->util = d; > + } > + > + d->index_status = DIFF_STATUS_ADDED; > + /* Leave {mode,oid}_head zero for adds. */ > + d->mode_index = mode; > + oidcpy(&d->oid_index, oid); > + s->committable = 1; > + return 0; > +} > + > static void wt_status_collect_changes_initial(struct wt_status *s) > { > struct index_state *istate = s->repo->index; > @@ -668,6 +696,28 @@ static void wt_status_collect_changes_initial(struct wt_status *s) > continue; > if (ce_intent_to_add(ce)) > continue; > + if (S_ISSPARSEDIR(ce->ce_mode)) { > + /* > + * This is a sparse directory entry, so we want to collect all > + * of the added files within the tree. This requires recursively > + * expanding the trees to find the elements that are new in this > + * tree and marking them with DIFF_STATUS_ADDED. > + */ > + struct strbuf base = STRBUF_INIT; > + struct pathspec ps; > + struct tree *tree = lookup_tree(istate->repo, &ce->oid); > + > + memset(&ps, 0, sizeof(ps)); > + ps.recursive = 1; > + ps.has_wildcard = 1; > + ps.max_depth = -1; > + > + strbuf_add(&base, ce->name, ce->ce_namelen); > + read_tree_at(istate->repo, tree, &base, &ps, > + add_file_to_list, s); > + continue; > + } > + > it = string_list_insert(&s->change, ce->name); > d = it->util; > if (!d) { > -- > gitgitgadget >
On 6/9/2021 1:27 AM, Elijah Newren wrote: > On Mon, Jun 7, 2021 at 5:34 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: ... >> +test_expect_success 'reset mixed and checkout orphan' ' >> + init_repos && >> + >> + test_all_match git checkout rename-out-to-in && >> + >> + # Sparse checkouts do not agree with full checkouts about >> + # how to report a directory/file conflict during a reset. >> + # This command would fail with test_all_match because the >> + # full checkout reports "T folder1/0/1" while a sparse >> + # checkout reports "D folder1/0/1". This matches because >> + # the sparse checkouts skip "adding" the other side of >> + # the conflict. >> + test_sparse_match git reset --mixed HEAD~1 && > > Ooh! I think you found a sparse-checkout bug here. I agree that > sparse-checkouts and full-checkouts should give different output in > this case, but I don't think the current difference is the correct > one. Digging in a little closer, before running `git reset --mixed > HEAD~1` I see: > > $ git ls-files -t | grep folder > S folder1/0/0/0 > S folder1/0/1 > S folder2/0/0/0 > S folder2/0/1/1 > S folder2/a > S folder2/larger-content > > and after running git reset --mixed HEAD~1, I see: > S folder1/0/0/0 > S folder1/0/1 > H folder1/a > H folder1/larger-content > S folder2/0/0/0 > H folder2/0/1 > S folder2/a > S folder2/larger-content > > meaning that the reset of the index failed. It thinks some entries > are present in the working copy, though it didn't actually check any > of them out, leaving them to be marked as deleted. This leaves the > sparse-checkout in a messed up state. To correct it, I need to run > either of the following: > > git diff --diff-filter=D --name-only | xargs git update-index > --skip-worktree > > or > > git sparse-checkout reapply > > (Though one could ask whether sparse-checkout reapply should take a > missing file that isn't SKIP_WORKTREE and determine it's okay to just > mark it as SKIP_WORKTREE rather than treating it as dirty. I'm not > sure the answer to that...) > > I really think that `git reset --mixed ...` should have been getting > the sparsity right on its own without the manual fixup afterwards that > I needed to add. > >> + test_sparse_match test-tool read-cache --table --expand && > > If both the full and the sparse checkouts do a reset --mixed, I would > think that this step should be able to use a test_all_match...at least > if reset --mixed weren't broken. I will add this to my list when getting to 'git reset' integration with sparse-checkout. Thanks. >> + test_sparse_match git status --porcelain=v2 && >> + test_sparse_match git status --porcelain=v2 && > > Why is this test run twice? > >> + >> + # At this point, sparse-checkouts behave differently >> + # from the full-checkout. >> + test_sparse_match git checkout --orphan new-branch && >> + test_sparse_match test-tool read-cache --table --expand && >> + test_sparse_match git status --porcelain=v2 && >> + test_sparse_match git status --porcelain=v2 > > And again, you run the status twice...why? > >> +' >> + >> +test_expect_success 'add everything with deep new file' ' >> + init_repos && >> + >> + run_on_sparse git sparse-checkout set deep/deeper1/deepest && >> + >> + run_on_all touch deep/deeper1/x && >> + test_all_match git add . && >> + test_all_match git status --porcelain=v2 && >> + test_all_match git status --porcelain=v2 > > same question. These double 'git status' calls are actually a bit subtle: there was a bug in an earlier version that only appeared when using 'git status' twice, because the first kept the sparse index without expanding it, and the bug actually had an incorrect result when writing that index. Only the second 'git status' would notice the problem. I started adding two calls to my tests, but it is not necessary any more. The reason to leave it out of the Git tests is that I'm testing all of my submissions against the Scalar functional tests which run 'git status' multiple times throughout each test situation and that catches the problem as well. In the future, we will have 'git add' keeping the sparse index in-memory; that will also expose this behavior sufficiently. Thanks, -Stolee
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 099dc2bf440f..39b86fbe2be6 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -521,4 +521,40 @@ test_expect_success 'sparse-index is not expanded' ' test_region ! index ensure_full_index trace2.txt ' +test_expect_success 'reset mixed and checkout orphan' ' + init_repos && + + test_all_match git checkout rename-out-to-in && + + # Sparse checkouts do not agree with full checkouts about + # how to report a directory/file conflict during a reset. + # This command would fail with test_all_match because the + # full checkout reports "T folder1/0/1" while a sparse + # checkout reports "D folder1/0/1". This matches because + # the sparse checkouts skip "adding" the other side of + # the conflict. + test_sparse_match git reset --mixed HEAD~1 && + test_sparse_match test-tool read-cache --table --expand && + test_sparse_match git status --porcelain=v2 && + test_sparse_match git status --porcelain=v2 && + + # At this point, sparse-checkouts behave differently + # from the full-checkout. + test_sparse_match git checkout --orphan new-branch && + test_sparse_match test-tool read-cache --table --expand && + test_sparse_match git status --porcelain=v2 && + test_sparse_match git status --porcelain=v2 +' + +test_expect_success 'add everything with deep new file' ' + init_repos && + + run_on_sparse git sparse-checkout set deep/deeper1/deepest && + + run_on_all touch deep/deeper1/x && + test_all_match git add . && + test_all_match git status --porcelain=v2 && + test_all_match git status --porcelain=v2 +' + test_done diff --git a/wt-status.c b/wt-status.c index 0425169c1895..90db8bd659fa 100644 --- a/wt-status.c +++ b/wt-status.c @@ -654,6 +654,34 @@ static void wt_status_collect_changes_index(struct wt_status *s) run_diff_index(&rev, 1); } +static int add_file_to_list(const struct object_id *oid, + struct strbuf *base, const char *path, + unsigned int mode, void *context) +{ + struct string_list_item *it; + struct wt_status_change_data *d; + struct wt_status *s = context; + char *full_name; + + if (S_ISDIR(mode)) + return READ_TREE_RECURSIVE; + + full_name = xstrfmt("%s%s", base->buf, path); + it = string_list_insert(&s->change, full_name); + d = it->util; + if (!d) { + CALLOC_ARRAY(d, 1); + it->util = d; + } + + d->index_status = DIFF_STATUS_ADDED; + /* Leave {mode,oid}_head zero for adds. */ + d->mode_index = mode; + oidcpy(&d->oid_index, oid); + s->committable = 1; + return 0; +} + static void wt_status_collect_changes_initial(struct wt_status *s) { struct index_state *istate = s->repo->index; @@ -668,6 +696,28 @@ static void wt_status_collect_changes_initial(struct wt_status *s) continue; if (ce_intent_to_add(ce)) continue; + if (S_ISSPARSEDIR(ce->ce_mode)) { + /* + * This is a sparse directory entry, so we want to collect all + * of the added files within the tree. This requires recursively + * expanding the trees to find the elements that are new in this + * tree and marking them with DIFF_STATUS_ADDED. + */ + struct strbuf base = STRBUF_INIT; + struct pathspec ps; + struct tree *tree = lookup_tree(istate->repo, &ce->oid); + + memset(&ps, 0, sizeof(ps)); + ps.recursive = 1; + ps.has_wildcard = 1; + ps.max_depth = -1; + + strbuf_add(&base, ce->name, ce->ce_namelen); + read_tree_at(istate->repo, tree, &base, &ps, + add_file_to_list, s); + continue; + } + it = string_list_insert(&s->change, ce->name); d = it->util; if (!d) {