Re: [PATCH v3] repo_read_index: add config to expect files outside sparse patterns

[Elijah Newren <newren@gmail.com>]

On Fri, Feb 25, 2022 at 8:33 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Elijah Newren wrote:
>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks, and sorry for the slow review.  My one remaining area for nits
> is the documentation, but that can be improved iteratively via patches
> on top.
>
> [...]
> > --- /dev/null
> > +++ b/Documentation/config/sparse.txt
> > @@ -0,0 +1,28 @@
> > +sparse.expectFilesOutsideOfPatterns::
> > +     Typically with sparse checkouts, files not matching any
> > +     sparsity patterns are marked as such in the index file and
> > +     missing from the working tree.  Accordingly, Git will
> > +     ordinarily check whether files that the index indicates are
> > +     outside of the sparse area are present in the working tree and
>
> Junio mentioned the "sparse area" could suggest that the area is
> itself sparse and devoid of files, so it might not have been the best
> choice of words on my part.  Perhaps "whether files that the index
> indicates are not checked out are present in the working tree" would
> work here?

I rewrote the paragraph.  I think it's more clear now; I'll resubmit
it here soon.

> > +     mark them as present in the index if so.  This option can be
> > +     used to tell Git that such present-but-unmatching files are
> > +     expected and to stop checking for them.
> > ++
> > +The default is `false`.  Paths which are marked as SKIP_WORKTREE
> > +despite being present (which can occur for a few different reasons)
> > +typically present a range of problems which are difficult for users to
> > +discover and recover from.  The default setting avoids such issues.
>
> The git-sparse-checkout(1) page never describes what SKIP_WORKTREE
> means, so it might not be obvious to them what this means.  Also, the
> "can occur for a few different reasons" may leave the user wondering
> whether they are subject to those reasons.  What the reader wants to
> know is "I should keep using the default because it makes Git work
> better", so how about something like
>
>  The default is `false`, which allows Git to automatically recover
>  from the list of files in the index and working tree falling out of
>  sync.
>  +
>
> ?

I like this.

> > ++
> > +A Git-based virtual file system (VFS) can turn the usual expectation
> > +on its head: files are present in the working copy but do not take
> > +up much disk space because their contents are not downloaded until
> > +they are accessed.  With such a virtual file system layer, most files
> > +do not match the sparsity patterns at first, and the VFS layer
> > +updates the sparsity patterns to add more files whenever files are
> > +written.  Setting this to `true` supports such a setup where files are
> > +expected to be present outside the sparse area and a separate, robust
> > +mechanism is responsible for keeping the sparsity patterns up to date.
>
> Here I spent most of the words explaining what a Git-based VFS layer
> is, which is also not too relevant to most users (who are just
> interested in "is `true` the right value for me?").  How about
> reducing it to the following?
>
>  Set this to `true` if you are in a setup where extra files are expected
>  to be present and a separate, robust mechanism is responsible for
>  keeping the sparsity patterns up to date, such as a Git-aware virtual
>  file system.
>
> ?

I like this, but I also added in some of the wording suggestions from
Junio here, so it's
a bit longer but has both some of his suggested wording and yours for
slightly different aspects that I think works well together.

>
> > ++
> > +Note that the checking and clearing of the SKIP_WORKTREE bit only
> > +happens when core.sparseCheckout is true, so this config option has no
> > +effect unless core.sparseCheckout is true.
>
> Good note.  Same nit about the user not necessarily knowing what
> SKIP_WORKTREE means applies.  Also, we can remove the extra words
> "Note that" since the dutiful reader should be noting everything we
> say. :)  I think that would make
>
>  +
>  Regardless of this setting, Git does not check for
>  present-but-unmatching files unless sparse checkout is enabled, so
>  this config option has no effect unless `core.sparseCheckout` is
>  `true`.

I like this too.  Thanks for the suggestions, the proposed changes,
and the review.