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

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Jul 8, 2021, 08:07 UTC
Message-ID: <93578e5a-c09b-14c2-8f87-eb354f4b576c@gmail.com>
In-Reply-To: <xmqqfswri4o6.fsf@gitster.g>

On 06/07/2021 22:11, Junio C Hamano wrote:

Show 28 quoted lines

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