git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <stolee@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Victoria Dye <vdye@github.com>
Subject: Re: [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests
Date: Wed, 5 Jan 2022 13:04:43 -0800	[thread overview]
Message-ID: <CABPp-BFqTqK3grMLv_odKGwDhUaU8p1epNzv825kTqLAgMrK9g@mail.gmail.com> (raw)
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:
>
> 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

  reply	other threads:[~2022-01-05 21:05 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-04 17:36 [PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index' Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 1/9] reset: fix validation in sparse index test Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 2/9] reset: reorder wildcard pathspec conditions Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 3/9] clean: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-04 17:36 ` [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests Victoria Dye via GitGitGadget
2022-01-05 21:04   ` Elijah Newren [this message]
2022-01-07 16:21     ` Elijah Newren
2022-01-04 17:36 ` [PATCH 5/9] checkout-index: add --ignore-skip-worktree-bits option Victoria Dye via GitGitGadget
2022-01-06  1:52   ` Elijah Newren
2022-01-06 15:07     ` Victoria Dye
2022-01-07 16:35       ` Elijah Newren
2022-01-04 17:36 ` [PATCH 6/9] checkout-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-06  1:59   ` Elijah Newren
2022-01-04 17:36 ` [PATCH 7/9] update-index: add tests for sparse-checkout compatibility Victoria Dye via GitGitGadget
2022-01-08 23:57   ` Elijah Newren
2022-01-10 15:47     ` Victoria Dye
2022-01-10 17:11       ` Elijah Newren
2022-01-10 18:01         ` Victoria Dye
2022-01-10 20:03           ` Elijah Newren
2022-01-04 17:36 ` [PATCH 8/9] update-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-09  1:49   ` Elijah Newren
2022-01-10 14:10     ` Victoria Dye
2022-01-10 15:52       ` Elijah Newren
2022-01-04 17:37 ` [PATCH 9/9] update-index: reduce scope of index expansion in do_reupdate Victoria Dye via GitGitGadget
2022-01-09  4:24   ` Elijah Newren
2022-01-09  4:41 ` [PATCH 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index' Elijah Newren
2022-01-11 18:04 ` [PATCH v2 " Victoria Dye via GitGitGadget
2022-01-11 18:04   ` [PATCH v2 1/9] reset: fix validation in sparse index test Victoria Dye via GitGitGadget
2022-01-11 18:04   ` [PATCH v2 2/9] reset: reorder wildcard pathspec conditions Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 3/9] clean: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 4/9] checkout-index: expand sparse checkout compatibility tests Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 5/9] checkout-index: add --ignore-skip-worktree-bits option Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 6/9] checkout-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 7/9] update-index: add tests for sparse-checkout compatibility Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 8/9] update-index: integrate with sparse index Victoria Dye via GitGitGadget
2022-01-11 18:05   ` [PATCH v2 9/9] update-index: reduce scope of index expansion in do_reupdate Victoria Dye via GitGitGadget
2022-01-13  3:02   ` [PATCH v2 0/9] Sparse index: integrate with 'clean', 'checkout-index', 'update-index' Elijah Newren
2022-01-27 16:36     ` Derrick Stolee
2022-01-27 20:04       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABPp-BFqTqK3grMLv_odKGwDhUaU8p1epNzv825kTqLAgMrK9g@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=stolee@gmail.com \
    --cc=vdye@github.com \
    --subject='Re: [PATCH 4/9] checkout-index: expand sparse checkout compatibility tests' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).