On Tue, Oct 26, 2021, Eric Sunshine wrote:
> On Tue, Oct 26, 2021 at 11:11 AM Jean-Noël Avila via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>> doc: fix grammar rules in commands'syntax
>
> Missing space.
>
>> According to the coding guidelines, the placeholders must:
>> * be in small letters
>> * enclosed in angle brackets
>>
>> Some other rules are also applied.
>
> Perhaps just mention them here?
>
> * use hyphens rather than underscores or spaces
> between words
There are a lot more places with spaces within placeholders. Will extend.
> * indicate repetition with `...` rather than `*`
>
> were some that I saw while reading.
>
> Overall, the patch looks good. One or two notes below...
Thanks for taking time to review this cumbersome patch.
>
>> Signed-off-by: Jean-Noël Avila <jn.avila@xxxxxxx>
>> ---
>> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
>> @@ -11,7 +11,7 @@ SYNOPSIS
>> -'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
>> +'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new-branch>] [<start-point>]
>
> Nice to see this attention to detail.
>
>> @@ -43,7 +43,7 @@ You could omit `<branch>`, in which case the command degenerates to
>> -'git checkout' -b|-B <new_branch> [<start point>]::
>> +'git checkout' -b|-B <new-branch> [<start-point>]::
>
> Likewise.
>
>> @@ -145,11 +145,11 @@ as `ours` (i.e. "our shared canonical history"), while what you did
>> --b <new_branch>::
>> +-b <new-branch>::
>> Create a new branch named `<new_branch>` and start it at
>> `<start_point>`; see linkgit:git-branch[1] for details.
>
> The names in the description need fixing too: s/_/-/g
Ack
>
>> --B <new_branch>::
>> +-B <new-branch>::
>> Creates the branch `<new_branch>` and start it at `<start_point>`;
>> if it already exists, then reset it to `<start_point>`. This is
>> equivalent to running "git branch" with "-f"; see
>
> Likewise: s/_/-/g
Ack
>
>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
>> @@ -9,20 +9,20 @@ git-config - Get and set repository or global options
>> -'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
>> +'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch <name> <URL>
>
> The commit message talks about using lowercase, so perhaps? s/URL/url/
URL is not a word, but an acronym. Lowercasing it looks weird, but
that's personal taste. URL appears with a lot different casings across
the documents.
>
>> @@ -102,7 +102,7 @@ OPTIONS
>> ---get-urlmatch name URL::
>> +--get-urlmatch <name> <URL>::
>> When given a two-part name section.key, the value for
>> section.<url>.key whose <url> part matches the best to the
>> given URL is returned (if no such key exists, the value for
>
> Ditto. In fact, lowercase <url> is already used in the description,
> but not in the item line.
>
> If wanting to match other documentation files, this would also be
> typeset as `<url>` rather than <url> in the description text, but that
> change may be well outside the scope of this patch.
>
>> @@ -145,7 +145,7 @@ See also <<FILES>>.
>> --f config-file::
>> +-f <config-file>::
>> --file config-file::
>
> Need to apply brackets around `config-file` for the `--file` option
> too, just as you did for short `-f`.
>
>> @@ -246,7 +246,7 @@ Valid `<type>`'s include:
>> ---get-colorbool name [stdout-is-tty]::
>> +--get-colorbool <name> [<stdout-is-tty>]::
>>
>> Find the color setting for `name` (e.g. `color.diff`) and output
>> "true" or "false". `stdout-is-tty` should be either "true" or
>
> Should you wrap `stdout-is-tty` within angle brackets within the
> description too?
>
The rules for referring to placeholder in text wasn't defined (use angle
brackets or not, use monospace or not) . Usage in manpages is diverse.
Logically this would be monospace for any token of the synopsis and
angle bracket when it's a placeholder.
However, this is a larger fixup in manpages, that would require its own
patch.
>> @@ -257,7 +257,7 @@ Valid `<type>`'s include:
>> ---get-color name [default]::
>> +--get-color <name> [<default>]::
>>
>> Find the color configured for `name` (e.g. `color.diff.new`) and
>> output it as the ANSI color escape sequence to the standard
>
> And here? <name> rather than `name`?
>
>> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
>> @@ -8,7 +8,7 @@ git-credential - Retrieve and store user credentials
>> -git credential <fill|approve|reject>
>> +git credential [fill|approve|reject]
>
> The original was indeed wrong but the revised text is also slightly
> misleading. The square brackets suggest that the "action" is optional,
> but in fact it's not, so this should be using parentheses:
>
> git credential (fill|approve|reject)
>
> Also, the usage text in builtin/credential.c is wrong:
>
> % git credential
> usage: git credential [fill|approve|reject]
>
> It should be using parentheses, as well, but fixing that may be
> outside the scope of this patch (and can be done later).
>
>> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
>> @@ -9,11 +9,11 @@ git-cvsimport - Salvage your data out of another SCM people love to hate
>> -'git cvsimport' [-o <branch-for-HEAD>] [-h] [-v] [-d <CVSROOT>]
>> +'git cvsimport' [-o <branch-for-HEAD>] [-h] [-v] [-d <cvsroot>]
>> [-A <author-conv-file>] [-p <options-for-cvsps>] [-P <file>]
>> - [-C <git_repository>] [-z <fuzz>] [-i] [-k] [-u] [-s <subst>]
>> + [-C <git-repository>] [-z <fuzz>] [-i] [-k] [-u] [-s <subst>]
>> [-a] [-m] [-M <regex>] [-S <regex>] [-L <commitlimit>]
>> - [-r <remote>] [-R] [<CVS_module>]
>> + [-r <remote>] [-R] [<CVS-module>]
>
> I wonder if <commitlimit> should be changed to <commit-limit>?
>
>> diff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt
>> @@ -12,7 +12,7 @@ SYNOPSIS
>> 'git fsck' [--tags] [--root] [--unreachable] [--cache] [--no-reflogs]
>> [--[no-]full] [--strict] [--verbose] [--lost-found]
>> [--[no-]dangling] [--[no-]progress] [--connectivity-only]
>> - [--[no-]name-objects] [<object>*]
>> + [--[no-]name-objects] [<object>...]
>
> Okay.
>
>> diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
>> @@ -79,7 +79,7 @@ repository. If not specified, fall back to the default name (currently
>> ---shared[=(false|true|umask|group|all|world|everybody|0xxx)]::
>> +--shared[=(false|true|umask|group|all|world|everybody|0<octal>)]::
>
> This feels slightly unusual; I'd have expected just plain `<octal>`
> without the leading `0`, and...
>
>> @@ -110,13 +110,14 @@ the repository permissions.
>> -'0xxx'::
>> +'0<octal>'::
>
> .. this would also say just `<octal>`, and...
>
>> -'0xxx' is an octal number and each file will have mode '0xxx'. '0xxx' will
>> -override users' umask(2) value (and not only loosen permissions as 'group' and
>> -'all' does). '0640' will create a repository which is group-readable, but not
>> -group-writable or accessible to others. '0660' will create a repo that is
>> -readable and writable to the current user and group, but inaccessible to others.
>> +'0<octal>' is an octal number and each file will have mode
>> +'0<octal>'. '0<octal>' will override users' umask(2) value (and not
>> +only loosen permissions as 'group' and 'all' does). '0640' will create
>> +a repository which is group-readable, but not group-writable or
>> +accessible to others. '0660' will create a repo that is readable and
>> +writable to the current user and group, but inaccessible to others.
>
> ... this would then go on to explain that `<octal>` "... is an octal
> number starting with literal `0`...".
>
> But it's subjective and others might feel differently.
>
>> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
>> @@ -81,7 +81,7 @@ produced by `--stat`, etc.
>> -<revision range>::
>> +<revision-range>::
>> Show only commits in the specified revision range. When no
>> <revision range> is specified, it defaults to `HEAD` (i.e. the
>> whole history leading to the current commit). `origin..HEAD`
>
> Also need to fix the description: s/<revision range>/<revision-range>/
>
>> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
>> @@ -10,8 +10,8 @@ SYNOPSIS
>> 'git ls-files' [-z] [-t] [-v] [-f]
>> - (--[cached|deleted|others|ignored|stage|unmerged|killed|modified])*
>> - (-[c|d|o|i|s|u|k|m])*
>> + [--(cached|deleted|others|ignored|stage|unmerged|killed|modified)...]
>> + [-(c|d|o|i|s|u|k|m)...]
>
> I wonder if we could make it easier on users if written like this:
>
> [--cached|--deleted|--others|--blah|--blah]...
> [-c|-d|-o|-i|-s|-u|-k|-m]...
>
> But that's subjective.
It would better match synopsis of other commands with the following
grammar which expresses alternative options as explained in the
remainder of the manpage:
[-c|--cached] [-d|--deleted] [-m|--modified] and so on.
>
>> diff --git a/Documentation/git-pack-redundant.txt b/Documentation/git-pack-redundant.txt
>> @@ -9,7 +9,7 @@ git-pack-redundant - Find redundant pack files
>> -'git pack-redundant' [ --verbose ] [ --alt-odb ] < --all | .pack filename ... >
>> +'git pack-redundant' [ --verbose ] [ --alt-odb ] ( --all | <.pack-filename>... )
>
> I'd probably drop the leading dot in <.pack-filename>. It shouldn't be
> difficult for a reader to figure out that these are the files with
> `.pack` extension, and if they do need help understanding that, then
> probably better to explain in prose that <pack-filename> is a pack
> file with `.pack` extension.
>
>> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
>> @@ -89,7 +89,7 @@ counts both authors and co-authors.
>> -<revision range>::
>> +<revision-range>::
>> Show only commits in the specified revision range. When no
>> <revision range> is specified, it defaults to `HEAD` (i.e. the
>> whole history leading to the current commit). `origin..HEAD`
>
> Need to update the description to: s/<revision range>/<revision-range>/
Ack
>
>> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
>> @@ -11,7 +11,7 @@ given by a list of patterns.
>> -'git sparse-checkout <subcommand> [options]'
>> +'git sparse-checkout <subcommand> [<options>...]'
>
> The addition of `...` would make more sense if it was spelled
> "option", but with it already being plural "options", I have trouble
> understanding why `...` is added.
>
>> diff --git a/Documentation/git-stage.txt b/Documentation/git-stage.txt
>> @@ -9,7 +9,7 @@ git-stage - Add file contents to the staging area
>> -'git stage' args...
>> +'git stage' <arg>...
>
> It's subjective, but I find plain `<args>` easier to interpret than
> `<arg>...`. Does our documentation favor one form over the other, or
> is there a random mix?
>
>> diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt
>> @@ -8,7 +8,7 @@ git-web--browse - Git helper script to launch a web browser
>> -'git web{litdd}browse' [<options>] <url|file>...
>> +'git web{litdd}browse' [<options>] (<url>|<file>)...
>
> Good.
>
