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

From: Derrick Stolee <derrickstolee@github.com>
Date: Nov 7, 2022, 20:44 UTC
Message-ID: <f3345a9e-e7f1-4230-d30a-0608eb69513d@github.com>
In-Reply-To: <pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com>

On 11/6/22 1:04 AM, Elijah Newren via GitGitGadget wrote:

Show 7 quoted lines

> The --sparse-index work has been mostly complete (or at least released
> into production even if some small edges remain) for quite some time
> now.  We have also had several discussions on flag and config names,
> though we never came to solid conclusions.  Stolee once upon a time
> suggested putting all these into some document in
> Documentation/technical[3], which Victoria recently also requested[4].
> I'm behind the times, but here's a patch attempting to finally do that.

This is a correct summary of where the sparse index feature is right now.

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.

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.

Show 13 quoted lines

> +  * 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).

Show 8 quoted lines

> +  * 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.

Show 11 quoted lines

> +  * 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.

Show 10 quoted lines

> +  * 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.

Show 8 quoted lines

> + * 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.

Show 6 quoted lines

> + * 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.

> +   * 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.

> +   * 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.

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!

Thanks, -Stolee