Re: [PATCH 06/27] checkout-index: ensure full index

From: Elijah Newren <newren@gmail.com>
Date: Mar 17, 2021, 21:10 UTC
Message-ID: <CABPp-BH-c8gzrkOFFNb=8b8R+X+VRXsziKoE_RtcR4mh6zjR4g@mail.gmail.com>
In-Reply-To: <08ffff48-7b9c-7113-1a5a-557f3efff26f@gmail.com>

On Wed, Mar 17, 2021 at 1:05 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 18 quoted lines

>
> On 3/17/2021 1:50 PM, Elijah Newren wrote:
> > On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > With the caveat in the commit message, this change looks okay, but
> > checkout-index may be buggy regardless of the presence of
> > ensure_full_index().  If ensure_full_index() really is needed here
> > because it needs to operate on all SKIP_WORKTREE paths and not just
> > leading directories, that's because it's writing all those
> > SKIP_WORKTREE entries to the working tree.  When it writes them to the
> > working tree, is it clearing the SKIP_WORKTREE bit?  If not, we're in
> > a bit of a pickle...
>
> Perhaps I'm unclear in my intentions with this series: _every_
> insertion of ensure_full_index() is intended to be audited with
> tests in the future. Some might need behavior change, and others
> will not. In this series, I'm just putting in the protections so
> we don't accidentally trigger unexpected behavior.

I think this may be part of my qualms -- what do you mean by not accidentally triggering unexpected behavior? In particular, does your statement imply that whatever behavior you get after putting in ensure_full_index() is "expected"? I think I'm reading that implication into it, and objecting that the behavior with the ensure_full_index() still isn't expected. You've only removed a certain class of unexpected behavior, namely code that wasn't written to expect tree entries that suddenly gets them. You haven't handled the class of "user wants to work with a subset of files, why are all these unrelated files being munged/updated/computed/shown/etc." unexpected behavior.

I'm worrying that expectations are being set up such that working with just a small section of the code will be unusably hard. There may be several commands/flags where it could make sense to operate on either (a) all files in the repo or (b) just on files within your sparse paths. If, though, folks interpret operate-on-all-files as the "normal" mode (and history suggests they will), then people start adding all kinds of --no-do-this-sparsely flags to each command, and then users who want sparse operation have to remember to type such a flag with each and every command they ever run -- despite having taken at least three steps already to get a sparse-index.

I believe the extended discussions (for _months_!) on just grep & rm, plus watching a --sparse patch being floated just in the last day for ls-files suggest to me that this is a _very_ likely outcome and I'm worried about it.

Show 29 quoted lines

> Since tests take time to write and review, I was hoping that these
> insertions were minimal enough to get us to a safe place where we
> can remove the guards carefully.
>
> So with that in mind...
>
> > Might be nice to add a
> > /* TODO: audit if this is needed; if it is, we may have other bugs... */
> > or something like that.  But then again, perhaps you're considering
> > all uses of ensure_full_index() to be need-to-be-reaudited codepaths?
> > If so, and we determine we really do need one and want to keep it
> > indefinitely, will we mark those with a comment about why it's
> > considered correct?
> >
> > I just want a way to know what still needs to be audited and what
> > doesn't without doing a lot of history spelunking...
>
> ...every insertion "needs to be audited" in the future. That's a
> big part of the next "phases" in the implementation plan.
>
> As you suggest, it might be a good idea to add a comment to every
> insertion, to mark it as un-audited, such as:
>
>         /* TODO: test if ensure_full_index() is necessary */
>
> We can come back later to delete the comment if it truly is
> necessary (and add tests to guarantee correct behavior). We can
> also remove the comment _and_ the call by modifying the loop
> behavior to do the right thing in some cases.

If it's "needs to be audited for both performance reasons (can we operate on fewer entries as an invisible doesn't-change-results optimization) and correctness reasons (should we operate on fewer entries and given a modified result within a sparse-index because users would expect that, but maybe provide a special flag for the users who want to operate on all files in the repo)" and there's also an agreement that either audited or unaudited ones will be marked (or both), then great, I'm happy. If not, can we discuss which part of my performance/correctness/marking we aren't in agreement on?

Thanks, Elijah