Re: [PATCH v4 02/15] help: move list_config_help to builtin/help

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o,
> > builtin/clean.o, and builtin/config.o. This meant that help.o was
> > unusable outside of the context of the main Git executable.
> >
> > To make help.o usable by other commands again, move list_config_help()
> > into builtin/help.c (where it makes sense to assume other builtin libraries
> > are present).
> >
> > When command-list.h is included but a member is not used, we start to
> > hear a compiler warning. Since the config list is generated in a fairly
> > different way than the command list, and since commands and config
> > options are semantically different, move the config list into its own
> > header and move the generator into its own script and build rule.
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> 
> OK.  Looks like a clean-up that is very worthy even without the
> remainder of the topic.
> 
> Thanks.

> 
> >  .gitignore             |  1 +
> >  Makefile               | 16 ++++++--
> >  builtin/help.c         | 86 ++++++++++++++++++++++++++++++++++++++++++
> >  generate-cmdlist.sh    | 19 ----------
> >  generate-configlist.sh | 24 ++++++++++++
> >  help.c                 | 85 -----------------------------------------
> >  help.h                 |  1 -
> >  7 files changed, 123 insertions(+), 109 deletions(-)
> >  create mode 100755 generate-configlist.sh
> >
> > diff --git a/.gitignore b/.gitignore
> > index 055a84c4a8..5dde2cc4c8 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -189,6 +189,7 @@
> >  /gitweb/gitweb.cgi
> >  /gitweb/static/gitweb.js
> >  /gitweb/static/gitweb.min.*
> > +/config-list.h
> >  /command-list.h
> >  *.tar.gz
> >  *.dsc
> > diff --git a/Makefile b/Makefile
> > index 9dff91436e..c49f55a521 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -815,6 +815,7 @@ LIB_FILE = libgit.a
> >  XDIFF_LIB = xdiff/lib.a
> >  VCSSVN_LIB = vcs-svn/lib.a
> >  
> > +GENERATED_H += config-list.h
> >  GENERATED_H += command-list.h
> >  
> >  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
> > @@ -2127,7 +2128,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
> >  
> >  help.sp help.s help.o: command-list.h
> >  
> > -builtin/help.sp builtin/help.s builtin/help.o: command-list.h GIT-PREFIX
> > +builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> >  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
> >  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> >  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> > @@ -2147,6 +2148,12 @@ $(BUILT_INS): git$X
> >  	ln -s $< $@ 2>/dev/null || \
> >  	cp $< $@
> >  
> > +config-list.h: generate-configlist.sh
> > +
> > +config-list.h:
> > +	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
> > +		>$@+ && mv $@+ $@
> > +
> >  command-list.h: generate-cmdlist.sh command-list.txt
> >  
> >  command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
> > @@ -2784,7 +2791,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
> >  .PHONY: sparse $(SP_OBJ)
> >  sparse: $(SP_OBJ)
> >  
> > -GEN_HDRS := command-list.h unicode-width.h
> > +GEN_HDRS := config-list.h command-list.h unicode-width.h
> >  EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%
> >  ifndef GCRYPT_SHA256
> >  	EXCEPT_HDRS += sha256/gcrypt.h
> > @@ -2807,7 +2814,7 @@ hdr-check: $(HCO)
> >  style:
> >  	git clang-format --style file --diff --extensions c,h
> >  
> > -check: command-list.h
> > +check: config-list.h command-list.h
> >  	@if sparse; \
> >  	then \
> >  		echo >&2 "Use 'make sparse' instead"; \
> > @@ -3110,7 +3117,8 @@ clean: profile-clean coverage-clean cocciclean
> >  	$(RM) $(HCC)
> >  	$(RM) -r bin-wrappers $(dep_dirs)
> >  	$(RM) -r po/build/
> > -	$(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags cscope*
> > +	$(RM) *.pyc *.pyo */*.pyc */*.pyo config-list.h command-list.h
> > +	$(RM) $(ETAGS_TARGET) tags cscope*
> >  	$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
> >  	$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
> >  	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
> > diff --git a/builtin/help.c b/builtin/help.c
> > index e5590d7787..1c5f2b9255 100644
> > --- a/builtin/help.c
> > +++ b/builtin/help.c
> > @@ -8,6 +8,7 @@
> >  #include "parse-options.h"
> >  #include "run-command.h"
> >  #include "column.h"
> > +#include "config-list.h"
> >  #include "help.h"
> >  #include "alias.h"
> >  
> > @@ -62,6 +63,91 @@ static const char * const builtin_help_usage[] = {
> >  	NULL
> >  };
> >  
> > +struct slot_expansion {
> > +	const char *prefix;
> > +	const char *placeholder;
> > +	void (*fn)(struct string_list *list, const char *prefix);
> > +	int found;
> > +};
> > +
> > +static void list_config_help(int for_human)
> > +{
> > +	struct slot_expansion slot_expansions[] = {
> > +		{ "advice", "*", list_config_advices },
> > +		{ "color.branch", "<slot>", list_config_color_branch_slots },
> > +		{ "color.decorate", "<slot>", list_config_color_decorate_slots },
> > +		{ "color.diff", "<slot>", list_config_color_diff_slots },
> > +		{ "color.grep", "<slot>", list_config_color_grep_slots },
> > +		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
> > +		{ "color.remote", "<slot>", list_config_color_sideband_slots },
> > +		{ "color.status", "<slot>", list_config_color_status_slots },
> > +		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
> > +		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
> > +		{ NULL, NULL, NULL }
> > +	};
> > +	const char **p;
> > +	struct slot_expansion *e;
> > +	struct string_list keys = STRING_LIST_INIT_DUP;
> > +	int i;
> > +
> > +	for (p = config_name_list; *p; p++) {
> > +		const char *var = *p;
> > +		struct strbuf sb = STRBUF_INIT;
> > +
> > +		for (e = slot_expansions; e->prefix; e++) {
> > +
> > +			strbuf_reset(&sb);
> > +			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
> > +			if (!strcasecmp(var, sb.buf)) {
> > +				e->fn(&keys, e->prefix);
> > +				e->found++;
> > +				break;
> > +			}
> > +		}
> > +		strbuf_release(&sb);
> > +		if (!e->prefix)
> > +			string_list_append(&keys, var);
> > +	}
> > +
> > +	for (e = slot_expansions; e->prefix; e++)
> > +		if (!e->found)
> > +			BUG("slot_expansion %s.%s is not used",
> > +			    e->prefix, e->placeholder);
> > +
> > +	string_list_sort(&keys);
> > +	for (i = 0; i < keys.nr; i++) {
> > +		const char *var = keys.items[i].string;
> > +		const char *wildcard, *tag, *cut;
> > +
> > +		if (for_human) {
> > +			puts(var);
> > +			continue;
> > +		}
> > +
> > +		wildcard = strchr(var, '*');
> > +		tag = strchr(var, '<');
> > +
> > +		if (!wildcard && !tag) {
> > +			puts(var);
> > +			continue;
> > +		}
> > +
> > +		if (wildcard && !tag)
> > +			cut = wildcard;
> > +		else if (!wildcard && tag)
> > +			cut = tag;
> > +		else
> > +			cut = wildcard < tag ? wildcard : tag;
> > +
> > +		/*
> > +		 * We may produce duplicates, but that's up to
> > +		 * git-completion.bash to handle
> > +		 */
> > +		printf("%.*s\n", (int)(cut - var), var);
> > +	}
> > +	string_list_clear(&keys, 0);
> > +}
> > +
> >  static enum help_format parse_help_format(const char *format)
> >  {
> >  	if (!strcmp(format, "man"))
> > diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> > index 71158f7d8b..45fecf8bdf 100755
> > --- a/generate-cmdlist.sh
> > +++ b/generate-cmdlist.sh
> > @@ -76,23 +76,6 @@ print_command_list () {
> >  	echo "};"
> >  }
> >  
> > -print_config_list () {
> > -	cat <<EOF
> > -static const char *config_name_list[] = {
> > -EOF
> > -	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
> > -	sed '/deprecated/d; s/::$//; s/,  */\n/g' |
> > -	sort |
> > -	while read line
> > -	do
> > -		echo "	\"$line\","
> > -	done
> > -	cat <<EOF
> > -	NULL,
> > -};
> > -EOF
> > -}
> > -
> >  exclude_programs=
> >  while test "--exclude-program" = "$1"
> >  do
> > @@ -113,5 +96,3 @@ echo
> >  define_category_names "$1"
> >  echo
> >  print_command_list "$1"
> > -echo
> > -print_config_list
> > diff --git a/generate-configlist.sh b/generate-configlist.sh
> > new file mode 100755
> > index 0000000000..eca6a00c30
> > --- /dev/null
> > +++ b/generate-configlist.sh
> > @@ -0,0 +1,24 @@
> > +#!/bin/sh
> > +
> > +echo "/* Automatically generated by generate-configlist.sh */"
> > +echo
> > +
> > +print_config_list () {
> > +	cat <<EOF
> > +static const char *config_name_list[] = {
> > +EOF
> > +	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
> > +	sed '/deprecated/d; s/::$//; s/,  */\n/g' |
> > +	sort |
> > +	while read line
> > +	do
> > +		echo "	\"$line\","
> > +	done
> > +	cat <<EOF
> > +	NULL,
> > +};
> > +EOF
> > +}
> > +
> > +echo
> > +print_config_list
> > diff --git a/help.c b/help.c
> > index cf67624a94..a21487db77 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -407,91 +407,6 @@ void list_common_guides_help(void)
> >  	putchar('\n');
> >  }
> >  
> > -struct slot_expansion {
> > -	const char *prefix;
> > -	const char *placeholder;
> > -	void (*fn)(struct string_list *list, const char *prefix);
> > -	int found;
> > -};
> > -
> > -void list_config_help(int for_human)
> > -{
> > -	struct slot_expansion slot_expansions[] = {
> > -		{ "advice", "*", list_config_advices },
> > -		{ "color.branch", "<slot>", list_config_color_branch_slots },
> > -		{ "color.decorate", "<slot>", list_config_color_decorate_slots },
> > -		{ "color.diff", "<slot>", list_config_color_diff_slots },
> > -		{ "color.grep", "<slot>", list_config_color_grep_slots },
> > -		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
> > -		{ "color.remote", "<slot>", list_config_color_sideband_slots },
> > -		{ "color.status", "<slot>", list_config_color_status_slots },
> > -		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
> > -		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
> > -		{ NULL, NULL, NULL }
> > -	};
> > -	const char **p;
> > -	struct slot_expansion *e;
> > -	struct string_list keys = STRING_LIST_INIT_DUP;
> > -	int i;
> > -
> > -	for (p = config_name_list; *p; p++) {
> > -		const char *var = *p;
> > -		struct strbuf sb = STRBUF_INIT;
> > -
> > -		for (e = slot_expansions; e->prefix; e++) {
> > -
> > -			strbuf_reset(&sb);
> > -			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
> > -			if (!strcasecmp(var, sb.buf)) {
> > -				e->fn(&keys, e->prefix);
> > -				e->found++;
> > -				break;
> > -			}
> > -		}
> > -		strbuf_release(&sb);
> > -		if (!e->prefix)
> > -			string_list_append(&keys, var);
> > -	}
> > -
> > -	for (e = slot_expansions; e->prefix; e++)
> > -		if (!e->found)
> > -			BUG("slot_expansion %s.%s is not used",
> > -			    e->prefix, e->placeholder);
> > -
> > -	string_list_sort(&keys);
> > -	for (i = 0; i < keys.nr; i++) {
> > -		const char *var = keys.items[i].string;
> > -		const char *wildcard, *tag, *cut;
> > -
> > -		if (for_human) {
> > -			puts(var);
> > -			continue;
> > -		}
> > -
> > -		wildcard = strchr(var, '*');
> > -		tag = strchr(var, '<');
> > -
> > -		if (!wildcard && !tag) {
> > -			puts(var);
> > -			continue;
> > -		}
> > -
> > -		if (wildcard && !tag)
> > -			cut = wildcard;
> > -		else if (!wildcard && tag)
> > -			cut = tag;
> > -		else
> > -			cut = wildcard < tag ? wildcard : tag;
> > -
> > -		/*
> > -		 * We may produce duplicates, but that's up to
> > -		 * git-completion.bash to handle
> > -		 */
> > -		printf("%.*s\n", (int)(cut - var), var);
> > -	}
> > -	string_list_clear(&keys, 0);
> > -}
> > -
> >  static int get_alias(const char *var, const char *value, void *data)
> >  {
> >  	struct string_list *list = data;
> > diff --git a/help.h b/help.h
> > index 7a455beeb7..9071894e8c 100644
> > --- a/help.h
> > +++ b/help.h
> > @@ -22,7 +22,6 @@ static inline void mput_char(char c, unsigned int num)
> >  void list_common_cmds_help(void);
> >  void list_all_cmds_help(void);
> >  void list_common_guides_help(void);
> > -void list_config_help(int for_human);
> >  
> >  void list_all_main_cmds(struct string_list *list);
> >  void list_all_other_cmds(struct string_list *list);