Re: [PATCH V3 4/5] Help.c: add list_common_guides_help() function
Philip Oakley <philipoakley@iee.org> writes:
Show 31 quoted lines
> 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.
Show 17 quoted lines
> 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?
Show 48 quoted lines
> 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,
Philip Oakley
