Re: [PATCH V3 4/5] Help.c: add list_common_guides_help() function

[Junio C Hamano <gitster@pobox.com>]

Philip Oakley <philipoakley@iee.org> writes:

> Re-use list_common_cmds_help but simply change the array name.
> Candidate for future refactoring to pass a pointer to the array.
>
> The common-guides.h list was generated with a simple variant of the
> generate-cmdlist.sh and command-list.txt.
>
> Do not list User-manual and Everday Git which not follow the naming
> convention, nor gitrepository-layout which doesn't fit within the
> name field size.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.org>
> ---
>  builtin/help.c  |  3 ++-
>  common-guides.h | 11 +++++++++++
>  help.c          | 18 ++++++++++++++++++
>  help.h          |  1 +
>  4 files changed, 32 insertions(+), 1 deletion(-)
>  create mode 100644 common-guides.h
>
> diff --git a/builtin/help.c b/builtin/help.c
> index 03d432b..91a6158 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -433,7 +433,8 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (show_guides) {
> -		/* do action - next patch */
> +		list_common_guides_help();
> +		printf("\n");
>  	}

This looks funny.  If you look at list_commands() that this patch is
mimicking, you will notice that the "trailing blank for clarity" is
done as part of the function, not done by the caller.  I think it is
better done the same way.

> diff --git a/common-guides.h b/common-guides.h
> new file mode 100644
> index 0000000..0e94fdc
> --- /dev/null
> +++ b/common-guides.h
> @@ -0,0 +1,11 @@
> +/* re-use struct cmdname_help in common-commands.h */
> +
> +static struct cmdname_help common_guides[] = {
> +  {"attributes", "defining attributes per path"},
> +  {"glossary", "A GIT Glossary"},
> +  {"ignore", "Specifies intentionally untracked files to ignore"},
> +  {"modules", "defining submodule properties"},
> +  {"revisions", "specifying revisions and ranges for git"},
> +  {"tutorial", "A tutorial introduction to git (for version 1.5.1 or newer)"},
> +  {"workflows", "An overview of recommended workflows with git"},
> +};

The _only_ reason we have common-cmds.h as a separat file even
though it defines data (hence should not be included in more than
one *.c file) is because it is a generated file.

For this array, there is no reason to have it in a separate header
file.  Just define it immediately before list_common_guies_help()
function that is the sole user of the array.

The function can live in builtin/help.c as a static, without
touching global help.c nor help.h, no?  Is there a reason why it
should be callable from other places?

> diff --git a/help.c b/help.c
> index 1dfa0b0..e0368ca 100644
> --- a/help.c
> +++ b/help.c
> @@ -4,6 +4,7 @@
>  #include "levenshtein.h"
>  #include "help.h"
>  #include "common-cmds.h"
> +#include "common-guides.h"
>  #include "string-list.h"
>  #include "column.h"
>  #include "version.h"
> @@ -240,6 +241,23 @@ void list_common_cmds_help(void)
>  	}
>  }
>  
> +void list_common_guides_help(void)
> +{
> +	int i, longest = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
> +		if (longest < strlen(common_guides[i].name))
> +			longest = strlen(common_guides[i].name);
> +	}
> +
> +	puts(_("The common Git guides are:\n"));
> +	for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
> +		printf("   %s   ", common_guides[i].name);
> +		mput_char(' ', longest - strlen(common_guides[i].name));
> +		puts(_(common_guides[i].help));
> +	}
> +}
> +
>  int is_in_cmdlist(struct cmdnames *c, const char *s)
>  {
>  	int i;
> diff --git a/help.h b/help.h
> index 0ae5a12..4ae1fd7 100644
> --- a/help.h
> +++ b/help.h
> @@ -17,6 +17,7 @@ static inline void mput_char(char c, unsigned int num)
>  }
>  
>  extern void list_common_cmds_help(void);
> +extern void list_common_guides_help(void);
>  extern const char *help_unknown_cmd(const char *cmd);
>  extern void load_command_list(const char *prefix,
>  			      struct cmdnames *main_cmds,