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

[Jonathan Nieder <jrnieder@gmail.com>]

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?

> +	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.
 +

?

> ++
> +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.

?

> ++
> +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`.

Thanks,
Jonathan