git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: "Matheus Tavares Bernardino" <matheus.bernardino@usp.br>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Nguyễn Thái Ngọc" <pclouds@gmail.com>,
	"Jonathan Tan" <jonathantanmy@google.com>
Subject: Re: [RFC PATCH 3/3] grep: add option to ignore sparsity patterns
Date: Wed, 29 Apr 2020 10:21:56 -0700	[thread overview]
Message-ID: <CABPp-BHKYR9QBcAG_pM6DniaeGS40X=ErKGGsvWa_KhogCZzEA@mail.gmail.com> (raw)
In-Reply-To: <49c1e9a5-b234-1696-03cc-95bf95f4663c@gmail.com>

Sorry for the super late reply...

On Tue, Mar 31, 2020 at 1:02 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/31/2020 3:12 PM, Elijah Newren wrote:
> > // adding Jonathan Tan to cc based on the fact that we keep bringing
> > up partial clones and how it relates...
> >
> > On Sun, Mar 29, 2020 at 8:23 PM Matheus Tavares Bernardino
> > <matheus.bernardino@usp.br> wrote:
> >>
> >> On Tue, Mar 24, 2020 at 3:30 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>> Elijah Newren <newren@gmail.com> writes:
> >>>
> >>>> On Mon, Mar 23, 2020 at 11:13 PM Matheus Tavares
> >>>> <matheus.bernardino@usp.br> wrote:
> >>>>>
> >>>>> In the last commit, git-grep learned to honor sparsity patterns. For
> >>>>> some use cases, however, it may be desirable to search outside the
> >>>>> sparse checkout. So add the '--ignore-sparsity' option, which restores
> >>>>> the old behavior. Also add the grep.ignoreSparsity configuration, to
> >>>>> allow setting this behavior by default.
> >>>>
> >>>> Should `--ignore-sparsity` be a global git option rather than a
> >>>> grep-specific one?  Also, should grep.ignoreSparsity rather be
> >>>> core.ignoreSparsity or core.searchOutsideSparsePaths or something?
> >>>
> >>> Great question.  I think "git diff" with various options would also
> >>> want to optionally be able to be confined within the sparse cone, or
> >>> checking the entire world by lazily fetching outside the sparsity.
> >> [...]
> >>> Regardless of the choice of the default, it would be a good
> >>> idea to make the subcommands consistently offer the same default and
> >>> allow the non-default views with the same UI.
> >>
> >> Yeah, it seems like a sensible path. Regarding implementation, there
> >> is the question that Elijah raised, of whether to use a global git
> >> option or separate but consistent options for each subcommand. I don't
> >> have much experience with sparse checkout to argument for one or
> >> another, so I would like to hear what others have to say about it.
> >>
> >> A question that comes to my mind regarding the global git option is:
> >> will --ignore-sparsity (or whichever name we choose for it [1]) be
> >> sufficient for all subcommands? Or may some of them require additional
> >> options for command-specific behaviors concerning sparsity patterns?
> >> Also, would it be OK if we just ignored the option in commands that do
> >> not operate differently in sparse checkouts (maybe, fetch, branch and
> >> send-email, for example)? And would it make sense to allow
> >> constructions such as `git --ignore-sparsity checkout` or even `git
> >> --ignore-sparsity sparse-checkout ...`?
> >
> > I think the same option would probably be sufficient for all
> > subcommands, though I have a minor question about the merge machinery
> > (below).  And generally, I think it would be unusual for people to
> > pass the command line flag; I suspect most would set a config option
> > for most cases and then only occasionally override it on the command
> > line.  Since that config option would always be set, I'd expect
> > commands that are unaffected to just ignore it (much like both "git -c
> > merge.detectRenames=true fetch" and "git --work-tree=othertree fetch"
> > will both ignore the irrelevant options rather than trying to detect
> > that they were specified and error out).
> >
> >> [1]: Does anyone have suggestions for the option/config name? The best
> >> I could come up with so far (without being too verbose) is
> >> --no-sparsity-constraints. But I fear this might sound generic. As
> >> Elijah already mentioned, --ignore-sparsity is not good either, as it
> >> introduces double negatives in code...
> >
> > Does verbosity matter that much?  I think people would set it in
> > config, and tab completion would make it pretty easy to complete in
> > any event.
> >
> > Anyway, maybe it will help if I provide a very rough first draft of
> > what changes we could introduce to Documentation/config/core.txt, and
> > then ask a bunch of my own questions about it below:
> >
> > """
> > core.restrictToSparsePaths::
> >         Only meaningful in conjuntion with core.sparseCheckoutCone.
> >         This option extends sparse checkouts (which limit which paths
> >         are written to the worktree), so that output and operations
> >         are also limited to the sparsity paths where possible and
> >         implemented.  The purpose of this option is to (1) focus
> >         output for the user on the portion of the repository that is
> >         of interest to them, and (2) enable potentially dramatic
> >         performance improvements, especially in conjunction with
> >         partial clones.
> > +
> > When this option is true, git commands such as log, diff, and grep may
> > limit their output to the directories specified by the sparse cone, or
> > to the intersection of those paths and any (like `*.c) that the user
> > might also specify on the command line.  (Note that this limit for
> > diff and grep only becomes relevant with --cached or when specifying a
> > REVISION, since a search of the working tree will automatically be
> > limited to the sparse paths that are present.)  Also, commands like
> > bisect may only select commits which modify paths within the sparsity
> > cone.  The merge machinery may use the sparse paths as a heuristic to
> > avoid trying to detect renames from within the sparsity cone to
> > outside the sparsity cone when at least one side of history only
> > touches paths within the sparsity cone (this can make the merge
> > machinery faster, but may risk modify/delete conflicts since upstream
> > can rename a file within the sparsity paths to a location outside
> > them).  Commands which export, integrity check, or create history will
> > always operate on full trees (e.g. fast-export, format-patch, fsck,
> > commit, etc.), unaffected by any sparsity patterns.
> > """
> >
> > Several questions here, of course:
> >
> >   * do people like or hate the name?  indifferent?  have alternate ideas?
>
> It's probably time to create a 'sparse-checkout' config space. That
> would allow
>
>         sparse-checkout.restrictGrep = true
>
> as an option. Or a more general
>
>         sparse-checkout.restrictCommands = true
>
> to make it clear that it affects multiple commands.

