git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Victoria Dye <vdye@github.com>,
	Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>,
	Matheus Tavares <matheus.bernardino@usp.br>,
	ZheNing Hu <adlternative@gmail.com>,
	Glen Choo <chooglen@google.com>,
	Martin von Zweigbergk <martinvonz@google.com>
Subject: Re: [PATCH v4] sparse-checkout.txt: new document with sparse-checkout directions
Date: Tue, 15 Nov 2022 20:39:02 -0800	[thread overview]
Message-ID: <CABPp-BGHMsMxP6e7p0HAZA=ugk+GY3XW6_EaTN=HzaLQYAzQYA@mail.gmail.com> (raw)
In-Reply-To: <f3345a9e-e7f1-4230-d30a-0608eb69513d@github.com>

On Mon, Nov 7, 2022 at 12:44 PM Derrick Stolee <derrickstolee@github.com> wrote:
>
> It also is a highly-requested document. Thank you for working so hard on
> it and sorry for being slow to sign off on your edits since v1.
>
> Today, I'm rereading the whole document anew, but I'll avoid any nits
> since I think you are converging on a solid foundation for us to build on.

Thanks for reading it over!

And sorry for my delay in responding; my Git time has been sadly
limited as of late.

> Mostly, if you asked a question in the doc, I'll reply. Nothing is binding
> since the point is to ask the question in the context of the problem
> statement and examples. We should remember to update this document when we
> actually implement the options, so the decisions are documented here
> instead of leaving answered questions lingering.

Yes, I think this sounds good.

> > +  * Do the options --scope={sparse,all} sound good to others?  Are there better
> > +    options?
> > +    * Names in use, or appearing in patches, or previously suggested:
> > +      * --sparse/--dense
> > +      * --ignore-skip-worktree-bits
> > +      * --ignore-skip-worktree-entries
> > +      * --ignore-sparsity
> > +      * --[no-]restrict-to-sparse-paths
> > +      * --full-tree/--sparse-tree
> > +      * --[no-]restrict
> > +      * --scope={sparse,all}
> > +      * --focus/--unfocus
> > +      * --limit/--unlimited
>
> I'm partial to --scope={sparse|all} (with the option to add another
> value if we see the need).
>
> > +  * If a config option is added (sparse.scope?) what should the values and
> > +    description be?  "sparse" (behavior A), "worktree-sparse-history-dense"
> > +    (behavior B), "dense" (behavior C)?  There's a risk of confusion,
> > +    because even for Behaviors A and B we want some commands to be
> > +    full-tree and others to operate sparsely, so the wording may need to be
> > +    more tied to the usecases and somehow explain that.  Also, right now,
> > +    the primary difference we are focusing is just the history-querying
> > +    commands (log/diff/grep).  Previous config suggestion here: [13]
>
> Personally, I think we should have the same values for 'sparse.scope' and
> '--scope=<X>'. For now, let's pick one behavior for the 'sparse' value and
> we can add a new value to differentiate between A and B when necessary in
> the future.

I think this is untenable.  For example, under behavior B:

   * default to --scope=all: diff REV, grep REV, log, etc.
   * default to --scope=sparse: restore, add, diff [without REV or
--cached], etc.

So sparse.scope=all would not yield behavior B.  In fact, there'd be
no way to behavior B since it is inherently a mix of different types
of scopes, as reflected in its "oversimplified" description:

   "Restrict worktree operations to sparse specification; have any
history operations work across all files"

I think it'd *also* potentially set us up for problems under behavior
A.  Behavior A is roughly thought of as --scope=sparse for everything,
but some commands ignore the sparse specification entirely -- commit,
fast-export, bundle, stash, apply, etc.  Perhaps those other
subcommands just never take a --scope option, and thus we have no
issues.  But what if someone asks for a feature where they want to
just apply a subset of the patch with "stash pop" or "apply", and
particularly the subset overlapping with the sparse specification?  Or
perhaps a user wants to do a fast-export of a subset of the repository
-- which they can already do by specifying paths already on the
command line -- but they don't want to have to type all the paths and
want a simple flag for limiting to the sparse specification?  If so,
--scope=sparse is a pretty clear flag that could be used.  But then
we'd have the problem that:

   * default to --scope=all: commit, fast-export, bundle, stash,
