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

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.