Thread (8 messages) flat view 8 messages, 3 authors, 2022-09-26

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)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help