Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 7, 2021, 15:52 UTC
Message-ID: <87mts14qgz.fsf@evledraar.gmail.com>
In-Reply-To: <51e00403-dcab-c57f-b77a-de01a61a60eb@gmail.com>

On Mon, Jun 07 2021, Derrick Stolee wrote:

Show 7 quoted lines

> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
>> Don't guard the calls to the progress.c API with "if (progress)". The
>> API itself will check this. This doesn't change any behavior, but
>> makes this code consistent with the rest of the codebase.
>
> Since stop_progress() closes a trace2 region, this actually
> does make a change in behavior, I think. In a good way.

I don't see the behavior change.

Yes start_delayed_progress() will call start_progress_delay() which mallocs and enters the trace2 region, and then if you don't call stop_progress() at all you won't leave it.

But in read-cache.c both before & after my change we only malloc & only enter the region if we're actually displaying the progress, there's an isatty() guard on it.

Once we start that progress bar we will leave the trace2 region, via stop_progress(), but note that stop_progress() will exit early if the pointer you dereference is NULL and not do that, and since we'll have "*progress = NULL" in the case of not wanting the progress bar we won't leave the region we didn't enter in the first place.

The change here is just to remove the needless nano-optimization of guarding the calls with a NULL check of "progress", which the API itself does, no?