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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2022, 01:23 UTC
Message-ID: <xmqq8rmgdunx.fsf@gitster.g>
In-Reply-To: <cafcedba-96a2-cb85-d593-ef47c8c8397c@github.com>

Victoria Dye <vdye@github.com> writes:

Show 21 quoted lines

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