On 9/13/2022 11:08 PM, Elijah Newren wrote:

Hi Shaoxuan,

Please note that it's customary to cc folks who have commented on
previous versions of your patch series when you re-roll.

Hi Elijah,

Sorry for the delay, I didn't have my computer with me during Merge 2022
and couldn't respond.

I'm sorry that I somehow lost you along the way :(

On Wed, Sep 7, 2022 at 5:28 PM Shaoxuan Yuan [off-list ref] wrote:

quoted

Add a --sparse option to `git-grep`.

It's awesome you're working on this.  Adding more of "behavior A"
(restricting querying commands to the sparse cone) is something I've
wanted for a long time.

Thanks :)

I think most of your code is beneficial, but I do have some issues
with high level direction you were implementing, which may require
some tweaks...

OK.

quoted

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.

"files outside of the sparse-checkout are unlikely to be important to
[users]" is certainly an issue.  But it's *much* wider than this.
Beyond `grep --cached`, it also affects `grep REVISION`, `log`, `diff
[REVISION]`, and related things...perhaps even something like `blame`.

Agree. Keep reading...

I think all those other commands probably deserve a mode where they
restrict output to the view associated with the user's cone.  I've

Agree.

brought that up before[1].  I was skeptical of making it the default,
because it'd probably take a long time to implement it everywhere.
Slowly changing defaults of all commands over many git releases seems
like a poor strategy, but I'm afraid that's what it looks like we are
doing here.

True.

I'm also worried that slowly changing the defaults without a
high-level plan will lead to users struggling to figure out what
flag(s) to pass.  Are we going to be stuck in a situation where users
have to remember that for a dense search, they use one flag for `grep
--cached`, a different one for  `grep [REVISION]`, no flag is needed
for `diff [REVISION]`, but yet a different flag is needed for `git
log`?

I think the inconsistency is certainly unsettling.

I'm also curious whether there shouldn't be a config option for
something like this, so folks don't have to specify it with every
invocation.  In particular, while I certainly have users that want to
just query git for information about the part of the history they are
interested in, there are other users who are fully aware they are
working in a bigger repository and want to search for additional
things to add to their sparse-checkout and predominantly use grep for
things like that.  They have even documented that `git grep --cached
<TERM>` can be used in sparse-checkouts for this purpose...and have
been using that for a few years.  (I did warn them at the time that
there was a risk they'd have to change their command, but it's still
going to be a behavioral change they might not expect.)  Further, when
I brought up changing the behavior of commands during sparse-checkouts
to limit to files matching the sparsity paths in that old thread at
[1], Stolee was a bit skeptical of making that the default.  That
suggests, at least, that two independent groups of users would want to
use the non-sparse searching frequently, and frequently enough that
they'd appreciate a config option.

A config option sounds good. Though I think

1. If this option is for global behavior: users may better off turning
off sparse-checkout if they want a config to do things densely everywhere.

2. If this option is for a single subcommand (e.g. 'grep'): I don't have
much thoughts here. It certainly can be nice for users who need to do
non-sparse searching frequently. This design, if necessary, should
belong to a patch where this config is added for every single subcommand?

I also brought up in that old thread that perhaps we want to avoid
adding a flag to every subcommand, and instead just having a
git-global flag for triggering this type of behavior.  (e.g. `git
--no-restrict grep --cached ...` or `git --dense grep --cached ...`).

This looks more like the answer to me. It's a peace of mind for users if
they don't have to worry about whether a subcommand is sparse-aware, and
how may their behaviors differ. Though we still may need to update the
actual behavior in each subcommand over an extended period of time
(though may not be difficult?), which you mentioned above "seems like a
poor strategy".

[1] https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/ (local)
and the responses to that email>

quoted

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.

I still think the flag name of `--sparse` is totally backwards and
highly confusing for the described behavior.  I missed Stolee's email
at the time (wasn't cc'ed) where he brought up that "--sparse" had
already been added to "git-add" and "git-rm", but in those cases the
commands aren't querying and I just don't see how they lead to the
same level of user confusion.  This one seems glaringly wrong to me
and both Junio and I flagged it on v1 when we first saw it.  (Perhaps
it also helps that for the add/rm cases, that a user is often given an
error message with the suggested flag to use, which just doesn't make
sense here either.)  If there is concern that this flag should be the
same as add and rm, then I think we need to do the backward
compatibility dance and fix add and rm by adding an alias over there
so that grep's flag won't be so confusing.

I guess I'm using "--sparse" here because "add", "rm" and "mv" all imply
that "when operating on a sparse path, ignores/warns unless '--sparse'
is used". I take it as an analogy so "when searching a sparse path,
ignores/warns unless '--sparse' is used". As the idea that "Git does
*not* care sparse contents unless '--[no-]sparse' is specified" is sort
of established through the implementations in "add", "rm", or "mv", I
don't see a big problem using "--sparse" here.

I *think*, as long as the users are informed that the default is to
ignore things outside of the sparse-checkout definition, and they have
to do something (using "--sparse" or a potential better name) to
override the default, we are safe to use a name that is famous (i.e.
"--sparse") even though its literal meaning is not perfectly descriptive.

One outlier I do find confusing though, is the "--sparse" option from
"git-ls-files". Without it, Git expands the index and show everything
outside of sparse-checkout definition, which seems a bit controversial.

...

I read over this patch and the other two patches.  Other than things
like variable names propagating the sparse/dense confusion, and the
high level goals already discussed, I didn't spot any other issues.

Thanks,
Shaoxuan