[PATCH 0/2] documentation: handle non-existing html pages and document 'git version'

These two patches are grouped as one patch series, because they arose from
the same Git for Windows issue [1], but they can be reviewed or applied
independent from one another.

[1] https://github.com/git-for-windows/git/issues/3308

Matthias Aßhauer (2):
  help: make sure local html page exists before calling external
    processes
  documentation: add documentation for 'git version'

 Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++
 builtin/help.c                |  9 ++++++++-
 t/t0012-help.sh               |  7 +++++++
 3 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/git-version.txt


base-commit: 8463beaeb69fe0b7f651065813def4aa6827cd5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1038%2Frimrul%2Fdoc-version-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1038/rimrul/doc-version-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1038
-- 
gitgitgadget

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <redacted>

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

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <redacted>

While 'git version' is probably the least complex git command,
it is a non-experimental user-facing builtin command. As such
it should have a help page.

Signed-off-by: Matthias Aßhauer <redacted>
---
 Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/git-version.txt

diff --git a/Documentation/git-version.txt b/Documentation/git-version.txt
new file mode 100644
index 00000000000..c7d6b496c8d
--- /dev/null
+++ b/Documentation/git-version.txt

@@ -0,0 +1,35 @@
+git-version(1)
+==============
+
+NAME
+----
+git-version - Display version information about Git
+
+
+SYNOPSIS
+--------
+[verse]
+'git version' [--build-options]
+
+
+DESCRIPTION
+-----------
+
+With no options given, the version of 'git' is printed
+on the standard output.
+
+If the option `--build-options` is given, information about how git was built is
+printed on the standard output in addition to the version number.
+
+Note that `git --version` is identical to `git version` because the
+former is internally converted into the latter.
+
+OPTIONS
+-------
+--build-options::
+	Prints out additional information about how git was built for diagnostic
+	purposes.
+
+GIT
+---
+Part of the linkgit:git[1] suite

-- 
gitgitgadget

On Mon, Sep 13 2021, Matthias Aßhauer via GitGitGadget wrote:

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <redacted>

While 'git version' is probably the least complex git command,
it is a non-experimental user-facing builtin command. As such
it should have a help page.

This looks good

Good

+
+If the option `--build-options` is given, information about how git was built is
+printed on the standard output in addition to the version number.

Let's just cover this in the OPTIONS section you added...

+Note that `git --version` is identical to `git version` because the
+former is internally converted into the latter.

Probably better to just have a new section:

SEE ALSO
--------

linkgit:git[1]'s `--version` option, which dispatches to this command.

+OPTIONS
+-------
+--build-options::
+	Prints out additional information about how git was built for diagnostic
+	purposes.
+
+GIT
+---
+Part of the linkgit:git[1] suite

It would also be good to update git.txt, which now says:

    Prints the Git suite version that the git program came from

To say e.g. "Dispatches to linkgit:git-version[1], prints the git
program version".

Or something like that, i.e. to cross-link the two.

On Mon, 13 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

On Mon, Sep 13 2021, Matthias Aßhauer via GitGitGadget wrote:

quoted

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <redacted>

While 'git version' is probably the least complex git command,
it is a non-experimental user-facing builtin command. As such
it should have a help page.

This looks good

quoted

Signed-off-by: Matthias Aßhauer <redacted>
---
 Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/git-version.txt

diff --git a/Documentation/git-version.txt b/Documentation/git-version.txt
new file mode 100644
index 00000000000..c7d6b496c8d
--- /dev/null
+++ b/Documentation/git-version.txt

@@ -0,0 +1,35 @@
+git-version(1)
+==============
+
+NAME
+----
+git-version - Display version information about Git
+
+
+SYNOPSIS
+--------
+[verse]
+'git version' [--build-options]

+
+DESCRIPTION
+-----------
+
+With no options given, the version of 'git' is printed
+on the standard output.

Good

quoted

+
+If the option `--build-options` is given, information about how git was built is
+printed on the standard output in addition to the version number.

Let's just cover this in the OPTIONS section you added...

Ok, I can absolutely do that.

quoted

+Note that `git --version` is identical to `git version` because the
+former is internally converted into the latter.

