From: Elijah Newren <newren@gmail.com>
To: Victoria Dye <vdye@github.com>
Cc: Junio C Hamano <gitster@pobox.com>,
Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>,
Derrick Stolee <derrickstolee@github.com>,
Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v5 1/3] builtin/grep.c: add --sparse option
Date: Tue, 20 Sep 2022 00:13:47 -0700 [thread overview]
Message-ID: <CABPp-BH7cKUSDrt60zC8TTR3Jpxru9i25=3XJM-KYMoFSPqhQw@mail.gmail.com> (raw)
In-Reply-To: <cafcedba-96a2-cb85-d593-ef47c8c8397c@github.com>
On Sun, Sep 18, 2022 at 12:52 PM Victoria Dye <vdye@github.com> 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).
Yeah, that's fair, and I apologize for the problems. I should have
pushed for a resolution and/or documentation of these issues at some
point; particularly since I was the one to bring it up in the first
place. Between Stolee asking us to defer for a year-ish on UI/UX
changes in sparse-checkout while he got sparse-index into place, and
various other things coming up in the meantime, I just didn't get back
to it. I probably should have, especially since we also had other
similar discussions going back to when git-sparse-checkout was first
introduced, but we've often focused on just solving the next subset of
usecases that were within reach rather than getting a bigger design
document. Knowing that these kinds of issues were lurking was part of
the reason I insisted on having the big scary warning in the docs:
"""
THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER
COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN
THE FUTURE.
"""
I'm glad I at least had the foresight to insist on that small measure... :-)
> 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.
Sure, I'll take a stab at it this week.
> 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.
Seems reasonable.
> [1] https://lore.kernel.org/git/20220908001854.206789-4-shaoxuan.yuan02@gmail.com/
next prev parent reply other threads:[~2022-09-20 7:14 UTC|newest]
Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-17 7:56 [PATCH v1 0/2] grep: integrate with sparse index Shaoxuan Yuan
2022-08-17 7:56 ` [PATCH v1 1/2] builtin/grep.c: add --sparse option Shaoxuan Yuan
2022-08-17 14:12 ` Derrick Stolee
2022-08-17 17:13 ` Junio C Hamano
2022-08-17 17:34 ` Victoria Dye
2022-08-17 17:43 ` Derrick Stolee
2022-08-17 18:47 ` Junio C Hamano
2022-08-17 17:37 ` Elijah Newren
2022-08-24 18:20 ` Shaoxuan Yuan
2022-08-24 19:08 ` Derrick Stolee
2022-08-17 7:56 ` [PATCH v1 2/2] builtin/grep.c: integrate with sparse index Shaoxuan Yuan
2022-08-17 14:23 ` Derrick Stolee
2022-08-24 21:06 ` Shaoxuan Yuan
2022-08-25 0:39 ` Derrick Stolee
2022-08-17 13:46 ` [PATCH v1 0/2] grep: " Derrick Stolee
2022-08-29 23:28 ` [PATCH v2 " Shaoxuan Yuan
2022-08-29 23:28 ` [PATCH v2 1/2] builtin/grep.c: add --sparse option Shaoxuan Yuan
2022-08-29 23:28 ` [PATCH v2 2/2] builtin/grep.c: integrate with sparse index Shaoxuan Yuan
2022-08-30 13:45 ` Derrick Stolee
2022-09-01 4:57 ` [PATCH v3 0/3] grep: " Shaoxuan Yuan
2022-09-01 4:57 ` [PATCH v3 1/3] builtin/grep.c: add --sparse option Shaoxuan Yuan
2022-09-01 4:57 ` [PATCH v3 2/3] builtin/grep.c: integrate with sparse index Shaoxuan Yuan
2022-09-01 4:57 ` [PATCH v3 3/3] builtin/grep.c: walking tree instead of expanding index with --sparse Shaoxuan Yuan
2022-09-01 17:03 ` Derrick Stolee
2022-09-01 18:31 ` Shaoxuan Yuan
2022-09-01 17:17 ` Junio C Hamano
2022-09-01 17:27 ` Junio C Hamano
2022-09-01 22:49 ` Shaoxuan Yuan
2022-09-01 22:36 ` Shaoxuan Yuan
2022-09-02 3:28 ` Victoria Dye
2022-09-02 18:47 ` Shaoxuan Yuan
2022-09-03 0:36 ` [PATCH v4 0/3] grep: integrate with sparse index Shaoxuan Yuan
2022-09-03 0:36 ` [PATCH v4 1/3] builtin/grep.c: add --sparse option Shaoxuan Yuan
2022-09-03 0:36 ` [PATCH v4 2/3] builtin/grep.c: integrate with sparse index Shaoxuan Yuan
2022-09-03 0:36 ` [PATCH v4 3/3] builtin/grep.c: walking tree instead of expanding index with --sparse Shaoxuan Yuan
2022-09-03 4:39 ` Junio C Hamano
2022-09-08 0:24 ` Shaoxuan Yuan
2022-09-08 0:18 ` [PATCH v5 0/3] grep: integrate with sparse index Shaoxuan Yuan
2022-09-08 0:18 ` [PATCH v5 1/3] builtin/grep.c: add --sparse option Shaoxuan Yuan
2022-09-10 1:07 ` Victoria Dye
2022-09-14 6:08 ` Elijah Newren
2022-09-15 2:57 ` Junio C Hamano
2022-09-18 2:14 ` Elijah Newren
2022-09-18 19:52 ` Victoria Dye
2022-09-19 1:23 ` Junio C Hamano
2022-09-19 4:27 ` Shaoxuan Yuan
2022-09-19 11:03 ` Ævar Arnfjörð Bjarmason
2022-09-20 7:13 ` Elijah Newren [this message]
2022-09-17 3:34 ` Shaoxuan Yuan
2022-09-18 4:24 ` Elijah Newren
2022-09-19 4:13 ` Shaoxuan Yuan
2022-09-17 3:45 ` Shaoxuan Yuan
2022-09-08 0:18 ` [PATCH v5 2/3] builtin/grep.c: integrate with sparse index Shaoxuan Yuan
2022-09-08 0:18 ` [PATCH v5 3/3] builtin/grep.c: walking tree instead of expanding index with --sparse Shaoxuan Yuan
2022-09-08 17:59 ` Junio C Hamano
2022-09-08 20:46 ` Derrick Stolee
2022-09-08 20:56 ` Junio C Hamano
2022-09-08 21:06 ` Shaoxuan Yuan
2022-09-09 12:49 ` Derrick Stolee
2022-09-13 17:23 ` Junio C Hamano
2022-09-10 2:04 ` Victoria Dye
2022-09-23 4:18 ` [PATCH v6 0/1] grep: integrate with sparse index Shaoxuan Yuan
2022-09-23 4:18 ` [PATCH v6 1/1] builtin/grep.c: " Shaoxuan Yuan
2022-09-23 16:40 ` Junio C Hamano
2022-09-23 16:58 ` Junio C Hamano
2022-09-26 17:28 ` Junio C Hamano
2022-09-23 14:13 ` [PATCH v6 0/1] grep: " Derrick Stolee
2022-09-23 16:01 ` Victoria Dye
2022-09-23 17:08 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABPp-BH7cKUSDrt60zC8TTR3Jpxru9i25=3XJM-KYMoFSPqhQw@mail.gmail.com' \
--to=newren@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=shaoxuan.yuan02@gmail.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).