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: Fri, 7 Jan 2022 08:21:13 -0800	[thread overview]
Message-ID: <CABPp-BFvPDKYhDQGJ8-qKojs+aTXa2ZTKTGJb+Aa4wER8Ug8Ow@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BFqTqK3grMLv_odKGwDhUaU8p1epNzv825kTqLAgMrK9g@mail.gmail.com>

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.

  reply	other threads:[~2022-01-07 16:21 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
2022-01-07 16:21     ` Elijah Newren [this message]
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-BFvPDKYhDQGJ8-qKojs+aTXa2ZTKTGJb+Aa4wER8Ug8Ow@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).