Re: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf

[Jeff King]

On Thu, Feb 16, 2017 at 02:28:28PM +0300, Maxim Moseychuk wrote:

> Git can't run bisect between 2048+ commits if use russian translation.
> Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
> 
> Dummy solution: just increase buffer size but is not safe.
> Size gettext string is a runtime value.

Hmm. I wondered if this used to work (because xsnprintf operated on a
fixed-size fmt) and was broken in the translation. And as a consequence,
if we needed to be searching for other cases with similar bugs.

But no, in this case the fixed-size buffer was actually introduced
during the i18n step from 14dc4899e (i18n: bisect: mark strings for
translation, 2016-06-17).

I guess the other type of bug could still exist, though.

> diff --git a/bisect.c b/bisect.c
> index 21bc6daa4..8670cc97a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
>   struct commit_list *tried;
>   int reaches = 0, all = 0, nr, steps;
>   const unsigned char *bisect_rev;
> - char steps_msg[32];
> + char *steps_msg;

So one solution would be to just bump the "32" higher here. The format
comes from the translation, so in practice it only needs to be large
enough to fit any of our translations.

That feels pretty hacky, though. In practice the set of translations is
contained, but it doesn't have to be (and certainly nobody would notice
if a new translation was added with a longer name until a user
complained).

> @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
>  
>   nr = all - reaches - 1;
>   steps = estimate_bisect_steps(all);
> - xsnprintf(steps_msg, sizeof(steps_msg),
> -   Q_("(roughly %d step)", "(roughly %d steps)", steps),
> -   steps);
> +
> + steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
> +   steps), steps);
>   /* TRANSLATORS: the last %s will be replaced with
>  "(roughly %d steps)" translation */
>   printf(Q_("Bisecting: %d revision left to test after this %s\n",
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
> + free(steps_msg);

I wondered if a viable solution would be to make the whole thing one
single translatable string. It would avoid the need for the TRANSLATORS
comment. But I guess we have two "Q_" invocations here, and they might
need to be handled separately (e.g., "2 revisions", "1 step").

I also wondered if we could just make this into two printf statements
("revisions left to test", followed by "roughly N steps").  But the
commit message for 14dc4899e mentions RTL languages. So I think we
really do need to build it up block by block and let the translators
adjust the ordering.

So I think your solution is the best we can do.

It's probably worth outlining these alternatives in the commit message.

-Peff

[PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf

[Maxim Moseychuk]

Git can't run bisect between 2048+ commits if use russian translation.
Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.

Dummy solution: just increase buffer size but is not safe.
Size gettext string is a runtime value.

Signed-off-by: Maxim Moseychuk 
---
 bisect.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 21bc6daa4..8670cc97a 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
-   char steps_msg[32];
+   char *steps_msg;
 
read_bisect_terms(_bad, _good);
if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
nr = all - reaches - 1;
steps = estimate_bisect_steps(all);
-   xsnprintf(steps_msg, sizeof(steps_msg),
- Q_("(roughly %d step)", "(roughly %d steps)", steps),
- steps);
+
+   steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
+ steps), steps);
/* TRANSLATORS: the last %s will be replaced with
   "(roughly %d steps)" translation */
printf(Q_("Bisecting: %d revision left to test after this %s\n",
  "Bisecting: %d revisions left to test after this %s\n",
  nr), nr, steps_msg);
+   free(steps_msg);
 
return bisect_checkout(bisect_rev, no_checkout);
 }
-- 
2.11.1