Re: [PATCH] perf: fix incorrect return value for else case

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Dec 22, 2009 at 09:27:12AM +0800, Wenji Huang wrote:
> John Kacur wrote:
>> On Mon, Dec 21, 2009 at 10:22 AM, Wenji Huang <wenji.huang@oracle.com> wrote:
>>> Return original cmd instead of adding prefix.
>>>
>>> Signed-off-by: Wenji Huang <wenji.huang@oracle.com>
>>> ---
>>>  tools/perf/builtin-help.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
>>> index 9f810b1..ca77df5 100644
>>> --- a/tools/perf/builtin-help.c
>>> +++ b/tools/perf/builtin-help.c
>>> @@ -317,7 +317,7 @@ static const char *cmd_to_page(const char *perf_cmd)
>>>        else if (is_perf_command(perf_cmd))
>>>                return prepend("perf-", perf_cmd);
>>>        else
>>> -               return prepend("perf-", perf_cmd);
>>> +               return perf_cmd;
>>>  }
>>>
>>>  static void setup_man_path(void)
>>> --
>>> 1.5.6
>>
>> Sorry - I believe we should NAK this patch.
>> It would turn the following
>>
>> ./perf nonsuchcmd --help
>> No manual entry for perf-nonsuchcmd
>>
>> into
>>
>> ./perf nonsuchcmd --help
>> No manual entry for nonsuchcmd
>>
>> The former is correct, the name of the man page includes the prefix "perf-"
>>
>> NAK
>>
>> (cc-ing Frederic in case he sees it differently)


I personally don't mind.
Having either

	./perf nonsuchcmd --help
	No manual entry for perf-nonsuchcmd
or

	./perf nonsuchcmd --help
	No manual entry for nonsuchcmd

both make sense for the user.


>>
> Thanks. Since we think the former is better, why not make
> the code compact? Like,
>
> diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
> index 9f810b1..65e2691 100644
> --- a/tools/perf/builtin-help.c
> +++ b/tools/perf/builtin-help.c
> @@ -314,8 +314,6 @@ static const char *cmd_to_page(const char *perf_cmd)
>                 return "perf";
>         else if (!prefixcmp(perf_cmd, "perf"))
>                 return perf_cmd;
> -       else if (is_perf_command(perf_cmd))
> -               return prepend("perf-", perf_cmd);
>         else
>                 return prepend("perf-", perf_cmd);
>  }


Agreed!
Thanks.