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@xxxxxxxxx> > Reviewed-by: Felipe Contreras <felipe.contreras@xxxxxxxxx> > --- 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@xxxxxxxxx/ > > 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... :)
