On Thu, Feb 16, 2017 at 02:28:29PM +0300, Maxim Moseychuk wrote:
> Replace local safe string buffer allocation by git_psprintf.
Hmm. Is this one actually broken in practice?
I see:
> @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
> *p_progress = NULL;
> if (progress->last_value != -1) {
> /* Force the last update */
> - char buf[128], *bufp;
> - size_t len = strlen(msg) + 5;
> + char *bufp;
We have a fixed-size buffer here, but...
> struct throughput *tp = progress->throughput;
>
> - bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
If it's not big enough we allocate a new one. So this works for any size
of translated string. It just tries to skip the allocation in the
"short" case.
If this were in the normal progress code, I might care more about trying
to skip an allocation for performance. But since this is just in
stop_progress_msg(), I don't think the complexity is worth it. I'm
especially happy to see the magic "5" above go away.
So I think this is worth doing, but the motivation is a simplification,
not a bugfix.
> if (tp) {
> unsigned int rate = !tp->avg_misecs ? 0 :
> tp->avg_bytes / tp->avg_misecs;
> throughput_string(&tp->display, tp->curr_total, rate);
> }
> progress_update = 1;
> - xsnprintf(bufp, len + 1, ", %s.\n", msg);
> + bufp = git_psprintf(", %s.\n", msg);
> display(progress, progress->last_value, bufp);
> - if (buf != bufp)
> - free(bufp);
> + free(bufp);
This parts looks good (modulo using xstrfmt). It's probably worth
renaming "bufp" to the more usual "buf", I would think.
I do wonder if the punctuation here will give translators headaches.
E.g., in a RTL language you probably wouldn't want that period at the
end. I don't know enough to say, so I'm willing to punt on that for now.
But I suspect in the long run we may need to just take the whole
translated message, punctuation included, from the caller. There are
only two callers that actually provide a string.
-Peff