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

[Victoria Dye <vdye@github.com>]

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.

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.

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