| Message ID | e6a8671f6be5afad9a495be66c0735f6b741c844.1641317820.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b553ef674965f41bfff4e0a2c330f9087b3cd6b7 |
| Headers | show |
| Series | Sparse index: integrate with 'clean', 'checkout-index', 'update-index' | expand |
On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Victoria Dye <vdye@github.com> > > Add tests to cover `checkout-index`, with a focus on cases interesting in a > sparse checkout (e.g., files specified outside sparse checkout definition). > New tests are intended to serve as a baseline for expected behavior and > performance when integrating `checkout-index` with the sparse index. > > Signed-off-by: Victoria Dye <vdye@github.com> > --- > t/perf/p2000-sparse-operations.sh | 1 + > t/t1092-sparse-checkout-compatibility.sh | 54 ++++++++++++++++++++++++ > 2 files changed, 55 insertions(+) > > diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh > index cb777c74a24..54f8602f3c1 100755 > --- a/t/perf/p2000-sparse-operations.sh > +++ b/t/perf/p2000-sparse-operations.sh > @@ -117,5 +117,6 @@ test_perf_on_all git diff > test_perf_on_all git diff --cached > test_perf_on_all git blame $SPARSE_CONE/a > test_perf_on_all git blame $SPARSE_CONE/f3/a > +test_perf_on_all git checkout-index -f --all > > test_done > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 05587361452..db7ad41109b 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -755,6 +755,60 @@ test_expect_success 'cherry-pick with conflicts' ' > test_all_match test_must_fail git cherry-pick to-cherry-pick > ' > > +test_expect_success 'checkout-index inside sparse definition' ' > + init_repos && > + > + run_on_all rm -f deep/a && > + test_all_match git checkout-index -- deep/a && > + test_all_match git status --porcelain=v2 && > + > + echo test >>new-a && > + run_on_all cp ../new-a a && > + test_all_match test_must_fail git checkout-index -- a && > + test_all_match git checkout-index -f -- a && > + test_all_match git status --porcelain=v2 > +' > + > +test_expect_success 'checkout-index outside sparse definition' ' > + init_repos && > + > + # File does not exist on disk yet for sparse checkouts, so checkout-index > + # succeeds without -f > + test_sparse_match git checkout-index -- folder1/a && Whoa, um, what is `git checkout-index -- folder1/a` even supposed to mean with the sparse-index and cone mode? That's an individual file, so should folder1 and everything under it have the SKIP_WORKTREE bit cleared? Should folder1 just stop being a sparse directory, with all siblings of folder1/a retaining the SKIP_WORKTREE bit (so folder1/0 could become a sparse directory) while folder1/a loses the SKIP_WORKTREE bit, kind of violating cone mode but perhaps just until the next sparse-checkout reapply? The test sadly doesn't look at the SKIP_WORKTREE bits at all, which feels like an important oversight (unless you meant to test this later?). Especially since my manual testing shows that this test leaves the files as SKIP_WORKTREE despite writing the files to the working tree. I think the present-despite-skip-worktree problem I've discussed elsewhere is growing in importance rather than shrinking; you don't need to address that problem with this series but I raise it mostly so we're aware to at least test it. I'll keep reading to see if you mention this or fix it up in subsequent patches. > + test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && > + test_cmp sparse-checkout/folder1/a full-checkout/folder1/a && > + > + run_on_sparse rm -rf folder1 && > + echo test >new-a && > + run_on_sparse mkdir -p folder1 && > + run_on_all cp ../new-a folder1/a && > + > + test_all_match test_must_fail git checkout-index -- folder1/a && > + test_all_match git checkout-index -f -- folder1/a && > + test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && > + test_cmp sparse-checkout/folder1/a full-checkout/folder1/a > +' > + > +test_expect_success 'checkout-index with folders' ' > + init_repos && > + > + # Inside checkout definition > + test_all_match test_must_fail git checkout-index -f -- deep/ && > + > + # Outside checkout definition > + test_all_match test_must_fail git checkout-index -f -- folder1/ Passing a directory to checkout-index? I'm glad they all immediately fail, but is the plan just to make sure that this continues? I'm not sure what could make sense otherwise. You've piqued my curiosity; I'll keep reading the series to see. > +' > + > +# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all > +# files (even those outside the sparse definition) on disk. However, these files > +# don't appear in the percentage of tracked files in git status. Ah, so you _are_ noticing the present-despite-SKIP_WORKTREE files. I think it might be nice to test for these a bit earlier, but it's nice to see some test here for them. I think that present-despite-SKIP_WORKTREE files is an erroneous condition, one we should avoid triggering, and one we should help users clean up from. For checkout-index, the fact that it currently makes more of these files is buggy. It should either (1) clear the SKIP_WORKTREE bit when it writes the files to the working tree, or (2) avoid writing the files to the working tree. And if we choose (1), there's already a --no-create option we could piggy-back on to allow folks to not write the SKIP_WORKTREE files. > +test_expect_failure 'checkout-index --all' ' > + init_repos && > + > + test_all_match git checkout-index --all && > + test_sparse_match test_path_is_missing folder1 Ah, it looks like you're choosing (2). That may be fine, but an interesting anecdote: While attempting to adopt sparse checkouts at $DAYJOB (and particularly using cone mode), we found the code structure just didn't quite work. We needed some directories to be ignored for sparse checkouts to be meaningful at all, but we had some files that were siblings to those directories that were needed for builds to function. We came up with a hack to "add a few files back", using "git checkout-index -- $FILENAME". We expected that hack to write the listed file(s) to the working tree -- though I think we also had logic to then run the stuff we needed and then delete these temporary files. We used this hack starting in Feb 2020, and eventually restructured the code to not need this hack in Feb 2021. I'll have to mull over whether your choice of option (2) might cause us some problems if someone (a) uses a new git version to (b) access an old version of our code and (c) really wants to work with a sparse checkout (since the "checkout-index" stuff was part of the build logic checked into the code). I think your change here is fine (because not using sparse checkouts is an option, we told folks it was experimental, and those are old versions that are only getting security fixes in special circumstances), but let me think about it for a bit... > +' > + > test_expect_success 'clean' ' > init_repos && > > -- > gitgitgadget
On Wed, Jan 5, 2022 at 1:04 PM Elijah Newren <newren@gmail.com> wrote: > > On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: Victoria Dye <vdye@github.com> > > ... > > +' > > + > > +# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all > > +# files (even those outside the sparse definition) on disk. However, these files > > +# don't appear in the percentage of tracked files in git status. > > Ah, so you _are_ noticing the present-despite-SKIP_WORKTREE files. I > think it might be nice to test for these a bit earlier, but it's nice > to see some test here for them. > > I think that present-despite-SKIP_WORKTREE files is an erroneous > condition, one we should avoid triggering, and one we should help > users clean up from. For checkout-index, the fact that it currently > makes more of these files is buggy. It should either (1) clear the > SKIP_WORKTREE bit when it writes the files to the working tree, or (2) > avoid writing the files to the working tree. > > And if we choose (1), there's already a --no-create option we could > piggy-back on to allow folks to not write the SKIP_WORKTREE files. > > > +test_expect_failure 'checkout-index --all' ' > > + init_repos && > > + > > + test_all_match git checkout-index --all && > > + test_sparse_match test_path_is_missing folder1 > > Ah, it looks like you're choosing (2). That may be fine, but an > interesting anecdote: > > While attempting to adopt sparse checkouts at $DAYJOB (and > particularly using cone mode), we found the code structure just didn't > quite work. We needed some directories to be ignored for sparse > checkouts to be meaningful at all, but we had some files that were > siblings to those directories that were needed for builds to function. > We came up with a hack to "add a few files back", using "git > checkout-index -- $FILENAME". We expected that hack to write the > listed file(s) to the working tree -- though I think we also had logic > to then run the stuff we needed and then delete these temporary files. > We used this hack starting in Feb 2020, and eventually restructured > the code to not need this hack in Feb 2021. I'll have to mull over > whether your choice of option (2) might cause us some problems if > someone (a) uses a new git version to (b) access an old version of our > code and (c) really wants to work with a sparse checkout (since the > "checkout-index" stuff was part of the build logic checked into the > code). I think your change here is fine (because not using sparse > checkouts is an option, we told folks it was experimental, and those > are old versions that are only getting security fixes in special > circumstances), but let me think about it for a bit... I brought this up a day or two ago with my co-workers and discussed the combination of conditions needed to trigger any problems. Their response was "meh, it's pretty unlikely that anyone ever hits it, the feature was labeled as experimental anyway, and there are multiple easy workarounds for the user to choose from". So, no need to worry about this angle.
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh index cb777c74a24..54f8602f3c1 100755 --- a/t/perf/p2000-sparse-operations.sh +++ b/t/perf/p2000-sparse-operations.sh @@ -117,5 +117,6 @@ test_perf_on_all git diff test_perf_on_all git diff --cached test_perf_on_all git blame $SPARSE_CONE/a test_perf_on_all git blame $SPARSE_CONE/f3/a +test_perf_on_all git checkout-index -f --all test_done diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 05587361452..db7ad41109b 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -755,6 +755,60 @@ test_expect_success 'cherry-pick with conflicts' ' test_all_match test_must_fail git cherry-pick to-cherry-pick ' +test_expect_success 'checkout-index inside sparse definition' ' + init_repos && + + run_on_all rm -f deep/a && + test_all_match git checkout-index -- deep/a && + test_all_match git status --porcelain=v2 && + + echo test >>new-a && + run_on_all cp ../new-a a && + test_all_match test_must_fail git checkout-index -- a && + test_all_match git checkout-index -f -- a && + test_all_match git status --porcelain=v2 +' + +test_expect_success 'checkout-index outside sparse definition' ' + init_repos && + + # File does not exist on disk yet for sparse checkouts, so checkout-index + # succeeds without -f + test_sparse_match git checkout-index -- folder1/a && + test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && + test_cmp sparse-checkout/folder1/a full-checkout/folder1/a && + + run_on_sparse rm -rf folder1 && + echo test >new-a && + run_on_sparse mkdir -p folder1 && + run_on_all cp ../new-a folder1/a && + + test_all_match test_must_fail git checkout-index -- folder1/a && + test_all_match git checkout-index -f -- folder1/a && + test_cmp sparse-checkout/folder1/a sparse-index/folder1/a && + test_cmp sparse-checkout/folder1/a full-checkout/folder1/a +' + +test_expect_success 'checkout-index with folders' ' + init_repos && + + # Inside checkout definition + test_all_match test_must_fail git checkout-index -f -- deep/ && + + # Outside checkout definition + test_all_match test_must_fail git checkout-index -f -- folder1/ +' + +# NEEDSWORK: even in sparse checkouts, checkout-index --all will create all +# files (even those outside the sparse definition) on disk. However, these files +# don't appear in the percentage of tracked files in git status. +test_expect_failure 'checkout-index --all' ' + init_repos && + + test_all_match git checkout-index --all && + test_sparse_match test_path_is_missing folder1 +' + test_expect_success 'clean' ' init_repos &&