As I mentioned to Matheus, would a "sparse" config space be nicer?

> >   * should we restrict this to core.sparseCheckoutCone as I suggested
> > above or also allow people to do it with core.sparseCheckout without
> > the cone mode?  I think attempting to weld partial clones together
> > with core.sparseCheckout is crazy, so I'm tempted to just make it be
> > specific to cone mode and to push people to use it.  But I'm
> > interested in thoughts on the matter.
>
> Personally, I prefer cone mode and think it covers 99% of cases.
> However, there are some who are using a big directory full of large
> binaries and relying on file-prefix matches to get only the big
> binaries they need. Until they restructure their repositories to
> take advantage of cone mode, we should be considerate of the full
> sparse-checkout specification when possible.

I agree with everything you say here except the last word; if you
replaced "possible" with "practical" then I'd agree.  In particular, I
like the idea of a partial clone that defaults to grabbing all the
blobs in the sparse path specification; I think it'd be reasonable to
transfer the sparseCone specification to the server and have it use
that to walk history and make a packfile.  Transfering a sparse
specification that does not match the cone mode requirements to a
server and making it use that as it walks over all of history sounds
like a good way to overload the server.

> >   * should worktrees be affected?  (I've been an advocate of new
> > worktrees inheriting the sparse patterns of the worktree in use at the
> > time the new worktree was created.  Junio once suggested he didn't
> > like that and that worktrees should start out dense.  That seems
> > problematic to me in big repos with partial clones and sparse chckouts
> > in use.  Perhaps dense new worktrees is the behavior you get when
> > core.restrictToSparsePaths is false?)
>
> We should probably consider a `--sparse` option for `git worktree add`
> so we can allow interested users to add worktrees that initialize to
> a sparse-checkout. Optionally create a config option that would copy
> the sparse-checkout file from the current repo to the worktree.

Okay, but if someone runs a future

$ git clone --sparse $RELEVANT_DIRECTORIES user@server.name:path/to/repo.git
$ cd repo
<Blissfully work in their smaller repo without commands suddenly
downloading reams of unwanted data>

