Re: [PATCH 1/2] help: make sure local html page exists before calling external processes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 13, 2021, 19:25 UTC
- Message-ID
- <xmqqa6kgffc8.fsf@gitster.g>
- In-Reply-To
- <8674d67a439a23425133fa005e519ebb6ac19c42.1631531219.git.gitgitgadget@gmail.com>
"Matthias Aßhauer via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 21 quoted lines
> diff --git a/builtin/help.c b/builtin/help.c
> index b7eec06c3de..77b1b926f60 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -467,11 +467,18 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
> if (!html_path)
> html_path = to_free = system_path(GIT_HTML_PATH);
>
> - /* Check that we have a git documentation directory. */
> + /*
> + * Check that we have a git documentation directory and the page we're
> + * looking for exists.
> + */
> if (!strstr(html_path, "://")) {
> if (stat(mkpath("%s/git.html", html_path), &st)
> || !S_ISREG(st.st_mode))
> die("'%s': not a documentation directory.", html_path);
> + if (stat(mkpath("%s/%s.html", html_path, page), &st)
> + || !S_ISREG(st.st_mode))
> + die("'%s/%s.html': documentation file not found.",
> + html_path, page);I do not remember why we did not originally use the "page" information and only checked "git.html", but because the "page" is ultimately what the end-user will see, I wonder if it even makes sense to still check "git.html" anymore.
If the request were "git help -w git", the new check added here would catch the missing page, and for "git help -w log", it is unfair to call the directory that we successfully found the "git-log.html" in as "not a doc directory" only because it is for whatever reason is missing "git.html" next to it.
It seems that this check dates back to 482cce82 (help: make 'git-help--browse' usable outside 'git-help'., 2008-02-02); even in the context of that commit, I think it would have been better to check for the generated page_path instead of git.html, so I actually prefer to see the existing hardcoded check for git.html replaced with the new check.
Thanks.
Show 21 quoted lines
> } > > strbuf_init(page_path, 0); > diff --git a/t/t0012-help.sh b/t/t0012-help.sh > index 5679e29c624..a83a59d44d9 100755 > --- a/t/t0012-help.sh > +++ b/t/t0012-help.sh > @@ -77,6 +77,13 @@ test_expect_success 'generate builtin list' ' > git --list-cmds=builtins >builtins > ' > > +test_expect_success 'git help fails for non-existing html pages' ' > + configure_help && > + mkdir html-doc && > + touch html-doc/git.html && > + test_must_fail git -c help.htmlpath=html-doc help status > +' > + > while read builtin > do > test_expect_success "$builtin can handle -h" '