On Fri, 6 Jun 2008, Junio C Hamano wrote:
> Boyd Lynn Gerber <gerberb@xxxxxxxxx> writes:
> > diff --git a/progress.c b/progress.c
> > index d19f80c..295c4e3 100644
> > --- a/progress.c
> > +++ b/progress.c
> > @@ -241,7 +241,8 @@ 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[strlen(msg) + 5];
> > + /* char buf[strlen(msg) + 5]; */
> > + char *buf = alloca (strlen(msg) + 5 );
> > struct throughput *tp = progress->throughput;
> > if (tp) {
> > unsigned int rate = !tp->avg_misecs ? 0 :
>
> I do not know the situation over there these days, but I have a distant
> but bitter memory of having to deal with AIX X-<. It insisted that
> inclusion of <alloca.h> to be the very first thing in the source before
> anything else. I would want to keep alloca() out of the codebase without
> very good reason. Not that I care much about portability to AIX, but not
> having to worry about alloca() unless necessary is a good thing.
You hit the nail on the head, AIX and any Novell derived Compiler code
requires it. Also the SCO OS's
> I do not think progress_msg() is a good reason to even worrying about a
> dynamically sized array. The function is designed to spit out a single
> line of message (so the incoming msg is expected to be shorter than 80
> chars or so). If you "git grep stop_progress_msg", you will see that
> there are only two callers of this function, one in progress.c itself that
> says "done", and the other one in index-pack.c that gives a string
> formatted into 48-byte buffer.
>
> So we can be lazy and say:
>
> char buf[128];
> ...
> snprintf(buf, sizeof(buf), ", %s.\n", msg)
>
> and be done with it.
I like the idea.
> If you really wanted to be safe and anal, you could do something like
> this, which would be just as efficient and much more straightforward:
>
> progress.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index d19f80c..55a8687 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -241,16 +241,21 @@ 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[strlen(msg) + 5];
> + char buf[128], *bufp;
> + size_t len = strlen(msg) + 5;
> struct throughput *tp = progress->throughput;
> +
> + bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
> if (tp) {
> unsigned int rate = !tp->avg_misecs ? 0 :
> tp->avg_bytes / tp->avg_misecs;
> throughput_string(tp, tp->curr_total, rate);
> }
> progress_update = 1;
> - sprintf(buf, ", %s.\n", msg);
> - display(progress, progress->last_value, buf);
> + sprintf(bufp, ", %s.\n", msg);
> + display(progress, progress->last_value, bufp);
> + if (buf != bufp)
> + free(bufp);
> }
> clear_progress_signal();
> free(progress->throughput);
>
>
Thanks for the suggestions. I am making changes based on all the feed
back. I will remove all the debug junk from the final patch. I am
putting options in and out a lot at the moment. Trying to make sure I do
not break anything on the 12 OS's. It is a real pain testing all the
changes on them to make sure I did not break anything.
Thanks,
--
Boyd Gerber <gerberb@xxxxxxxxx>
ZENEZ 1042 East Fort Union #135, Midvale Utah 84047
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
