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

[Junio C Hamano <gitster@pobox.com>]

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