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

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

[Joseph Musser]

Looks like a simple typo.


Joseph Musser

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

[Junio C Hamano]

Joseph Musser <me@jnm2.com> writes:

> Looks like a simple typo.

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

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

[Lars Schneider]

> On 12 Aug 2016, at 17:48, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Joseph Musser <me@jnm2.com> writes:
> 
>> 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

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

[Joseph Musser]

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
<larsxschneider@gmail.com> wrote:
>
>> On 12 Aug 2016, at 17:48, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Joseph Musser <me@jnm2.com> writes:
>>
>>> 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

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

[Junio C Hamano]

On Fri, Aug 12, 2016 at 9:15 AM, Joseph Musser <me@jnm2.com> 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.

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

[Jacob Keller]

On Fri, Aug 12, 2016 at 9:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On Fri, Aug 12, 2016 at 9:15 AM, Joseph Musser <me@jnm2.com> 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 ;-)
>

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

[PATCH] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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 <ralf.thielow@gmail.com>
---
 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

Re: [PATCH] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> 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 <ralf.thielow@gmail.com>
> ---

I love it when I say "This shouldn't be too hard; somebody may want
to do a patch", with just a vague implemention idea in my head, and
a patch magically appears with even a better design than I had in
mind when I said it [*1*] ;-)

>  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);
> +	}
> +}

Looks quite reasonable to reuse help_unknown_cmd() there.

Thanks, will queue.


[Footnote]

*1* The vague thing I had in my mind was to use is_git_command() and
    alias_lookup() to prevent the "git foo --help" -> "git help foo" 
    magic from triggering for 'foo' that is not known.  Your solution
    is MUCH cleaner and more straight-forward.

Re: [PATCH] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I love it when I say "This shouldn't be too hard; somebody may want
> to do a patch", with just a vague implemention idea in my head, and
> a patch magically appears with even a better design than I had in
> mind when I said it [*1*] ;-)

Having said that, I wonder if we could do a bit better.

    $ git -c help.autocorrect=1 whatchange --help
    WARNING: You called a Git command named 'whatchange', which does not exist.
    Continuing under the assumption that you meant 'whatchanged'
    in 0.1 seconds automatically...
    No manual entry for gitwhatchange

We are guessing that the user meant "whatchanged"; shouldn't we be
able to feed the corrected name of the command to the machinery to
drive the manpage viewer?

Re: [PATCH] help: make option --help open man pages only for Git commands

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
To: "Ralf Thielow" <ralf.thielow@gmail.com>
Cc: <git@vger.kernel.org>; <larsxschneider@gmail.com>; <me@jnm2.com>
Sent: Friday, August 12, 2016 11:53 PM
Subject: Re: [PATCH] help: make option --help open man pages only for Git 
commands


> Junio C Hamano <gitster@pobox.com> writes:
>
>> I love it when I say "This shouldn't be too hard; somebody may want
>> to do a patch", with just a vague implemention idea in my head, and
>> a patch magically appears with even a better design than I had in
>> mind when I said it [*1*] ;-)
>
> Having said that, I wonder if we could do a bit better.
>
>    $ git -c help.autocorrect=1 whatchange --help
>    WARNING: You called a Git command named 'whatchange', which does not 
> exist.
>    Continuing under the assumption that you meant 'whatchanged'
>    in 0.1 seconds automatically...
>    No manual entry for gitwhatchange
>
> We are guessing that the user meant "whatchanged"; shouldn't we be
> able to feed the corrected name of the command to the machinery to
> drive the manpage viewer?
>
But does it cope with the Guides? Should it cope if spelt that way?

git help revisions
git revisions --help

?

I suspect that the former should work, but not the latter, as a reasonable 
approach. I just haven't checked.

The other moderately low hanging fruit is to do the (full) list of guides as 
part of 'help'.

Philip 

Re: [PATCH] help: make option --help open man pages only for Git commands

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> But does it cope with the Guides? Should it cope if spelt that way?
>
> git help revisions
> git revisions --help

Hmph.  Ralf's patch is not just "I wonder if we could do a bit
better" but is also a regression.  I do not particularly care
if the latter stops working, but the former definitely should,
as "git help -g" encourages readers to type that, but with the
change "git help <concept>" seems to stop working.

[PATCH v2] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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 <ralf.thielow@gmail.com>
---
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

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Philip Oakley]

