Re: [PATCH v3 3/4] help.c - group common commands by theme

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 3, 2015, 19:44 UTC
Message-ID: <CAPig+cS1K0uZy1-MxKCqyi7Vy5kXYN2Qm-AC4s0YyY5Oe948CA@mail.gmail.com>
In-Reply-To: <55456A91.3040408@gmail.com>

On Sat, May 2, 2015 at 8:23 PM, Sébastien Guimmara <sebastien.guimmara@gmail.com> wrote:

Show 30 quoted lines

> When 'git help' is called, a list of common commands are printed:
>
> The most commonly used git commands are:
>    add        Add file contents to the index
>    bisect     Find by binary search the change that introduced a bug
>    branch     List, create, or delete branches
>    checkout   Checkout a branch or paths to the working tree
>    clone      Clone a repository into a new directory
>    commit     Record changes to the repository
>    [...]
>
> Instead of a less than optimal alphabetical order, print those
> commands in theme-related groups:
>
> The most commonly used git commands are:
>    clone      Clone a repository into a new directory
>    init       Create an empty Git repository or reinitialize an existing one
>
>    log        Show commit logs
>    show       Show various types of objects
>    status     Show the working tree status
>
>    add        Add file contents to the index
>    mv         Move or rename a file, a directory, or a symlink
>    reset      Reset current HEAD to the specified state
>    rm         Remove files from the working tree and from the index
>    [...]
>
> To achieve this, qsort the common_cmds array by group name,
> then print those common commands, skipping a line between each group.

Just a minor observation regarding the final paragraph (feel free to ignore):

This level of detail is probably overkill. The overall intent of the change has already been spelled out nicely via the examples, so mentioning qsort() is unnecessary since it's a mere implementation detail; one which a reader can easily discover by reading the patch proper. Likewise, the example "new output" already shows clearly the blank line between groups, so there is no need to mention it in prose.

If I was composing the commit message, I'd probably drop the final paragraph altogether and rephrase something like this:

    'git help' shows common commands in alphabetical order like this:

        <current output>

    without any indication of how commands relate to high-level
    concepts or each other. Revise the output to group commands by
    concept, like this:

        <revised output>

More below.

Show 14 quoted lines

> Signed-off-by: Sébastien Guimmara <sebastien.guimmara@gmail.com>
> ---
> diff --git a/help.c b/help.c
> index 2072a87..2169a59 100644
> --- a/help.c
> +++ b/help.c
> @@ -218,17 +218,38 @@ void list_commands(unsigned int colopts,
>         }
>  }
>  +/* sort the command name struct by group name */
> +int cmd_group_cmp(const void *elem1, const void *elem2)
> +{
> +       struct cmdname_help *cmd1 = (struct cmdname_help*) elem1;
> +       struct cmdname_help *cmd2 = (struct cmdname_help*) elem2;

As this is C rather than C++, you can drop the cast.

Also, if you were to keep the cast, the prevailing style in git code is to insert a space before the '*' and to drop the space after the ')'.

Show 14 quoted lines

> +       return strcmp(cmd1->group, cmd2->group);
> +}
> +
>  void list_common_cmds_help(void)
>  {
>         int i, longest = 0;
> +       char *current_grp = NULL;
>         for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
>                 if (longest < strlen(common_cmds[i].name))
>                         longest = strlen(common_cmds[i].name);
>         }
>  +      /* sort common commands by group (i.e, beginner's relevance) */
> +       qsort(common_cmds, ARRAY_SIZE(common_cmds),
> +               sizeof(struct cmdname_help), cmd_group_cmp);

Slightly more maintenance free would be sizeof(common_cmds[0]) instead of sizeof(struct cmdname_help).

>         puts(_("The most commonly used git commands are:"));
>         for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
> +

Style: Drop this blank line.

> +               /* skip a line each time we encounter a new command group */
> +               if (current_grp != NULL && strcmp(common_cmds[i].group,
> current_grp))
> +                       printf("\n");

This chunk of code is pretty well self-explanatory, so the comment isn't really adding anything. A comment which merely repeat what the code itself already says is typically considered distracting rather than illuminating, thus it could (and probably should) be dropped. The same observation probably applies to the comment preceding the qsort() invocation, as well.

Finally, elsewhere in this source file, a blank line is emitted with the more idiomatic putchar('\n') rather than printf("\n").

Show 8 quoted lines

> +
> +               current_grp = common_cmds[i].group;
> +
>                 printf("   %s   ", common_cmds[i].name);
>                 mput_char(' ', longest - strlen(common_cmds[i].name));
>                 puts(_(common_cmds[i].help));
> --
> 2.4.0