apply, and a few others
   * default to --scope=sparse: pretty much everything else

If any of the full-tree commands ever morphs in this direction, then
sparse.scope=sparse would *not* yield behavior A, and there'd be no
way to get it, because behavior A would also be a mix of different
types of scopes.

Personally, I can't imagine that either having --scope=sparse or
--scope=all be the default for all commands would even be a useful
mode for anyone.  So, I think the values of scope.sparse should not be
either "sparse" or "all".

> > +  * Is `--no-expand` a good alias for ls-files's `--sparse` option?
> > +    (`--sparse` does not map to either `--scope=sparse` or `--scope=all`,
> > +    because in non-cone mode it does nothing and in cone-mode it shows the
> > +    sparse directory entries which are technically outside the sparse
> > +    specification)
> > +
> > +  * Under Behavior A:
> > +    * Does ls-files' `--no-expand` override the default `--scope=all`, or
> > +      does it need an extra flag?
> > +    * Does ls-files' `-t` option imply `--scope=all`?
> > +    * Does update-index's `--[no-]skip-worktree` option imply `--scope=all`?
>
> Since the --no-expand option is rather new, and we have a big experimental
> banner on the sparse-checkout documentation, it might be good to plan for
> a deprecation of these non-standard options. We could start by making them
> aliases for the --scope=sparse option, but with a warning that the option
> is deprecated and we will _remove_ the option in a future version. We can
> document here which versions we expect those removals to happen.

I do agree that elsewhere aliasing flags to --scope=sparse makes sense.

But that's not applicable here.  `--no-expand` does not exist yet; it
was suggested as a rename for `--sparse` because ls-files' `--sparse`
option cannot be mapped to either --scope=sparse or --scope=all (nor
any other --scope= option we thought of).  The reason for a different
name was specifically that this option name didn't fit the mold and we
know of no analogous options anywhere.  --scope=sparse means only show
the non-SKIP_WORKTREE entries (which would exclude the sparse
directories and everything under them), while --scope=all means show
all the files (without the directories).  This option, in contrast,
means to show the non-SKIP_WORKTREE file entries plus the
SKIP_WORKTREE directory entries.

> > +  * sparse-checkout: once behavior A is fully implemented, should we take
> > +    an interim measure to ease people into switching the default?  Namely,
> > +    if folks are not already in a sparse checkout, then require
> > +    `sparse-checkout init/set` to take a
> > +    `--set-scope=(sparse|worktree-sparse-history-dense|dense)` flag (which
> > +    would set sparse.scope according to the setting given), and throw an
> > +    error if the flag is not provided?  That error would be a great place
> > +    to warn folks that the default may change in the future, and get them
> > +    used to specifying what they want so that the eventual default switch
> > +    is seamless for them.
>
> I'm not sure that we need a warning here. I think picking an initial default
> is good enough. Let's reconsider this warning after we have more implementation
> changes that provide a choice between behaviors A and B.
>
> > +=== Implementation Goals/Plans ===
> > +
> > + * Get buy-in on this document in general.
>
> Consider me bought-in.

Wahoo!