From: "Ralf Thielow" <ralf.thielow@gmail.com>
> 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

>
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

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

I think we should do both; "git help cvs-migration" should keep the
same codeflow and behaviour as we have today (so that it would still
work), while "git cvs-migration --help" should say "'cvs-migration'
is not a git command".  That would be a good clean-up anyway.

It obviously cannot be done if git.c::handle_builtin() does the same
"swap <word> --help to help <word>" hack, but we could improve that
part (e.g. rewrite it to "help --swapped <word>" to allow cmd_help()
to notice).  When the user said "<word> --help", we don't do guides,
when we swapped the word order, we check with guides, too.

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> 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.
>
> I think we should do both; "git help cvs-migration" should keep the
> same codeflow and behaviour as we have today (so that it would still
> work), while "git cvs-migration --help" should say "'cvs-migration'
> is not a git command".  That would be a good clean-up anyway.
>
> It obviously cannot be done if git.c::handle_builtin() does the same
> "swap <word> --help to help <word>" hack, but we could improve that
> part (e.g. rewrite it to "help --swapped <word>" to allow cmd_help()
> to notice).  When the user said "<word> --help", we don't do guides,
> when we swapped the word order, we check with guides, too.
>
The other option is to simply build a guide-list in exactly the same format 
as the command list (which if it works can be merged later). Re-use the 
existing code, etc.

I did propose that in my very first patch series, but it was probably a step 
too far at the time, as it stepped on the toes of your (junio's) script for 
the command list.

The link in my previous patch got mangled a possible start point for Ralf 
for looking at building a guide list would be 
http://public-inbox.org/git/1361660761-1932-7-git-send-email-philipoakley@iee.org/ 
(which worked back then ;-)

Philip

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> The other option is to simply build a guide-list in exactly the same
> format as the command list (which if it works can be merged
> later). Re-use the existing code, etc.

Yeah, that sounds like a good way to go forward.  To implement typo
correction for "git help <guidename>", having guide-list would be
very useful.

A related tangent is that I think "git <guide> --help" shouldn't
fall back to "git help <guide>", regardless of typo correction.  It
happens to "work" only because we blindly turned "<w> --help" to
"help <w>" without even checking what <w> is.  Making it stop
"working" would be a bugfix.

And having both command and guide list would be helpful to prevent
"git <guide> --help" from falling back to "git help <guide>".

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[John Keeping]

On Mon, Aug 15, 2016 at 09:40:54PM +0100, Philip Oakley wrote:
> From: "Junio C Hamano" <gitster@pobox.com>
> > "Philip Oakley" <philipoakley@iee.org> writes:
> >
> >> 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.
> >
> > I think we should do both; "git help cvs-migration" should keep the
> > same codeflow and behaviour as we have today (so that it would still
> > work), while "git cvs-migration --help" should say "'cvs-migration'
> > is not a git command".  That would be a good clean-up anyway.
> >
> > It obviously cannot be done if git.c::handle_builtin() does the same
> > "swap <word> --help to help <word>" hack, but we could improve that
> > part (e.g. rewrite it to "help --swapped <word>" to allow cmd_help()
> > to notice).  When the user said "<word> --help", we don't do guides,
> > when we swapped the word order, we check with guides, too.
> >
> The other option is to simply build a guide-list in exactly the same format 
> as the command list (which if it works can be merged later). Re-use the 
> existing code, etc.

One nice thing at the moment is that third-party Git commands can
install documentation and have "git help" work correctly (shameless plug
for git-integration[1] which does this).  I think Junio's suggestion
above keeps that working whereas having a hardcoded list of guides will
break this.

[1] https://github.com/johnkeeping/git-integration

[PATCH v3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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 <ralf.thielow@gmail.com>
---
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

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[John Keeping]

On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:
> 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 <ralf.thielow@gmail.com>
> ---
> 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_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),

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

2016-08-16 18:33 GMT+02:00 John Keeping <john@keeping.me.uk>:
> On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:
>>  static struct option builtin_help_options[] = {
>> +     OPT_BOOL('s', "swapped", &swapped, "mark as being called by <cmd> --help"),
>
> OPT_HIDDEN_BOOL maybe?
>

Yeah >_<

Thanks!

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

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

This is not the first offender (show_guides above does so, too), but
please do not initialize static explicitly to 0 or NULL.

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

Style: "static const char *check_git_cmd(const char *cmd)".  The
asterisk that turns the base type to a pointer to the base type
sticks to the identifier, not to the type.

> +{
> +	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);

I am guilty of suggesting "swapped"; even if we are going to mark
this as OPT_HIDDEN, I think we should be able to think of a better
name.  I think the meaning of this boolean is "we know that this is
not a guide and is meant to be a command.", and I hope we can come
up with a name that concisely expresses that (e.g. "--not-a-guide",
"--must-be-a-command").

> +	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)

It is more idiomatic to say

			if (!i)

around here.

> +				argv_array_push(&args, "--swapped");

> +		}
> +
> +		argc++;
> +		argv = argv_array_detach(&args);
>  	}
>  
>  	builtin = get_builtin(cmd);

