[PATCH 1/3] help: use parseopt

[PATCH 1/3] help: use parseopt

[Jeff King]

This patch converts cmd_help to use parseopt, along with a
few style cleanups, including:

  - enum constants are now ALL_CAPS

  - parse_help_format returns an enum value rather than
    setting a global as a side effect

Signed-off-by: Jeff King <peff@peff.net>
---
Sorry, the diff turned out quite messy because of the cleanups. It's
probably easier to just read the result.

 help.c |  123 ++++++++++++++++++++++++++++++---------------------------------
 1 files changed, 59 insertions(+), 64 deletions(-)

diff --git a/help.c b/help.c
index 8143dcd..5feb849 100644
--- a/help.c
+++ b/help.c
@@ -7,40 +7,49 @@
 #include "builtin.h"
 #include "exec_cmd.h"
 #include "common-cmds.h"
-
-static const char *help_default_format;
-
-static enum help_format {
-	man_format,
-	info_format,
-	web_format,
-} help_format = man_format;
-
-static void parse_help_format(const char *format)
+#include "parse-options.h"
+
+enum help_format {
+	HELP_FORMAT_MAN,
+	HELP_FORMAT_INFO,
+	HELP_FORMAT_WEB,
+};
+
+static int show_all = 0;
+static enum help_format help_format = HELP_FORMAT_MAN;
+static struct option builtin_help_options[] = {
+	OPT_BOOLEAN('a', "all", &show_all, "print all available commands"),
+	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),
+	OPT_SET_INT('i', "info", &help_format, "show info page",
+			HELP_FORMAT_INFO),
+};
+
+static const char * const builtin_help_usage[] = {
+	"git-help [--all] [--man|--web|--info] [command]",
+	NULL
+};
+
+static enum help_format parse_help_format(const char *format)
 {
-	if (!format) {
-		help_format = man_format;
-		return;
-	}
-	if (!strcmp(format, "man")) {
-		help_format = man_format;
-		return;
-	}
-	if (!strcmp(format, "info")) {
-		help_format = info_format;
-		return;
-	}
-	if (!strcmp(format, "web") || !strcmp(format, "html")) {
-		help_format = web_format;
-		return;
-	}
+	if (!strcmp(format, "man"))
+		return HELP_FORMAT_MAN;
+	if (!strcmp(format, "info"))
+		return HELP_FORMAT_INFO;
+	if (!strcmp(format, "web") || !strcmp(format, "html"))
+		return HELP_FORMAT_WEB;
 	die("unrecognized help format '%s'", format);
 }
 
 static int git_help_config(const char *var, const char *value)
 {
-	if (!strcmp(var, "help.format"))
-		return git_config_string(&help_default_format, var, value);
+	if (!strcmp(var, "help.format")) {
+		if (!value)
+			return config_error_nonbool(var);
+		help_format = parse_help_format(value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -362,50 +371,36 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
-	const char *help_cmd = argv[1];
+	int nongit;
 
-	if (argc < 2) {
-		printf("usage: %s\n\n", git_usage_string);
-		list_common_cmds_help();
-		exit(0);
-	}
+	setup_git_directory_gently(&nongit);
+	git_config(git_help_config);
 
-	if (!strcmp(help_cmd, "--all") || !strcmp(help_cmd, "-a")) {
+	argc = parse_options(argc, argv, builtin_help_options,
+			builtin_help_usage, 0);
+
+	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
 		list_commands();
+		return 0;
 	}
 
-	else if (!strcmp(help_cmd, "--web") || !strcmp(help_cmd, "-w")) {
-		show_html_page(argc > 2 ? argv[2] : NULL);
-	}
-
-	else if (!strcmp(help_cmd, "--info") || !strcmp(help_cmd, "-i")) {
-		show_info_page(argc > 2 ? argv[2] : NULL);
-	}
-
-	else if (!strcmp(help_cmd, "--man") || !strcmp(help_cmd, "-m")) {
-		show_man_page(argc > 2 ? argv[2] : NULL);
+	if (!argv[0]) {
+		printf("usage: %s\n\n", git_usage_string);
+		list_common_cmds_help();
+		return 0;
 	}
 
-	else {
-		int nongit;
-
-		setup_git_directory_gently(&nongit);
-		git_config(git_help_config);
-		if (help_default_format)
-			parse_help_format(help_default_format);
-
-		switch (help_format) {
-		case man_format:
-			show_man_page(help_cmd);
-			break;
-		case info_format:
-			show_info_page(help_cmd);
-			break;
-		case web_format:
-			show_html_page(help_cmd);
-			break;
-		}
+	switch (help_format) {
+	case HELP_FORMAT_MAN:
+		show_man_page(argv[0]);
+		break;
+	case HELP_FORMAT_INFO:
+		show_info_page(argv[0]);
+		break;
+	case HELP_FORMAT_WEB:
+		show_html_page(argv[0]);
+		break;
 	}
 
 	return 0;
-- 
1.5.4.3.305.g073a4

Re: [PATCH 1/3] help: use parseopt

[Christian Couder]

Le dimanche 24 février 2008, Jeff King a écrit :
> This patch converts cmd_help to use parseopt, along with a
> few style cleanups, including:
>
>   - enum constants are now ALL_CAPS

Ok.

>   - parse_help_format returns an enum value rather than
>     setting a global as a side effect

Nice cleanup.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Sorry, the diff turned out quite messy because of the cleanups. It's
> probably easier to just read the result.

Or could you send the cleanups in some separated patches ?

Thanks in advance,
Christian.

Re: [PATCH 1/3] help: use parseopt

[Jeff King]

On Mon, Feb 25, 2008 at 07:50:52AM +0100, Christian Couder wrote:

> > Sorry, the diff turned out quite messy because of the cleanups. It's
> > probably easier to just read the result.
> 
> Or could you send the cleanups in some separated patches ?

Looking at it again, it's actually not too bad except for the cmd_help()
function itself, which isn't due to the cleanups at all, but because the
flow changes quite a bit. So I don't think splitting it actually will
make the diff any friendlier.

-Peff

Re: [PATCH 1/3] help: use parseopt

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 25, 2008 at 07:50:52AM +0100, Christian Couder wrote:
>
>> > Sorry, the diff turned out quite messy because of the cleanups. It's
>> > probably easier to just read the result.
>> 
>> Or could you send the cleanups in some separated patches ?
>
> Looking at it again, it's actually not too bad except for the cmd_help()
> function itself, which isn't due to the cleanups at all, but because the
> flow changes quite a bit. So I don't think splitting it actually will
> make the diff any friendlier.

Your patch is fine as-is.

[PATCH 2/3] make alias lookup a public, procedural function

[Jeff King]

This converts git_config_alias to the public alias_lookup
function. Because of the nature of our config parser, we
still have to rely on setting static data. However, that
interface is wrapped so that you can just say

  value = alias_lookup(key);

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile |    3 ++-
 alias.c  |   22 ++++++++++++++++++++++
 cache.h  |    2 ++
 git.c    |   17 +++--------------
 4 files changed, 29 insertions(+), 15 deletions(-)
 create mode 100644 alias.c

diff --git a/Makefile b/Makefile
index d33a556..b460bb2 100644
--- a/Makefile
+++ b/Makefile
@@ -327,7 +327,8 @@ LIB_OBJS = \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
-	transport.o bundle.o walker.o parse-options.o ws.o archive.o
+	transport.o bundle.o walker.o parse-options.o ws.o archive.o \
+	alias.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/alias.c b/alias.c
new file mode 100644
index 0000000..116cac8
--- /dev/null
+++ b/alias.c
@@ -0,0 +1,22 @@
+#include "cache.h"
+
+static const char *alias_key;
+static char *alias_val;
+static int alias_lookup_cb(const char *k, const char *v)
+{
+	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key)) {
+		if (!v)
+			return config_error_nonbool(k);
+		alias_val = xstrdup(v);
+		return 0;
+	}
+	return 0;
+}
+
+char *alias_lookup(const char *alias)
+{
+	alias_key = alias;
+	alias_val = NULL;
+	git_config(alias_lookup_cb);
+	return alias_val;
+}
diff --git a/cache.h b/cache.h
index fa5a9e5..1e9c937 100644
--- a/cache.h
+++ b/cache.h
@@ -765,4 +765,6 @@ int pathspec_match(const char **spec, char *matched, const char *filename, int s
 int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
+char *alias_lookup(const char *alias);
+
 #endif /* CACHE_H */
diff --git a/git.c b/git.c
index 0cb8688..8f08b12 100644
--- a/git.c
+++ b/git.c
@@ -87,19 +87,6 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 	return handled;
 }
 
-static const char *alias_command;
-static char *alias_string;
-
-static int git_alias_config(const char *var, const char *value)
-{
-	if (!prefixcmp(var, "alias.") && !strcmp(var + 6, alias_command)) {
-		if (!value)
-			return config_error_nonbool(var);
-		alias_string = xstrdup(value);
-	}
-	return 0;
-}
-
 static int split_cmdline(char *cmdline, const char ***argv)
 {
 	int src, dst, count = 0, size = 16;
@@ -159,11 +146,13 @@ static int handle_alias(int *argcp, const char ***argv)
 	const char *subdir;
 	int count, option_count;
 	const char** new_argv;
+	const char *alias_command;
+	char *alias_string;
 
 	subdir = setup_git_directory_gently(&nongit);
 
 	alias_command = (*argv)[0];
-	git_config(git_alias_config);
+	alias_string = alias_lookup(alias_command);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			if (*argcp > 1) {
-- 
1.5.4.3.305.g073a4

[PATCH 3/3] help: respect aliases

[Jeff King]

If we have an alias "foo" defined, then the help text for
"foo" (via "git help foo" or "git foo --help") now shows the
definition of the alias.

Before showing an alias definition, we make sure that there
is no git command which would override the alias (so that
even though you may have a "log" alias, even though it will
not work, we don't want to it supersede "git help log").

Signed-off-by: Jeff King <peff@peff.net>
---
 help.c |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/help.c b/help.c
index 5feb849..e57a50e 100644
--- a/help.c
+++ b/help.c
@@ -210,7 +210,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static void list_commands(void)
+static unsigned int load_command_list(void)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -250,6 +250,14 @@ static void list_commands(void)
 	uniq(&other_cmds);
 	exclude_cmds(&other_cmds, &main_cmds);
 
+	return longest;
+}
+
+static void list_commands(void)
+{
+	unsigned int longest = load_command_list();
+	const char *exec_path = git_exec_path();
+
 	if (main_cmds.cnt) {
 		printf("available git commands in '%s'\n", exec_path);
 		printf("----------------------------");
@@ -284,6 +292,22 @@ void list_common_cmds_help(void)
 	}
 }
 
+static int is_in_cmdlist(struct cmdnames *c, const char *s)
+{
+	int i;
+	for (i = 0; i < c->cnt; i++)
+		if (!strcmp(s, c->names[i]->name))
+			return 1;
+	return 0;
+}
+
+static int is_git_command(const char *s)
+{
+	load_command_list();
+	return is_in_cmdlist(&main_cmds, s) ||
+		is_in_cmdlist(&other_cmds, s);
+}
+
 static const char *cmd_to_page(const char *git_cmd)
 {
 	if (!git_cmd)
@@ -372,6 +396,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
+	const char *alias;
 
 	setup_git_directory_gently(&nongit);
 	git_config(git_help_config);
@@ -391,6 +416,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
+	alias = alias_lookup(argv[0]);
+	if (alias && !is_git_command(argv[0])) {
+		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
+		return 0;
+	}
+
 	switch (help_format) {
 	case HELP_FORMAT_MAN:
 		show_man_page(argv[0]);
-- 
1.5.4.3.305.g073a4

Re: [PATCH 3/3] help: respect aliases

[Jay Soffian]

On Sun, Feb 24, 2008 at 5:17 PM, Jeff King <peff@peff.net> wrote:
> If we have an alias "foo" defined, then the help text for
>  "foo" (via "git help foo" or "git foo --help") now shows the
>  definition of the alias.

Heh, now I can kill this alias of mine:

    h          = "!sh -c 'git alias \"$1\" || git help \"$1\"' -"

:-)

This too would be less ugly as a built-in:

    alias      = "! sh -c 'if test -z \"$1\"; then git config --list \
                  | expand \
                  | sed \"/^alias\\./!d; s/^alias\\.//; s/=/ /; s/  */ /g\" \
                  | sort | while read n v; do \
                  printf \"%-16s\" \"$n\"; echo \"$v\" | gnused \
                  \"s/\\(.\\{68\\}\\) /\\1\\n\t\t/g\" | expand; \
                  done; else git config \"alias.$1\"; fi' -"
:-)

j.

Re: [PATCH 3/3] help: respect aliases

[Johannes Schindelin]

Hi,

On Sun, 24 Feb 2008, Jay Soffian wrote:

> This too would be less ugly as a built-in:
> 
>     alias      = "! sh -c 'if test -z \"$1\"; then git config --list \
>                   | expand \
>                   | sed \"/^alias\\./!d; s/^alias\\.//; s/=/ /; s/  */ /g\" \
>                   | sort | while read n v; do \
>                   printf \"%-16s\" \"$n\"; echo \"$v\" | gnused \
>                   \"s/\\(.\\{68\\}\\) /\\1\\n\t\t/g\" | expand; \
>                   done; else git config \"alias.$1\"; fi' -"

Wow.  This would look less ugly as an alias like this, too:

alias = "!sh -c 'case $# in \
        0) git config --get-regexp \"^alias\\.\" | sed \"s/^alias\\.//\";; \
        *) git config \"alias.$0\" ;; \
        esac'"

which incidentally fixes a bug in your alias: you ignore $0 which is the 
first parameter when using the sh -c '' idiom.

Of course, you can change the sed call to your liking...

Ciao,
Dscho

Re: [PATCH 3/3] help: respect aliases

[Jay Soffian]

On Tue, Feb 26, 2008 at 7:43 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
>  On Sun, 24 Feb 2008, Jay Soffian wrote:
>
>  > This too would be less ugly as a built-in:
>  >
>  >  [elided so as not to burn anyone's eyes out again :-) -- jay]
>
>  Wow.  This would look less ugly as an alias like this, too:
>
>  alias = "!sh -c 'case $# in \
>         0) git config --get-regexp \"^alias\\.\" | sed \"s/^alias\\.//\";; \
>         *) git config \"alias.$0\" ;; \
>         esac'"
>
>  which incidentally fixes a bug in your alias: you ignore $0 which is the
>  first parameter when using the sh -c '' idiom.

Test mine. Test yours. See which works. :-)

At least on my system, I had to use:

  sh -c '...' -

And then refer to $1.

>  Of course, you can change the sed call to your liking...

Most of the ugliness in mine is that I crammed the whitespace down and
that I'm using a quite involved pipeline to reformat all my aliases so
they fit into my terminal window.

But the --get-regexp is a good tip.

j.

Re: [PATCH 3/3] help: respect aliases

[Johannes Schindelin]

Hi,

On Tue, 26 Feb 2008, Jay Soffian wrote:

> On Tue, Feb 26, 2008 at 7:43 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> >  On Sun, 24 Feb 2008, Jay Soffian wrote:
> >
> >  > This too would be less ugly as a built-in:
> >  >
> >  >  [elided so as not to burn anyone's eyes out again :-) -- jay]
> >
> >  Wow.  This would look less ugly as an alias like this, too:
> >
> >  alias = "!sh -c 'case $# in \
> >         0) git config --get-regexp \"^alias\\.\" | sed \"s/^alias\\.//\";; \
> >         *) git config \"alias.$0\" ;; \
> >         esac'"
> >
> >  which incidentally fixes a bug in your alias: you ignore $0 which is the
> >  first parameter when using the sh -c '' idiom.
> 
> Test mine. Test yours. See which works. :-)

I tested only mine.  It works.

Ciao,
Dscho

Re: [PATCH 3/3] help: respect aliases

[Junio C Hamano]

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

> Hi,
>
> On Sun, 24 Feb 2008, Jay Soffian wrote:
>
>> This too would be less ugly as a built-in:
>> 
>>     alias      = "! sh -c 'if test -z \"$1\"; then git config --list \
>>                   | expand \
>>                   | sed \"/^alias\\./!d; s/^alias\\.//; s/=/ /; s/  */ /g\" \
>>                   | sort | while read n v; do \
>>                   printf \"%-16s\" \"$n\"; echo \"$v\" | gnused \
>>                   \"s/\\(.\\{68\\}\\) /\\1\\n\t\t/g\" | expand; \
>>                   done; else git config \"alias.$1\"; fi' -"
>
> Wow.  This would look less ugly as an alias like this, too:
>
> alias = "!sh -c 'case $# in \
>         0) git config --get-regexp \"^alias\\.\" | sed \"s/^alias\\.//\";; \
>         *) git config \"alias.$0\" ;; \
>         esac'"
>
> which incidentally fixes a bug in your alias: you ignore $0 which is the 
> first parameter when using the sh -c '' idiom.

I notice that he is feeding an exra '-' for that.

> Of course, you can change the sed call to your liking...

Sure.  The combination of expand, sed, and gnused hurts my eyes
;-)

One good lesson to take home is that by ending your lines with
'|', you can tell the shell that you are not done with the
pipeline you are forming yet.  This allows you to lose all these
ugly backslashes, like this:

	xyzzy |
        frotz |
        nitfol

instead of

	xyzzy \
        | frotz \
        | nitfol

Re: [PATCH 3/3] help: respect aliases

[Jay Soffian]

On Tue, Feb 26, 2008 at 3:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>  Sure.  The combination of expand, sed, and gnused hurts my eyes
>  ;-)
>
>  One good lesson to take home is that by ending your lines with
>  '|', you can tell the shell that you are not done with the
>  pipeline you are forming yet.

The backslashes aren't there for the shell, they are there for the git
config parser. Anyway, I reworked it a bit:

alias = "! sh -c 'case $# in \
               0) git config --get-regexp \"^alias\\.\" \
                  | sort | sed \"s/^alias\\.//; s/=/ /\" \
                  | while read n v; do \
                     printf \"%-16s\" \"$n\"; \
                     echo \"$v\" | fmt -sw 63 | sed \"2,$ s/^/\t\t/\" ;\
                  done ;; \
               1) git config \"alias.$1\" | fmt -sw 78 ;; esac' -"

And yes, it's still ugly.

Always happy to have folks help me bikeshed my git config. :-)

j.