Re: [Qemu-devel] [PATCH] qemu-img.c: add help for each command

[Programmingkid <programmingkidx@gmail.com>]

> On Sep 10, 2018, at 4:16 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> Am 08.09.2018 um 05:16 hat Programmingkid geschrieben:
>> 
>>> On Sep 7, 2018, at 11:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 
>>> On 8 September 2018 at 04:01, John Arbuckle <programmingkidx@gmail.com> wrote:
>>> 
>>>> +    /* print the help for this command */
>>>> +    if (strcmp("--help", argv[optind + 1]) == 0) {
>>>> +        if (strcmp("amend", cmdname) == 0) {
>>>> +            help_amend();
>>>> +        } else if (strcmp("bench", cmdname) == 0) {
>>>> +            help_bench();
>>>> +        } else if (strcmp("check", cmdname) == 0) {
>>>> +            help_check();
>>>> +        } else if (strcmp("commit", cmdname) == 0) {
>>>> +            help_commit();
>>>> +        } else if (strcmp("compare", cmdname) == 0) {
>>>> +            help_compare();
>>>> +        } else if (strcmp("convert", cmdname) == 0) {
>>>> +            help_convert();
>>>> +        } else if (strcmp("create", cmdname) == 0) {
>>>> +            help_create();
>>>> +        } else if (strcmp("dd", cmdname) == 0) {
>>>> +            help_dd();
>>>> +        } else if (strcmp("info", cmdname) == 0) {
>>>> +            help_info();
>>>> +        } else if (strcmp("map", cmdname) == 0) {
>>>> +            help_map();
>>>> +        } else if (strcmp("measure", cmdname) == 0) {
>>>> +            help_measure();
>>>> +        } else if (strcmp("snapshot", cmdname) == 0) {
>>>> +            help_snapshot();
>>>> +        } else if (strcmp("rebase", cmdname) == 0) {
>>>> +            help_rebase();
>>>> +        } else if (strcmp("resize", cmdname) == 0) {
>>>> +            help_resize();
>>> 
>>> Any time you find yourself writing very repetitive code like
>>> this, it's a good idea to ask yourself "is there a way to make
>>> this data-driven?".
>>> 
>>> thanks
>>> -- PMM
>> 
>> Did you want me to loop thru an array of strings instead?
> 
> There is already an array of all subcommands, img_cmds. You should
> probably add another field there for the help text.

Even though I would prefer to leave the img_cmds array alone, I do
see the advantages of your suggestion. All the code that checks which
command is being used could go. I will ad a help_text field to this
array.

> Also, your line wrapping (even mid-word!) is awful. I'm not sure we
> really have to fill 80 characters in the output and can't simply keep
> the lines a bit shorter so that 80 characters in the source are enough.

Yeah it does look very unreadable like that.

> But if we do want to use the full 80 characters in the output, I'd
> rather break the limit from the coding style and use longer lines in the
> source coe than doing what you did.

Using lines longer than 80 would make the patch a lot easier to read. I will do this.

Thank you.