Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode

From: Emily Shaffer <emilyshaffer@google.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>, git@vger.kernel.org
Subject: Re: [PATCH 1/2] progress: create progress struct in 'verbose' mode
Date: Fri, 10 Jul 2020 12:24:51 -0700	[thread overview]
Message-ID: <20200710192451.GD3189386@google.com> (raw)
In-Reply-To: <20200710021451.GI9782@camp.crustytoothpaste.net>

On Fri, Jul 10, 2020 at 02:14:51AM +0000, brian m. carlson wrote:
> 
> On 2020-07-10 at 01:42:41, Emily Shaffer wrote:
> > Before now, the progress API is used by conditionally calling
> > start_progress() or a similar call, and then unconditionally calling
> > display_progress() and stop_progress(), both of which are tolerant of
> > NULL or uninitialized inputs. However, in
> > 98a136474082cdc7228d7e0e45672c5274fab701 (trace: log progress time and
> > throughput), the progress library learned to log traces during expensive
> > operations. In cases where progress should not be displayed to the user
> > - such as when Git is called by a script - no traces will be logged,
> > because the progress object is never created.
> > 
> > Instead, to allow us to collect traces from scripted Git commands, teach
> > a progress->verbose flag, which is specified via a new argument to
> > start_progress() and friends. display_progress() also learns to filter
> > for that flag. With these changes, start_progress() can be called
> > unconditionally but with a conditional as an argument to determine
> > whether to report progress to the user.
> 
> So to make sure I understand this right, we'll collect traces regardless
> if it's enabled, but we'll still honor the --quiet flag if the user
> doesn't want to see them?

Yes, precisely. display_progress() used to check whether to display or
not based on whether the 'struct progress' was valid; now it also checks
whether 'progress->verbose' is set. display() (where the real logic
lives for printing to the user) still never gets called with this
change.

>
> If so, I'm definitely in favor of this
> change.  I was worried when I read the cover letter that we'd display
> them to the user regardless, but from reading the patch and the commit
> message, it seems I misunderstood.
> 
> I think the making the verbose flag a parameter simplifies the code
> nicely and puts the rendering decision in the right place.

Thanks!

 - Emily