Re: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf
- From
Jeff King <peff@peff.net>
- Date
- Feb 16, 2017, 17:26 UTC
- Message-ID
- <20170216172638.m6obbuca2nqpudeo@sigill.intra.peff.net>
- In-Reply-To
- <20170216112829.18079-4-franchesko.salias.hudro.pedros@gmail.com>
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:
Show 7 quoted lines
> @@ -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.
Show 12 quoted lines
> 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