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

[Junio C Hamano <gitster@pobox.com>]

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