Re: [PATCH v3 03/10] progress.c tests: make start/stop verbs on stdin

> >>  	progress_testing = 1;
> >> -	progress = start_progress(title, total);
> >>  	while (strbuf_getline(&line, stdin) != EOF) {
> >>  		char *end;
> >>
> >> -		if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
> >> +		if (skip_prefix(line.buf, "start ", (const char **) &end)) {
> >> +			uint64_t total = strtoull(end, &end, 10);
> >> +			if (*end == '\0') {
> >> +				progress = start_progress(default_title, total);
> >> +			} else if (*end == ' ') {
> >> +				item = string_list_insert(&list, end + 1);
> >> +				progress = start_progress(item->string, total);
> >
> > Why is it necessary to use a string_list here?  This is the only place
> > where it is used, so I don't understand why we should store anything
> > in it.
>
> The progress.c API doesn't xstrdup() the title you give it, so you can't
> free() it after while it's alive.
>
> Here we're re-setting the same strbuf in a loop, so if we hand a "title"
> to progress.c we need to keep it around, when we later add a BUG
> assertion in 10/10:
>
> +	if (global_progress)
> +		BUG("'%s' progress still active when trying to start '%s'",
> +		    global_progress->title, progress->title);
> +	global_progress = progress;
>
> The "old" title would point to already-free'd or to a nonsensical value
> if we didn't do that.