Thread (51 messages) 51 messages, 6 authors, 2022-09-23

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

From: Victoria Dye <hidden>
Date: 2022-09-18 19:52:44

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/ (local)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help