| Message ID | 20210704153912.2742106-1-rybak.a.v@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d5659f856f3084d33d2379958dd3eefd8a24379f |
| Headers | show |
| Series | [resend] help: convert git_cmd to page in one place | expand |
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... :)
Æ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.
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.
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.
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; }