The code does this after it:

	if (builtin)
        	exit(run_builtin(...));

and returns.  If we didn't get builtin, we risk leaking args.argv
here, but we assume argv[0] = cmd = "help" is always a builtin,
which I think is a safe assumption, so the code is OK.  Static
checkers that are only half intelligent may yell at you for not
releasing the resources, though.

> 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

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

2016-08-16 19:27 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Ralf Thielow <ralf.thielow@gmail.com> writes:
>> +
>> +     if (swapped)
>> +             return help_unknown_cmd(cmd);
>
> I am guilty of suggesting "swapped"; even if we are going to mark
> this as OPT_HIDDEN, I think we should be able to think of a better
> name.  I think the meaning of this boolean is "we know that this is
> not a guide and is meant to be a command.", and I hope we can come
> up with a name that concisely expresses that (e.g. "--not-a-guide",
> "--must-be-a-command").
>

I think "--cmd-only" is a good name.  With a good name I think it's worth
to make this option visible to the user.

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> 2016-08-16 19:27 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>> Ralf Thielow <ralf.thielow@gmail.com> writes:
>>> +
>>> +     if (swapped)
>>> +             return help_unknown_cmd(cmd);
>>
>> I am guilty of suggesting "swapped"; even if we are going to mark
>> this as OPT_HIDDEN, I think we should be able to think of a better
>> name.  I think the meaning of this boolean is "we know that this is
>> not a guide and is meant to be a command.", and I hope we can come
>> up with a name that concisely expresses that (e.g. "--not-a-guide",
>> "--must-be-a-command").
>>
>
> I think "--cmd-only" is a good name.  With a good name I think it's worth
> to make this option visible to the user.

Sure.  I'd prefer "cmd" to be spelled out in anything that is
end-user facing, though.

[PATCH 0/2] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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

[PATCH 1/2] help: introduce option --command-only

[Ralf Thielow]

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 <ralf.thielow@gmail.com>
---
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

[PATCH 2/2] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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 <ralf.thielow@gmail.com>
---
 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

Re: [PATCH 2/2] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

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

What the patch does is correct, I think, but the explanation may
invite a false alarm.  If you added a custom command git-who in your
$PATH, with an appropriate documentation for git-who(1), we would
still show its documentation, no?

The same comment applies to 1/2, too, in that the word "command"
will be interpreted differently by different people.  For example,
"git co --help" and "git help co" would work, with or without 1/2 in
place when you have "[alias] co = checkout", so we are calling "Git
subcommands that we ship, custom commands 'git-$foo' the users have
in their $PATH, and aliases the users create" collectively "command".

As long as the reader understands that definition, both the log
messages of 1/2 and 2/2 _and_ the updated description for "git help"
we have in 1/2 are all very clear.  I do not care too much about the
commit log message, but we may want to think about the documentation
a bit more.

Here is what 1/2 adds to "git help" documentation:

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

First, I do not think a short form is unnecessary; the users are not
expected to use that form, once they started typing "git help...".
If we flip the polarity and call it --exclude-guides or something,
would it make it less ambiguous?

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

I wouldn't call that a breakage; "git everyday --help" shouldn't
have worked in the first place.  It did something useful merely by
accident ;-).

> 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);

The code does this after it:

    if (builtin)
                exit(run_builtin(...));

