Re: [PATCH] cmd-list.perl: fix identifying man sections
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-09-23 07:47:02
On Fri, Sep 23 2022, Martin Ågren wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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/ (local)
2. https://lore.kernel.org/git/070f195f027e5601b88ca6a0d4c9991b472ad9ab.1662134210.git.gitgitgadget@gmail.com/ (local)