Re: [PATCH v3 2/4] generate-cmdlist.sh: parse common command groups

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 3, 2015, 20:40 UTC
Message-ID: <CAPig+cSRiw7QeAzuxAJF8mikeK1YiNoNATKzMz71L8E29LLp4Q@mail.gmail.com>
In-Reply-To: <xmqqpp6hv94x.fsf@gitster.dls.corp.google.com>

On Sun, May 3, 2015 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Sébastien Guimmara  <sebastien.guimmara@gmail.com> writes:
>
>> Teach generate-cmdlist.sh to parse common command groups
>> found in command-list.txt in the form
>>
>> common-3_worktree ('3_worktree' being the group identifier)
>>
>> Extract the $grp variable, in addition to the previous $cmd,
>> and inject it as a third field in the cmdname_help struct:
>>
>> {"add", N_("Add file contents to the index"), "3_worktree"},
>>
>> So that when 'git' is called, we can display common commands
>> grouped by theme instead of a less useful alphabetical order.
>>
>> Reviewed by: Luke Diamand <luke@diamand.org>
>> Reviewed by: Andreas Schwab <schwab@linux-m68k.org>
>
> These people may have helped you to polish your earlier round to
> come up with this version, but I do not think they should be listed
> as reviewed-by (in the sense that they would say "Yes, I read this
> version and consider it very good--I endorse the change!") yet.

Minor addendum: If you'd like to acknowledge Luke for $(...) and Andreas for IFS munging, then Helped-by: would be appropriate (and doesn't require their consent).

More below.

Show 32 quoted lines

>> Signed-off-by: Sébastien Guimmara <sebastien.guimmara@gmail.com>
>> ---
>> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
>> index 9a4c9b9..98f937b 100755
>> --- a/generate-cmdlist.sh
>> +++ b/generate-cmdlist.sh
>> @@ -4,19 +4,20 @@ echo "/* Automatically generated by $0 */
>>  struct cmdname_help {
>>      char name[16];
>>      char help[80];
>> +    char group[20];
>
> Storing group name in duplicated text for all commands and then
> using string comparison at runtime is grossly wasteful, don't you
> think, especially when the list is not manually maintained?
>
> command-list.txt is something manually maintained, i.e. the source,
> and it makes sense to use easy-to-see strings like "3_worktree"
> label there instead of cryptic number, but the whole point of this
> script is to pre-process that text into .c file to a form that is
> more suited for machine consumption.  I think this script should:
>
>  - read command list once to make the list of groups in a new
>    separate array;
>
>  - add the group as an "unsigned char group_number" (as we won't
>    have more than 200 groups) field to this struct;
>
>  - read command list again and for each command give the group
>    number here.
>
> or something like that.

Some additional observations:

The "common-N_group" form seems both redundant and unsightly. Redundant because you could just as easily pluck out 'common' lines by searching for "N_group", thus making the "common-" prefix unnecessary. Unsightly because the mix of "-" and "_" is hard to read. (In general, "-" is easier to read, thus probably a better choice.)

Thinking out loud, without regard to implementation complexity...

I might expect to see the table of groups to be declared first, in the order that they should be displayed by "git help". Having the groups in their own table also allows you to give them human-readable descriptions. For actual use, you'd tag each command with the bare group name.

So, something like this, perhaps:

    [groups]
    init    starting a working area
    info    examining history and state
    ...
    branching    branching and merging histories

    [commands]
    git-branch    mainporcelain branching
    ...
    git-clone    mainporcelain init
    ...

This way, the 'N' in "N_group" is unnecessary since presentation order is implied by the [groups] table, and you don't need the "common-" prefix anymore since any command having an attribute from the [groups] table is automatically considered common.