and returns.  If we didn't get builtin, we risk leaking args.argv
here, but we assume argv[0] = cmd = "help" is always a builtin,
which I think is a safe assumption, so the code is OK.  Static
checkers that are only half intelligent may yell at you for not
releasing the resources, though.  I wonder if it is worth doing

    static void handle_builtin(int argc, const char **argv)
    {
            struct argv_array args = ARGV_ARRAY_INIT;
            ...
            if (argc > 1 && !strcmp(argv[1], "--help")) {
                    ...
                    argv = args.argv;
            }
            builtin = get_builtin(cmd);
            if (builtin)
                    exit(run_builtin(...));
            argv_array_clear(&args);
    }   

to help unconfuse them.

By the way, I do not see these patches on gmane, public-inbox or
usual suspects.  Perhaps vger is having a bad day or something?

Re: [PATCH 2/2] help: make option --help open man pages only for Git commands

[Ralf Thielow]

Sorry for being late in responding. It's been busy days.

2016-08-18 21:51 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Ralf Thielow <ralf.thielow@gmail.com> writes:
>
> The same comment applies to 1/2, too, in that the word "command"
> will be interpreted differently by different people.  For example,
> "git co --help" and "git help co" would work, with or without 1/2 in
> place when you have "[alias] co = checkout", so we are calling "Git
> subcommands that we ship, custom commands 'git-$foo' the users have
> in their $PATH, and aliases the users create" collectively "command".
>
> As long as the reader understands that definition, both the log
> messages of 1/2 and 2/2 _and_ the updated description for "git help"
> we have in 1/2 are all very clear.  I do not care too much about the
> commit log message, but we may want to think about the documentation
> a bit more.
>
> Here is what 1/2 adds to "git help" documentation:
>
>     +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.
>     +
>
> First, I do not think a short form is unnecessary; the users are not
> expected to use that form, once they started typing "git help...".
> If we flip the polarity and call it --exclude-guides or something,
> would it make it less ambiguous?
>

Sure.  Since "command" is understood as both Git command
and guide in this context, the name --exclude-guides would describe the
behaviour of that option less ambiguous.  I'll rename it.

>> This breaks "git <concept> --help" while "git help <concept>" still works.
>
> I wouldn't call that a breakage; "git everyday --help" shouldn't
> have worked in the first place.  It did something useful merely by
> accident ;-).
>

OK, I'll call it "doesn't work anymore".

>> 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);
>
> The code does this after it:
>
>     if (builtin)
>                 exit(run_builtin(...));
>
> and returns.  If we didn't get builtin, we risk leaking args.argv
> here, but we assume argv[0] = cmd = "help" is always a builtin,
> which I think is a safe assumption, so the code is OK.  Static
> checkers that are only half intelligent may yell at you for not
> releasing the resources, though.  I wonder if it is worth doing
>
>     static void handle_builtin(int argc, const char **argv)
>     {
>             struct argv_array args = ARGV_ARRAY_INIT;
>             ...
>             if (argc > 1 && !strcmp(argv[1], "--help")) {
>                     ...
>                     argv = args.argv;
>             }
>             builtin = get_builtin(cmd);
>             if (builtin)
>                     exit(run_builtin(...));
>             argv_array_clear(&args);
>     }
>
> to help unconfuse them.
>

I'll do it this way.

Thanks!

Re: [PATCH 1/2] help: introduce option --command-only

[Philip Oakley]

From: "Ralf Thielow" <ralf.thielow@gmail.com>
> 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 <ralf.thielow@gmail.com>
> ---
> 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.

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

s/commands/known 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")),

s/commands/known 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
>
>
--
Philip 

Re: [PATCH 1/2] help: introduce option --command-only

[Johannes Schindelin]

Hi Ralf,

On Thu, 18 Aug 2016, Ralf Thielow wrote:

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

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

Re: [PATCH 1/2] help: introduce option --command-only

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

A very constructive suggestion to show a good direction.  Because we
are not in the business of verifying the "man" works as expected and
how it wants the files in MANPATH are structured, it is a very good
idea to do our testing with the HTML format, which gives us tighter
control of what we actually use for our testing with help.browser
configuration variable.  I really like it.

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

Yup!

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

Yup, yup!  The "browser" can be something that parrots its command
line to the standard output and does not even have to care if the
file pointed at is HTML at all.

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

Good.

Re: [PATCH 1/2] help: introduce option --command-only

[Ralf Thielow]

2016-08-19 10:32 GMT+02:00 Johannes Schindelin <Johannes.Schindelin@gmx.de>:

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

Re: [PATCH 1/2] help: introduce option --command-only

