Re: [PATCH v4 1/1] Documentation/git-sparse-checkout.txt: add an OPTIONS section

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2022, 16:47 UTC
Message-ID: <xmqqfsnfb42c.fsf@gitster.g>
In-Reply-To: <20220317123718.480093-2-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:

> Add an OPTIONS section to the manual and move the descriptions about
> these options from COMMANDS to the section.

The above description does not seem to match what the patch does at all, though. Sent a wrong patch, possibly just the tip of a series, when you needed to send more than one patches? For example, the header for the hunk -103,33 tells us that the file already has OPTIONS section before this patch gets applied.

Show 13 quoted lines

> @@ -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

That reads well.

Show 10 quoted lines

> @@ -59,8 +67,10 @@ file. See linkgit:git-worktree[1] and the documentation of
>  'add'::
>  	Update the sparse-checkout file to include additional directories
>  	(in cone mode) or patterns (in non-cone mode).  By default, these
> -	directories or patterns are read from the command-line arguments,
> -	but they can be read from stdin using the `--stdin` option.
> +	directories or patterns are read from the command-line arguments.
> +  These directories or patterns are interpreted the same way as stated
> +  above in `set` command, and they can be read from stdin using the
> +  `--stdin` option.

The original removed by the patch said "directories or patterns", and I understand that this change is an attempt to say that the command chooses between directories and patterns using the same criteria as the "set" command, but the added "are interpreted the same way as ..." in the middle interrupts the flow of thought the sentence conveys. To me, it looks like the first sentence gives clear enough explanation of that already.

Broken indentation aside, I do not see why this change is needed.

Show 12 quoted lines

> @@ -103,33 +113,9 @@ OPTIONS
>  	Use with the `set` and `reapply` commands.
>  	Specify using cone mode or not. The default is to use cone mode.
>  +
> -For `set` command:
> ...
> -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.
> +For the `set` command, the option to use cone mode or not changes
> +the interpretation of the remaining arguments to either be a list
> +of directories or a list of patterns.

These three lines are not technically incorrect, but is not written in a way that helps readers.

Read these three lines a few times, pretending that you have never used the sparse-checkout, and try to answer this question: "I am giving a few arguments to the command using the cone mode. Are they taken as directories, or patterns?"

It is hard for me to guess what is being improved upon because I do not have the preimage of this hunk, but the new text is much less clear than "directories (in cone mode) or patterns (in non-cone mode)" we saw earlier in the description for the 'add' command, which would help us answer the question (answer: they are taken as directories).

Thanks.