Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
