Re: [PATCH] cmd-list.perl: fix identifying man sections
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 23, 2022, 07:38 UTC
- Message-ID
- <220923.86wn9u4joo.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220923070334.1970213-1-martin.agren@gmail.com>
On Fri, Sep 23 2022, Martin Ågren wrote:
Show 45 quoted lines
> We attribute each documentation text file to a man section by finding a
> line in the file that looks like "gitfoo(<digit>)". Commit cc75e556a9
> ("scalar: add to 'git help -a' command list", 2022-09-02) updated this
> logic to look not only for "gitfoo" but also "scalarfoo". In doing so,
> it forgot to account for the fact that after the updated regex has found
> a match, the man section is no longer to be found in `$1` but now lives
> in `$2`.
>
> This makes our git(1) manpage look as follows:
>
> Main porcelain commands
> git-add(git)
> Add file contents to the index.
>
> [...]
>
> gitk(git)
> The Git repository browser.
>
> scalar(scalar)
> A tool for managing large Git repositories.
>
> Restore the man sections by grabbing the correct value out of the regex
> match.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> This is a v2.38.0-rc1 regression.
>
> Documentation/cmd-list.perl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
> index 9515a499a3..16451d81b8 100755
> --- a/Documentation/cmd-list.perl
> +++ b/Documentation/cmd-list.perl
> @@ -11,7 +11,7 @@ sub format_one {
> open I, '<', "$name.txt" or die "No such file $name.txt";
> while (<I>) {
> if (/^(git|scalar)[a-z0-9-]*\(([0-9])\)$/) {
> - $mansection = $1;
> + $mansection = $2;
> next;
> }
> if (/^NAME$/) {Ouch, good catch!
In the v1 review for scalar[1] I pointed out that I had a local patch where this is instead:
if (/^[a-z0-9-]*\(([0-9])\)$/) {
$mansection = $1;The v2 at [2] which I didn't get around to reviewing (sorry!) then introduced this logic error.
Victoria: Did you find some reason to not just take the version I had? The doc-diff with Martin's above is empty, i.e. it's the same in practice, but if we don't need to hard-code our top-level commands for no reason I don't see why we'd maintain this list of them here.
On the proposed rc patch: FWIW we can also just use (?:) groupings instead of a capture:
if (/^(?:git|scalar)[a-z0-9-]*\(([0-9])\)$/) {It has the same effect, but arguably makes more sense. I.e. if we don't care about $1 let's not capture the thing we don't care about into $1 in the first place.
1. https://lore.kernel.org/git/220831.86y1v48h2x.gmgdl@evledraar.gmail.com/ 2. https://lore.kernel.org/git/070f195f027e5601b88ca6a0d4c9991b472ad9ab.1662134210.git.gitgitgadget@gmail.com/