git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Matthias Aßhauer via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Matthias Aßhauer" <mha1993@live.de>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Matthias Aßhauer" <mha1993@live.de>
Subject: [PATCH v2 0/2] documentation: handle non-existing html pages and document 'git version'
Date: Tue, 14 Sep 2021 13:27:16 +0000	[thread overview]
Message-ID: <pull.1038.v2.git.1631626038.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1038.git.1631531218.gitgitgadget@gmail.com>

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 <mha1993@live.de>
      
     @@ 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 <mha1993@live.de>
      
       ## 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

  parent reply	other threads:[~2021-09-14 13:27 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-13 11:06 [PATCH 0/2] documentation: handle non-existing html pages and document 'git version' Matthias Aßhauer via GitGitGadget
2021-09-13 11:06 ` [PATCH 1/2] help: make sure local html page exists before calling external processes Matthias Aßhauer via GitGitGadget
2021-09-13 15:59   ` Eric Sunshine
2021-09-13 16:17     ` Matthias Aßhauer
2021-09-13 19:25   ` Junio C Hamano
2021-09-13 11:06 ` [PATCH 2/2] documentation: add documentation for 'git version' Matthias Aßhauer via GitGitGadget
2021-09-13 11:19   ` Ævar Arnfjörð Bjarmason
2021-09-13 11:46     ` Matthias Aßhauer
2021-09-13 19:43     ` Junio C Hamano
2021-09-14 13:27 ` Matthias Aßhauer via GitGitGadget [this message]
2021-09-14 13:27   ` [PATCH v2 1/2] help: make sure local html page exists before calling external processes Matthias Aßhauer via GitGitGadget
2021-09-14 13:27   ` [PATCH v2 2/2] documentation: add documentation for 'git version' Matthias Aßhauer via GitGitGadget
2021-09-24 13:00     ` Is "make check-docs" useful anymore? Ævar Arnfjörð Bjarmason
2021-09-24 17:59       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.1038.v2.git.1631626038.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mha1993@live.de \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).