From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: git@vger.kernel.org, Victoria Dye <vdye@github.com>
Subject: Re: [PATCH] cmd-list.perl: fix identifying man sections
Date: Fri, 23 Sep 2022 09:38:12 +0200 [thread overview]
Message-ID: <220923.86wn9u4joo.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20220923070334.1970213-1-martin.agren@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/
next prev parent reply other threads:[~2022-09-23 7:47 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-23 7:03 [PATCH] cmd-list.perl: fix identifying man sections Martin Ågren
2022-09-23 7:38 ` Ævar Arnfjörð Bjarmason [this message]
2022-09-23 8:07 ` [PATCH v2] " Martin Ågren
2022-09-23 8:37 ` Ævar Arnfjörð Bjarmason
2022-09-23 11:42 ` Martin Ågren
2022-09-23 17:01 ` Junio C Hamano
2022-09-23 22:07 ` [PATCH] " Jeff King
2022-09-26 7:16 ` Martin Ågren
2022-09-26 17:06 ` Junio C Hamano
2022-09-26 13:35 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220923.86wn9u4joo.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=martin.agren@gmail.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).