Re: [PATCH 1/2] help: make sure local html page exists before calling external processes
From: Junio C Hamano <hidden>
Date: 2021-09-13 19:25:15
"Matthias Aßhauer via GitGitGadget" [off-list ref] writes:
quoted hunk
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.
quoted hunk
} 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" '