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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 25, 2022, 16:33 UTC
Message-ID: <YhkE2vxI4nM3ut0K@google.com>
In-Reply-To: <20220224052259.30498-1-newren@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.

[...]

Show 9 quoted lines

> --- /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?

Show 8 quoted lines

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

Show 10 quoted lines

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