Skip to contentSkip to conversation

help: use parseopt

PATCH In review Jeff King 5 participants 12 messages Jun 15, 2016 1 message not in the archive Kbuild

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jeff King

author
#1 lore raw

PATCH 1/3 help: use parseopt

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

---
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(-)
help.c +59 -64
index 8143dcd..5feb849 100644
@@ -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;
Signature · 1 line
1.5.4.3.305.g073a4
#2 lore raw
Quoted 4 lines · Jeff King

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

- enum constants are now ALL_CAPS

Ok.

Quoted 2 lines

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

Nice cleanup.

Quoted 4 lines

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

author
#3 lore raw
Quoted 4 lines · Christian Couder
Quoted 2 lines

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

#4 lore raw
Quoted 11 lines · Jeff King
Quoted 4 lines · Christian Couder
Quoted 2 lines

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.

author
#5 lore raw

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

In reply to a message not in the archive

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);
---
 Makefile |    3 ++-
 alias.c  |   22 ++++++++++++++++++++++
 cache.h  |    2 ++
 git.c    |   17 +++--------------
 4 files changed, 29 insertions(+), 15 deletions(-)
 create mode 100644 alias.c
Makefile +2 -1
index d33a556..b460bb2 100644
@@ -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 \
alias.c new file +22 -0
new file mode 100644
index 0000000..116cac8
@@ -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;
+}
cache.h +2 -0
index fa5a9e5..1e9c937 100644
@@ -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 */
git.c +3 -14
index 0cb8688..8f08b12 100644
@@ -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) {
Signature · 1 line
1.5.4.3.305.g073a4
author
#6 lore raw

PATCH 3/3 help: respect aliases

In reply to a message not in the archive

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

---
 help.c |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)
help.c +32 -1
index 5feb849..e57a50e 100644
@@ -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]);
Signature · 1 line
1.5.4.3.305.g073a4
#7 lore raw

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

Quoted 3 lines · 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.

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.

#8 lore raw

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

Hi,

Quoted 9 lines · Jay Soffian

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

#9 lore raw

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

Quoted 9 lines · Johannes Schindelin
Quoted 3 lines · Jay Soffian

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:

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

Quoted 1 line

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.

#10 lore raw

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

Hi,

Quoted 16 lines · Jay Soffian
Quoted 14 lines · Johannes Schindelin
Quoted 3 lines · Jay Soffian

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'"
Quoted 4 lines
Quoted 2 lines

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

#11 lore raw

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

In reply to Johannes Schindelin
Quoted 16 lines · Johannes Schindelin

Hi,

Quoted 9 lines · Jay Soffian

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:

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

Quoted 1 line

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
#12 lore raw

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

Quoted 7 lines · Junio C Hamano

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.