Re: [PATCH v3 03/10] progress.c tests: make start/stop verbs on stdin
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 22, 2021, 14:18 UTC
- Message-ID
- <211022.864k99kune.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20211021223044.GA3615@szeder.dev>
On Fri, Oct 22 2021, SZEDER Gábor wrote:
Show 8 quoted lines
> On Thu, Oct 14, 2021 at 12:28:19AM +0200, Ævar Arnfjörð Bjarmason wrote: >> Subject: [PATCH v3 03/10] progress.c tests: make start/stop verbs on stdin > > s/verbs/commands/ or instructions. > > Please call them what they are in the context of Git in general, or in > a test script in particular, instead of what part of speech they would > be if they were to appear in a sentence.
*nod*, although barring further issues I don't think I'll re-roll with just that commit message adjustment...
Show 17 quoted lines
>> 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.