On Sun, May 3, 2015 at 1:55 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Sébastien Guimmara <sebastien.guimmara@xxxxxxxxx> 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@xxxxxxxxxxx>
>> Reviewed by: Andreas Schwab <schwab@xxxxxxxxxxxxxx>
>
> 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.
>> Signed-off-by: Sébastien Guimmara <sebastien.guimmara@xxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html