[Johannes Schindelin]

Hi Ralf,

On Tue, 23 Aug 2016, Ralf Thielow wrote:

> 2016-08-19 10:32 GMT+02:00 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> 
> > 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

Re: [PATCH 1/2] help: introduce option --command-only

[Remi Galan Alfonso]

Hi Ralf,

Ralf Thielow <ralf.thielow@gmail.com> 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

Re: [PATCH 1/2] help: introduce option --command-only

[Ralf Thielow]

2016-08-19 10:39 GMT+02:00 Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr>:
> Hi Ralf,
>
> Ralf Thielow <ralf.thielow@gmail.com> 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!

> Thanks,
> Rémi

[PATCH v2 0/3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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

[PATCH v2 1/3] Revert "display HTML in default browser using Windows' shell API"

[Ralf Thielow]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
 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

[PATCH v2 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

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 <johannes.schindelin@gmx.de>
Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
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

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

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

Let's hide this option from command help of "git help" itself, drop
the short-and-sweet "-e", not command-line complete it, and leave it
not-mentioned here.

It is a different story if this option would really help the end
users, but I do not think that is the case.  If this were to face
the end users properly, we would need to worry about making sure
that "git help -g -e" would error out, and getting all the other
possible corner cases right.  I do not think the amount of effort
required to do so (even the "trying to enumerate what other possible
corner cases there may be" part) is worth it.

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

I think this is OK; with s|As we pass a URL|As we pass a string with
:// in it|, the first sentence can be a in-code comment in the test
that does this and will help readers of the code in the future.

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

Are you talking about the hardcoded list in valid_tool() helper
function in git-web--browse.sh?  If we use the established escape
hatch implemented by valid_custom_tool() helper there by setting
browser.*.cmd, would that be sufficient to work around the "Git
checks if the browser is known"?

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

So, let's not do 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")),

So I'd suggest using PARSE_OPT_HIDDEN for this one and drop 'e' shorthand.
The only caller of this mode does not use it.

> 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

So, let's not do this.

> 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 
> +}

Would replacing the last line with:

	test_config browser.test.cmd ./test-browser &&
	test_config help.browser test

and then writing to test-browser work just as well?  If so, that
would be much cleaner and more preferrable.

> +
> +test_expect_success "setup" "
> +	write_script firefox <<-\EOF
> +	exit 0
> +	EOF
> +"

Unless there is a good reason you MUST do so, avoid quoting the test
body with double quotes, as it invites mistakes [*1*].

Also, how about using something like:

	write_script test-browser <<-\EOF
	i=0
	for arg
        do
		i=$(( $i + 1 ))
		echo "$i: $arg"
	done >test-browser.log
        EOF

instead?  That way, you can ensure that "git help status" attempts
to call git-status.html with the expected path, not gitstatus.html
or status.html, or somesuch, immediately after running "git help
status" in the next test by inspecting test-browser.log ...

> +test_expect_success "works for commands and guides by default" "
> +	configure_help &&
> +	git help status &&

... right here.

The output from the test-browser does not have to be multi-line;
just doing

	echo "$*"

might be sufficient.

> +	git help revisions
> +"

Thanks.

[Footnote]

*1* Can you immediately tell why this test is broken?

test_expect_success "two commits do not have the same ID" "
	git commit --allow-empty -m first &&
	one=$(git rev-parse --verify HEAD) &&
	test_tick &&
	git commit --allow-empty -m second &&
	two=$(git rev-parse --verify HEAD) &&
	test $one != $two
"

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Let's hide this option from command help of "git help" itself, drop
> the short-and-sweet "-e", not command-line complete it, and leave it
> not-mentioned here.
> ...
> Unless there is a good reason you MUST do so, avoid quoting the test
> body with double quotes, as it invites mistakes [*1*].
>
> Also, how about using something like:
> ...
> instead?  That way, you can ensure that "git help status" attempts
> to call git-status.html with the expected path, not gitstatus.html
> or status.html, or somesuch, immediately after running "git help
> status" in the next test by inspecting test-browser.log ...

Taking all of these together, I'll queue this as a proposed fix-up
directly on top of yours.

 Documentation/git-help.txt             |  6 +-----
 builtin/help.c                         |  2 +-
 contrib/completion/git-completion.bash |  2 +-
 t/t0012-help.sh                        | 33 ++++++++++++++++++---------------
 4 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index eeb1950..40d328a 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] [-e|--exclude-guides] [-g|--guide]
+'git help' [-a|--all] [-g|--guide]
 	   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION
@@ -43,10 +43,6 @@ 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 40901a9..49f7a07 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -40,7 +40,7 @@ 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_HIDDEN_BOOL(0, "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"),
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 63cccb9..bd25b0a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1340,7 +1340,7 @@ _git_help ()
 {
 	case "$cur" in
 	--*)
-		__gitcomp "--all --exclude-guides --guides --info --man --web"
+		__gitcomp "--all --guides --info --man --web"
 		return
 		;;
 	esac
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index f91088b..9d99812 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -7,27 +7,30 @@ test_description='help'
 configure_help () {
 	test_config help.format html &&
 	test_config help.htmlpath test://html &&
-	test_config help.browser firefox
+	test_config browser.test.cmd ./test-browser &&
+	test_config help.browser test
 }
 
-test_expect_success "setup" "
-	write_script firefox <<-\EOF
-	exit 0
+test_expect_success "setup" '
+	write_script test-browser <<-\EOF
+	echo "$*" >test-browser.log
 	EOF
-"
+'
 
-test_expect_success "works for commands and guides by default" "
+test_expect_success "works for commands and guides by default" '
 	configure_help &&
 	git help status &&
-	git help revisions
-"
+	echo "test://html/git-status.html" >expect &&
+	test_cmp expect test-browser.log &&
+	git help revisions &&
+	echo "test://html/gitrevisions.html" >expect &&
+	test_cmp expect test-browser.log
+'
 
-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_expect_success "--exclude-guides does not work for guides" '
+	>test-browser.log &&
+	test_must_fail git help --exclude-guides revisions &&
+	test_must_be_empty test-browser.log
+'
 
 test_done
-- 
2.10.0-rc1-260-gbdd1a2a

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

2016-08-26 21:42 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Junio C Hamano <gitster@pobox.com> writes:
>
>
> Taking all of these together, I'll queue this as a proposed fix-up
> directly on top of yours.
>

Thanks!

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> 2016-08-26 21:42 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>
>> Taking all of these together, I'll queue this as a proposed fix-up
>> directly on top of yours.
>>
>
> Thanks!

Thank you for starting this topic.  I forgot to add comment on that
test://html part, though, so I'd have to redo it further, but
perhaps not today.

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

2016-08-26 21:06 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Ralf Thielow <ralf.thielow@gmail.com> writes:
>
>> Introduce option --exclude-guides to the help command.  With this option
>> being passed, "git help" will open man pages only for actual commands.
>
> Let's hide this option from command help of "git help" itself, drop
> the short-and-sweet "-e", not command-line complete it, and leave it
> not-mentioned here.
>
> It is a different story if this option would really help the end
> users, but I do not think that is the case.  If this were to face
> the end users properly, we would need to worry about making sure
> that "git help -g -e" would error out, and getting all the other
> possible corner cases right.  I do not think the amount of effort
> required to do so (even the "trying to enumerate what other possible
> corner cases there may be" part) is worth it.
>

I'm fine with that as the reason for me was just a "why not?", and
you just gave the reason to not do this. Thanks

>> 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.
>
> I think this is OK; with s|As we pass a URL|As we pass a string with
> :// in it|, the first sentence can be a in-code comment in the test
> that does this and will help readers of the code in the future.
>

Hmm. The "://" is really a URL thing. That's why it's in the code, no?
The code may have some room for improvements in checking for
URLs.

>>> +       test_config help.browser firefox
>>
>> Git checks if the browser is known, so the "test-browser" needs to
>> pretend it is one of them.
>
> Are you talking about the hardcoded list in valid_tool() helper
> function in git-web--browse.sh?  If we use the established escape
> hatch implemented by valid_custom_tool() helper there by setting
> browser.*.cmd, would that be sufficient to work around the "Git
> checks if the browser is known"?
>
>> 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]
>
> So, let's not do 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")),
>
> So I'd suggest using PARSE_OPT_HIDDEN for this one and drop 'e' shorthand.
> The only caller of this mode does not use it.
>
>> 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
>
> So, let's not do this.
>
>> 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
>> +}
>
> Would replacing the last line with:
>
>         test_config browser.test.cmd ./test-browser &&
>         test_config help.browser test
>
> and then writing to test-browser work just as well?  If so, that
> would be much cleaner and more preferrable.
>

