[PATCH] help: convert git_cmd to page in one place

[PATCH] help: convert git_cmd to page in one place

[Andrei Rybak]

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>
---
 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;
 	}
 
-- 
2.32.0

RE: [PATCH] help: convert git_cmd to page in one place

[Felipe Contreras]

Andrei Rybak wrote:
> 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.

Makes sense.

Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>

-- 
Felipe Contreras

[PATCH resend] help: convert git_cmd to page in one place

[Andrei Rybak]

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;
 	}
 
-- 
2.32.0

Re: [PATCH resend] help: convert git_cmd to page in one place

[Ævar Arnfjörð Bjarmason]

On Sun, Jul 04 2021, Andrei Rybak wrote:

> 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>
> ---

This patch looks good to me:

> 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]);

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... :)

Re: [PATCH resend] help: convert git_cmd to page in one place

[Junio C Hamano]

Æ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;
>>  	}
>
> 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... :)

True, too.

Re: [PATCH resend] help: convert git_cmd to page in one place

[Andrei Rybak]

On 06/07/2021 22:11, Junio C Hamano wrote:
> Æ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.

Re: [PATCH resend] help: convert git_cmd to page in one place

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> 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.

It is a bad idea.  It gives readers a false impression that there
are users of information other than show_$fmt_page() functions that
do not want the original argv[0] but the variant massaged by the
cmd_to_page() function later in the code after these functions
return, and more importantly, it would make it impossible to recover
the original value of argv[0] if the code later wants to.

To be perfectly honest, I do not see much value in the patch being
discussed (I am not saying "I see no value"---just "not much")
[*1*].  The patch under discussion may not be making things worse,
but overwriting argv[0] WILL make it worse than the current code, I
would think.


[Footnote]

*1* This is because I see nothing wrong in the original code before
    applying this patch.  How the manual pages are named after the
    actual command name may happen to be the same across all three
    show_$fmt_page() backends (and that is why they happen to all
    call cmd_to_page() helper function), but there is no strong
    reason why that has to stay that way.  E.g. "man git-cat-file"
    is used by the man backend only because cmd_to_page() yields one
    'git-cat-file' string, but "man 1 git-cat-file" would equally be
    a good way to drive the "man" viewer.  Other format backends may
    not have good use for the section information---it is more
    natural to allow each of show_$fmt_page() to use their own way
    to derive where the documentation is found for each command.