[PATCH 1/3] help: Fix help message for aliases

[PATCH 1/3] help: Fix help message for aliases

[Namhyung Kim]

The current "`git br' is aliased to `branch'" looks a bit
strange. Prepend 'git' to aliased output too so that the
end result will be looked like this:

 $ git help br
 `git br' is aliased to `git branch'

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 builtin/help.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 61ff798..f85c870 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -445,7 +445,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	alias = alias_lookup(argv[0]);
 	if (alias && !is_git_command(argv[0])) {
-		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
+		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);
 		return 0;
 	}
 
-- 
1.7.9

[PATCH 2/3] help: Add '--follow-alias' option

[Namhyung Kim]

The --follow-alias option will look up the alias definitions and
use the first word as a command. For example, if I set my aliases
as follows:

 $ git help br
 `git br' is aliased to `git branch'
 $ git help ru
 `git ru' is aliased to `git remote update'

adding --follow-alias (or -f) option will show man pages of
git-branch and git-remote, respectively.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 Documentation/git-help.txt |    7 ++++++-
 builtin/help.c             |   25 ++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 9e0b3f6..debf293 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|-i|--info|-m|--man|-w|--web] [COMMAND]
+'git help' [-a|--all|-f|--follow-alias|-i|--info|-m|--man|-w|--web] [COMMAND]
 
 DESCRIPTION
 -----------
@@ -34,6 +34,11 @@ OPTIONS
 	Prints all the available commands on the standard output. This
 	option supersedes any other option.
 
+-f::
+--follow-alias::
+	Read alias definitions and use its first word as a command name
+	(if any).
+
 -i::
 --info::
 	Display manual page for the command in the 'info' format. The
diff --git a/builtin/help.c b/builtin/help.c
index f85c870..00392ea 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -30,9 +30,12 @@ enum help_format {
 };
 
 static int show_all = 0;
+static int follow_alias = 0;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static struct option builtin_help_options[] = {
 	OPT_BOOLEAN('a', "all", &show_all, "print all available commands"),
+	OPT_BOOLEAN('f', "follow-alias", &follow_alias,
+			"follow alias to resolve command name"),
 	OPT_SET_INT('m', "man", &help_format, "show man page", HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, "show manual in web browser",
 			HELP_FORMAT_WEB),
@@ -415,6 +418,7 @@ static void show_html_page(const char *git_cmd)
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
+	const char *cmd;
 	const char *alias;
 	enum help_format parsed_help_format;
 	load_command_list("git-", &main_cmds, &other_cmds);
@@ -443,22 +447,33 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	if (parsed_help_format != HELP_FORMAT_NONE)
 		help_format = parsed_help_format;
 
+	cmd = argv[0];
 	alias = alias_lookup(argv[0]);
 	if (alias && !is_git_command(argv[0])) {
-		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);
-		return 0;
+		char *p;
+
+		if (!follow_alias) {
+			printf("`git %s' is aliased to `git %s'\n",
+			       argv[0], alias);
+			return 0;
+		}
+
+		p = strchr(alias, ' ');
+		if (p)
+			*p = '\0';
+		cmd = alias;
 	}
 
 	switch (help_format) {
 	case HELP_FORMAT_NONE:
 	case HELP_FORMAT_MAN:
-		show_man_page(argv[0]);
+		show_man_page(cmd);
 		break;
 	case HELP_FORMAT_INFO:
-		show_info_page(argv[0]);
+		show_info_page(cmd);
 		break;
 	case HELP_FORMAT_WEB:
-		show_html_page(argv[0]);
+		show_html_page(cmd);
 		break;
 	}
 
-- 
1.7.9

Re: [PATCH 2/3] help: Add '--follow-alias' option

[Junio C Hamano]

Namhyung Kim <namhyung.kim@lge.com> writes:

> The --follow-alias option will look up the alias definitions and
> use the first word as a command. For example, if I set my aliases
> as follows:
>
>  $ git help br
>  `git br' is aliased to `git branch'
>  $ git help ru
>  `git ru' is aliased to `git remote update'
>
> adding --follow-alias (or -f) option will show man pages of
> git-branch and git-remote, respectively.