I wasn't aware that this is a way to configure things. Thanks.

>> +
>> +test_expect_success "setup" "
>> +     write_script firefox <<-\EOF
>> +     exit 0
>> +     EOF
>> +"
>
> Unless there is a good reason you MUST do so, avoid quoting the test
> body with double quotes, as it invites mistakes [*1*].
>

The test-browser was supposed to be returning just a success
which is good enough for my usage.

> Also, how about using something like:
>
>         write_script test-browser <<-\EOF
>         i=0
>         for arg
>         do
>                 i=$(( $i + 1 ))
>                 echo "$i: $arg"
>         done >test-browser.log
>         EOF
>
> instead?  That way, you can ensure that "git help status" attempts
> to call git-status.html with the expected path, not gitstatus.html
> or status.html, or somesuch, immediately after running "git help
> status" in the next test by inspecting test-browser.log ...
>

We can use this to check whether the correct file was tried to open.
Not a part of this "does it work" test, but good for new ones.

>> +test_expect_success "works for commands and guides by default" "
>> +     configure_help &&
>> +     git help status &&
>
> ... right here.
>
> The output from the test-browser does not have to be multi-line;
> just doing
>
>         echo "$*"
>
> might be sufficient.
>
>> +     git help revisions
>> +"
>
> Thanks.
>
> [Footnote]
>
> *1* Can you immediately tell why this test is broken?
>
> test_expect_success "two commits do not have the same ID" "
>         git commit --allow-empty -m first &&
>         one=$(git rev-parse --verify HEAD) &&
>         test_tick &&
>         git commit --allow-empty -m second &&
>         two=$(git rev-parse --verify HEAD) &&
>         test $one != $two
> "
>

