Re: [PATCH v2 05/11] i18n: tag.c factorize i18n strings

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 27, 2021, 23:45 UTC
Message-ID: <211228.86wnjpip1i.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqpmphvclf.fsf@gitster.g>

On Mon, Dec 27 2021, Junio C Hamano wrote:

Show 67 quoted lines

> "Jean-Noël Avila via GitGitGadget"  <gitgitgadget@gmail.com> writes:
>
>> From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr>
>>
>> Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
>> ---
>>  builtin/tag.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 6f7cd0e3ef5..a2ab2b15304 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -483,6 +483,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>  		OPT_END()
>>  	};
>>  	int ret = 0;
>> +	const char *only_in_list = NULL;
>>  
>>  	setup_ref_filter_porcelain_msg();
>>  
>> @@ -542,13 +543,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>  		goto cleanup;
>>  	}
>>  	if (filter.lines != -1)
>> -		die(_("-n option is only allowed in list mode"));
>> +		only_in_list = "-n";
>>  	if (filter.with_commit)
>> -		die(_("--contains option is only allowed in list mode"));
>> +		only_in_list = "--contains";
>>  	if (filter.no_commit)
>> -		die(_("--no-contains option is only allowed in list mode"));
>> +		only_in_list = "--no-contains";
>>  	if (filter.points_at.nr)
>> -		die(_("--points-at option is only allowed in list mode"));
>> +		only_in_list = "--points-at";
>> +	if (only_in_list)
>> +		die("the '%s' option is only allowed in list mode", only_in_list);
>>  	if (filter.reachable_from || filter.unreachable_from)
>>  		die(_("--merged and --no-merged options are only allowed in list mode"));
>>  	if (cmdmode == 'd') {
>
> The original died with the first problematic condition that was
> detected, so it was possible to detect a condition and die, and
> check a different condition in a way that may segfault when the
> first condition was true, because we would have called die() before
> making such a risky check for the second condition.  In the above
> cascade, however, there is luckily no such dependency, so the above
> change is safe.
>
> But it still changes the semantics.  Given "tag -d -n 4 --with master",
> we would have complained about "-n", but now we will complain about
> the "--contains", no?
>
> We can fix both of the above issues by making these into an if/else
> if/ cascade, i.e.
>
> 	if (filter.lines != -1)
> 		only_in_list = "-n";
> 	else if (filter.with_commit)
> 		only_in_list = "--contains";
> 	...
> 	if (only_in_list)
> 		die(_("the '%s' option is only allowed..."), only_in_list);
>
> And I think you forgot to mark the message that was factored out for
> translation.

Does it really matter? I.e. we've got plenty of options parsing code in various places that might complain about issues A and B with your command-line, but will only emit one of those at a time.

So if you've got A and B you'll need to play the whack-a-mole game of fixing A, only to then run into B.

It really doesn't change things from a practical POV that we'll now complain about B first, only to complain about A later.

Ideally we'd spot and emit all of the issues, as e.g. my [1] does for some unrelated code.

But until such an improvement something as trivial as minimizing the diff size (i.e. not needing "if" -> "else if") seems preferrable to slavishly maintaining compatibility with the exact sequence of errors we'd emit before.

1. https://lore.kernel.org/git/RFC-patch-09.21-9c6af87c6c9-20211115T220831Z-avarab@gmail.com/