Re: [PATCH/RFC 3/5] generate-cmdlist.sh: keep all information in common-cmds.h

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy
<pclouds@gmail.com> wrote:
> common-cmds.h is used to extract the list of common commands (by
> group) and a one-line summary of each command. Some information is
> dropped, for example command category or summary of other commands.
> Update generate-cmdlist.sh to keep all the information. The extra info
> will be used shortly.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -2,9 +2,10 @@
>  struct cmdname_help {
> -       char name[16];
> +       char name[32];
>         char help[80];
> -       unsigned char group;
> +       unsigned int category;
> +       unsigned int group;
>  };
> @@ -23,27 +24,50 @@ sed -n '
> +echo "#define GROUP_NONE 0xff /* no common group */"
> +echo "#define GROUP_ 0xff /* no common group */"

Meh, this "GROUP_" alias of "GROUP_NONE" isn't so nice.

>  n=0
> -substnum=
>  while read grp
>  do
> -       echo "^git-..*[         ]$grp"
> -       substnum="$substnum${substnum:+;}s/[    ]$grp/$n/"
> +       echo "#define GROUP_$grp $n"
>         n=$(($n+1))
> -done <"$grps" >"$match"
> +done <"$grps"

This patch drops all use of the file $match. Earlier in this script,
not seen in the context, are a couple references to $match which ought
to be adjusted to take its retirement into account:

    match=match$$.tmp
    trap "rm -f '$grps' '$match'" 0 1 2 3 15

However, I'm concerned that this change may be going in the wrong
direction. A line in "### command list" section looks like this:

    command-name  category [deprecated] [common]

Although we don't currently have any commands marked with tag
"deprecated", we very well may have some day. More generally, new
optional or required tags may be added in the future. As such, the
line format is relatively free-form. Current clients don't even care
in what order the tags appears (following 'category') nor how many
tags there are. The new code added by this patch, however, is far less
flexible and accommodating since it assumes hard-coded columns for the
tags (and doesn't even take 'deprecated' into account).

The point of the $match file was to be able to extract only lines
which mentioned one of the "common groups", and the point of the
'substnum' transformation was to transform the group name into a group
number -- both of these operations were done without caring about the
exact column the "common group" tag occupied.

Obviously, one option for addressing this concern would be to change
the definition to make the tag columns fixed and non-optional, which
would allow the simpler implementation used by this patch. Doing so
may require fixing other consumers of command-list.txt (though, I'm
pretty sure existing consumers wouldn't be bothered).

(Perl would be an obvious good choice for retaining the current
relatively free-form line definition without having to jump through
hoops in the shell. Unfortunately, though, a Perl dependency in the
build system can be problematic[1].)

[1]: https://public-inbox.org/git/1440365469-9928-1-git-send-email-sunshine@sunshineco.com/

> -printf 'static struct cmdname_help common_cmds[] = {\n'
> -grep -f "$match" "$1" |
> +echo '/*'
> +printf 'static const char *cmd_categories[] = {\n'
> +grep '^git-' "$1" |

This "grep '^git-'" (and those below) misses some commands, such as
"gitk" and "gitweb". Is that intentional? If not, then you'll probably
need to grab lines following "### command list", as is done earlier in
the script. Same comment for the other couple grep's later in the
patch.

> +awk '{print $2;}' |

At one time, Junio expressed concerns[2] about having an 'awk'
dependency in the build system (in fact, with regards to this same
generation process). Whether he still has such concerns is unknown,
but it should be easy enough to avoid it here (and below).

[2]: https://public-inbox.org/git/20150519004356.GA12854@flurp.local/

> +sort |
> +uniq |
> +while read category; do
> +       printf '\t\"'$category'\",\n'
> +done
> +printf '\tNULL\n};\n\n'
> +echo '*/'
> diff --git a/help.c b/help.c
> @@ -190,6 +190,28 @@ void list_commands(unsigned int colopts,
> +static void extract_common_cmds(struct cmdname_help **p_common_cmds,
> +                               int *p_nr)
> +{
> +       int i, nr = 0;
> +       struct cmdname_help *common_cmds;
> +
> +       ALLOC_ARRAY(common_cmds, ARRAY_SIZE(command_list));
> +
> +       for (i = 0; i < ARRAY_SIZE(command_list); i++) {
> +               const struct cmdname_help *cmd = command_list + i;
> +
> +               if (cmd->category != CAT_mainporcelain ||
> +                   cmd->group == GROUP_NONE)
> +                       continue;

Is the CAT_mainporcelain condition necessary? Before this patch, the
command list would contain only commands with an associated group, so
it seems that you could get by just with the GROUP_NONE condition.

> +
> +               common_cmds[nr++] = *cmd;
> +       }
> +
> +       *p_common_cmds = common_cmds;
> +       *p_nr = nr;
> +}