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

[Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>]

Hi Victoria, :-)

On 9/18/2022 12:52 PM, Victoria Dye wrote:
> Elijah Newren wrote:
>> == Overall ==
>>
>> For existing querying commands (just ls-files), `--sparse` already
>> means restrict to the sparse cone.  If we keep using the existing flag
>> names, grep should follow suit.
>>
>> For existing modification commands already released (add, rm), the
>> fact that the command is modifying actually gives a different way to
>> interpret things such that it's not clear `--sparse` was even a
>> problem.  However, perhaps the name of the flag is bad just because
>> there are multiple ways to view it and those who view it one way will
>> see it as counter-intuitive.
>>
>> == Flag rename? ==
>>
>> There's another reason to potentially rename the flag.  We already
>> have `--sparse` and `--dense` flags for rev-list and friends.  So,
>> when we want to enable those other commands to restrict to the
>> sparsity patterns, we probably need a different name.  So, perhaps, we
>> should rename our `--sparse/--dense` to `--restrict/--no-restrict`.
>> Such a rename would also likely clear up the ambiguity about which way
>> to interpret the command for the add & rm commands (though it'd pick
>> the second one and suggest we were using the wrong name after all).
>>
>> (There are also two other commands that use `--sparse` -- pack-objects
>> and show-branch, though in a much different way and neither would ever
>> be affected by our new --sparse/--dense/--restrict/--no-restrict
>> flags.)
>>
>> Other names are also possible.  Any suggestions?
>>
>> == global flag vs subcommand flags ==
>>
>> Do we want to make --[no-]restrict a flag for each subcommand, or just
>> make it a global git flag?  I kind of think it'd make sense to do the
>> latter
>>
>> == Defaults ==
>>
>> As discussed before, we probably want querying commands (ls-files,
>> grep, log, etc.) to default to --no-restrict for now, since we are
>> otherwise slowly changing the defaults.  We may want to swap that
>> default in the future.
>>
>> 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). 
> 
> Elijah, would you be interested in compiling your thoughts into a document
> in 'Documentation/technical'? If not, Stolee or I could do it. If we could
> settle on some guidelines (option names, behavior, etc.) for better
> incorporating 'sparse-checkout' support into existing commands, it'd make
> future sparse index work substantially easier for everyone involved.

This sounds good! I am always confused about the inconsistency of the
meaning of "--sparse" across a variety of commands. A guideline
definitely corrects prior integrations and helps future ones.

> 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.
> Shaoxuan's updates in patch 3 [1] make 'git grep' sparse index-compatible
> for *all* invocations (not just those without '--sparse'), so we don't need
> the new option for sparse index compatibility. It can then be re-introduced
> later (possibly modified) in a series dedicated to unifying the
> sparse-checkout UX.

Are you suggesting that we should still follow the original "use --cache
to search within the index and show SKIP_WORKTREE entries found"? I'm
asking because the tests in the second patch [2] are still using the
lately-introduced "--sparse". If yes, then I think it sounds good to
re-introduce the (potentially) modified UI in the future :-).

[2]
https://lore.kernel.org/git/20220908001854.206789-3-shaoxuan.yuan02@gmail.com/

> 
> [1] https://lore.kernel.org/git/20220908001854.206789-4-shaoxuan.yuan02@gmail.com/

Thanks,
Shaoxuan