On Mon, Jun 07 2021, Derrick Stolee wrote: > 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?