What would happen when somebody has this?

	[alias]
        	br = branch --list

	$ git help --follow-alias br
	... man page for git-branch is shown ...

NAK.

> +-f::
> +--follow-alias::
> +	Read alias definitions and use its first word as a command name
> +	(if any).

Also, please do not let a potentially ill-conceived option to squat on a
short and sweet single letter option name, until it proves to be useful.

Re: [PATCH 2/3] help: Add '--follow-alias' option

[김남형]

2012-03-15 3:00 PM, Junio C Hamano wrote:
> Namhyung Kim<namhyung.kim@lge.com>  writes:
>
>> The --follow-alias option will look up the alias definitions and
>> use the first word as a command. For example, if I set my aliases
>> as follows:
>>
>>   $ git help br
>>   `git br' is aliased to `git branch'
>>   $ git help ru
>>   `git ru' is aliased to `git remote update'
>>
>> adding --follow-alias (or -f) option will show man pages of
>> git-branch and git-remote, respectively.
>
> What would happen when somebody has this?
>
> 	[alias]
>          	br = branch --list
>
> 	$ git help --follow-alias br
> 	... man page for git-branch is shown ...
>
> NAK.
>

I'm sorry I don't understand what you meant by this. What should be happened 
for this?


>> +-f::
>> +--follow-alias::
>> +	Read alias definitions and use its first word as a command name
>> +	(if any).
>
> Also, please do not let a potentially ill-conceived option to squat on a
> short and sweet single letter option name, until it proves to be useful.
>

OK, will be careful next time.

Thanks,
Namhyung

Re: [PATCH 2/3] help: Add '--follow-alias' option

[Junio C Hamano]

김남형 <namhyung.kim@lge.com> writes:

> 2012-03-15 3:00 PM, Junio C Hamano wrote:
>> Namhyung Kim<namhyung.kim@lge.com>  writes:
>>
>>> The --follow-alias option will look up the alias definitions and
>>> use the first word as a command. For example, if I set my aliases
>>> as follows:
>>>
>>>   $ git help br
>>>   `git br' is aliased to `git branch'
>>>   $ git help ru
>>>   `git ru' is aliased to `git remote update'
>>>
>>> adding --follow-alias (or -f) option will show man pages of
>>> git-branch and git-remote, respectively.
>>
>> What would happen when somebody has this?
>>
>> 	[alias]
>>          	br = branch --list
>>
>> 	$ git help --follow-alias br
>> 	... man page for git-branch is shown ...
>>
>> NAK.
>
> I'm sorry I don't understand what you meant by this. What should be
> happened for this?

You _somehow_ restict the output, or at least draw the user's attention,
to the description of --list mode in the resulting "git-branch" manual.

I do not think that is feasible.

But showing the whole manual page, without telling the user that "br" is
not aliased to a plain vanilla "branch" without any option, is not a
solution, especially if you are going to let the user set a configuration
variable to allow him to forget about this setting. Progressive revelation
would not have such a downside and I think it is more appropriate approach
for something like "help".

Re: [PATCH 2/3] help: Add '--follow-alias' option

[Namhyung Kim]

2012-03-15 3:23 PM, Junio C Hamano wrote:
> 김남형 <namhyung.kim@lge.com> writes:
>> 2012-03-15 3:00 PM, Junio C Hamano wrote:
>>> Namhyung Kim <namhyung.kim@lge.com> writes:
>>>
>>>> The --follow-alias option will look up the alias definitions and
>>>> use the first word as a command. For example, if I set my aliases
>>>> as follows:
>>>>
>>>>    $ git help br
>>>>    `git br' is aliased to `git branch'
>>>>    $ git help ru
>>>>    `git ru' is aliased to `git remote update'
>>>>
>>>> adding --follow-alias (or -f) option will show man pages of
>>>> git-branch and git-remote, respectively.
>>>
>>> What would happen when somebody has this?
>>>
>>> 	[alias]
>>>           	br = branch --list
>>>
>>> 	$ git help --follow-alias br
>>> 	... man page for git-branch is shown ...
>>>
>>> NAK.
>>
>> I'm sorry I don't understand what you meant by this. What should be
>> happened for this?
>
> You _somehow_ restict the output, or at least draw the user's attention,
> to the description of --list mode in the resulting "git-branch" manual.
>
> I do not think that is feasible.
>

