Re: [PATCH] cmd-list.perl: fix identifying man sections

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Sep 23 2022, Martin Ågren wrote:

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