should the clone command automatically set this option for the user?
I don't like the idea of users having to remember to set this option
(and the restrictToSparsePaths option, and whatever other options are
needed to work in their smaller environment).  I'd really like there
to be a single flag, in the form of some clone option, that sets all
of this up.

> >   * does my idea for the merge machinery make folks uncomfortable?
> > Should that be a different option?  Being able to do trivial *tree*
> > merges for the huge portion of the tree outside the sparsity paths
> > would be a huge win, especially with partial clones, but it certainly
> > is different.  Then again, microsoft has disabled rename detection
> > entirely based on it being too expensive, so perhaps the idea of
> > rename-detection-within-your-cone-if-you-really-didn't-modify-anything-outside-the-cone-on-your-side-of-history
> > is a reasonable middle ground between off and on for rename detection.
>
> The part where you say " when at least one side of history only
> touches paths within the sparsity cone" makes me want to entertain
> the idea if it can be done cleanly.

Yeah, I still have to dig in and verify that this really works.

> I'm more concerned about the "git bisect" logic being restricted to
> the cone, since that is such an open-ended command for what is
> considered "good" or "bad".

If the sparse checkout has sufficient information for them to build
and test whatever predicate they are interested in, then surely
bisecting in a way that restricts to the cone would be a nice
optimization, right?  And if the cone doesn't have enough information
for them to build and test commits, then they would need to leave the
sparse checkout in order to bisect anyway.

> >   * what should the default be?  Junio suggested elsewhere[1] that
> > sparse-checkouts and partial clones should probably be welded together
> > (with partial clones downloading just history in the sparsity paths by
> > default), in which case having this option be true would be useful.
>
> My opinion on this is as follows: filtering blobs based on sparse-
> checkout patterns does not filter enough, and filtering trees based
> on sparse-checkout patterns filters too much. The costs are just
> flipped: having extra trees is not a huge problem but recovering from
> a "tree miss" is problematic. Having extra blobs is painful, but
> recovering from a "blob miss" is not a big deal.

Sounds like --filter=blob:none already solves your issues.  It doesn't
make me happy; I really want the history within the sparse cone to be
downloaded as part of the initial clone.  (I can see various ways that
downloading all trees would be easier, so if we end up downloading all
commits and all trees and just the blobs within the sparse cone, that
sounds fine to me.)

> > But it may also be slightly weird because it'll probably take us a
> > while to implement this; while the big warning in
> > git-sparse-checkout.txt certainly allows this:
> >         THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER
> >         COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN
> >         THE FUTURE.
> > It may still be slightly weird that the default behavior of commands
> > in the presence of sparse-checkouts changes release to release until
> > we get it all implemented.
>
> I appreciate that we put that warning at the top. We will be
> able to do more experimental things with the feature because
> of it. The idea I'm toying with is to have "git clone --sparse"
> set core.sparseCheckoutCone = true.

Sounds good to me.  We might also want to set worktrees.copySparsity
and sparse.restrictToCone (or whatever these end up being named) as
well.

> Also, if we are creating the "sparse-checkout.*" config space,
> we should "rename" core.sparseCheckoutCone to sparse-checkout.coneMode
> or something. We would need to support both for a while, for sure.

And, if we automatically migrate the setting and delete the old one,
do we prevent someone from successfully using an older git version
with the repo?  Or, if we don't automatically unset the old one, do we
risk the two values getting out of sync if they do switch to an older
git version?

  parent reply	other threads:[~2020-04-29 17:22 UTC|newest]

