[PATCH 1/2] help: make sure local html page exists before calling external processes

From: Matthias Aßhauer via GitGitGadget <gitgitgadget@gmail.com>
Date: Sep 13, 2021, 11:06 UTC
Message-ID: <8674d67a439a23425133fa005e519ebb6ac19c42.1631531219.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.1038.git.1631531218.gitgitgadget@gmail.com>

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>

We already check that git.html exists, regardless of the page the user wants to open. Additionally checking wether the requested page exists gives us a smoother user experience when it doesn't.

When calling a git command and there is an error, most users reasonably expect git to produce an error message on the standard error stream, but in this case we pass the filepath to git web--browse wich passes it on to a browser (or a helper programm like xdg-open or start that should in turn open a browser) without any error and many GUI based browsers or helpers won't output such a message onto the standard error stream.

Especialy the helper programs tend to show the corresponding error message in a message box and wait for user input before exiting. This leaves users in interactive console sessions without an error message in their console, without a console prompt and without the help page they expected.

The performance cost of the additional stat should be negliggible compared to the two or more pocesses that we spawn after the checks.

Signed-off-by: Matthias Aßhauer <mha1993@live.de>
---
 builtin/help.c  | 9 ++++++++-
 t/t0012-help.sh | 7 +++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

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);
 	}
 
 	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" '

-- 
gitgitgadget