Probably better to just have a new section:

SEE ALSO
--------

linkgit:git[1]'s `--version` option, which dispatches to this command.

I've closely based this on git-help.txt, since `--help` and `--version` 
both are options that get internally converted to the corresponding command.

quoted

+OPTIONS
+-------
+--build-options::
+	Prints out additional information about how git was built for diagnostic
+	purposes.
+
+GIT
+---
+Part of the linkgit:git[1] suite

It would also be good to update git.txt, which now says:

   Prints the Git suite version that the git program came from

To say e.g. "Dispatches to linkgit:git-version[1], prints the git
program version".

Or something like that, i.e. to cross-link the two.

That makes sense. Should we do the same for '--help'?

Best regards

Matthias

On Mon, Sep 13, 2021 at 7:07 AM Matthias Aßhauer via GitGitGadget
[off-list ref] 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 <redacted>

On Mon, 13 Sep 2021, Eric Sunshine wrote:

On Mon, Sep 13, 2021 at 7:07 AM Matthias Aßhauer via GitGitGadget
[off-list ref] wrote:

quoted

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/

quoted

smoother user experience when it doesn't.

quoted

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/

quoted

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

s/programm/program/

quoted

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/

quoted

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/

quoted

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

s/pocesses/processes/

quoted

Signed-off-by: Matthias Aßhauer <redacted>

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

Best regards

Matthias

These two patches are grouped as one patch series, because they arose from
the same Git for Windows issue [1], but they can be reviewed or applied
independent from one another.

One interesting oddity I found while preparing V2: git --version --help gets
converted to git version --help which then calls git help version, but git
--help --version gets converted to git help --version and git help doesn't
know what to do with --version.

[1] https://github.com/git-for-windows/git/issues/3308

Changes since V1:

 * fixed various typos in the log message of patch 1
 * changed patch 1 to just check the requested page instead of both the
   requested page and git.html
 * moved the test up before the "generate builtin list" test
 * reworked the test slightly
 * added a second test
 * removed the redundant description of --build-options from the DESCRIPTION
   section
 * added a small paragraph to Documentation/git.txt that links to the new
   git version page (like we already do for git help)

Matthias Aßhauer (2):
  help: make sure local html page exists before calling external
    processes
  documentation: add documentation for 'git version'

 Documentation/git-version.txt | 28 ++++++++++++++++++++++++++++
 Documentation/git.txt         |  4 ++++
 builtin/help.c                |  9 ++++++---
 t/t0012-help.sh               | 16 ++++++++++++++++
 4 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/git-version.txt


base-commit: 8463beaeb69fe0b7f651065813def4aa6827cd5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1038%2Frimrul%2Fdoc-version-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1038/rimrul/doc-version-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1038