Thread overview: 123+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24  6:04 [RFC PATCH 0/3] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-03-24  6:11 ` [RFC PATCH 1/3] doc: grep: unify info on configuration variables Matheus Tavares
2020-03-24  7:57   ` Elijah Newren
2020-03-24 21:26     ` Junio C Hamano
2020-03-24 23:38       ` Matheus Tavares
2020-03-24  6:12 ` [RFC PATCH 2/3] grep: honor sparse checkout patterns Matheus Tavares
2020-03-24  7:15   ` Elijah Newren
2020-03-24 15:12     ` Derrick Stolee
2020-03-24 16:16       ` Elijah Newren
2020-03-24 17:02         ` Derrick Stolee
2020-03-24 23:01       ` Matheus Tavares Bernardino
2020-03-24 22:55     ` Matheus Tavares Bernardino
2020-04-21  2:10       ` Matheus Tavares Bernardino
2020-04-21  3:08         ` Elijah Newren
2020-04-22 12:08           ` Derrick Stolee
2020-04-23  6:09           ` Matheus Tavares Bernardino
2020-03-24  6:13 ` [RFC PATCH 3/3] grep: add option to ignore sparsity patterns Matheus Tavares
2020-03-24  7:54   ` Elijah Newren
2020-03-24 18:30     ` Junio C Hamano
2020-03-24 19:07       ` Elijah Newren
2020-03-25 20:18         ` Junio C Hamano
2020-03-30  3:23       ` Matheus Tavares Bernardino
2020-03-31 19:12         ` Elijah Newren
2020-03-31 20:02           ` Derrick Stolee
2020-04-27 17:15             ` Matheus Tavares Bernardino
2020-04-29 16:46               ` Elijah Newren
2020-04-29 17:21             ` Elijah Newren [this message]
2020-03-25 23:15     ` Matheus Tavares Bernardino
2020-03-26  6:02       ` Elijah Newren
2020-03-27 15:51         ` Junio C Hamano
2020-03-27 19:01           ` Elijah Newren
2020-03-30  1:12         ` Matheus Tavares Bernardino
2020-03-31 16:48           ` Elijah Newren
2020-05-10  0:41 ` [RFC PATCH v2 0/4] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-05-10  0:41   ` [RFC PATCH v2 1/4] doc: grep: unify info on configuration variables Matheus Tavares
2020-05-10  0:41   ` [RFC PATCH v2 2/4] config: load the correct config.worktree file Matheus Tavares
2020-05-11 19:10     ` Junio C Hamano
2020-05-12 22:55       ` Matheus Tavares Bernardino
2020-05-12 23:22         ` Junio C Hamano
2020-05-10  0:41   ` [RFC PATCH v2 3/4] grep: honor sparse checkout patterns Matheus Tavares
2020-05-11 19:35     ` Junio C Hamano
2020-05-13  0:05       ` Matheus Tavares Bernardino
2020-05-13  0:17         ` Junio C Hamano
2020-05-21  7:26           ` Elijah Newren
2020-05-21 17:35             ` Matheus Tavares Bernardino
2020-05-21 17:52               ` Elijah Newren
2020-05-22  5:49                 ` Matheus Tavares Bernardino
2020-05-22 14:26                   ` Elijah Newren
2020-05-22 15:36                     ` Elijah Newren
2020-05-22 20:54                       ` Matheus Tavares Bernardino
2020-05-22 21:06                         ` Elijah Newren
2020-06-10 11:40                     ` Derrick Stolee
2020-06-10 16:22                       ` Matheus Tavares Bernardino
2020-06-10 17:42                         ` Derrick Stolee
2020-06-10 18:14                           ` Matheus Tavares Bernardino
2020-06-10 20:12                         ` Elijah Newren
2020-06-10 19:58                       ` Elijah Newren
2020-05-21  7:36       ` Elijah Newren
2020-05-10  0:41   ` [RFC PATCH v2 4/4] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-05-10  4:23     ` Matheus Tavares Bernardino
2020-05-21 17:18       ` Elijah Newren
2020-05-21  7:09     ` Elijah Newren
2020-05-28  1:12   ` [PATCH v3 0/5] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-05-28  1:12     ` [PATCH v3 1/5] doc: grep: unify info on configuration variables Matheus Tavares
2020-05-28  1:13     ` [PATCH v3 2/5] t/helper/test-config: return exit codes consistently Matheus Tavares
2020-05-30 14:29       ` Elijah Newren
2020-06-01  4:36         ` Matheus Tavares Bernardino
2020-05-28  1:13     ` [PATCH v3 3/5] config: correctly read worktree configs in submodules Matheus Tavares
2020-05-30 14:49       ` Elijah Newren
2020-06-01  4:38         ` Matheus Tavares Bernardino
2020-05-28  1:13     ` [PATCH v3 4/5] grep: honor sparse checkout patterns Matheus Tavares
2020-05-30 15:48       ` Elijah Newren
2020-06-01  4:44         ` Matheus Tavares Bernardino
2020-06-03  2:38           ` Elijah Newren
2020-06-10 17:08             ` Matheus Tavares Bernardino
2020-05-28  1:13     ` [PATCH v3 5/5] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-05-30 16:18       ` Elijah Newren
2020-06-01  4:45         ` Matheus Tavares Bernardino
2020-06-03  2:39           ` Elijah Newren
2020-06-10 21:15             ` Matheus Tavares Bernardino
2020-06-11  0:35               ` Elijah Newren
2020-06-12 15:44     ` [PATCH v4 0/6] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-06-12 15:44       ` [PATCH v4 1/6] doc: grep: unify info on configuration variables Matheus Tavares
2020-06-12 15:45       ` [PATCH v4 2/6] t/helper/test-config: return exit codes consistently Matheus Tavares
2020-06-12 15:45       ` [PATCH v4 3/6] t/helper/test-config: facilitate addition of new cli options Matheus Tavares
2020-06-12 15:45       ` [PATCH v4 4/6] config: correctly read worktree configs in submodules Matheus Tavares
2020-06-16 19:13         ` Elijah Newren
2020-06-21 16:05           ` Matheus Tavares Bernardino
2020-09-01  2:41         ` Jonathan Nieder
2020-09-01 21:44           ` Matheus Tavares Bernardino
2020-06-12 15:45       ` [PATCH v4 5/6] grep: honor sparse checkout patterns Matheus Tavares
2020-06-12 15:45       ` [PATCH v4 6/6] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-06-16 22:31       ` [PATCH v4 0/6] grep: honor sparse checkout and add option to ignore it Elijah Newren
2020-09-02  6:17       ` [PATCH v5 0/8] " Matheus Tavares
2020-09-02  6:17         ` [PATCH v5 1/8] doc: grep: unify info on configuration variables Matheus Tavares
2020-09-02  6:17         ` [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Matheus Tavares
2020-09-02  6:57           ` Eric Sunshine
2020-09-02 16:16             ` Matheus Tavares Bernardino
2020-09-02 16:38               ` Eric Sunshine
2020-09-02  6:17         ` [PATCH v5 3/8] t/helper/test-config: be consistent with exit codes Matheus Tavares
2020-09-02  6:17         ` [PATCH v5 4/8] t/helper/test-config: check argc before accessing argv Matheus Tavares
2020-09-02  7:18           ` Eric Sunshine
2020-09-02  6:17         ` [PATCH v5 5/8] t/helper/test-config: unify exit labels Matheus Tavares
2020-09-02  7:30           ` Eric Sunshine
2020-09-02  6:17         ` [PATCH v5 6/8] config: correctly read worktree configs in submodules Matheus Tavares
2020-09-02 20:15           ` Jonathan Nieder
2020-09-09 13:04             ` Matheus Tavares Bernardino
2020-09-09 23:32               ` Jonathan Nieder
2020-09-02  6:17         ` [PATCH v5 7/8] grep: honor sparse checkout patterns Matheus Tavares
2020-09-02  6:17         ` [PATCH v5 8/8] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-09-10 17:21         ` [PATCH v6 0/9] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 1/9] doc: grep: unify info on configuration variables Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 2/9] t1308-config-set: avoid false positives when using test-config Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 3/9] t/helper/test-config: be consistent with exit codes Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 4/9] t/helper/test-config: diagnose missing arguments Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 5/9] t/helper/test-config: unify exit labels Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 6/9] config: make do_git_config_sequence receive a 'struct repository' Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 7/9] config: correctly read worktree configs in submodules Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 8/9] grep: honor sparse checkout patterns Matheus Tavares
2020-09-10 17:21           ` [PATCH v6 9/9] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2021-02-09 21:33           ` [PATCH v7] grep: honor sparse-checkout on working tree searches Matheus Tavares
2021-02-09 23:23             ` Junio C Hamano
2021-02-10  6:12               ` Elijah Newren

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-BHKYR9QBcAG_pM6DniaeGS40X=ErKGGsvWa_KhogCZzEA@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=matheus.bernardino@usp.br \
    --cc=pclouds@gmail.com \
    --cc=stolee@gmail.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).