Actually I was thinking about moving to an appropriate point in the document 
by checking second argument (if any) and searching it (using regexp?). But I 
coundn't be sure it'd be possible too.


> But showing the whole manual page, without telling the user that "br" is
> not aliased to a plain vanilla "branch" without any option, is not a
> solution, especially if you are going to let the user set a configuration
> variable to allow him to forget about this setting. Progressive revelation
> would not have such a downside and I think it is more appropriate approach
> for something like "help".
>

OK. That makes sense to me too. This was just a quick thought hoping that can 
be helpful for someone who uses alias as a simple abbreviation.

Thanks for taking your time and giving me detailed comments.
Namhyung

Re: [PATCH 2/3] help: Add '--follow-alias' option

[Junio C Hamano]

Namhyung Kim <namhyung.kim@lge.com> writes:

>> You _somehow_ restict the output, or at least draw the user's attention,
>> to the description of --list mode in the resulting "git-branch" manual.
>>
>> I do not think that is feasible.
>
> Actually I was thinking about moving to an appropriate point in the
> document by checking second argument (if any) and searching it (using
> regexp?). But I coundn't be sure it'd be possible too.

I don't think it is feasible.
Think "[alias] lgf = log --oneline --first-parent".

>> But showing the whole manual page, without telling the user that "br" is
>> not aliased to a plain vanilla "branch" without any option, is not a
>> solution, especially if you are going to let the user set a configuration
>> variable to allow him to forget about this setting. Progressive revelation
>> would not have such a downside and I think it is more appropriate approach
>> for something like "help".
>
> OK. That makes sense to me too. This was just a quick thought hoping
> that can be helpful for someone who uses alias as a simple
> abbreviation.

Yeah, I would have been the first to cheer-lead an effort to make things
clever, cute and even dwim if this were about a real command that is an
integral part of people's everyday life. Once people understand what it is
doing, they will even appreciate the cleverness.

But help is the entry point for the understanding. It is safer to keep it
simple and predictable.

[PATCH 3/3] help: Add 'help.follow-alias' config item

[Namhyung Kim]

The 'help.follow-alias' config option is used to determine the
default value of '--follow-alias' option. To do this, move some
codes to honor the priority of command line options.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 Documentation/git-help.txt |    8 ++++++++
 builtin/help.c             |   16 ++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index debf293..9725e9f 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -80,6 +80,14 @@ line option:
 * "info" corresponds to '-i|--info',
 * "web" or "html" correspond to '-w|--web'.
 
+help.follow-alias
+~~~~~~~~~~~~~~~~~
+
+If --[no-]follow-alias command line option is not passed, the
+'help.follow-alias' configuration variable will be checked. If it's
+set to true, alias name will be resolved to its original command and
+then will be displayed.
+
 help.browser, web.browser and browser.<tool>.path
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/help.c b/builtin/help.c
index 00392ea..bf782ee 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -260,6 +260,10 @@ static int git_help_config(const char *var, const char *value, void *cb)
 		help_format = parse_help_format(value);
 		return 0;
 	}
