patchhelp: convert git_cmd to page in one place

> Depending on the chosen format of help pages, git-help uses function
> show_man_page, show_info_page, or show_html_page.  The first thing all
> three functions do is to convert given `git_cmd` to a `page` using
> function cmd_to_page.
> 
> Move the common part of these three functions to function cmd_help to
> avoid code duplication.

> Depending on the chosen format of help pages, git-help uses function
> show_man_page, show_info_page, or show_html_page.  The first thing all
> three functions do is to convert given `git_cmd` to a `page` using
> function cmd_to_page.
>
> Move the common part of these three functions to function cmd_help to
> avoid code duplication.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

> Resending to make sure that this patch isn't forgotten.
> Originally sent as https://lore.kernel.org/git/20210626163219.4137317-1-rybak.a.v@gmail.com/
>
>  builtin/help.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index bb339f0fc8..b7eec06c3d 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -436,10 +436,9 @@ static void exec_viewer(const char *name, const char *page)
>  		warning(_("'%s': unknown man viewer."), name);
>  }
>  
> -static void show_man_page(const char *git_cmd)
> +static void show_man_page(const char *page)
>  {
>  	struct man_viewer_list *viewer;
> -	const char *page = cmd_to_page(git_cmd);
>  	const char *fallback = getenv("GIT_MAN_VIEWER");
>  
>  	setup_man_path();
> @@ -453,9 +452,8 @@ static void show_man_page(const char *git_cmd)
>  	die(_("no man viewer handled the request"));
>  }
>  
> -static void show_info_page(const char *git_cmd)
> +static void show_info_page(const char *page)
>  {
> -	const char *page = cmd_to_page(git_cmd);
>  	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
>  	execlp("info", "info", "gitman", page, (char *)NULL);
>  	die(_("no info viewer handled the request"));
> @@ -486,9 +484,8 @@ static void open_html(const char *path)
>  	execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
>  }
>  
> -static void show_html_page(const char *git_cmd)
> +static void show_html_page(const char *page)
>  {
> -	const char *page = cmd_to_page(git_cmd);
>  	struct strbuf page_path; /* it leaks but we exec bellow */
>  
>  	get_html_page_path(&page_path, page);
> @@ -548,6 +545,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  {
>  	int nongit;
>  	enum help_format parsed_help_format;
> +	const char *page;
>  
>  	argc = parse_options(argc, argv, prefix, builtin_help_options,
>  			builtin_help_usage, 0);
> @@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  
>  	argv[0] = check_git_cmd(argv[0]);
>  
> +	page = cmd_to_page(argv[0]);

>  	switch (help_format) {
>  	case HELP_FORMAT_NONE:
>  	case HELP_FORMAT_MAN:
> -		show_man_page(argv[0]);
> +		show_man_page(page);
>  		break;
>  	case HELP_FORMAT_INFO:
> -		show_info_page(argv[0]);
> +		show_info_page(page);
>  		break;
>  	case HELP_FORMAT_WEB:
> -		show_html_page(argv[0]);
> +		show_html_page(page);
>  		break;
>  	}

>> @@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>  
>>  	argv[0] = check_git_cmd(argv[0]);
>>  
>> +	page = cmd_to_page(argv[0]);
>
> Nit not requring a re-roll: I'd snuggle this with the argv[0], not the
> switch statement, i.e. like the existing code.

>>  	switch (help_format) {
>>  	case HELP_FORMAT_NONE:
>>  	case HELP_FORMAT_MAN:
>> -		show_man_page(argv[0]);
>> +		show_man_page(page);
>>  		break;
>>  	case HELP_FORMAT_INFO:
>> -		show_info_page(argv[0]);
>> +		show_info_page(page);
>>  		break;
>>  	case HELP_FORMAT_WEB:
>> -		show_html_page(argv[0]);
>> +		show_html_page(page);
>>  		break;
>>  	}
>
> More generally (not the fault of this patch) the control flow in that
> file is quite a mess. I wondered why we can't just add this to
> check_git_cmd() then, it's also only called in that one place.
>
> We can, but it and cmd_to_page() return in multiple places, and
> help_unknown_cmd() might return, might exit(1) itself etc, so fixing
> similar to my 338abb0f045 (builtins + test helpers: use return instead
> of exit() in cmd_*, 2021-06-08) should probably be part of some general
> refactoring... :)

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
>>> @@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>>   
>>>   	argv[0] = check_git_cmd(argv[0]);
>>>   
>>> +	page = cmd_to_page(argv[0]);
>>
>> Nit not requring a re-roll: I'd snuggle this with the argv[0], not the
>> switch statement, i.e. like the existing code.
> 
> Makes sense.
> 
>>>   	switch (help_format) {
>>>   	case HELP_FORMAT_NONE:
>>>   	case HELP_FORMAT_MAN:
>>> -		show_man_page(argv[0]);
>>> +		show_man_page(page);
>>>   		break;
>>>   	case HELP_FORMAT_INFO:
>>> -		show_info_page(argv[0]);
>>> +		show_info_page(page);
>>>   		break;
>>>   	case HELP_FORMAT_WEB:
>>> -		show_html_page(argv[0]);
>>> +		show_html_page(page);
>>>   		break;
>>>   	}

> Is reusing "argv[0]" one more time instead of introducing the variable
> "page" is a good idea? It could either be:
>
> 	argv[0] = cmd_to_page(check_git_cmd(argv[0]));
>
> or
>
> 	argv[0] = check_git_cmd(argv[0]);
> 	argv[0] = cmd_to_page(argv[0]);
>
> That way, the quoted hunk above (touching calls to show_*_page) wouldn't
> be in the patch.