On Fri, Oct 22, 2021 at 06:14:34PM -0400, Taylor Blau wrote:
> On Fri, Oct 22, 2021 at 04:18:16PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > >> 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.
>
> *nod*. I figured as much, but a comment indicating that may be helpful
> for reviewers and future readers of this code.
Yeah, a comment would be nice; though perhaps giving that list
variable a descriptive name, e.g. 'titles' (plural!) would have been
enough to set me on the right track.
