{"thread":{"id":"56009","subject":"[PATCH] help: convert git_cmd to page in one place","startedAt":"2021-06-26T16:32:28Z","lastAt":"2021-07-08T15:12:12Z","messageCount":7,"participants":["Andrei Rybak","Felipe Contreras","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"428523","messageId":"20210626163219.4137317-1-rybak.a.v@gmail.com","threadId":"56009","inReplyTo":null,"subject":"[PATCH] help: convert git_cmd to page in one place","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-06-26T16:32:19Z","receivedAt":"2021-06-26T16:32:28Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Depending on the chosen format of help pages, git-help uses function\nshow_man_page, show_info_page, or show_html_page.  The first thing all\nthree functions do is to convert given `git_cmd` to a `page` using\nfunction cmd_to_page.\n\nMove the common part of these three functions to function cmd_help to\navoid code duplication.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n builtin/help.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex bb339f0fc8..b7eec06c3d 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -436,10 +436,9 @@ static void exec_viewer(const char *name, const char *page)\n \t\twarning(_(\"'%s': unknown man viewer.\"), name);\n }\n \n-static void show_man_page(const char *git_cmd)\n+static void show_man_page(const char *page)\n {\n \tstruct man_viewer_list *viewer;\n-\tconst char *page = cmd_to_page(git_cmd);\n \tconst char *fallback = getenv(\"GIT_MAN_VIEWER\");\n \n \tsetup_man_path();\n@@ -453,9 +452,8 @@ static void show_man_page(const char *git_cmd)\n \tdie(_(\"no man viewer handled the request\"));\n }\n \n-static void show_info_page(const char *git_cmd)\n+static void show_info_page(const char *page)\n {\n-\tconst char *page = cmd_to_page(git_cmd);\n \tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(_(\"no info viewer handled the request\"));\n@@ -486,9 +484,8 @@ static void open_html(const char *path)\n \texecl_git_cmd(\"web--browse\", \"-c\", \"help.browser\", path, (char *)NULL);\n }\n \n-static void show_html_page(const char *git_cmd)\n+static void show_html_page(const char *page)\n {\n-\tconst char *page = cmd_to_page(git_cmd);\n \tstruct strbuf page_path; /* it leaks but we exec bellow */\n \n \tget_html_page_path(&page_path, page);\n@@ -548,6 +545,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n {\n \tint nongit;\n \tenum help_format parsed_help_format;\n+\tconst char *page;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n@@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \n \targv[0] = check_git_cmd(argv[0]);\n \n+\tpage = cmd_to_page(argv[0]);\n \tswitch (help_format) {\n \tcase HELP_FORMAT_NONE:\n \tcase HELP_FORMAT_MAN:\n-\t\tshow_man_page(argv[0]);\n+\t\tshow_man_page(page);\n \t\tbreak;\n \tcase HELP_FORMAT_INFO:\n-\t\tshow_info_page(argv[0]);\n+\t\tshow_info_page(page);\n \t\tbreak;\n \tcase HELP_FORMAT_WEB:\n-\t\tshow_html_page(argv[0]);\n+\t\tshow_html_page(page);\n \t\tbreak;\n \t}\n \n-- \n2.32.0\n\n"},{"id":"428530","messageId":"60d76a8f5d2d6_1740d20882@natae.notmuch","threadId":"56009","inReplyTo":"20210626163219.4137317-1-rybak.a.v@gmail.com","subject":"RE: [PATCH] help: convert git_cmd to page in one place","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-26T17:57:35Z","receivedAt":"2021-06-26T17:57:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Andrei Rybak wrote:\n> Depending on the chosen format of help pages, git-help uses function\n> show_man_page, show_info_page, or show_html_page.  The first thing all\n> three functions do is to convert given `git_cmd` to a `page` using\n> function cmd_to_page.\n> \n> Move the common part of these three functions to function cmd_help to\n> avoid code duplication.\n\nMakes sense.\n\nReviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\n\n-- \nFelipe Contreras\n"},{"id":"429166","messageId":"20210704153912.2742106-1-rybak.a.v@gmail.com","threadId":"56009","inReplyTo":"20210626163219.4137317-1-rybak.a.v@gmail.com","subject":"[PATCH resend] help: convert git_cmd to page in one place","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-07-04T15:39:12Z","receivedAt":"2021-07-04T15:39:21Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Depending on the chosen format of help pages, git-help uses function\nshow_man_page, show_info_page, or show_html_page.  The first thing all\nthree functions do is to convert given `git_cmd` to a `page` using\nfunction cmd_to_page.\n\nMove the common part of these three functions to function cmd_help to\navoid code duplication.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\nReviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n\nResending to make sure that this patch isn't forgotten.\nOriginally sent as https://lore.kernel.org/git/20210626163219.4137317-1-rybak.a.v@gmail.com/\n\n builtin/help.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex bb339f0fc8..b7eec06c3d 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -436,10 +436,9 @@ static void exec_viewer(const char *name, const char *page)\n \t\twarning(_(\"'%s': unknown man viewer.\"), name);\n }\n \n-static void show_man_page(const char *git_cmd)\n+static void show_man_page(const char *page)\n {\n \tstruct man_viewer_list *viewer;\n-\tconst char *page = cmd_to_page(git_cmd);\n \tconst char *fallback = getenv(\"GIT_MAN_VIEWER\");\n \n \tsetup_man_path();\n@@ -453,9 +452,8 @@ static void show_man_page(const char *git_cmd)\n \tdie(_(\"no man viewer handled the request\"));\n }\n \n-static void show_info_page(const char *git_cmd)\n+static void show_info_page(const char *page)\n {\n-\tconst char *page = cmd_to_page(git_cmd);\n \tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(_(\"no info viewer handled the request\"));\n@@ -486,9 +484,8 @@ static void open_html(const char *path)\n \texecl_git_cmd(\"web--browse\", \"-c\", \"help.browser\", path, (char *)NULL);\n }\n \n-static void show_html_page(const char *git_cmd)\n+static void show_html_page(const char *page)\n {\n-\tconst char *page = cmd_to_page(git_cmd);\n \tstruct strbuf page_path; /* it leaks but we exec bellow */\n \n \tget_html_page_path(&page_path, page);\n@@ -548,6 +545,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n {\n \tint nongit;\n \tenum help_format parsed_help_format;\n+\tconst char *page;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n@@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \n \targv[0] = check_git_cmd(argv[0]);\n \n+\tpage = cmd_to_page(argv[0]);\n \tswitch (help_format) {\n \tcase HELP_FORMAT_NONE:\n \tcase HELP_FORMAT_MAN:\n-\t\tshow_man_page(argv[0]);\n+\t\tshow_man_page(page);\n \t\tbreak;\n \tcase HELP_FORMAT_INFO:\n-\t\tshow_info_page(argv[0]);\n+\t\tshow_info_page(page);\n \t\tbreak;\n \tcase HELP_FORMAT_WEB:\n-\t\tshow_html_page(argv[0]);\n+\t\tshow_html_page(page);\n \t\tbreak;\n \t}\n \n-- \n2.32.0\n\n"},{"id":"429181","messageId":"87tul9706u.fsf@evledraar.gmail.com","threadId":"56009","inReplyTo":"20210704153912.2742106-1-rybak.a.v@gmail.com","subject":"Re: [PATCH resend] help: convert git_cmd to page in one place","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-05T06:15:25Z","receivedAt":"2021-07-05T06:18:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jul 04 2021, Andrei Rybak wrote:\n\n> Depending on the chosen format of help pages, git-help uses function\n> show_man_page, show_info_page, or show_html_page.  The first thing all\n> three functions do is to convert given `git_cmd` to a `page` using\n> function cmd_to_page.\n>\n> Move the common part of these three functions to function cmd_help to\n> avoid code duplication.\n>\n> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n> Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n\nThis patch looks good to me:\n\n> Resending to make sure that this patch isn't forgotten.\n> Originally sent as https://lore.kernel.org/git/20210626163219.4137317-1-rybak.a.v@gmail.com/\n>\n>  builtin/help.c | 17 ++++++++---------\n>  1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/help.c b/builtin/help.c\n> index bb339f0fc8..b7eec06c3d 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -436,10 +436,9 @@ static void exec_viewer(const char *name, const char *page)\n>  \t\twarning(_(\"'%s': unknown man viewer.\"), name);\n>  }\n>  \n> -static void show_man_page(const char *git_cmd)\n> +static void show_man_page(const char *page)\n>  {\n>  \tstruct man_viewer_list *viewer;\n> -\tconst char *page = cmd_to_page(git_cmd);\n>  \tconst char *fallback = getenv(\"GIT_MAN_VIEWER\");\n>  \n>  \tsetup_man_path();\n> @@ -453,9 +452,8 @@ static void show_man_page(const char *git_cmd)\n>  \tdie(_(\"no man viewer handled the request\"));\n>  }\n>  \n> -static void show_info_page(const char *git_cmd)\n> +static void show_info_page(const char *page)\n>  {\n> -\tconst char *page = cmd_to_page(git_cmd);\n>  \tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n>  \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n>  \tdie(_(\"no info viewer handled the request\"));\n> @@ -486,9 +484,8 @@ static void open_html(const char *path)\n>  \texecl_git_cmd(\"web--browse\", \"-c\", \"help.browser\", path, (char *)NULL);\n>  }\n>  \n> -static void show_html_page(const char *git_cmd)\n> +static void show_html_page(const char *page)\n>  {\n> -\tconst char *page = cmd_to_page(git_cmd);\n>  \tstruct strbuf page_path; /* it leaks but we exec bellow */\n>  \n>  \tget_html_page_path(&page_path, page);\n> @@ -548,6 +545,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint nongit;\n>  \tenum help_format parsed_help_format;\n> +\tconst char *page;\n>  \n>  \targc = parse_options(argc, argv, prefix, builtin_help_options,\n>  \t\t\tbuiltin_help_usage, 0);\n> @@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>  \n>  \targv[0] = check_git_cmd(argv[0]);\n>  \n> +\tpage = cmd_to_page(argv[0]);\n\nNit not requring a re-roll: I'd snuggle this with the argv[0], not the\nswitch statement, i.e. like the existing code.\n\n>  \tswitch (help_format) {\n>  \tcase HELP_FORMAT_NONE:\n>  \tcase HELP_FORMAT_MAN:\n> -\t\tshow_man_page(argv[0]);\n> +\t\tshow_man_page(page);\n>  \t\tbreak;\n>  \tcase HELP_FORMAT_INFO:\n> -\t\tshow_info_page(argv[0]);\n> +\t\tshow_info_page(page);\n>  \t\tbreak;\n>  \tcase HELP_FORMAT_WEB:\n> -\t\tshow_html_page(argv[0]);\n> +\t\tshow_html_page(page);\n>  \t\tbreak;\n>  \t}\n\nMore generally (not the fault of this patch) the control flow in that\nfile is quite a mess. I wondered why we can't just add this to\ncheck_git_cmd() then, it's also only called in that one place.\n\nWe can, but it and cmd_to_page() return in multiple places, and\nhelp_unknown_cmd() might return, might exit(1) itself etc, so fixing\nsimilar to my 338abb0f045 (builtins + test helpers: use return instead\nof exit() in cmd_*, 2021-06-08) should probably be part of some general\nrefactoring... :)\n"},{"id":"429339","messageId":"xmqqfswri4o6.fsf@gitster.g","threadId":"56009","inReplyTo":"87tul9706u.fsf@evledraar.gmail.com","subject":"Re: [PATCH resend] help: convert git_cmd to page in one place","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-06T20:11:05Z","receivedAt":"2021-07-06T20:11:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> @@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>>  \n>>  \targv[0] = check_git_cmd(argv[0]);\n>>  \n>> +\tpage = cmd_to_page(argv[0]);\n>\n> Nit not requring a re-roll: I'd snuggle this with the argv[0], not the\n> switch statement, i.e. like the existing code.\n\nMakes sense.\n\n>>  \tswitch (help_format) {\n>>  \tcase HELP_FORMAT_NONE:\n>>  \tcase HELP_FORMAT_MAN:\n>> -\t\tshow_man_page(argv[0]);\n>> +\t\tshow_man_page(page);\n>>  \t\tbreak;\n>>  \tcase HELP_FORMAT_INFO:\n>> -\t\tshow_info_page(argv[0]);\n>> +\t\tshow_info_page(page);\n>>  \t\tbreak;\n>>  \tcase HELP_FORMAT_WEB:\n>> -\t\tshow_html_page(argv[0]);\n>> +\t\tshow_html_page(page);\n>>  \t\tbreak;\n>>  \t}\n>\n> More generally (not the fault of this patch) the control flow in that\n> file is quite a mess. I wondered why we can't just add this to\n> check_git_cmd() then, it's also only called in that one place.\n>\n> We can, but it and cmd_to_page() return in multiple places, and\n> help_unknown_cmd() might return, might exit(1) itself etc, so fixing\n> similar to my 338abb0f045 (builtins + test helpers: use return instead\n> of exit() in cmd_*, 2021-06-08) should probably be part of some general\n> refactoring... :)\n\nTrue, too.\n"},{"id":"429474","messageId":"93578e5a-c09b-14c2-8f87-eb354f4b576c@gmail.com","threadId":"56009","inReplyTo":"xmqqfswri4o6.fsf@gitster.g","subject":"Re: [PATCH resend] help: convert git_cmd to page in one place","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-07-08T08:07:31Z","receivedAt":"2021-07-08T08:07:39Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 06/07/2021 22:11, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n>>> @@ -606,16 +604,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>>>   \n>>>   \targv[0] = check_git_cmd(argv[0]);\n>>>   \n>>> +\tpage = cmd_to_page(argv[0]);\n>>\n>> Nit not requring a re-roll: I'd snuggle this with the argv[0], not the\n>> switch statement, i.e. like the existing code.\n> \n> Makes sense.\n> \n>>>   \tswitch (help_format) {\n>>>   \tcase HELP_FORMAT_NONE:\n>>>   \tcase HELP_FORMAT_MAN:\n>>> -\t\tshow_man_page(argv[0]);\n>>> +\t\tshow_man_page(page);\n>>>   \t\tbreak;\n>>>   \tcase HELP_FORMAT_INFO:\n>>> -\t\tshow_info_page(argv[0]);\n>>> +\t\tshow_info_page(page);\n>>>   \t\tbreak;\n>>>   \tcase HELP_FORMAT_WEB:\n>>> -\t\tshow_html_page(argv[0]);\n>>> +\t\tshow_html_page(page);\n>>>   \t\tbreak;\n>>>   \t}\n\nIs reusing \"argv[0]\" one more time instead of introducing the variable\n\"page\" is a good idea? It could either be:\n\n\targv[0] = cmd_to_page(check_git_cmd(argv[0]));\n\nor\n\n\targv[0] = check_git_cmd(argv[0]);\n\targv[0] = cmd_to_page(argv[0]);\n\nThat way, the quoted hunk above (touching calls to show_*_page) wouldn't\nbe in the patch.\n"},{"id":"429491","messageId":"xmqqsg0oc01m.fsf@gitster.g","threadId":"56009","inReplyTo":"93578e5a-c09b-14c2-8f87-eb354f4b576c@gmail.com","subject":"Re: [PATCH resend] help: convert git_cmd to page in one place","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-08T15:12:05Z","receivedAt":"2021-07-08T15:12:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> Is reusing \"argv[0]\" one more time instead of introducing the variable\n> \"page\" is a good idea? It could either be:\n>\n> \targv[0] = cmd_to_page(check_git_cmd(argv[0]));\n>\n> or\n>\n> \targv[0] = check_git_cmd(argv[0]);\n> \targv[0] = cmd_to_page(argv[0]);\n>\n> That way, the quoted hunk above (touching calls to show_*_page) wouldn't\n> be in the patch.\n\nIt is a bad idea.  It gives readers a false impression that there\nare users of information other than show_$fmt_page() functions that\ndo not want the original argv[0] but the variant massaged by the\ncmd_to_page() function later in the code after these functions\nreturn, and more importantly, it would make it impossible to recover\nthe original value of argv[0] if the code later wants to.\n\nTo be perfectly honest, I do not see much value in the patch being\ndiscussed (I am not saying \"I see no value\"---just \"not much\")\n[*1*].  The patch under discussion may not be making things worse,\nbut overwriting argv[0] WILL make it worse than the current code, I\nwould think.\n\n\n[Footnote]\n\n*1* This is because I see nothing wrong in the original code before\n    applying this patch.  How the manual pages are named after the\n    actual command name may happen to be the same across all three\n    show_$fmt_page() backends (and that is why they happen to all\n    call cmd_to_page() helper function), but there is no strong\n    reason why that has to stay that way.  E.g. \"man git-cat-file\"\n    is used by the man backend only because cmd_to_page() yields one\n    'git-cat-file' string, but \"man 1 git-cat-file\" would equally be\n    a good way to drive the \"man\" viewer.  Other format backends may\n    not have good use for the section information---it is more\n    natural to allow each of show_$fmt_page() to use their own way\n    to derive where the documentation is found for each command.\n"}]}