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

Đoàn Trần Công Danh  [off-list ref] writes:

quoted

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.

quoted

2.
status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?
                                   _("...") :
				   _("...."), comment_line_char);

install_branch_config() uses this style.

quoted

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.