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

[Derrick Stolee <stolee@gmail.com>]

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.

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.

Thanks,
-Stolee