diff mbox series

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

Message ID 8674d67a439a23425133fa005e519ebb6ac19c42.1631531219.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series documentation: handle non-existing html pages and document 'git version' | expand

Commit Message

Matthias Aßhauer Sept. 13, 2021, 11:06 a.m. UTC 
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(-)

Comments

Eric Sunshine Sept. 13, 2021, 3:59 p.m. UTC | #1 
On Mon, Sep 13, 2021 at 7:07 AM Matthias Aßhauer via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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

s/wether/whether/

> 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

s/wich/which/

> helper programm like xdg-open or start that should in turn open a browser)

s/programm/program/

> 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

s/Especialy/Especially/

> 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

s/negliggible/negligible/

> the two or more pocesses that we spawn after the checks.

s/pocesses/processes/

> Signed-off-by: Matthias Aßhauer <mha1993@live.de>
Matthias Aßhauer Sept. 13, 2021, 4:17 p.m. UTC | #2 
On Mon, 13 Sep 2021, Eric Sunshine wrote:

> On Mon, Sep 13, 2021 at 7:07 AM Matthias Aßhauer via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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
>
> s/wether/whether/
>
>> 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
>
> s/wich/which/
>
>> helper programm like xdg-open or start that should in turn open a browser)
>
> s/programm/program/
>
>> 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
>
> s/Especialy/Especially/
>
>> 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
>
> s/negliggible/negligible/
>
>> the two or more pocesses that we spawn after the checks.
>
> s/pocesses/processes/
>
>> Signed-off-by: Matthias Aßhauer <mha1993@live.de>
>

Thank you for pointing out this embarrassing amount of typos.
I've fixed them for the next version.

Best regards

Matthias
Junio C Hamano Sept. 13, 2021, 7:25 p.m. UTC | #3 
"Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> 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.


>  	}
>  
>  	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" '
diff mbox series

Patch

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" '