Range-diff vs v1:

 1:  8674d67a439 ! 1:  c55360272d5 help: make sure local html page exists before calling external processes
     @@ Metadata
       ## Commit message ##
          help: make sure local html page exists before calling external processes
      
     -    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.
     +    We check that git.html exists, regardless of the page the user wants to open.
     +    Checking whether the requested page exists instead gives us a smoother user
     +    experience in two use cases:
     +
     +    1) The requested page doesn't exist
      
          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)
     +    we pass the filepath to git web--browse which passes it on to a browser (or a
     +    helper program 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
     +    Especially 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.
     +    2) git.html is missing for some reason, but the user asked for some other page
     +
     +    We currently refuse to show any local html help page when we can't find
     +    git.html. Even if the requested help page exists. If we check for the requested
     +    page instead, we can show the user all available pages and only error out on
     +    those that don't exist.
      
          Signed-off-by: Matthias Aßhauer [off-list ref]
      
     @@ builtin/help.c: static void get_html_page_path(struct strbuf *page_path, const c
       
      -	/* Check that we have a git documentation directory. */
      +	/*
     -+	 * Check that we have a git documentation directory and the page we're
     -+	 * looking for exists.
     ++	 * Check that 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/git.html", html_path), &st)
      +		if (stat(mkpath("%s/%s.html", html_path, page), &st)
     -+		    || !S_ISREG(st.st_mode))
     + 		    || !S_ISREG(st.st_mode))
     +-			die("'%s': not a documentation directory.", html_path);
      +			die("'%s/%s.html': documentation file not found.",
      +				html_path, page);
       	}
     @@ builtin/help.c: static void get_html_page_path(struct strbuf *page_path, const c
       	strbuf_init(page_path, 0);
      
       ## t/t0012-help.sh ##
     -@@ t/t0012-help.sh: test_expect_success 'generate builtin list' '
     - 	git --list-cmds=builtins >builtins
     +@@ t/t0012-help.sh: test_expect_success 'git help -g' '
     + 	test_i18ngrep "^   tutorial   " help.output
       '
       
      +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
     ++	mkdir html-empty &&
     ++	test_must_fail git -c help.htmlpath=html-empty help status &&
     ++	test_must_be_empty test-browser.log
      +'
      +
     - while read builtin
     - do
     - 	test_expect_success "$builtin can handle -h" '
     ++test_expect_success 'git help succeeds without git.html' '
     ++	configure_help &&
     ++	mkdir html-with-docs &&
     ++	touch html-with-docs/git-status.html &&
     ++	git -c help.htmlpath=html-with-docs help status &&
     ++	echo "html-with-docs/git-status.html" >expect &&
     ++	test_cmp expect test-browser.log
     ++'
     ++
     + test_expect_success 'generate builtin list' '
     + 	git --list-cmds=builtins >builtins
     + '
 2:  d3635cbfd6e ! 2:  bc9a4534f5b documentation: add documentation for 'git version'
     @@ Commit message
          it is a non-experimental user-facing builtin command. As such
          it should have a help page.
      
     +    Both `git help` and `git version` can be called as options
     +    (`--help`/`--version`) that internally get converted to the
     +    corresponding command. Add a small paragraph to
     +    Documentation/git.txt describing how these two options
     +    interact with each other and link to this help page for the
     +    sub-options that `--version` can take. Well, currently there
     +    is only one sub-option, but that could potentially increase
     +    in future versions of Git.
     +
          Signed-off-by: Matthias Aßhauer [off-list ref]
      
       ## Documentation/git-version.txt (new) ##
     @@ Documentation/git-version.txt (new)
      +----
      +git-version - Display version information about Git
      +
     -+
      +SYNOPSIS
      +--------
      +[verse]
      +'git version' [--build-options]
      +
     -+
      +DESCRIPTION
      +-----------
     -+
     -+With no options given, the version of 'git' is printed
     -+on the standard output.
     -+
     -+If the option `--build-options` is given, information about how git was built is
     -+printed on the standard output in addition to the version number.
     ++With no options given, the version of 'git' is printed on the standard output.
      +
      +Note that `git --version` is identical to `git version` because the
      +former is internally converted into the latter.
     @@ Documentation/git-version.txt (new)
      +OPTIONS
      +-------
      +--build-options::
     -+	Prints out additional information about how git was built for diagnostic
     ++	Include additional information about how git was built for diagnostic
      +	purposes.
      +
      +GIT
      +---
      +Part of the linkgit:git[1] suite
     +
     + ## Documentation/git.txt ##
     +@@ Documentation/git.txt: OPTIONS
     + -------
     + --version::
     + 	Prints the Git suite version that the 'git' program came from.
     +++
     ++This option is internaly converted to `git version ...` and accepts
     ++the same options as the linkgit:git-version[1] command. If `--help` is
     ++also given, it takes precedence over `--version`.
     + 
     + --help::
     + 	Prints the synopsis and a list of the most commonly used

-- 
gitgitgadget

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <redacted>

We check that git.html exists, regardless of the page the user wants to open.
Checking whether the requested page exists instead gives us a smoother user
experience in two use cases:

1) The requested page doesn't exist

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 which passes it on to a browser (or a
helper program 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.

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

2) git.html is missing for some reason, but the user asked for some other page

We currently refuse to show any local html help page when we can't find
git.html. Even if the requested help page exists. If we check for the requested
page instead, we can show the user all available pages and only error out on
those that don't exist.

Signed-off-by: Matthias Aßhauer <redacted>
---
 builtin/help.c  |  9 ++++++---
 t/t0012-help.sh | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index b7eec06c3de..7731659765c 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -467,11 +467,14 @@ 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 the page we're looking for exists.
+	 */
 	if (!strstr(html_path, "://")) {
-		if (stat(mkpath("%s/git.html", html_path), &st)
+		if (stat(mkpath("%s/%s.html", html_path, page), &st)
 		    || !S_ISREG(st.st_mode))
-			die("'%s': not a documentation directory.", html_path);
+			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..913f34c8e9d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh

@@ -73,6 +73,22 @@ test_expect_success 'git help -g' '
 	test_i18ngrep "^   tutorial   " help.output
 '
 
+test_expect_success 'git help fails for non-existing html pages' '
+	configure_help &&
+	mkdir html-empty &&
+	test_must_fail git -c help.htmlpath=html-empty help status &&
+	test_must_be_empty test-browser.log
+'
+
+test_expect_success 'git help succeeds without git.html' '
+	configure_help &&
+	mkdir html-with-docs &&
+	touch html-with-docs/git-status.html &&
+	git -c help.htmlpath=html-with-docs help status &&
+	echo "html-with-docs/git-status.html" >expect &&
+	test_cmp expect test-browser.log
+'
+
 test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '

-- 
gitgitgadget

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <redacted>

While 'git version' is probably the least complex git command,
it is a non-experimental user-facing builtin command. As such
it should have a help page.

Both `git help` and `git version` can be called as options
(`--help`/`--version`) that internally get converted to the
corresponding command. Add a small paragraph to
Documentation/git.txt describing how these two options
interact with each other and link to this help page for the
sub-options that `--version` can take. Well, currently there
is only one sub-option, but that could potentially increase
in future versions of Git.

Signed-off-by: Matthias Aßhauer <redacted>
---
 Documentation/git-version.txt | 28 ++++++++++++++++++++++++++++
 Documentation/git.txt         |  4 ++++
 2 files changed, 32 insertions(+)
 create mode 100644 Documentation/git-version.txt

diff --git a/Documentation/git-version.txt b/Documentation/git-version.txt
new file mode 100644
index 00000000000..80fa7754a6d
--- /dev/null
+++ b/Documentation/git-version.txt

@@ -0,0 +1,28 @@
+git-version(1)
+==============
+
+NAME
+----
+git-version - Display version information about Git
+
+SYNOPSIS
+--------
+[verse]
+'git version' [--build-options]
+
+DESCRIPTION
+-----------
+With no options given, the version of 'git' is printed on the standard output.
+
+Note that `git --version` is identical to `git version` because the
+former is internally converted into the latter.
+
+OPTIONS
+-------
+--build-options::
+	Include additional information about how git was built for diagnostic
+	purposes.
+
+GIT
+---
+Part of the linkgit:git[1] suite

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6dd241ef838..95fe6f31b4f 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -41,6 +41,10 @@ OPTIONS
 -------
 --version::
 	Prints the Git suite version that the 'git' program came from.
++
+This option is internaly converted to `git version ...` and accepts
+the same options as the linkgit:git-version[1] command. If `--help` is
+also given, it takes precedence over `--version`.
 
 --help::
 	Prints the synopsis and a list of the most commonly used

-- 
gitgitgadget

On Tue, Sep 14 2021, Matthias Aßhauer via GitGitGadget wrote:

I didn't notice until after it hit master that this caused a regression
in "make check-docs":

    $ make -s check-docs
    removed but documented: git-version

The "fix" is rather easy, i.e. adding "git-version" to the whitelist.

But I wondered about $subject, i.e. we want to run the "lint" part, but
do we really need something reminding us that there isn't a mapping
between Documentation/*.txt and *.o files present at the top-level?

That whole part seems to have been some "reminder to document" addition
in 8c989ec5288 (Makefile: $(MAKE) check-docs, 2006-04-13).

If we're going to keep it in pretty much its current form then the CI
integration added in b98712b9aa9 (travis-ci: build documentation,
2016-05-04) seems rather useless when it comes to this, i.e. we should
either adjust it to exit non-zero, or check if we've got output under
"make -s" and fail the check then.