On Mon, Feb 14, 2022 at 8:19 AM Derrick Stolee [off-list ref] wrote:

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:

quoted

Note (reason for RFC): this is RFC primarily because of dependencies (you
may not want to pick this up yet, Junio), though there is also a question of
whether to split patch 7 into two steps -- one for now and one we take in
some future release. In particular, the first step could be to have
sparse-checkout error out if neither --no-cone nor --cone are specified and
then change the default to be --cone in some future release. I don't think
splitting it into two steps is needed given (a) users who are unaware of the
change will still get useful error messages telling them that directories
are expected due to patches 4-6 of this series, and (b) the huge
"EXPERIMENTAL" warning and explicit note about likely behavioral changes in
git-sparse-checkout.txt serves as warning about the changes. However, the
two step approach is an alternative.

I support this change. This will also require an update to the 'git clone'
documentation around the '--sparse' option, as I imagine we are going to
be changing behavior there. (If not, then we should do that as part of the
deprecation.)

Why would that be needed?  The documentation does not specify anything
about cone vs. non-cone mode, only that the initial working tree will
only have files from the toplevel directory present.  So, the
documentation is correct without any needed changes.

quoted

Note 2 (dependencies): this depends on en/present-despite-skipped (which
depends on vd/sparse-clean-etc) and on
ds/sparse-checkout-requires-per-worktree-config, because of otherwise heavy
text conflicts in patch 7 to git-sparse-checkout.txt. Given that neither of
those have merged to next yet, it may be premature to pick up this series.

Yes, hopefully things will start to settle down a little, especially since
vd/sparse-clean-etc is due to merge any day now.

Yeah, and en/present-despite-skipped addressed all reviewer comments a
full month ago, so I'd like to believe it's ready to merge down soon
too.

quoted

This series continues attempts to make sparse-checkouts more user friendly.
A quick overview:

 * Patches 1-2 fix existing bugs from en/sparse-checkout-set
 * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
   https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/ (local)),
   at least in cone mode. In non-cone mode it is not clear if patch 3 is a
   "fix" or a "break" (see the "NON-CONE PROBLEMS" section of the manual
   added in patch 7, and
   https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/ (local)
   where Stolee suggested it might be incorrect).
 * Patches 4-6 check positional arguments to set/add and provide
   errors/warnings for very likely mistakes. It also adds a --skip-checks
   flag for overridding in case you have a very unusual situation.

I took a close look at these patches and mostly have minor typo fixes. There
was one behavior issue: I don't think you should warn for file paths in non-
cone-mode. Being able to select a single file in a directory full of large
files is one of the main reasons to use non-cone-mode, in my experience.

In which case shouldn't we still show a warning when users specify a
path rather than a pattern, since the former risks selecting more than
one file?  (Adding a leading slash should be recommended for such a
case, right?)

It might be worth adding some documentation about how to reorganize a repo
to fit cone mode patterns, but that's not necessary.

quoted

 * Patch 7 makes cone mode the default, and makes large updates to the
   documentation both to explain why we changed the default, and to simplify
   the documentation since users can just use directories and ignore the
   intricacies of gitignore-style patterns and how they relate to sparse
   checkouts.

I'm a fan of the end-result of this patch. I responded with some specific
comments and a suggestion for splitting it into a series of four patches.

Your first 6 patches are likely to be noncontroversial and could merge
more quickly than the deprecation.

Will do.

I think it would be good to get the
full deprecation under full review as soon as possible so we can give the
community a long window to comment on it.

We can also consider if we need a release or two where this behavior
change is announced, but not actually done. I'm not sure if that is
necessary. Making '--no-cone' required might stir up some noise that
indicate how much of an impact the change would make.

We can discuss this more later, but I think it's worthwhile to
consider what happens even if folks didn't read the BIG warning about
behavioral changes in the git-sparse-checkout.txt manual, and didn't
know about the default change, and tried to use it anyway.  If they
specify something other than a directory, then they'd get an error
message due to the first six patches of this series -- at which point
they can look to the manual and decide to add --no-cone to their
command.  In other words, it basically does the same thing that we'd
do if we decided to have an interim period with an error when neither
--cone or --no-cone were specified, other than the error message
perhaps being slightly different.  What if the user does specify
directories and doesn't know about the default mode change?  Well,
that's where the two modes overlap and things work fine (with only
minor differences in behavior, such as better performance, and files
from leading directories being included), so the user would be able to
continue with their work.  So, I'm not sure that an interim period
where we error out when neither --cone or --no-cone are specified is
going to buy us much of anything.  And besides, we do have that super
big scary warning in the manual.  Anyway, I'll bring this all up again
when I resubmit the final patch broken up into a separate series.