> > + * Figure out answers to the 'Implementation Questions' sections (above)
> > +
> > + * Fix bugs in the 'Known bugs' section (below)
> > +
> > + * Provide some kind of method for backfilling the blobs within the sparse
> > +   specification in a partial clone
> > +
> > + [Below here is kind of spitballing since the first two haven't been resolved]
>
> We can update this document as we gain clarity after the first few updates.
>
> > + * update-index: flip the default to --no-ignore-skip-worktree-entries,
> > +   nuke this stupid "Oh, there's a bug?  Let me add a flag to let users
> > +   request that they not trigger this bug." flag
> > +
> > + * Flags & Config
> > +   * Make `--sparse` in add/rm/mv a deprecated alias for `--scope=all`
>
> This '--sparse' deprecation can eventually be a removal, I think.

Sounds fair.  Should I clarify that in the document as well?

> > +   * Make `--ignore-skip-worktree-bits` in checkout-index/checkout/restore
> > +     a deprecated aliases for `--scope=all`
>
> This one might be harder to remove since it's much older. We can consider
> it, though.

Yeah, if we end up with deprecated-but-kept-around, that's fine so
long as we recommend the new flag over the old one.

> > +   * Create config option (sparse.scope?), tie it to the "Cliff notes"
> > +     overview
>
> Implementation detail: it might be nice to create a parse-opt macro that
> will read the '--scope={sparse|all}' command-line option but _also_
> create a method to initialize the value to the 'sparse.scope' config
> option. These can both happen with the very first implementation of the
> command-line option and all future integrations can follow that pattern to
> get both options.

I'm not sure how this could work, since `sparse.scope` should not use
the values {sparse,all}, and the correct default scope is
command-dependent for both behavior B and behavior A.

> Thanks for working so hard on this doc. I think this version is ready to
> merge down. Let's get started on this work. I'm excited!

:-)

  reply	other threads:[~2022-11-16  4:39 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-25  0:09 [PATCH] sparse-checkout.txt: new document with sparse-checkout directions Elijah Newren via GitGitGadget
2022-09-26 17:20 ` Junio C Hamano
2022-09-26 17:38 ` Junio C Hamano
2022-09-27  3:05   ` Elijah Newren
2022-09-27  4:30     ` Junio C Hamano
2022-09-26 20:08 ` Victoria Dye
2022-09-26 22:36   ` Junio C Hamano
2022-09-27  7:30     ` Elijah Newren
2022-09-27 16:07       ` Junio C Hamano
2022-09-28  6:13         ` Elijah Newren
2022-09-27  6:09   ` Elijah Newren
2022-09-27 16:42   ` Derrick Stolee
2022-09-28  5:42     ` Elijah Newren
2022-09-27 15:43 ` Junio C Hamano
2022-09-28  7:49   ` Elijah Newren
2022-09-27 16:36 ` Derrick Stolee
2022-09-28  5:38   ` Elijah Newren
2022-09-28 13:22     ` Derrick Stolee
2022-10-06  7:10       ` Elijah Newren
2022-10-06 18:27         ` Derrick Stolee
2022-10-07  2:56           ` Elijah Newren
2022-09-30  9:54     ` ZheNing Hu
2022-10-06  7:53       ` Elijah Newren
2022-10-15  2:17         ` ZheNing Hu
2022-10-15  4:37           ` Elijah Newren
2022-10-15 14:49             ` ZheNing Hu
2022-09-30  9:09   ` ZheNing Hu
2022-09-28  8:32 ` [PATCH v2] " Elijah Newren via GitGitGadget
2022-10-08 22:52   ` [PATCH v3] " Elijah Newren via GitGitGadget
2022-11-06  6:04     ` [PATCH v4] " Elijah Newren via GitGitGadget
2022-11-07 20:44       ` Derrick Stolee
2022-11-16  4:39         ` Elijah Newren [this message]
2022-11-15  4:03       ` ZheNing Hu
2022-11-16  3:18         ` ZheNing Hu
2022-11-16  6:51           ` Elijah Newren
2022-11-16  5:49         ` Elijah Newren
2022-11-16 10:04           ` ZheNing Hu
2022-11-16 10:10             ` ZheNing Hu
2022-11-16 14:33               ` ZheNing Hu
2022-11-19  2:36                 ` Elijah Newren
2022-11-19  2:15             ` Elijah Newren
2022-11-23  9:08               ` ZheNing Hu
2023-01-14 10:18           ` ZheNing Hu
2023-01-20  4:30             ` Elijah Newren
2023-01-23 15:05               ` ZheNing Hu
2023-01-24  3:17                 ` 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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BGHMsMxP6e7p0HAZA=ugk+GY3XW6_EaTN=HzaLQYAzQYA@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=adlternative@gmail.com \
    --cc=chooglen@google.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=martinvonz@google.com \
    --cc=matheus.bernardino@usp.br \
    --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).