threads / patch / 12304

patch, 3 partshelp: use parseopt

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

## tl;dr

12 messages between Feb 24, 2008 and Feb 26, 2008. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Jeff King· Feb 24, 2008, 22:16 UTC · lore

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(-)
Show changes to help.c +59 −64
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
Christian Couder· Feb 25, 2008, 06:50 UTC · re: Jeff King · lore

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

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.

Jeff King· Feb 25, 2008, 06:57 UTC · re: Christian Couder · lore

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

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
Junio C Hamano· Feb 25, 2008, 07:16 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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.
Jeff King· Feb 24, 2008, 22:17 UTC · lore

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

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
Show changes to 4 files +29 −15

Makefile, alias.c, cache.h, git.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
Jeff King· Feb 24, 2008, 22:17 UTC · lore

[PATCH 3/3] help: respect aliases

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(-)
Show changes to help.c +32 −1
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
Jay Soffian· Feb 25, 2008, 02:10 UTC · re: Jeff King · lore

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

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.
Johannes Schindelin· Feb 26, 2008, 12:43 UTC · re: Jay Soffian · lore

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

Hi,
On Sun, 24 Feb 2008, Jay Soffian wrote:
Show 9 quoted lines
> 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

Jay Soffian· Feb 26, 2008, 14:56 UTC · re: Johannes Schindelin · lore

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

On Tue, Feb 26, 2008 at 7:43 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 16 quoted lines
>
>  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.
Johannes Schindelin· Feb 26, 2008, 15:15 UTC · re: Jay Soffian · lore

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

Hi,
On Tue, 26 Feb 2008, Jay Soffian wrote:
Show 20 quoted lines
> 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

Junio C Hamano· Feb 26, 2008, 20:14 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 23 quoted lines
> 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
Jay Soffian· Feb 26, 2008, 20:31 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 26, 2008 at 3:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
>
>  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.

← back to recent threads