Re: [PATCH v5 1/3] builtin/grep.c: add --sparse option

Victoria Dye [off-list ref] writes:

Elijah Newren wrote:
...

quoted

However, for modification commands, I think we want the default to be
--restrict, regardless of the default for querying commands.  There
are some potentially very negative surprises for users if we don't,
and those surprises will be delayed rather than occur at the time the
user runs the command.  In fact, those negative surprises are likely
why those commands were the first to gain an option controlling
whether they operated on paths outside the sparsity specification.
(Also, the modification commands print a warning if they could have
affected other files but didn't due the the default of restricting, so
I think we have their default correct, even if the flag name is
suboptimal.)

One of the things I've found myself a bit frustrated with while working on
these sparse index integrations is that we haven't had a clear set of
guidelines for times when we need to make UI/UX changes relating to
'sparse-checkout' compatibility. I think what you've outlined here is a good
start to a larger discussion on the topic, but in the middle of this series
might not be the best place for that discussion (at least in terms of
preserving for later reference). 

Yup, I think we were a bit too quick to add the "hide outside sparse
cones" feature without first coming up with a reasonable guideline
that is designed to keep things consistent.

It might have been nice if we did this "make X sparse checkout
aware" effort in two separate steps.  The first step will not change
any behaviour, i.e. no optional or default "hide outside sparse
cones" at all, just "we do not upfront expand the index fully;
instead as we discover we need to inspect the contents in a
subdirectory that is compacted to a tree in the index, we lazily
expand it" as performance optimization.  And once we made sure we
taught all commands that used to expand the index fully upfront not
to do so, we do the "guideline" design for UI to "hide outside
sparse cones", and add that feature to the commands in the second
step.

Unfortunately we all get excited too much when we find a new shiny
toy, and we ended up getting ahead of ourselves before designing a
consistent end user experience.  But better late than never ;-)

As for this series, I think the best way to move the sparse index work along
is to drop this patch ("builtin/grep.c: add --sparse option") altogether.

Does that roughly correspond to the first step in my "It would have
been nice if we did these in two steps" above?  That would be a
sensible thing to do, as it would be less surprises to the users, I
hope.

Thanks.