Re: [PATCH v4] sparse-checkout.txt: new document with sparse-checkout directions

[Elijah Newren <newren@gmail.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!

:-)