Re: [PATCH v1 1/2] builtin/grep.c: add --sparse option

[Victoria Dye <vdye@github.com>]

Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>> On 8/17/2022 3:56 AM, Shaoxuan Yuan wrote:
>>> Add a --sparse option to `git-grep`. This option is mainly used to:
>>>
>>> If searching in the index (using --cached):
>>>
>>> With --sparse, proceed the action when the current cache_entry is
>>
>> This phrasing is awkward. It might be better to reframe to describe the
>> _why_ before the _what_
> 
> Thanks for an excellent suggestion.  As a project participant, I
> could guess the motivation, but couldn't link the parts of the
> proposed log message to what I thought was being said X-<.  The
> below is much clearer.
> 
>>   When the '--cached' option is used with the 'git grep' command, the
>>   search is limited to the blobs found in the index, not in the worktree.
>>   If the user has enabled sparse-checkout, this might present more results
>>   than they would like, since the files outside of the sparse-checkout are
>>   unlikely to be important to them.
> 
> Great.  As an explanation of the reasoning behind the design
> decision, I do not think it is bad to go even stronger than "might
> ... would like" and assume or declare that those users who use
> sparse-checkout are the ones who do NOT want to see the parts of the
> tree that are sparsed out.  And based on that assumption, "grep" and
> "grep --cached" should not bother reporting hit from the part that
> the user is not interested in.
> 
> By stating the design and the reasoning behind that decision clearly
> like so, we allow future developers to reconsider the earlier design
> decision more easily.  In 7 years, they may find that the Git users
> in their era use sparse-checkout even when they still care about the
> contents in the sparsed out area, in which case the basic assumption
> behind this change is no longer valid and would allow them to make
> "grep" and "grep --cached" behave differently.
> 
>>   Change the default behavior of 'git grep' to focus on the files within
>>   the sparse-checkout definition. To enable the previous behavior, add a
>>   '--sparse' option to 'git grep' that triggers the old behavior that
>>   inspects paths outside of the sparse-checkout definition when paired
>>   with the '--cached' option.
> 
> Yup.  Is that "--sparse" or "--unsparse"?  We are busting the sparse
> boundary and looking for everything, and calling the option to do so
> "--sparse" somehow feels counter-intuitive, at least to me.

It is a bit unintuitive, but '--sparse' is already used to mean "operate on
SKIP_WORKTREE entries (i.e., pretend the repo isn't a sparse-checkout)" in
both 'add' (0299a69694 (add: implement the --sparse option, 2021-09-24)) and
'rm' (f9786f9b85 (rm: add --sparse option, 2021-09-24)). The
'checkout-index' option '--ignore-skip-worktree-bits' indicates similar
behavior (and is, IMO, similarly confusing with its use of "ignore").

I'm not sure '--unsparse' would fit as an alternative, though, since 'git
grep' isn't really "unsparsifying" the repo (to me, that would imply
updating the index to remove the 'SKIP_WORKTREE' flag). Rather, it's looking
at files that are sparse when, by default, it does not. 

I still like the consistency of '--sparse' with existing similar options in
other commands but, if we want to try something clearer here, maybe
something like '--search-sparse' is more descriptive?

> 
> Thanks.