Re: `git stash --help` tries to pull up nonexistent file gitstack.html

Joseph Musser [off-list ref] writes:

Looks like a simple typo.

Unfortunately this does not reproduce to me (built from source on
Ubuntu Linux).

On 12 Aug 2016, at 17:48, Junio C Hamano [off-list ref] wrote:

Joseph Musser [off-list ref] writes:

quoted

Looks like a simple typo.

Unfortunately this does not reproduce to me (built from source on
Ubuntu Linux).

I tried it with the latest released version on Windows and OSX (2.9.2)
and was not able to reproduce it, too.

- Lars

Oh, I'm embarrassed. The typo was mine, I must have typed `git stack
--help`. I would have expected a syntax error message or "did you
mean" suggestions; it didn't even enter my mind that it would look up
whatever I typed before --help and assume it existed on disk.

I'm sorry!

On Fri, Aug 12, 2016 at 12:03 PM, Lars Schneider
[off-list ref] wrote:

quoted

On 12 Aug 2016, at 17:48, Junio C Hamano [off-list ref] wrote:

Joseph Musser [off-list ref] writes:

quoted

Looks like a simple typo.

Unfortunately this does not reproduce to me (built from source on
Ubuntu Linux).

I tried it with the latest released version on Windows and OSX (2.9.2)
and was not able to reproduce it, too.

- Lars

On Fri, Aug 12, 2016 at 9:15 AM, Joseph Musser [off-list ref] wrote:

Oh, I'm embarrassed. The typo was mine, I must have typed `git stack
--help`. I would have expected a syntax error message or "did you
mean" suggestions; it didn't even enter my mind that it would look up
whatever I typed before --help and assume it existed on disk.

I actually think you found an interesting (albeit minor) bug.
I think whenever "git" sees any word followed by "--help" and nothing else,
it blindly turns it into "git help" followed by that word. I think it
is reasonable
to expect that "git foo --help" responds with "foo: no such subcommand",
instead of "No manual entry for gitfoo".

It may not be too hard to arrange; this might be another low-hanging
fruit if somebody wants to try a patch ;-)

Thanks.

On Fri, Aug 12, 2016 at 9:25 AM, Junio C Hamano [off-list ref] wrote:

On Fri, Aug 12, 2016 at 9:15 AM, Joseph Musser [off-list ref] wrote:

quoted

Oh, I'm embarrassed. The typo was mine, I must have typed `git stack
--help`. I would have expected a syntax error message or "did you
mean" suggestions; it didn't even enter my mind that it would look up
whatever I typed before --help and assume it existed on disk.

I actually think you found an interesting (albeit minor) bug.
I think whenever "git" sees any word followed by "--help" and nothing else,
it blindly turns it into "git help" followed by that word. I think it
is reasonable
to expect that "git foo --help" responds with "foo: no such subcommand",
instead of "No manual entry for gitfoo".

It may not be too hard to arrange; this might be another low-hanging
fruit if somebody wants to try a patch ;-)

What about extension subcommands that aren't core? Wouldn't we prefer
if it still tried to find help for those also? Just a thought to add
to this.

Thanks,
Jake

If option --help is passed to a Git command, we try to open
the man page of that command. However, we do it even for commands
we don't know.  Make sure the command is known to Git before try
to open the man page.  If we don't know the command, give the
usual advice.

Signed-off-by: Ralf Thielow <redacted>
---
 builtin/help.c  | 21 ++++++++++++++-------
 t/t0012-help.sh | 15 +++++++++++++++
 2 files changed, 29 insertions(+), 7 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..55d45de 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -433,10 +433,22 @@ static void list_common_guides_help(void)
 	putchar('\n');
 }
 
+static void check_git_cmd(const char* cmd) {
+	char *alias = alias_lookup(cmd);
+
+	if (!is_git_command(cmd)) {
+		if (alias) {
+			printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+			free(alias);
+			exit(0);
+		} else
+			help_unknown_cmd(cmd);
+	}
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
-	char *alias;
 	enum help_format parsed_help_format;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,

@@ -476,12 +488,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (help_format == HELP_FORMAT_NONE)
 		help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
-		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-		free(alias);
-		return 0;
-	}
+	check_git_cmd(argv[0]);
 
 	switch (help_format) {
 	case HELP_FORMAT_NONE:

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 0000000..0dab88d
--- /dev/null
+++ b/t/t0012-help.sh

@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+test_expect_success "pass --help to unknown command" "
+	cat <<-EOF >expected &&
+		git: '123' is not a git command. See 'git --help'.
+	EOF
+	(git 123 --help 2>actual || true) &&
+	test_i18ncmp expected actual
+"
+
+test_done

-- 
2.9.2.911.g31804cd.dirty

If option --help is passed to a Git command, we try to open
the man page of that command. However, we do it even for commands
we don't know.  Make sure the command is known to Git before try
to open the man page.  If we don't know the command, give the
usual advice.

Signed-off-by: Ralf Thielow <redacted>
---
Changes in v2:
- not only check for commands but also for guides
- use the command assumed by "help_unknown_cmd"

 builtin/help.c  | 34 +++++++++++++++++++++++++++-------
 t/t0012-help.sh | 15 +++++++++++++++
 2 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..7d2110e 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -433,10 +433,35 @@ static void list_common_guides_help(void)
 	putchar('\n');
 }
 
+static int is_common_guide(const char* cmd)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(common_guides); i++)
+		if (!strcmp(cmd, common_guides[i].name))
+			return 1;
+	return 0;
+}
+
+static const char* check_git_cmd(const char* cmd)
+{
+	char *alias;
+
+	if (is_git_command(cmd) || is_common_guide(cmd))
+		return cmd;
+
+	alias = alias_lookup(cmd);
+	if (alias) {
+		printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+		free(alias);
+		exit(0);
+	} else
+		return help_unknown_cmd(cmd);
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
-	char *alias;
 	enum help_format parsed_help_format;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,

@@ -476,12 +501,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (help_format == HELP_FORMAT_NONE)
 		help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
-		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-		free(alias);
-		return 0;
-	}
+	argv[0] = check_git_cmd(argv[0]);
 
 	switch (help_format) {
 	case HELP_FORMAT_NONE:

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 0000000..0dab88d
--- /dev/null
+++ b/t/t0012-help.sh

@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+test_expect_success "pass --help to unknown command" "
+	cat <<-EOF >expected &&
+		git: '123' is not a git command. See 'git --help'.
+	EOF
+	(git 123 --help 2>actual || true) &&
+	test_i18ncmp expected actual
+"
+
+test_done

-- 
2.9.2.912.g51c4565.dirty

From: "Ralf Thielow" <redacted>

If option --help is passed to a Git command, we try to open
the man page of that command. However, we do it even for commands
we don't know.  Make sure the command is known to Git before try
to open the man page.  If we don't know the command, give the
usual advice.

I'm still not sure this is enough. One of the problems back when I 
introduced the --guides option (65f9835 (builtin/help.c: add --guide option, 
2013-04-02)) was that we had no easy way of determining what guides were 
available, especially given the *nix/Windows split where the help defaults 
are different (--man/--html).

At the time[1] we (I) punted on trying to determine which guides were 
actually installed, and just created a short list of the important guides, 
which I believe you now check. However the less common guides are still 
there (gitcvs-migration?), and others may be added locally.

One option may be to report that "no command or common guide found, will 
search for other guide (may fail)", which at least allows you to check the 
command list first, and then the common guide list, and only then warn 
(option?), and finally go on the rabbit hunt (possibly fruitless) for the 
missing guide (we've already decided it can't be a command!)

--
Philip

[1] 
https://public-inbox.org/git/1364942392-576-1-git-send-email-philipoakley@iee.org/ 
(V3) plus previous discussions
https://public-inbox.org/git/1362342072-1412-1-git-send-email-philipoakley@iee.org/ 
(V2) see note
Patch 6 - 13:
All dropped.
Drop the separate guide list.txt and extraction script, which was
copied from the common command list and script. If the guide usage
list is useful, extend the command-list.txt and generate-cmdlist.sh
at a later 
datehttps://public-inbox.org/git/1361660761-1932-1-git-send-email-philipoakley@iee.org/#t 
(V1) the original series

If option --help is passed to a Git command, we try to open
the man page of that command.  However, we do it even for commands
we don't know.  Make sure it is a Git command by using "help_unknown_cmd"
which is even able to assume a command if the user made a typo.

This breaks "git <concept> --help" while "git help <concept>" still works.

As "<cmd> --help" will internally be turned into "help <cmd>",
introduce the hidden option "--swapped" in order to know which
version has been called.

Signed-off-by: Ralf Thielow <redacted>
---
Thanks, all, for the help!

Changes since v2:
- don't check for common guides as the list is very incomplete
- only check for git commands when called via <cmd> --help (introduce
  option --swapped for that), as suggested by Junio
- change test case to check for --help being passed to a concept
  used as a git command

 builtin/help.c  | 30 +++++++++++++++++++++++-------
 git.c           | 15 ++++++++++++++-
 t/t0012-help.sh | 15 +++++++++++++++
 3 files changed, 52 insertions(+), 8 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..76f07c7 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -37,7 +37,9 @@ static int show_all = 0;
 static int show_guides = 0;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
+static int swapped = 0;
 static struct option builtin_help_options[] = {
+	OPT_BOOL('s', "swapped", &swapped, "mark as being called by <cmd> --help"),
 	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
 	OPT_BOOL('g', "guides", &show_guides, N_("print list of useful guides")),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),

@@ -433,10 +435,29 @@ static void list_common_guides_help(void)
 	putchar('\n');
 }
 
+static const char* check_git_cmd(const char* cmd)
+{
+	char *alias;
+
+	if (is_git_command(cmd))
+		return cmd;
+
+	alias = alias_lookup(cmd);
+	if (alias) {
+		printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+		free(alias);
+		exit(0);
+	}
+
+	if (swapped)
+		return help_unknown_cmd(cmd);
+
+	return cmd;
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
-	char *alias;
 	enum help_format parsed_help_format;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,

@@ -476,12 +497,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (help_format == HELP_FORMAT_NONE)
 		help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
-		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-		free(alias);
-		return 0;
-	}
+	argv[0] = check_git_cmd(argv[0]);
 
 	switch (help_format) {
 	case HELP_FORMAT_NONE:

diff --git a/git.c b/git.c
index 0f1937f..71ea983 100644
--- a/git.c
+++ b/git.c

@@ -528,10 +528,23 @@ static void handle_builtin(int argc, const char **argv)
 	strip_extension(argv);
 	cmd = argv[0];
 
-	/* Turn "git cmd --help" into "git help cmd" */
+	/* Turn "git cmd --help" into "git help --swapped cmd" */
 	if (argc > 1 && !strcmp(argv[1], "--help")) {
+		struct argv_array args;
+		int i;
+
 		argv[1] = argv[0];
 		argv[0] = cmd = "help";
+
+		argv_array_init(&args);
+		for (i = 0; i < argc; i++) {
+			argv_array_push(&args, argv[i]);
+			if (i == 0)
+				argv_array_push(&args, "--swapped");
+		}
+
+		argc++;
+		argv = argv_array_detach(&args);
 	}
 
 	builtin = get_builtin(cmd);

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 0000000..6f700b1
--- /dev/null
+++ b/t/t0012-help.sh

@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+test_expect_success "pass --help to common guide" "
+	cat <<-EOF >expected &&
+		git: 'revisions' is not a git command. See 'git --help'.
+	EOF
+	(git revisions --help 2>actual || true) &&
+	test_i18ncmp expected actual
+"
+
+test_done

-- 
2.9.2.912.g69c5047

On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:

OPT_HIDDEN_BOOL maybe?

 	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
 	OPT_BOOL('g', "guides", &show_guides, N_("print list of useful guides")),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),

2016-08-16 18:33 GMT+02:00 John Keeping [off-list ref]:

On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:

quoted

 static struct option builtin_help_options[] = {
+     OPT_BOOL('s', "swapped", &swapped, "mark as being called by <cmd> --help"),

OPT_HIDDEN_BOOL maybe?

Yeah >_<

Thanks!

Introduce option --command-only to the help command.  With this option
being passed, "git help" will open man pages only for commands.

Since we know it is a command, we can use function help_unknown_command
to give the user advice on typos.

Signed-off-by: Ralf Thielow <redacted>
---
I am not sure about the first test case, but I think it'd have
prevented me from making earlier mistakes of this change. That's
why I added it.
Just calling a git command that succeeds in a test isn't really
a check, so ... I dunno

 Documentation/git-help.txt             | 11 ++++++++---
 builtin/help.c                         | 30 +++++++++++++++++++++++-------
 contrib/completion/git-completion.bash |  2 +-
 t/t0012-help.sh                        | 21 +++++++++++++++++++++
 4 files changed, 53 insertions(+), 11 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 40d328a..cf6a414 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt

@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all] [-g|--guide]
+'git help' [-a|--all] [-c|--command-only] [-g|--guide]
 	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION

@@ -29,8 +29,9 @@ guide is brought up. The 'man' program is used by default for this
 purpose, but this can be overridden by other options or configuration
 variables.
 
-Note that `git --help ...` is identical to `git help ...` because the
-former is internally converted into the latter.
+Note that `git --help ...` is almost identical to `git help ...` because
+the former is internally converted into the latter with option --command-only
+being added.
 
 To display the linkgit:git[1] man page, use `git help git`.
 

@@ -43,6 +44,10 @@ OPTIONS
 	Prints all the available commands on the standard output. This
 	option overrides any given command or guide name.
 
+-c::
+--command-only::
+	Display help information only for commands.
+
 -g::
 --guides::
 	Prints a list of useful guides on the standard output. This

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..2249a67 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -37,8 +37,10 @@ static int show_all = 0;
 static int show_guides = 0;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
+static int cmd_only;
 static struct option builtin_help_options[] = {
 	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
+	OPT_BOOL('c', "command-only", &cmd_only, N_("show help only for commands")),
 	OPT_BOOL('g', "guides", &show_guides, N_("print list of useful guides")),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"),

@@ -433,10 +435,29 @@ static void list_common_guides_help(void)
 	putchar('\n');
 }
 
+static const char *check_git_cmd(const char* cmd)
+{
+	char *alias;
+
+	if (is_git_command(cmd))
+		return cmd;
+
+	alias = alias_lookup(cmd);
+	if (alias) {
+		printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+		free(alias);
+		exit(0);
+	}
+
+	if (cmd_only)
+		return help_unknown_cmd(cmd);
+
+	return cmd;
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
-	char *alias;
 	enum help_format parsed_help_format;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,

@@ -476,12 +497,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (help_format == HELP_FORMAT_NONE)
 		help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
-		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-		free(alias);
-		return 0;
-	}
+	argv[0] = check_git_cmd(argv[0]);
 
 	switch (help_format) {
 	case HELP_FORMAT_NONE:

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c1b2135..354afe5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1393,7 +1393,7 @@ _git_help ()
 {
 	case "$cur" in
 	--*)
-		__gitcomp "--all --guides --info --man --web"
+		__gitcomp "--all --command-only --guides --info --man --web"
 		return
 		;;
 	esac

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 0000000..e20f907
--- /dev/null
+++ b/t/t0012-help.sh

@@ -0,0 +1,21 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+test_expect_success "works for commands and guides by default" "
+	git help status &&
+	git help revisions
+"
+
+test_expect_success "--command-only does not work for guides" "
+	git help --command-only status &&
+	cat <<-EOF >expected &&
+		git: 'revisions' is not a git command. See 'git --help'.
+	EOF
+	(git help --command-only revisions 2>actual || true) &&
+	test_i18ncmp expected actual
+"
+
+test_done

-- 
2.9.2.912.gd0c0e83

In this version, one patch has been turned into two.  The first introduces the
option "command-only" to make 'help' only working for commands and additionally
give some nice help on typos.  The second makes option --help only work for actual
Git commands.

Ralf Thielow (2):
  help: introduce option --command-only
  help: make option --help open man pages only for Git commands

 Documentation/git-help.txt             | 11 ++++++++---
 builtin/help.c                         | 30 +++++++++++++++++++++++-------
 contrib/completion/git-completion.bash |  2 +-
 git.c                                  | 15 ++++++++++++++-
 t/t0012-help.sh                        | 29 +++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 12 deletions(-)
 create mode 100755 t/t0012-help.sh

-- 
2.9.2.912.gd0c0e83

From: "Ralf Thielow" <redacted>

Introduce option --command-only to the help command.  With this option
being passed, "git help" will open man pages only for commands.

Since we know it is a command, we can use function help_unknown_command
to give the user advice on typos.

Signed-off-by: Ralf Thielow <redacted>
---
I am not sure about the first test case, but I think it'd have
prevented me from making earlier mistakes of this change. That's
why I added it.
Just calling a git command that succeeds in a test isn't really
a check, so ... I dunno

Do the tests work on both *nix and Windows, given that Windows uses 
the --web option by default, so is likely to fire up a browser instead of 
the man pages? Otherwise it sounds to be a reasonable check.

s/commands/known commands/ ?

s/commands/known commands/ ?

--
Philip 

If option --help is passed to a Git command, we try to open
the man page of that command.  However, we do it even for commands
we don't know.  Make sure it is a Git command.

This breaks "git <concept> --help" while "git help <concept>" still works.

Signed-off-by: Ralf Thielow <redacted>
---
 git.c           | 15 ++++++++++++++-
 t/t0012-help.sh |  8 ++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 0f1937f..2cd2e06 100644
--- a/git.c
+++ b/git.c

@@ -528,10 +528,23 @@ static void handle_builtin(int argc, const char **argv)
 	strip_extension(argv);
 	cmd = argv[0];
 
-	/* Turn "git cmd --help" into "git help cmd" */
+	/* Turn "git cmd --help" into "git help --command-only cmd" */
 	if (argc > 1 && !strcmp(argv[1], "--help")) {
+		struct argv_array args;
+		int i;
+
 		argv[1] = argv[0];
 		argv[0] = cmd = "help";
+
+		argv_array_init(&args);
+		for (i = 0; i < argc; i++) {
+			argv_array_push(&args, argv[i]);
+			if (!i)
+				argv_array_push(&args, "--command-only");
+		}
+
+		argc++;
+		argv = argv_array_detach(&args);
 	}
 
 	builtin = get_builtin(cmd);

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index e20f907..81fec90 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh

@@ -18,4 +18,12 @@ test_expect_success "--command-only does not work for guides" "
 	test_i18ncmp expected actual
 "
 
+test_expect_success "--help does not work for guides" "
+	cat <<-EOF >expected &&
+		git: 'revisions' is not a git command. See 'git --help'.
+	EOF
+	(git revisions --help 2>actual || true) &&
+	test_i18ncmp expected actual
+"
+
 test_done

-- 
2.9.2.912.gd0c0e83

Hi Ralf,

Ralf Thielow [off-list ref] writes:

[...]
+test_expect_success "works for commands and guides by default" "
+        git help status &&
+        git help revisions
+"
+
+test_expect_success "--command-only does not work for guides" "
+        git help --command-only status &&
+        cat <<-EOF >expected &&
+                git: 'revisions' is not a git command. See 'git --help'.
+        EOF
+        (git help --command-only revisions 2>actual || true) &&

I think you want to use
  `test_must_fail git help --command-only revisions 2>actual`
here to make sure that the command does fail.

Thanks,
Rémi

Hi Ralf,

On Thu, 18 Aug 2016, Ralf Thielow wrote:

Apart from using double quotes (which is inconsistent with the single
quotes used literally everwhere else in the test suite), this test is
incorrect. If the man page is not *installed*, it will fail:

$ sudo mv /usr/share/man/man1/git-status.1.gz \
	/usr/share/man/man1/git-status.old.1.gz

$ sh t0012-help.sh -i -v -x
Initialized empty Git repository in .../trash directory.t0012-help/.git/
expecting success:
        git help status &&
        git help revisions

+ git help status
No manual entry for git-status
See 'man 7 undocumented' for help when manual pages are not available.
error: last command exited with $?=16
not ok 1 - works for commands and guides by default
#
#               git help status &&
#               git help revisions
#

It gets even worse.

On Windows, the default format is *not* man pages but html pages. So those
would have to be installed, too, to guarantee that the test succeeds.

It gets *even* worse.

On Windows, there is really no central location for man/html pages for
documentation, so we have to emulate that "prefix" (which is typically
/usr on Linux) via a "runtime prefix", i.e. a prefix determined relative
to the location of the currently running git executable. In the test
suite's case, it is typically the top-level directory of the git.git
checkout [*1*]. There are no man/html pages in that directory structure by
default (I, for one, rarely build them myself), and certainly not in the
place expected by this test.

It gets *even worse*.

Since the help.format is html on Windows, the page is opened by the
default viewer for HTML pages. So even if all of the above would be fixed,
running t0012-help of a supposedly unsupervised test suite would open new
tabs in the web browser. Probably forcing it into the foreground, too.

So how about fixing that? I would suggest to do it this way:

- configure help.format = html (for "man", the current code would always
  add $(prefix)/share/man to the MANPATH when testing, not what we want,
  and hacking this code *just* for testing is both ugly and unnecessary).

- configure help.htmlpath to point to a subdirectory that is created and
  populated in the same test script.

- configure help.browser to point to a script that is created in the same
  script and whose output we can verify, too.

The last point actually requires a patch that was recently introduced into
Git for Windows [*1*] (and that did not make it upstream yet) which
reverts that change whereby web--browse was sidestepped. That sidestepping
was well-intentioned but turned out to cause more harm than good.

Ciao,
Johannes

Footnote *1*: That statement is actually not even correct. As the git
executable can live in both $(prefix)/bin/ and $(prefix)/libexec/git-core,
i.e. at different directory levels below the prefix, we need to inspect
the *name* of the directory in which git.exe lives, and a git.git checkout
typically lives in a .../git/ directory which matches *none* of the
expected suffixes, so the runtime prefix defaults to "/", i.e. the
*current drive's root directory*. So your current test would only succeed
if the man pages for git-status and gitrevisions were copied into
C:\mingw64\share\man\man1!

Footnote *2*: https://github.com/git-for-windows/git/commit/243c72f5b0

2016-08-19 10:39 GMT+02:00 Remi Galan Alfonso
[off-list ref]:

Hi Ralf,

Ralf Thielow [off-list ref] writes:

quoted

[...]
+test_expect_success "works for commands and guides by default" "
+        git help status &&
+        git help revisions
+"
+
+test_expect_success "--command-only does not work for guides" "
+        git help --command-only status &&
+        cat <<-EOF >expected &&
+                git: 'revisions' is not a git command. See 'git --help'.
+        EOF
+        (git help --command-only revisions 2>actual || true) &&

I think you want to use
  `test_must_fail git help --command-only revisions 2>actual`
here to make sure that the command does fail.

Thanks!

Thanks,
Rémi

2016-08-19 10:32 GMT+02:00 Johannes Schindelin [off-list ref]:

So how about fixing that? I would suggest to do it this way:

- configure help.format = html (for "man", the current code would always
  add $(prefix)/share/man to the MANPATH when testing, not what we want,
  and hacking this code *just* for testing is both ugly and unnecessary).

- configure help.htmlpath to point to a subdirectory that is created and
  populated in the same test script.

- configure help.browser to point to a script that is created in the same
  script and whose output we can verify, too.

The last point actually requires a patch that was recently introduced into
Git for Windows [*1*] (and that did not make it upstream yet) which
reverts that change whereby web--browse was sidestepped. That sidestepping
was well-intentioned but turned out to cause more harm than good.

So I'll pickup the patch you sent [1] to my series and prepare the test cases
the way you described to verify that the 'help' command works.

Thanks!

[1]
http://public-inbox.org/git/03ae6a9d47cb95a54960bfdc90c5392f890ff1e3.1471595956.git.johannes.schindelin@gmx.de/

Hi Ralf,

On Tue, 23 Aug 2016, Ralf Thielow wrote:

2016-08-19 10:32 GMT+02:00 Johannes Schindelin [off-list ref]:

quoted

So how about fixing that? I would suggest to do it this way:

- configure help.format = html (for "man", the current code would always
  add $(prefix)/share/man to the MANPATH when testing, not what we want,
  and hacking this code *just* for testing is both ugly and unnecessary).

- configure help.htmlpath to point to a subdirectory that is created and
  populated in the same test script.

- configure help.browser to point to a script that is created in the same
  script and whose output we can verify, too.

The last point actually requires a patch that was recently introduced into
Git for Windows [*1*] (and that did not make it upstream yet) which
reverts that change whereby web--browse was sidestepped. That sidestepping
was well-intentioned but turned out to cause more harm than good.

So I'll pickup the patch you sent [1] to my series and prepare the test cases
the way you described to verify that the 'help' command works.

Thanks!

Ciao,
Johannes

Changes in v2 are:
- add a patch from Dscho to make config variable 'help.browser' work on Windows again
- rename option "--command-only" to "--exclude-guides" which is less ambiguous in 'help' context
- improve test script
- refactor usage of argv_array in handle_builtin

Johannes Schindelin (1):
  Revert "display HTML in default browser using Windows' shell API"

Ralf Thielow (2):
  help: introduce option --exclude-guides
  help: make option --help open man pages only for Git commands

 Documentation/git-help.txt             | 11 ++++++---
 builtin/help.c                         | 37 ++++++++++++++++++------------
 compat/mingw.c                         | 42 ----------------------------------
 compat/mingw.h                         |  3 ---
 contrib/completion/git-completion.bash |  2 +-
 git.c                                  | 15 +++++++++++-
 t/t0012-help.sh                        | 41 +++++++++++++++++++++++++++++++++
 7 files changed, 87 insertions(+), 64 deletions(-)
 create mode 100755 t/t0012-help.sh

-- 
2.9.2.912.gd0c0e83

From: Johannes Schindelin <redacted>

Since 4804aab (help (Windows): Display HTML in default browser using
Windows' shell API, 2008-07-13), Git for Windows used to call
`ShellExecute()` to launch the default Windows handler for `.html`
files.

The idea was to avoid going through a shell script, for performance
reasons.

However, this change ignores the `help.browser` config setting. Together
with browsing help not being a performance-critical operation, let's
just revert that patch.

Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Ralf Thielow <redacted>
---
 builtin/help.c |  7 -------
 compat/mingw.c | 42 ------------------------------------------
 compat/mingw.h |  3 ---
 3 files changed, 52 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..e8f79d7 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -379,17 +379,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
 	free(to_free);
 }
 
-/*
- * If open_html is not defined in a platform-specific way (see for
- * example compat/mingw.h), we use the script web--browse to display
- * HTML.
- */
-#ifndef open_html
 static void open_html(const char *path)
 {
 	execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
 }
-#endif
 
 static void show_html_page(const char *git_cmd)
 {

diff --git a/compat/mingw.c b/compat/mingw.c
index 2b5467d..3fbfda5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -1930,48 +1930,6 @@ int mingw_raise(int sig)
 	}
 }
 
-
-static const char *make_backslash_path(const char *path)
-{
-	static char buf[PATH_MAX + 1];
-	char *c;
-
-	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-		die("Too long path: %.*s", 60, path);
-
-	for (c = buf; *c; c++) {
-		if (*c == '/')
-			*c = '\\';
-	}
-	return buf;
-}
-
-void mingw_open_html(const char *unixpath)
-{
-	const char *htmlpath = make_backslash_path(unixpath);
-	typedef HINSTANCE (WINAPI *T)(HWND, const char *,
-			const char *, const char *, const char *, INT);
-	T ShellExecute;
-	HMODULE shell32;
-	int r;
-
-	shell32 = LoadLibrary("shell32.dll");
-	if (!shell32)
-		die("cannot load shell32.dll");
-	ShellExecute = (T)GetProcAddress(shell32, "ShellExecuteA");
-	if (!ShellExecute)
-		die("cannot run browser");
-
-	printf("Launching default browser to display HTML ...\n");
-	r = HCAST(int, ShellExecute(NULL, "open", htmlpath,
-				NULL, "\\", SW_SHOWNORMAL));
-	FreeLibrary(shell32);
-	/* see the MSDN documentation referring to the result codes here */
-	if (r <= 32) {
-		die("failed to launch browser for %.*s", MAX_PATH, unixpath);
-	}
-}
-
 int link(const char *oldpath, const char *newpath)
 {
 	typedef BOOL (WINAPI *T)(LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES);

diff --git a/compat/mingw.h b/compat/mingw.h
index 95e128f..2cadb81 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -417,9 +417,6 @@ int mingw_offset_1st_component(const char *path);
 #include <inttypes.h>
 #endif
 
-void mingw_open_html(const char *path);
-#define open_html mingw_open_html
-
 /**
  * Converts UTF-8 encoded string to UTF-16LE.
  *

-- 
2.9.2.912.gd0c0e83

If option --help is passed to a Git command, we try to open
the man page of that command.  However, we do it for both commands
and concepts.  Make sure it is an actual command.

This makes "git <concept> --help" not working anymore, while
"git help <concept>" still works.

Signed-off-by: Ralf Thielow <redacted>
---
 Documentation/git-help.txt |  5 +++--
 git.c                      | 15 ++++++++++++++-
 t/t0012-help.sh            |  8 ++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index eeb1950..8d21e9f 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt

@@ -29,8 +29,9 @@ guide is brought up. The 'man' program is used by default for this
 purpose, but this can be overridden by other options or configuration
 variables.
 
-Note that `git --help ...` is identical to `git help ...` because the
-former is internally converted into the latter.
+Note that `git --help ...` is almost identical to `git help ...` because
+the former is internally converted into the latter with option --exclude-guides
+being added.
 
 To display the linkgit:git[1] man page, use `git help git`.
 

diff --git a/git.c b/git.c
index 0f1937f..1c61151 100644
--- a/git.c
+++ b/git.c

@@ -522,21 +522,34 @@ static void strip_extension(const char **argv)
 
 static void handle_builtin(int argc, const char **argv)
 {
+	struct argv_array args = ARGV_ARRAY_INIT;
 	const char *cmd;
 	struct cmd_struct *builtin;
 
 	strip_extension(argv);
 	cmd = argv[0];
 
-	/* Turn "git cmd --help" into "git help cmd" */
+	/* Turn "git cmd --help" into "git help --exclude-guides cmd" */
 	if (argc > 1 && !strcmp(argv[1], "--help")) {
+		int i;
+
 		argv[1] = argv[0];
 		argv[0] = cmd = "help";
+
+		for (i = 0; i < argc; i++) {
+			argv_array_push(&args, argv[i]);
+			if (!i)
+				argv_array_push(&args, "--exclude-guides");
+		}
+
+		argc++;
+		argv = args.argv;
 	}
 
 	builtin = get_builtin(cmd);
 	if (builtin)
 		exit(run_builtin(builtin, argc, argv));
+	argv_array_clear(&args);
 }
 
 static void execv_dashed_external(const char **argv)

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index fb1abd7..2b90947 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh

@@ -30,4 +30,12 @@ test_expect_success "--exclude-guides does not work for guides" "
 	test_i18ncmp expected actual
 "
 
+test_expect_success "--help does not work for guides" "
+	cat <<-EOF >expected &&
+		git: 'revisions' is not a git command. See 'git --help'.
+	EOF
+	test_must_fail git revisions --help 2>actual &&
+	test_i18ncmp expected actual
+"
+
 test_done

-- 
2.9.2.912.gd0c0e83

Introduce option --exclude-guides to the help command.  With this option
being passed, "git help" will open man pages only for actual commands.

Since we know it is a command, we can use function help_unknown_command
to give the user advice on typos.

Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Ralf Thielow <redacted>
---
In the test script we do two things I'd like to point out:

+       test_config help.htmlpath test://html &&

As we pass a URL, Git won't check if the given path looks like
a documentation directory.  Another solution would be to create
a directory, add a file "git.html" to it and just use this path.

+       test_config help.browser firefox

Git checks if the browser is known, so the "test-browser" needs to
pretend it is one of them.

 Documentation/git-help.txt             |  6 +++++-
 builtin/help.c                         | 30 +++++++++++++++++++++++-------
 contrib/completion/git-completion.bash |  2 +-
 t/t0012-help.sh                        | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 9 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 40d328a..eeb1950 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt

@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 --------
 [verse]
-'git help' [-a|--all] [-g|--guide]
+'git help' [-a|--all] [-e|--exclude-guides] [-g|--guide]
 	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION

@@ -43,6 +43,10 @@ OPTIONS
 	Prints all the available commands on the standard output. This
 	option overrides any given command or guide name.
 
+-e::
+--exclude-guides::
+	Do not show help for guides.
+
 -g::
 --guides::
 	Prints a list of useful guides on the standard output. This

diff --git a/builtin/help.c b/builtin/help.c
index e8f79d7..40901a9 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -37,8 +37,10 @@ static int show_all = 0;
 static int show_guides = 0;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
+static int exclude_guides;
 static struct option builtin_help_options[] = {
 	OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
+	OPT_BOOL('e', "exclude-guides", &exclude_guides, N_("exclude guides")),
 	OPT_BOOL('g', "guides", &show_guides, N_("print list of useful guides")),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"),

@@ -426,10 +428,29 @@ static void list_common_guides_help(void)
 	putchar('\n');
 }
 
+static const char *check_git_cmd(const char* cmd)
+{
+	char *alias;
+
+	if (is_git_command(cmd))
+		return cmd;
+
+	alias = alias_lookup(cmd);
+	if (alias) {
+		printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+		free(alias);
+		exit(0);
+	}
+
+	if (exclude_guides)
+		return help_unknown_cmd(cmd);
+
+	return cmd;
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
-	char *alias;
 	enum help_format parsed_help_format;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,

@@ -469,12 +490,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (help_format == HELP_FORMAT_NONE)
 		help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
-		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-		free(alias);
-		return 0;
-	}
+	argv[0] = check_git_cmd(argv[0]);
 
 	switch (help_format) {
 	case HELP_FORMAT_NONE:

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c1b2135..b148164 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1393,7 +1393,7 @@ _git_help ()
 {
 	case "$cur" in
 	--*)
-		__gitcomp "--all --guides --info --man --web"
+		__gitcomp "--all --exclude-guides --guides --info --man --web"
 		return
 		;;
 	esac

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 0000000..fb1abd7
--- /dev/null
+++ b/t/t0012-help.sh

@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+configure_help () {
+	test_config help.format html &&
+	test_config help.htmlpath test://html &&
+	test_config help.browser firefox 
+}
+
+test_expect_success "setup" "
+	write_script firefox <<-\EOF
+	exit 0
+	EOF
+"
+
+test_expect_success "works for commands and guides by default" "
+	configure_help &&
+	git help status &&
+	git help revisions
+"
+
+test_expect_success "--exclude-guides does not work for guides" "
+	cat <<-EOF >expected &&
+		git: 'revisions' is not a git command. See 'git --help'.
+	EOF
+	test_must_fail git help --exclude-guides revisions 2>actual &&
+	test_i18ncmp expected actual
+"
+
+test_done

-- 
2.9.2.912.gd0c0e83