git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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/

  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

  List information: http://vger.kernel.org/majordomo-info.html

* 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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).