I'm afraid I can't.

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

>>> 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.
>>
>> I think this is OK; with s|As we pass a URL|As we pass a string with
>> :// in it|, the first sentence can be a in-code comment in the test
>> that does this and will help readers of the code in the future.
>
> Hmm. The "://" is really a URL thing.

Perhaps you thought so, but no, "mailto:ralf.thielow@gmail.com" is a
perfectly valid URL.

Because you are explaining why test://html was chosen, and the real
reason is any path that is !strstr(path, "://") is subject to an
additional "This must be a local path" check and you wanted to avoid
it, "As we pass a URL" is unnecessarily vague (and incorrect--we
cannot use a mailto: URL to sidestep the check).

>> *1* Can you immediately tell why this test is broken?
>>
>> test_expect_success "two commits do not have the same ID" "
>>         git commit --allow-empty -m first &&
>>         one=$(git rev-parse --verify HEAD) &&
>>         test_tick &&
>>         git commit --allow-empty -m second &&
>>         two=$(git rev-parse --verify HEAD) &&
>>         test $one != $two
>> "
>>
>
> I'm afraid I can't.

The reason becomes clear if you put your feet into shell's shues.
Before being ablt to call test_expect_success, you would need to
figure out what strings you give as its parameters.  $1 is clear in
this case, a simple string "two commits do not have the same ID"
(without double quotes).

But what goes in $2?  Especially the part around "one=..."?

Because the whole thing is inside a double-quote pair, $() and $name
are all interpolated even before test_expect_success is called.
So the above becomes equivalent to

>> test_expect_success "two commits do not have the same ID" '
>>         git commit --allow-empty -m first &&
>>         one=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>         test_tick &&
>>         git commit --allow-empty -m second &&
>>         two=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>         test !=
>> '

(using whatever commit HEAD was pointing at before this test starts
to run), which obviously is not what we expected to see.

Re: [PATCH v2 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

2016-08-26 22:20 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>
> Because the whole thing is inside a double-quote pair, $() and $name
> are all interpolated even before test_expect_success is called.
> So the above becomes equivalent to
>
>>> test_expect_success "two commits do not have the same ID" '
>>>         git commit --allow-empty -m first &&
>>>         one=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>>         test_tick &&
>>>         git commit --allow-empty -m second &&
>>>         two=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>>         test !=
>>> '
>

I got it, thanks.  My understanding in when a part is being interpreted
was obviously very wrong.  Thanks again!

[PATCH v2 3/3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

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 <ralf.thielow@gmail.com>
---
 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