Re: [PATCH v2 05/11] i18n: tag.c factorize i18n strings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 27, 2021, 23:42 UTC
- Message-ID
- <xmqqpmphvclf.fsf@gitster.g>
- In-Reply-To
- <6d89c23a248d1c11db0e92c4f06392272f00c3a2.1640647438.git.gitgitgadget@gmail.com>
"Jean-Noël Avila via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 39 quoted lines
> 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.