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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 9, 2018, 04:59 UTC
Message-ID: <CAPig+cQHzB3zLqhRmDKuYk_w8283202x80BcuH7Mu3NqL95cZQ@mail.gmail.com>
In-Reply-To: <20180326165520.802-4-pclouds@gmail.com>

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 21 quoted lines

> 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.

Show 10 quoted lines

>  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/

Show 5 quoted lines

> -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/

Show 23 quoted lines

> +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.

Show 7 quoted lines

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