{"thread":{"id":"56492","subject":"[PATCH 0/2] documentation: handle non-existing html pages and document 'git version'","startedAt":"2021-09-13T11:07:03Z","lastAt":"2021-09-24T17:59:13Z","messageCount":14,"participants":["Matthias Aßhauer via GitGitGadget","Ævar Arnfjörð Bjarmason","Matthias Aßhauer","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"435703","messageId":"pull.1038.git.1631531218.gitgitgadget@gmail.com","threadId":"56492","inReplyTo":null,"subject":"[PATCH 0/2] documentation: handle non-existing html pages and document 'git version'","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T11:06:56Z","receivedAt":"2021-09-13T11:07:03Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"These two patches are grouped as one patch series, because they arose from\nthe same Git for Windows issue [1], but they can be reviewed or applied\nindependent from one another.\n\n[1] https://github.com/git-for-windows/git/issues/3308\n\nMatthias Aßhauer (2):\n  help: make sure local html page exists before calling external\n    processes\n  documentation: add documentation for 'git version'\n\n Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++\n builtin/help.c                |  9 ++++++++-\n t/t0012-help.sh               |  7 +++++++\n 3 files changed, 50 insertions(+), 1 deletion(-)\n create mode 100644 Documentation/git-version.txt\n\n\nbase-commit: 8463beaeb69fe0b7f651065813def4aa6827cd5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1038%2Frimrul%2Fdoc-version-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1038/rimrul/doc-version-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1038\n-- \ngitgitgadget\n"},{"id":"435704","messageId":"8674d67a439a23425133fa005e519ebb6ac19c42.1631531219.git.gitgitgadget@gmail.com","threadId":"56492","inReplyTo":"pull.1038.git.1631531218.gitgitgadget@gmail.com","subject":"[PATCH 1/2] help: make sure local html page exists before calling external processes","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T11:06:57Z","receivedAt":"2021-09-13T11:07:04Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n\nWe already check that git.html exists, regardless of the page the user wants\nto open. Additionally checking wether the requested page exists gives us a\nsmoother user experience when it doesn't.\n\nWhen calling a git command and there is an error, most users reasonably expect\ngit to produce an error message on the standard error stream, but in this case\nwe pass the filepath to git web--browse wich passes it on to a browser (or a\nhelper programm like xdg-open or start that should in turn open a browser)\nwithout any error and many GUI based browsers or helpers won't output such a\nmessage onto the standard error stream.\n\nEspecialy the helper programs tend to show the corresponding error message in\na message box and wait for user input before exiting. This leaves users in\ninteractive console sessions without an error message in their console,\nwithout a console prompt and without the help page they expected.\n\nThe performance cost of the additional stat should be negliggible compared to\nthe two or more pocesses that we spawn after the checks.\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n builtin/help.c  | 9 ++++++++-\n t/t0012-help.sh | 7 +++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b7eec06c3de..77b1b926f60 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -467,11 +467,18 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n \tif (!html_path)\n \t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n \n-\t/* Check that we have a git documentation directory. */\n+\t/*\n+\t * Check that we have a git documentation directory and the page we're\n+\t * looking for exists.\n+\t */\n \tif (!strstr(html_path, \"://\")) {\n \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n \t\t    || !S_ISREG(st.st_mode))\n \t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n+\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n+\t\t    || !S_ISREG(st.st_mode))\n+\t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n+\t\t\t\thtml_path, page);\n \t}\n \n \tstrbuf_init(page_path, 0);\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 5679e29c624..a83a59d44d9 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -77,6 +77,13 @@ test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n \n+test_expect_success 'git help fails for non-existing html pages' '\n+\tconfigure_help &&\n+\tmkdir html-doc &&\n+\ttouch html-doc/git.html &&\n+\ttest_must_fail git -c help.htmlpath=html-doc help status\n+'\n+\n while read builtin\n do\n \ttest_expect_success \"$builtin can handle -h\" '\n-- \ngitgitgadget\n\n"},{"id":"435705","messageId":"d3635cbfd6ef0d47ebf28c516476dcd0b718afd4.1631531219.git.gitgitgadget@gmail.com","threadId":"56492","inReplyTo":"pull.1038.git.1631531218.gitgitgadget@gmail.com","subject":"[PATCH 2/2] documentation: add documentation for 'git version'","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T11:06:58Z","receivedAt":"2021-09-13T11:07:07Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n\nWhile 'git version' is probably the least complex git command,\nit is a non-experimental user-facing builtin command. As such\nit should have a help page.\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++\n 1 file changed, 35 insertions(+)\n create mode 100644 Documentation/git-version.txt\n\ndiff --git a/Documentation/git-version.txt b/Documentation/git-version.txt\nnew file mode 100644\nindex 00000000000..c7d6b496c8d\n--- /dev/null\n+++ b/Documentation/git-version.txt\n@@ -0,0 +1,35 @@\n+git-version(1)\n+==============\n+\n+NAME\n+----\n+git-version - Display version information about Git\n+\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git version' [--build-options]\n+\n+\n+DESCRIPTION\n+-----------\n+\n+With no options given, the version of 'git' is printed\n+on the standard output.\n+\n+If the option `--build-options` is given, information about how git was built is\n+printed on the standard output in addition to the version number.\n+\n+Note that `git --version` is identical to `git version` because the\n+former is internally converted into the latter.\n+\n+OPTIONS\n+-------\n+--build-options::\n+\tPrints out additional information about how git was built for diagnostic\n+\tpurposes.\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\n-- \ngitgitgadget\n"},{"id":"435706","messageId":"87r1ds4t3w.fsf@evledraar.gmail.com","threadId":"56492","inReplyTo":"d3635cbfd6ef0d47ebf28c516476dcd0b718afd4.1631531219.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] documentation: add documentation for 'git version'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T11:19:11Z","receivedAt":"2021-09-13T11:23:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 13 2021, Matthias Aßhauer via GitGitGadget wrote:\n\n> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n>\n> While 'git version' is probably the least complex git command,\n> it is a non-experimental user-facing builtin command. As such\n> it should have a help page.\n\nThis looks good\n\n> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n> ---\n>  Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++\n>  1 file changed, 35 insertions(+)\n>  create mode 100644 Documentation/git-version.txt\n>\n> diff --git a/Documentation/git-version.txt b/Documentation/git-version.txt\n> new file mode 100644\n> index 00000000000..c7d6b496c8d\n> --- /dev/null\n> +++ b/Documentation/git-version.txt\n> @@ -0,0 +1,35 @@\n> +git-version(1)\n> +==============\n> +\n> +NAME\n> +----\n> +git-version - Display version information about Git\n> +\n> +\n> +SYNOPSIS\n> +--------\n> +[verse]\n> +'git version' [--build-options]\n>\n> +\n> +DESCRIPTION\n> +-----------\n> +\n> +With no options given, the version of 'git' is printed\n> +on the standard output.\n\nGood\n\n> +\n> +If the option `--build-options` is given, information about how git was built is\n> +printed on the standard output in addition to the version number.\n\nLet's just cover this in the OPTIONS section you added...\n\n> +Note that `git --version` is identical to `git version` because the\n> +former is internally converted into the latter.\n\nProbably better to just have a new section:\n\nSEE ALSO\n--------\n\nlinkgit:git[1]'s `--version` option, which dispatches to this command.\n\n\n> +OPTIONS\n> +-------\n> +--build-options::\n> +\tPrints out additional information about how git was built for diagnostic\n> +\tpurposes.\n> +\n> +GIT\n> +---\n> +Part of the linkgit:git[1] suite\n\n\nIt would also be good to update git.txt, which now says:\n\n    Prints the Git suite version that the git program came from\n\nTo say e.g. \"Dispatches to linkgit:git-version[1], prints the git\nprogram version\".\n\nOr something like that, i.e. to cross-link the two.\n"},{"id":"435711","messageId":"AM0PR04MB6019DD0BBD77BEA85771B9E9A5D99@AM0PR04MB6019.eurprd04.prod.outlook.com","threadId":"56492","inReplyTo":"87r1ds4t3w.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] documentation: add documentation for 'git version'","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2021-09-13T11:46:42Z","receivedAt":"2021-09-13T11:46:47Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"\n\nOn Mon, 13 Sep 2021, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Mon, Sep 13 2021, Matthias Aßhauer via GitGitGadget wrote:\n>\n>> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n>>\n>> While 'git version' is probably the least complex git command,\n>> it is a non-experimental user-facing builtin command. As such\n>> it should have a help page.\n>\n> This looks good\n>\n>> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n>> ---\n>>  Documentation/git-version.txt | 35 +++++++++++++++++++++++++++++++++++\n>>  1 file changed, 35 insertions(+)\n>>  create mode 100644 Documentation/git-version.txt\n>>\n>> diff --git a/Documentation/git-version.txt b/Documentation/git-version.txt\n>> new file mode 100644\n>> index 00000000000..c7d6b496c8d\n>> --- /dev/null\n>> +++ b/Documentation/git-version.txt\n>> @@ -0,0 +1,35 @@\n>> +git-version(1)\n>> +==============\n>> +\n>> +NAME\n>> +----\n>> +git-version - Display version information about Git\n>> +\n>> +\n>> +SYNOPSIS\n>> +--------\n>> +[verse]\n>> +'git version' [--build-options]\n>>\n>> +\n>> +DESCRIPTION\n>> +-----------\n>> +\n>> +With no options given, the version of 'git' is printed\n>> +on the standard output.\n>\n> Good\n>\n>> +\n>> +If the option `--build-options` is given, information about how git was built is\n>> +printed on the standard output in addition to the version number.\n>\n> Let's just cover this in the OPTIONS section you added...\n\nOk, I can absolutely do that.\n\n>\n>> +Note that `git --version` is identical to `git version` because the\n>> +former is internally converted into the latter.\n>\n> Probably better to just have a new section:\n>\n> SEE ALSO\n> --------\n>\n> linkgit:git[1]'s `--version` option, which dispatches to this command.\n>\n>\n\nI've closely based this on git-help.txt, since `--help` and `--version` \nboth are options that get internally converted to the corresponding command.\n\n>> +OPTIONS\n>> +-------\n>> +--build-options::\n>> +\tPrints out additional information about how git was built for diagnostic\n>> +\tpurposes.\n>> +\n>> +GIT\n>> +---\n>> +Part of the linkgit:git[1] suite\n>\n>\n> It would also be good to update git.txt, which now says:\n>\n>    Prints the Git suite version that the git program came from\n>\n> To say e.g. \"Dispatches to linkgit:git-version[1], prints the git\n> program version\".\n>\n> Or something like that, i.e. to cross-link the two.\n\nThat makes sense. Should we do the same for '--help'?\n\nBest regards\n\nMatthias\n"},{"id":"435744","messageId":"CAPig+cS=fhE1Dm1ESps8SME9XO2+RPJ7LGtquuZQ5XPFB1uk3Q@mail.gmail.com","threadId":"56492","inReplyTo":"8674d67a439a23425133fa005e519ebb6ac19c42.1631531219.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] help: make sure local html page exists before calling external processes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-13T15:59:48Z","receivedAt":"2021-09-13T16:00:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 13, 2021 at 7:07 AM Matthias Aßhauer via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> We already check that git.html exists, regardless of the page the user wants\n> to open. Additionally checking wether the requested page exists gives us a\n\ns/wether/whether/\n\n> smoother user experience when it doesn't.\n\n> When calling a git command and there is an error, most users reasonably expect\n> git to produce an error message on the standard error stream, but in this case\n> we pass the filepath to git web--browse wich passes it on to a browser (or a\n\ns/wich/which/\n\n> helper programm like xdg-open or start that should in turn open a browser)\n\ns/programm/program/\n\n> without any error and many GUI based browsers or helpers won't output such a\n> message onto the standard error stream.\n>\n> Especialy the helper programs tend to show the corresponding error message in\n\ns/Especialy/Especially/\n\n> a message box and wait for user input before exiting. This leaves users in\n> interactive console sessions without an error message in their console,\n> without a console prompt and without the help page they expected.\n>\n> The performance cost of the additional stat should be negliggible compared to\n\ns/negliggible/negligible/\n\n> the two or more pocesses that we spawn after the checks.\n\ns/pocesses/processes/\n\n> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n"},{"id":"435745","messageId":"AM0PR04MB6019F785D38C7F8B4F66BC08A5D99@AM0PR04MB6019.eurprd04.prod.outlook.com","threadId":"56492","inReplyTo":"CAPig+cS=fhE1Dm1ESps8SME9XO2+RPJ7LGtquuZQ5XPFB1uk3Q@mail.gmail.com","subject":"Re: [PATCH 1/2] help: make sure local html page exists before calling external processes","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2021-09-13T16:17:01Z","receivedAt":"2021-09-13T16:17:10Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"\n\nOn Mon, 13 Sep 2021, Eric Sunshine wrote:\n\n> On Mon, Sep 13, 2021 at 7:07 AM Matthias Aßhauer via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> We already check that git.html exists, regardless of the page the user wants\n>> to open. Additionally checking wether the requested page exists gives us a\n>\n> s/wether/whether/\n>\n>> smoother user experience when it doesn't.\n>\n>> When calling a git command and there is an error, most users reasonably expect\n>> git to produce an error message on the standard error stream, but in this case\n>> we pass the filepath to git web--browse wich passes it on to a browser (or a\n>\n> s/wich/which/\n>\n>> helper programm like xdg-open or start that should in turn open a browser)\n>\n> s/programm/program/\n>\n>> without any error and many GUI based browsers or helpers won't output such a\n>> message onto the standard error stream.\n>>\n>> Especialy the helper programs tend to show the corresponding error message in\n>\n> s/Especialy/Especially/\n>\n>> a message box and wait for user input before exiting. This leaves users in\n>> interactive console sessions without an error message in their console,\n>> without a console prompt and without the help page they expected.\n>>\n>> The performance cost of the additional stat should be negliggible compared to\n>\n> s/negliggible/negligible/\n>\n>> the two or more pocesses that we spawn after the checks.\n>\n> s/pocesses/processes/\n>\n>> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n>\n\nThank you for pointing out this embarrassing amount of typos.\nI've fixed them for the next version.\n\nBest regards\n\nMatthias\n"},{"id":"435773","messageId":"xmqqa6kgffc8.fsf@gitster.g","threadId":"56492","inReplyTo":"8674d67a439a23425133fa005e519ebb6ac19c42.1631531219.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] help: make sure local html page exists before calling external processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T19:25:11Z","receivedAt":"2021-09-13T19:25:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matthias Aßhauer via GitGitGadget\"  <gitgitgadget@gmail.com>\nwrites:\n\n> diff --git a/builtin/help.c b/builtin/help.c\n> index b7eec06c3de..77b1b926f60 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -467,11 +467,18 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n>  \tif (!html_path)\n>  \t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n>  \n> -\t/* Check that we have a git documentation directory. */\n> +\t/*\n> +\t * Check that we have a git documentation directory and the page we're\n> +\t * looking for exists.\n> +\t */\n>  \tif (!strstr(html_path, \"://\")) {\n>  \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n>  \t\t    || !S_ISREG(st.st_mode))\n>  \t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n> +\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n> +\t\t    || !S_ISREG(st.st_mode))\n> +\t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n> +\t\t\t\thtml_path, page);\n\nI do not remember why we did not originally use the \"page\"\ninformation and only checked \"git.html\", but because the \"page\" is\nultimately what the end-user will see, I wonder if it even makes\nsense to still check \"git.html\" anymore.\n\nIf the request were \"git help -w git\", the new check added here\nwould catch the missing page, and for \"git help -w log\", it is\nunfair to call the directory that we successfully found the\n\"git-log.html\" in as \"not a doc directory\" only because it is for\nwhatever reason is missing \"git.html\" next to it.\n\nIt seems that this check dates back to 482cce82 (help: make\n'git-help--browse' usable outside 'git-help'., 2008-02-02); even in\nthe context of that commit, I think it would have been better to\ncheck for the generated page_path instead of git.html, so I actually\nprefer to see the existing hardcoded check for git.html replaced with\nthe new check.\n\nThanks.\n\n\n>  \t}\n>  \n>  \tstrbuf_init(page_path, 0);\n> diff --git a/t/t0012-help.sh b/t/t0012-help.sh\n> index 5679e29c624..a83a59d44d9 100755\n> --- a/t/t0012-help.sh\n> +++ b/t/t0012-help.sh\n> @@ -77,6 +77,13 @@ test_expect_success 'generate builtin list' '\n>  \tgit --list-cmds=builtins >builtins\n>  '\n>  \n> +test_expect_success 'git help fails for non-existing html pages' '\n> +\tconfigure_help &&\n> +\tmkdir html-doc &&\n> +\ttouch html-doc/git.html &&\n> +\ttest_must_fail git -c help.htmlpath=html-doc help status\n> +'\n> +\n>  while read builtin\n>  do\n>  \ttest_expect_success \"$builtin can handle -h\" '\n"},{"id":"435783","messageId":"xmqq4kaofehr.fsf@gitster.g","threadId":"56492","inReplyTo":"87r1ds4t3w.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] documentation: add documentation for 'git version'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T19:43:28Z","receivedAt":"2021-09-13T19:43:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> +Note that `git --version` is identical to `git version` because the\n>> +former is internally converted into the latter.\n>\n> Probably better to just have a new section:\n>\n> SEE ALSO\n> --------\n>\n> linkgit:git[1]'s `--version` option, which dispatches to this command.\n\nHmph, I am not sure if this is a good move.\n\nIf we are not giving any more information than what the reader has\nalready learned from this page, other than \"git --version\" does the\nsame thing, we probably do not want to do this.  By seeing also that\nother page, the user will not learn anything new about \"git version\".\n\nIf a related \"git --version-something-else\" is described over there\nand may fill the need the reader had when visiting this page, that\nis a different story, but I do not think it is the case.\n\n>> +OPTIONS\n>> +-------\n>> +--build-options::\n>> +\tPrints out additional information about how git was built for diagnostic\n>> +\tpurposes.\n>> +\n>> +GIT\n>> +---\n>> +Part of the linkgit:git[1] suite\n>\n>\n> It would also be good to update git.txt, which now says:\n>\n>     Prints the Git suite version that the git program came from\n>\n> To say e.g. \"Dispatches to linkgit:git-version[1], prints the git\n> program version\".\n\nThis one may be a good idea, I think.\n\n\"git --version --build-options\" also works and we do not want to\nclutter git[1] with descriptions on suboptions of \"git version\".\n\nIf we are not doing so for the \"--help\" option in git[1], we should\ndo so as well.\n\nThanks.\n"},{"id":"435846","messageId":"pull.1038.v2.git.1631626038.gitgitgadget@gmail.com","threadId":"56492","inReplyTo":"pull.1038.git.1631531218.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] documentation: handle non-existing html pages and document 'git version'","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-14T13:27:16Z","receivedAt":"2021-09-14T13:27:23Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"These two patches are grouped as one patch series, because they arose from\nthe same Git for Windows issue [1], but they can be reviewed or applied\nindependent from one another.\n\nOne interesting oddity I found while preparing V2: git --version --help gets\nconverted to git version --help which then calls git help version, but git\n--help --version gets converted to git help --version and git help doesn't\nknow what to do with --version.\n\n[1] https://github.com/git-for-windows/git/issues/3308\n\nChanges since V1:\n\n * fixed various typos in the log message of patch 1\n * changed patch 1 to just check the requested page instead of both the\n   requested page and git.html\n * moved the test up before the \"generate builtin list\" test\n * reworked the test slightly\n * added a second test\n * removed the redundant description of --build-options from the DESCRIPTION\n   section\n * added a small paragraph to Documentation/git.txt that links to the new\n   git version page (like we already do for git help)\n\nMatthias Aßhauer (2):\n  help: make sure local html page exists before calling external\n    processes\n  documentation: add documentation for 'git version'\n\n Documentation/git-version.txt | 28 ++++++++++++++++++++++++++++\n Documentation/git.txt         |  4 ++++\n builtin/help.c                |  9 ++++++---\n t/t0012-help.sh               | 16 ++++++++++++++++\n 4 files changed, 54 insertions(+), 3 deletions(-)\n create mode 100644 Documentation/git-version.txt\n\n\nbase-commit: 8463beaeb69fe0b7f651065813def4aa6827cd5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1038%2Frimrul%2Fdoc-version-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1038/rimrul/doc-version-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1038\n\nRange-diff vs v1:\n\n 1:  8674d67a439 ! 1:  c55360272d5 help: make sure local html page exists before calling external processes\n     @@ Metadata\n       ## Commit message ##\n          help: make sure local html page exists before calling external processes\n      \n     -    We already check that git.html exists, regardless of the page the user wants\n     -    to open. Additionally checking wether the requested page exists gives us a\n     -    smoother user experience when it doesn't.\n     +    We check that git.html exists, regardless of the page the user wants to open.\n     +    Checking whether the requested page exists instead gives us a smoother user\n     +    experience in two use cases:\n     +\n     +    1) The requested page doesn't exist\n      \n          When calling a git command and there is an error, most users reasonably expect\n          git to produce an error message on the standard error stream, but in this case\n     -    we pass the filepath to git web--browse wich passes it on to a browser (or a\n     -    helper programm like xdg-open or start that should in turn open a browser)\n     +    we pass the filepath to git web--browse which passes it on to a browser (or a\n     +    helper program like xdg-open or start that should in turn open a browser)\n          without any error and many GUI based browsers or helpers won't output such a\n          message onto the standard error stream.\n      \n     -    Especialy the helper programs tend to show the corresponding error message in\n     +    Especially the helper programs tend to show the corresponding error message in\n          a message box and wait for user input before exiting. This leaves users in\n          interactive console sessions without an error message in their console,\n          without a console prompt and without the help page they expected.\n      \n     -    The performance cost of the additional stat should be negliggible compared to\n     -    the two or more pocesses that we spawn after the checks.\n     +    2) git.html is missing for some reason, but the user asked for some other page\n     +\n     +    We currently refuse to show any local html help page when we can't find\n     +    git.html. Even if the requested help page exists. If we check for the requested\n     +    page instead, we can show the user all available pages and only error out on\n     +    those that don't exist.\n      \n          Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n      \n     @@ builtin/help.c: static void get_html_page_path(struct strbuf *page_path, const c\n       \n      -\t/* Check that we have a git documentation directory. */\n      +\t/*\n     -+\t * Check that we have a git documentation directory and the page we're\n     -+\t * looking for exists.\n     ++\t * Check that the page we're looking for exists.\n      +\t */\n       \tif (!strstr(html_path, \"://\")) {\n     - \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n     - \t\t    || !S_ISREG(st.st_mode))\n     - \t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n     +-\t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n      +\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n     -+\t\t    || !S_ISREG(st.st_mode))\n     + \t\t    || !S_ISREG(st.st_mode))\n     +-\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n      +\t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n      +\t\t\t\thtml_path, page);\n       \t}\n     @@ builtin/help.c: static void get_html_page_path(struct strbuf *page_path, const c\n       \tstrbuf_init(page_path, 0);\n      \n       ## t/t0012-help.sh ##\n     -@@ t/t0012-help.sh: test_expect_success 'generate builtin list' '\n     - \tgit --list-cmds=builtins >builtins\n     +@@ t/t0012-help.sh: test_expect_success 'git help -g' '\n     + \ttest_i18ngrep \"^   tutorial   \" help.output\n       '\n       \n      +test_expect_success 'git help fails for non-existing html pages' '\n      +\tconfigure_help &&\n     -+\tmkdir html-doc &&\n     -+\ttouch html-doc/git.html &&\n     -+\ttest_must_fail git -c help.htmlpath=html-doc help status\n     ++\tmkdir html-empty &&\n     ++\ttest_must_fail git -c help.htmlpath=html-empty help status &&\n     ++\ttest_must_be_empty test-browser.log\n      +'\n      +\n     - while read builtin\n     - do\n     - \ttest_expect_success \"$builtin can handle -h\" '\n     ++test_expect_success 'git help succeeds without git.html' '\n     ++\tconfigure_help &&\n     ++\tmkdir html-with-docs &&\n     ++\ttouch html-with-docs/git-status.html &&\n     ++\tgit -c help.htmlpath=html-with-docs help status &&\n     ++\techo \"html-with-docs/git-status.html\" >expect &&\n     ++\ttest_cmp expect test-browser.log\n     ++'\n     ++\n     + test_expect_success 'generate builtin list' '\n     + \tgit --list-cmds=builtins >builtins\n     + '\n 2:  d3635cbfd6e ! 2:  bc9a4534f5b documentation: add documentation for 'git version'\n     @@ Commit message\n          it is a non-experimental user-facing builtin command. As such\n          it should have a help page.\n      \n     +    Both `git help` and `git version` can be called as options\n     +    (`--help`/`--version`) that internally get converted to the\n     +    corresponding command. Add a small paragraph to\n     +    Documentation/git.txt describing how these two options\n     +    interact with each other and link to this help page for the\n     +    sub-options that `--version` can take. Well, currently there\n     +    is only one sub-option, but that could potentially increase\n     +    in future versions of Git.\n     +\n          Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n      \n       ## Documentation/git-version.txt (new) ##\n     @@ Documentation/git-version.txt (new)\n      +----\n      +git-version - Display version information about Git\n      +\n     -+\n      +SYNOPSIS\n      +--------\n      +[verse]\n      +'git version' [--build-options]\n      +\n     -+\n      +DESCRIPTION\n      +-----------\n     -+\n     -+With no options given, the version of 'git' is printed\n     -+on the standard output.\n     -+\n     -+If the option `--build-options` is given, information about how git was built is\n     -+printed on the standard output in addition to the version number.\n     ++With no options given, the version of 'git' is printed on the standard output.\n      +\n      +Note that `git --version` is identical to `git version` because the\n      +former is internally converted into the latter.\n     @@ Documentation/git-version.txt (new)\n      +OPTIONS\n      +-------\n      +--build-options::\n     -+\tPrints out additional information about how git was built for diagnostic\n     ++\tInclude additional information about how git was built for diagnostic\n      +\tpurposes.\n      +\n      +GIT\n      +---\n      +Part of the linkgit:git[1] suite\n     +\n     + ## Documentation/git.txt ##\n     +@@ Documentation/git.txt: OPTIONS\n     + -------\n     + --version::\n     + \tPrints the Git suite version that the 'git' program came from.\n     +++\n     ++This option is internaly converted to `git version ...` and accepts\n     ++the same options as the linkgit:git-version[1] command. If `--help` is\n     ++also given, it takes precedence over `--version`.\n     + \n     + --help::\n     + \tPrints the synopsis and a list of the most commonly used\n\n-- \ngitgitgadget\n"},{"id":"435847","messageId":"c55360272d581b0629e337186482fd5a2f13f4a3.1631626038.git.gitgitgadget@gmail.com","threadId":"56492","inReplyTo":"pull.1038.v2.git.1631626038.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] help: make sure local html page exists before calling external processes","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-14T13:27:17Z","receivedAt":"2021-09-14T13:27:24Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n\nWe check that git.html exists, regardless of the page the user wants to open.\nChecking whether the requested page exists instead gives us a smoother user\nexperience in two use cases:\n\n1) The requested page doesn't exist\n\nWhen calling a git command and there is an error, most users reasonably expect\ngit to produce an error message on the standard error stream, but in this case\nwe pass the filepath to git web--browse which passes it on to a browser (or a\nhelper program like xdg-open or start that should in turn open a browser)\nwithout any error and many GUI based browsers or helpers won't output such a\nmessage onto the standard error stream.\n\nEspecially the helper programs tend to show the corresponding error message in\na message box and wait for user input before exiting. This leaves users in\ninteractive console sessions without an error message in their console,\nwithout a console prompt and without the help page they expected.\n\n2) git.html is missing for some reason, but the user asked for some other page\n\nWe currently refuse to show any local html help page when we can't find\ngit.html. Even if the requested help page exists. If we check for the requested\npage instead, we can show the user all available pages and only error out on\nthose that don't exist.\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n builtin/help.c  |  9 ++++++---\n t/t0012-help.sh | 16 ++++++++++++++++\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b7eec06c3de..7731659765c 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -467,11 +467,14 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n \tif (!html_path)\n \t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n \n-\t/* Check that we have a git documentation directory. */\n+\t/*\n+\t * Check that the page we're looking for exists.\n+\t */\n \tif (!strstr(html_path, \"://\")) {\n-\t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n+\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n \t\t    || !S_ISREG(st.st_mode))\n-\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n+\t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n+\t\t\t\thtml_path, page);\n \t}\n \n \tstrbuf_init(page_path, 0);\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 5679e29c624..913f34c8e9d 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -73,6 +73,22 @@ test_expect_success 'git help -g' '\n \ttest_i18ngrep \"^   tutorial   \" help.output\n '\n \n+test_expect_success 'git help fails for non-existing html pages' '\n+\tconfigure_help &&\n+\tmkdir html-empty &&\n+\ttest_must_fail git -c help.htmlpath=html-empty help status &&\n+\ttest_must_be_empty test-browser.log\n+'\n+\n+test_expect_success 'git help succeeds without git.html' '\n+\tconfigure_help &&\n+\tmkdir html-with-docs &&\n+\ttouch html-with-docs/git-status.html &&\n+\tgit -c help.htmlpath=html-with-docs help status &&\n+\techo \"html-with-docs/git-status.html\" >expect &&\n+\ttest_cmp expect test-browser.log\n+'\n+\n test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n-- \ngitgitgadget\n\n"},{"id":"435848","messageId":"bc9a4534f5bc6756ab2df869b55e390183c4ff30.1631626038.git.gitgitgadget@gmail.com","threadId":"56492","inReplyTo":"pull.1038.v2.git.1631626038.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] documentation: add documentation for 'git version'","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-14T13:27:18Z","receivedAt":"2021-09-14T13:27:29Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n\nWhile 'git version' is probably the least complex git command,\nit is a non-experimental user-facing builtin command. As such\nit should have a help page.\n\nBoth `git help` and `git version` can be called as options\n(`--help`/`--version`) that internally get converted to the\ncorresponding command. Add a small paragraph to\nDocumentation/git.txt describing how these two options\ninteract with each other and link to this help page for the\nsub-options that `--version` can take. Well, currently there\nis only one sub-option, but that could potentially increase\nin future versions of Git.\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n Documentation/git-version.txt | 28 ++++++++++++++++++++++++++++\n Documentation/git.txt         |  4 ++++\n 2 files changed, 32 insertions(+)\n create mode 100644 Documentation/git-version.txt\n\ndiff --git a/Documentation/git-version.txt b/Documentation/git-version.txt\nnew file mode 100644\nindex 00000000000..80fa7754a6d\n--- /dev/null\n+++ b/Documentation/git-version.txt\n@@ -0,0 +1,28 @@\n+git-version(1)\n+==============\n+\n+NAME\n+----\n+git-version - Display version information about Git\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git version' [--build-options]\n+\n+DESCRIPTION\n+-----------\n+With no options given, the version of 'git' is printed on the standard output.\n+\n+Note that `git --version` is identical to `git version` because the\n+former is internally converted into the latter.\n+\n+OPTIONS\n+-------\n+--build-options::\n+\tInclude additional information about how git was built for diagnostic\n+\tpurposes.\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 6dd241ef838..95fe6f31b4f 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -41,6 +41,10 @@ OPTIONS\n -------\n --version::\n \tPrints the Git suite version that the 'git' program came from.\n++\n+This option is internaly converted to `git version ...` and accepts\n+the same options as the linkgit:git-version[1] command. If `--help` is\n+also given, it takes precedence over `--version`.\n \n --help::\n \tPrints the synopsis and a list of the most commonly used\n-- \ngitgitgadget\n"},{"id":"436947","messageId":"87o88i2keu.fsf@evledraar.gmail.com","threadId":"56492","inReplyTo":"bc9a4534f5bc6756ab2df869b55e390183c4ff30.1631626038.git.gitgitgadget@gmail.com","subject":"Is \"make check-docs\" useful anymore?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-24T13:00:42Z","receivedAt":"2021-09-24T13:27:14Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 14 2021, Matthias Aßhauer via GitGitGadget wrote:\n\n> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n>\n> While 'git version' is probably the least complex git command,\n> it is a non-experimental user-facing builtin command. As such\n> it should have a help page.\n>\n> Both `git help` and `git version` can be called as options\n> (`--help`/`--version`) that internally get converted to the\n> corresponding command. Add a small paragraph to\n> Documentation/git.txt describing how these two options\n> interact with each other and link to this help page for the\n> sub-options that `--version` can take. Well, currently there\n> is only one sub-option, but that could potentially increase\n> in future versions of Git.\n>\n> Signed-off-by: Matthias Aßhauer <mha1993@live.de>\n> ---\n>  Documentation/git-version.txt | 28 ++++++++++++++++++++++++++++\n>  Documentation/git.txt         |  4 ++++\n>  2 files changed, 32 insertions(+)\n>  create mode 100644 Documentation/git-version.txt\n>\n> diff --git a/Documentation/git-version.txt b/Documentation/git-version.txt\n> new file mode 100644\n> index 00000000000..80fa7754a6d\n> --- /dev/null\n> +++ b/Documentation/git-version.txt\n> @@ -0,0 +1,28 @@\n> +git-version(1)\n> +==============\n> +\n> +NAME\n> +----\n> +git-version - Display version information about Git\n> +\n> +SYNOPSIS\n> +--------\n> +[verse]\n> +'git version' [--build-options]\n> +\n> +DESCRIPTION\n> +-----------\n> +With no options given, the version of 'git' is printed on the standard output.\n> +\n> +Note that `git --version` is identical to `git version` because the\n> +former is internally converted into the latter.\n> +\n> +OPTIONS\n> +-------\n> +--build-options::\n> +\tInclude additional information about how git was built for diagnostic\n> +\tpurposes.\n> +\n> +GIT\n> +---\n> +Part of the linkgit:git[1] suite\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 6dd241ef838..95fe6f31b4f 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -41,6 +41,10 @@ OPTIONS\n>  -------\n>  --version::\n>  \tPrints the Git suite version that the 'git' program came from.\n> ++\n> +This option is internaly converted to `git version ...` and accepts\n> +the same options as the linkgit:git-version[1] command. If `--help` is\n> +also given, it takes precedence over `--version`.\n>  \n>  --help::\n>  \tPrints the synopsis and a list of the most commonly used\n\nI didn't notice until after it hit master that this caused a regression\nin \"make check-docs\":\n\n    $ make -s check-docs\n    removed but documented: git-version\n\nThe \"fix\" is rather easy, i.e. adding \"git-version\" to the whitelist.\n\nBut I wondered about $subject, i.e. we want to run the \"lint\" part, but\ndo we really need something reminding us that there isn't a mapping\nbetween Documentation/*.txt and *.o files present at the top-level?\n\nThat whole part seems to have been some \"reminder to document\" addition\nin 8c989ec5288 (Makefile: $(MAKE) check-docs, 2006-04-13).\n\nIf we're going to keep it in pretty much its current form then the CI\nintegration added in b98712b9aa9 (travis-ci: build documentation,\n2016-05-04) seems rather useless when it comes to this, i.e. we should\neither adjust it to exit non-zero, or check if we've got output under\n\"make -s\" and fail the check then.\n"},{"id":"436990","messageId":"xmqqpmsxvor8.fsf@gitster.g","threadId":"56492","inReplyTo":"87o88i2keu.fsf@evledraar.gmail.com","subject":"Re: Is \"make check-docs\" useful anymore?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-24T17:59:07Z","receivedAt":"2021-09-24T17:59:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I didn't notice until after it hit master that this caused a regression\n> in \"make check-docs\":\n>\n>     $ make -s check-docs\n>     removed but documented: git-version\n>\n> The \"fix\" is rather easy, i.e. adding \"git-version\" to the whitelist.\n>\n> But I wondered about $subject, i.e. we want to run the \"lint\" part, but\n> do we really need something reminding us that there isn't a mapping\n> between Documentation/*.txt and *.o files present at the top-level?\n\nThere were multiple things check-docs wanted to catch originally.\n\n - commands not referred to from the main page\n - a new command added without documentation\n - an old command removed while leaving documentation\n\nIt may be that we no longer remove commands, so the last check may\nbe less useful.\n\n> If we're going to keep it in pretty much its current form then the CI\n> integration added in b98712b9aa9 (travis-ci: build documentation,\n> 2016-05-04) seems rather useless when it comes to this, i.e. we should\n> either adjust it to exit non-zero,...\n\nYes, that is a good thing to do.\n\nThanks.\n"}]}