+	if (!strcmp(var, "help.follow-alias")) {
+		follow_alias = git_config_bool(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "man.viewer")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -420,12 +424,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	int nongit;
 	const char *cmd;
 	const char *alias;
-	enum help_format parsed_help_format;
+
 	load_command_list("git-", &main_cmds, &other_cmds);
 
+	setup_git_directory_gently(&nongit);
+	git_config(git_help_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
-	parsed_help_format = help_format;
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
@@ -441,12 +447,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	setup_git_directory_gently(&nongit);
-	git_config(git_help_config, NULL);
-
-	if (parsed_help_format != HELP_FORMAT_NONE)
-		help_format = parsed_help_format;
-
 	cmd = argv[0];
 	alias = alias_lookup(argv[0]);
 	if (alias && !is_git_command(argv[0])) {
-- 
1.7.9

Re: [PATCH 3/3] help: Add 'help.follow-alias' config item

[Junio C Hamano]

Namhyung Kim <namhyung.kim@lge.com> writes:

> The 'help.follow-alias' config option is used to determine the
> default value of '--follow-alias' option. To do this, move some
> codes to honor the priority of command line options.

How badly would this change break what 7c3baa9 (help -a: do not
unnecessarily look for a repository, 2009-09-04) tried to fix?

> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index debf293..9725e9f 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -80,6 +80,14 @@ line option:
>  * "info" corresponds to '-i|--info',
>  * "web" or "html" correspond to '-w|--web'.
>  
> +help.follow-alias

Please be consistent with existing ones.  I think we spell multi-word
variable names without hyphens.

Re: [PATCH 3/3] help: Add 'help.follow-alias' config item

[Namhyung Kim]

2012-03-15 3:06 PM, Junio C Hamano wrote:
> Namhyung Kim<namhyung.kim@lge.com>  writes:
>
>> The 'help.follow-alias' config option is used to determine the
>> default value of '--follow-alias' option. To do this, move some
>> codes to honor the priority of command line options.
>
> How badly would this change break what 7c3baa9 (help -a: do not
> unnecessarily look for a repository, 2009-09-04) tried to fix?
>

Oops, I was completely unaware of the problem, sorry :(.


>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
>> index debf293..9725e9f 100644
>> --- a/Documentation/git-help.txt
>> +++ b/Documentation/git-help.txt
>> @@ -80,6 +80,14 @@ line option:
>>   * "info" corresponds to '-i|--info',
>>   * "web" or "html" correspond to '-w|--web'.
>>
>> +help.follow-alias
>
> Please be consistent with existing ones.  I think we spell multi-word
> variable names without hyphens.
>

Will fix if you're OK with this series.

Thanks,
Namhyung

Re: [PATCH 1/3] help: Fix help message for aliases

[Jeff King]

On Thu, Mar 15, 2012 at 11:52:47AM +0900, Namhyung Kim wrote:

> The current "`git br' is aliased to `branch'" looks a bit
> strange. Prepend 'git' to aliased output too so that the
> end result will be looked like this:
> 
>  $ git help br
>  `git br' is aliased to `git branch'

I think that is a sensible improvement, but...

> diff --git a/builtin/help.c b/builtin/help.c
> index 61ff798..f85c870 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -445,7 +445,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  
>  	alias = alias_lookup(argv[0]);
>  	if (alias && !is_git_command(argv[0])) {
> -		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
> +		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);
>  		return 0;

What output does this produce for:

  $ git config alias.foo '!f() { git log $1@{u}..$1; }; f'
  $ git help foo

?

-Peff

Re: [PATCH 1/3] help: Fix help message for aliases

[김남형]

Hi,

2012-03-15 1:17 PM, Jeff King wrote:
> On Thu, Mar 15, 2012 at 11:52:47AM +0900, Namhyung Kim wrote:
>
>> The current "`git br' is aliased to `branch'" looks a bit
>> strange. Prepend 'git' to aliased output too so that the
>> end result will be looked like this:
>>
>>   $ git help br
>>   `git br' is aliased to `git branch'
>
> I think that is a sensible improvement, but...
>
>> diff --git a/builtin/help.c b/builtin/help.c
>> index 61ff798..f85c870 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -445,7 +445,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>
>>   	alias = alias_lookup(argv[0]);
>>   	if (alias && !is_git_command(argv[0])) {
>> -		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
>> +		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);
>>   		return 0;
>
> What output does this produce for:
>
>    $ git config alias.foo '!f() { git log $1@{u}..$1; }; f'
>    $ git help foo
>
> ?
>

Oh, I didn't think of such a complicated case. Hmm, how about checking whether 
the first word is a git command or not:

		printf("`git %s' is aliased to `%s%s'\n", argv[0],
			is_git_command(first_word) ? "git " : "", alias);

Thanks,
Namhyung

Re: [PATCH 1/3] help: Fix help message for aliases

[Jeff King]

On Thu, Mar 15, 2012 at 02:15:14PM +0900, 김남형 wrote:

> >What output does this produce for:
> >
> >   $ git config alias.foo '!f() { git log $1@{u}..$1; }; f'
> >   $ git help foo
> >
> >?
> >
> 
> Oh, I didn't think of such a complicated case. Hmm, how about
> checking whether the first word is a git command or not:
> 
> 		printf("`git %s' is aliased to `%s%s'\n", argv[0],
> 			is_git_command(first_word) ? "git " : "", alias);

I think the right solution is to just look for "!", which is what tells
git it is a shell command.

-Peff

Re: [PATCH 1/3] help: Fix help message for aliases

[Junio C Hamano]

Namhyung Kim <namhyung.kim@lge.com> writes:

> -		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
> +		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);

NAK.

What would the above change will do to one of my favorite alias?

$ git help who
`git who' is aliased to `!sh -c 'git log -1 --format="%an <%ae>" --author="$1"' -'

Wouldn't removing "git " from the first phrase be a better solution?

Re: [PATCH 1/3] help: Fix help message for aliases

[김남형]

Hi,

2012-03-15 2:23 PM, Junio C Hamano wrote:
> Namhyung Kim <namhyung.kim@lge.com> writes:
>
>> -		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
>> +		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);
>
> NAK.
>
> What would the above change will do to one of my favorite alias?
>
> $ git help who
> `git who' is aliased to `!sh -c 'git log -1 --format="%an<%ae>" --author="$1"' -'
>
> Wouldn't removing "git " from the first phrase be a better solution?
>

Right. But as I replied to Jeff King, it can be improved to check whether the 
aliased output is such a complicated commands or not. Now I see that we can 
examine if the first letter is '!'.

Thanks,
Namhyung

Re: [PATCH 1/3] help: Fix help message for aliases

[Junio C Hamano]

김남형 <namhyung.kim@lge.com> writes:

> 2012-03-15 2:23 PM, Junio C Hamano wrote:
>> Namhyung Kim <namhyung.kim@lge.com> writes:
>>
>>> -		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
>>> +		printf("`git %s' is aliased to `git %s'\n", argv[0], alias);
>>
>> NAK.
>>
>> What would the above change will do to one of my favorite alias?
>>
>> $ git help who
>> `git who' is aliased to `!sh -c 'git log -1 --format="%an<%ae>" --author="$1"' -'
>>
>> Wouldn't removing "git " from the first phrase be a better solution?
>
> Right. But as I replied to Jeff King, it can be improved to check
> whether the aliased output is such a complicated commands or not. Now
> I see that we can examine if the first letter is '!'.

Trimming the four bytes from the beginning would be an improvement by
itself---it makes the output shorter without losing information.

And for your "[alias] br = branch", you will see:

	`br` is aliased to `branch`

which after all is exactly what the user wrote in the configuration.

There is even a worse problem with your "I can look at '!' at the
beginning".  By tweaking the part that is the answer to what the user
asked, depending on the value the user configured, you would not be able
to tell from this output:

	`git br` is aliased to `git branch`

which one the user really has between these two, no?

	[alias] br = branch
        [alias] br = !git branch

In short, I do not think there is any merit trying to be clever and cute
when answering "git help <alias>".  The cleverness will actively hurt by
obscuring the details of the answer you are giving in response to user's
question.

Exactly the same "don't obscure by trying to be clever and cute" comment
applies to the --follow-alias patch. There is nothing wrong in the series
of revelation:

 1. The user types 'git help br',
 2. The user then realizes it is aliased to 'branch' (this could even
    be 'branch --list'), then
 3. The user asks 'git help branch' (and perhaps goes to read on --list)