Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2021, 18:26 UTC
Message-ID: <xmqqfswobr1u.fsf@gitster.g>
In-Reply-To: <YOciZUlWnF5ur5ec@danh.dev>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Show 8 quoted lines

>> In other words, which of the following 3 is the most acceptable?
>> 
>> 1. As Junio suggested, quoted above.
>
> I think this approach is the most expensive one, _() needs to query
> the gettext infrastructure, which is usually costly.
> However, I think that cost doesn't matter much since we're about to
> open an editor soon.

See note below.

Show 23 quoted lines

>> 2.
>> status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?
>>                                    _("...") :
>> 				   _("...."), comment_line_char);
>
> install_branch_config() uses this style.
>
>> 
>> 3.
>> const char *hint_foo = allow_empty_message ?
>>                        _("...") :
>> 		       _("....");
>
> builtin/remote.c:show_local_info_item() writes:
>
> 	const char *msg;
> 	if (condition)
> 		msg = _("some message");
> 	else
> 		msg = _("other message");
>
> So, I guess it's fine either way. And people will need to see the
> patch to see which one is better.

Yeah, #1 and #3 are better than the patch posted or #2 in that by extracting the large message body out-of-line from the code that do use the messages, they make it simpler to follow the logic that uses these messages. That is

	if (cleanup_mode == CLEANUP_ALL)
		status_printf(..., hint_cleanup_all);
	else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
		...;
	else /* all the rest */
		status_printf(..., hint_cleanup_space, comment_line_char);

would be far easier to follow than

	if (cleanup_mode == CLEANUP_ALL)
		status_printf(..., 
		condition 
		? large-large-message-1
		: large-large-message-1-plus-note-about-empty-message);
	else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
		...;
	else /* all the rest */
		status_printf(...,
		condition
		? large-large-message-2
		: large-large-message-2-plus-note-about-empty-message,
		comment_line_char);

as the overall structure is easier to follow without the minute detail of using slightly different messages depending on the allow-empty setting.

By the way, if you want to avoid calling _() twice with the approach #1, you can do

	hint_cleanup_all = N_("<cleanup and note about empty message>");
 	...

	if (condition) {
		hint_cleanup_all = N_("<cleanup without note>");
		...
	}

and use _(hint_cleanup_all) at the site that uses the message.