Subject: Re: [PATCH v5 0/4] Documentation/git-sparse-checkout.txt: add an OPTIONS section

From: Derrick Stolee <[email-masked]>
Date: Tue, 22 Mar 2022 11:05:12 -0400

On 3/19/22 2:19 AM, Shaoxuan Yuan wrote:
> == Updates Log ==
> 
> Changes since v4
> 
>  * ship a multi-patch series to reflect the history
>  * base on Elijah's series [1]
>  * changes according to Junio [2]
> 
> [1] https://lore.kernel.org/git/[email-masked]/
> [2] https://lore.kernel.org/git/[email-masked]/
> 
> == Overview ==
> 
> Add an OPTIONS section to the manual. Also provide corresponding text changes
> for better understanding the topic.
> Shaoxuan Yuan (4):
>   Documentation/git-sparse-checkout.txt: add an OPTIONS section
>   Documentation/git-sparse-checkout.txt: move OPTIONS after COMMANDS
>   Documentation/git-sparse-checkout.txt: some reword and modifications
>   Documentation/git-sparse-checkout.txt: some reword and modifications

Hi Shaoxuan,

This commit organization seems to be one that you created naturally from
responding to feedback. Unfortunately, we expect that each patch is
expected to be created with the intent of presenting changes in an
efficient way. This helps both with review and future history
investigations. See Documentation/SubmittingPatches for more information.

In particular, patches 2-4 are of the mode "Modify the changes from patch
1 based on feedback". Instead, we expect that you update that single patch
unless there is a clear reason why two separate changes are valuable.

When submitting new versions to a series, adding a range-diff to your
cover letter helps reviewers see what changed between versions, so we
still see how you responded to our feedback in a more isolated way.

I took your changes and applied them to en/sparse-cone-becomes-default and
got the diff below end-to-end. This is much easier to read. It also makes
some things clear (like an unnecessary whitespace change).

I think that this diff would be better served as a single patch. I'll
review this diff inline below:

> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index e4a29a2baa9..482ecfe28c5 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -48,6 +48,14 @@ COMMANDS
>  	following the 'set' subcommand, and update the working directory to
>  	match.
>  +
> +By default, the arguments to the `set` command are interpreted as a
> +list of directories. The sparse-checkout patterns are set to match
> +all files within those directories, recursively, as well as any file
> +directly contained in a parent of those directories. See INTERNALS
> +-- CONE PATTERN SET below for full details. If --no-cone is specified,
> +then the arguments are interpreted as sparse-checkout patterns. See
> +INTERNALS -- FULL PATTERN SET below for more information.
> ++
>  To ensure that adjusting the sparse-checkout settings within a worktree
>  does not alter the sparse-checkout settings in other worktrees, the 'set'
>  subcommand will upgrade your repository config to use worktree-specific
> @@ -55,48 +63,10 @@ config if not already present. The sparsity defined by the arguments to
>  the 'set' subcommand are stored in the worktree-specific sparse-checkout
>  file. See linkgit:git-worktree[1] and the documentation of
>  `extensions.worktreeConfig` in linkgit:git-config[1] for more details.
> -+
> -When the `--stdin` option is provided, the directories or patterns are
> -read from standard in as a newline-delimited list instead of from the
> -arguments.
> -+
> -By default, the input list is considered a list of directories, matching
> -the output of `git ls-tree -d --name-only`.  This includes interpreting
> -pathnames that begin with a double quote (") as C-style quoted strings.
> -Note that all files under the specified directories (at any depth) will
> -be included in the sparse checkout, as well as files that are siblings
> -of either the given directory or any of its ancestors (see 'CONE PATTERN
> -SET' below for more details).  In the past, this was not the default,
> -and `--cone` needed to be specified or `core.sparseCheckoutCone` needed
> -to be enabled.
> -+
> -When `--no-cone` is passed, the input list is considered a list of
> -patterns.  This mode is harder to use, and unless you can keep the
> -number of patterns small, its design also scales poorly.  It used to be
> -the default mode, but we do not recommend using it.  It does not work
> -with the `--sparse-index` option, and will likely be incompatible with
> -other new features as they are added.  See the "Non-cone Problems"
> -section below and the "Sparse Checkout" section of
> -linkgit:git-read-tree[1] for more details.
> -+
> -Use the `--[no-]sparse-index` option to use a sparse index (the
> -default is to not use it).  A sparse index reduces the size of the
> -index to be more closely aligned with your sparse-checkout
> -definition. This can have significant performance advantages for
> -commands such as `git status` or `git add`.  This feature is still
> -experimental. Some commands might be slower with a sparse index until
> -they are properly integrated with the feature.
> -+
> -**WARNING:** Using a sparse index requires modifying the index in a way
> -that is not completely understood by external tools. If you have trouble
> -with this compatibility, then run `git sparse-checkout init --no-sparse-index`
> -to rewrite your index to not be sparse. Older versions of Git will not
> -understand the sparse directory entries index extension and may fail to
> -interact with your repository until it is disabled.
>  
>  'add'::
>  	Update the sparse-checkout file to include additional directories
> -	(in cone mode) or patterns (in non-cone mode).  By default, these
> +	(in cone mode) or patterns (in non-cone mode). By default, these

This whitespace change doesn't need to be here. I do think a single space
is more common in our docs, but it has no effect on the rendered docs, so
this is unnecessary noise for the diff.

>  	directories or patterns are read from the command-line arguments,
>  	but they can be read from stdin using the `--stdin` option.
>  
> @@ -109,11 +79,6 @@ interact with your repository until it is disabled.
>  	cases, it can make sense to run `git sparse-checkout reapply` later
>  	after cleaning up affected paths (e.g. resolving conflicts, undoing
>  	or committing changes, etc.).
> -+
> -The `reapply` command can also take `--[no-]cone` and `--[no-]sparse-index`
> -flags, with the same meaning as the flags from the `set` command, in order
> -to change which sparsity mode you are using without needing to also respecify
> -all sparsity paths.
>  
>  'disable'::
>  	Disable the `core.sparseCheckout` config setting, and restore the
> @@ -139,6 +104,41 @@ paths to pass to a subsequent 'set' or 'add' command.  However,
>  the disable command, so the easy restore of calling a plain `init`
>  decreased in utility.
>  
> +
> +OPTIONS
> +-------
> +'--[no-]cone'::
> +	Use with the `set` and `reapply` commands.
> +	Specify using cone mode or not. The default is to use cone mode.
> +
> +'--[no-]sparse-index'::
> +	Use with the `set` and `reapply` commands.
> +	Specify using a sparse index or not. The default is to not use a
> +	sparse index.
> ++
> +Use the `--[no-]sparse-index` option to use a sparse index (the

I find this a bit confusing. Maybe just "`--sparse-index`" would make
it clear that leaving out the [no-] is for enabling it.

> +default is to not use it).  A sparse index reduces the size of the
> +index to be more closely aligned with your sparse-checkout
> +definition. This can have significant performance advantages for
> +commands such as `git status` or `git add`.  This feature is still
> +experimental. Some commands might be slower with a sparse index until
> +they are properly integrated with the feature.
> ++
> +**WARNING:** Using a sparse index requires modifying the index in a way
> +that is not completely understood by external tools. If you have trouble
> +with this compatibility, then run `git sparse-checkout init --no-sparse-index`
> +to rewrite your index to not be sparse. Older versions of Git will not
> +understand the sparse directory entries index extension and may fail to
> +interact with your repository until it is disabled.
> +
> +'--stdin'::
> +	Use with the `set` and `add` commands.
> ++
> +When the `--stdin` option is provided, the directories or patterns are
> +read from standard in as a newline-delimited list instead of from the
> +arguments.
> +
> +

I find it a lot easier to see these options being moved and updated at the
same time.

Thanks,
-Stolee