git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests

From
Elijah Newren <newren@gmail.com>
Date
Jan 5, 2022, 21:04 UTC
Message-ID
<CABPp-BFqTqK3grMLv_odKGwDhUaU8p1epNzv825kTqLAgMrK9g@mail.gmail.com>
In-Reply-To
<e6a8671f6be5afad9a495be66c0735f6b741c844.1641317820.git.gitgitgadget@gmail.com>

On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 53 quoted lines
>
> 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.

Show 22 quoted lines
> +       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.

Show 5 quoted lines
> +'
> +
> +# 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.

Show 5 quoted lines
> +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...

Show 7 quoted lines
> +'
> +
>  test_expect_success 'clean' '
>         init_repos &&
>
> --
> gitgitgadget
Previous: Victoria Dye via GitGitGadgetNext: Elijah Newren
Message 6 of 39 in “Sparse index: integrate with 'clean', 'checkout-index', 'update-index'”
  1. 0/9 Sparse index: integrate with 'clean', 'checkout-index', 'update-index'Victoria Dye via GitGitGadget, Jan 4, 2022
  2. 1/9 reset: fix validation in sparse index testVictoria Dye via GitGitGadget, Jan 4, 2022
  3. 2/9 reset: reorder wildcard pathspec conditionsVictoria Dye via GitGitGadget, Jan 4, 2022
  4. 3/9 clean: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 4, 2022
  5. 4/9 checkout-index: expand sparse checkout compatibility testsVictoria Dye via GitGitGadget, Jan 4, 2022
  6. Elijah NewrenJan 5, 2022
  7. Elijah NewrenJan 7, 2022
  8. 5/9 checkout-index: add --ignore-skip-worktree-bits optionVictoria Dye via GitGitGadget, Jan 4, 2022
  9. Elijah NewrenJan 6, 2022
  10. Victoria DyeJan 6, 2022
  11. Elijah NewrenJan 7, 2022
  12. 6/9 checkout-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 4, 2022
  13. Elijah NewrenJan 6, 2022
  14. 7/9 update-index: add tests for sparse-checkout compatibilityVictoria Dye via GitGitGadget, Jan 4, 2022
  15. Elijah NewrenJan 8, 2022
  16. Victoria DyeJan 10, 2022
  17. Elijah NewrenJan 10, 2022
  18. Victoria DyeJan 10, 2022
  19. Elijah NewrenJan 10, 2022
  20. 8/9 update-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 4, 2022
  21. Elijah NewrenJan 9, 2022
  22. Victoria DyeJan 10, 2022
  23. Elijah NewrenJan 10, 2022
  24. 9/9 update-index: reduce scope of index expansion in do_reupdateVictoria Dye via GitGitGadget, Jan 4, 2022
  25. Elijah NewrenJan 9, 2022
  26. Elijah NewrenJan 9, 2022
  27. 0/9 Sparse index: integrate with 'clean', 'checkout-index', 'update-index'Victoria Dye via GitGitGadget, Jan 11, 2022
  28. 1/9 reset: fix validation in sparse index testVictoria Dye via GitGitGadget, Jan 11, 2022
  29. 2/9 reset: reorder wildcard pathspec conditionsVictoria Dye via GitGitGadget, Jan 11, 2022
  30. 4/9 checkout-index: expand sparse checkout compatibility testsVictoria Dye via GitGitGadget, Jan 11, 2022
  31. 3/9 clean: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 11, 2022
  32. 6/9 checkout-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 11, 2022
  33. 9/9 update-index: reduce scope of index expansion in do_reupdateVictoria Dye via GitGitGadget, Jan 11, 2022
  34. 8/9 update-index: integrate with sparse indexVictoria Dye via GitGitGadget, Jan 11, 2022
  35. 7/9 update-index: add tests for sparse-checkout compatibilityVictoria Dye via GitGitGadget, Jan 11, 2022
  36. 5/9 checkout-index: add --ignore-skip-worktree-bits optionVictoria Dye via GitGitGadget, Jan 11, 2022
  37. Elijah NewrenJan 13, 2022
  38. Derrick StoleeJan 27, 2022
  39. Junio C HamanoJan 27, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.