On Fri, Feb 25, 2022 at 8:33 AM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > > Hi, > > Elijah Newren wrote: > > > Signed-off-by: Elijah Newren <newren@xxxxxxxxx> > > Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> > > 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.
