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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 22 Oct 2021 16:18:16 +0200

On Fri, Oct 22 2021, SZEDER Gábor wrote:

> 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...

>>  	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.