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

[Junio C Hamano <gitster@pobox.com>]

Victoria Dye <vdye@github.com> writes:

> Elijah Newren wrote:
> ...
>> 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.