Re: [PATCH 2/5] progress: return early when in the background

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 25 2019, SZEDER Gábor wrote:
> 
> > When a git process runs in the background, it doesn't display
> > progress, only the final "done" line [1].  The condition to check that
> > are a bit too deep in the display() function, and thus it calculates
> > the progress percentage even when no progress will be displayed
> > anyway.
> >
> > Restructure the display() function to return early when we are in the
> > background, which prevents the unnecessary progress percentae
> > calculation, and make the function look a bit better by losing one
> > level of indentation.
> >
> > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)
> 
> CC-ing the author of that patch.
> 
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  progress.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/progress.c b/progress.c
> > index 02a20e7d58..b57c0dae16 100644
> > --- a/progress.c
> > +++ b/progress.c
> > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
> >  		return;
> >
> >  	progress->last_value = n;
> > +
> > +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> > +		progress_update = 0;
> > +		return;
> > +	}
> > +
> >  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
> >  	eol = done ? done : "   \r";
> >  	if (progress->total) {
> >  		unsigned percent = n * 100 / progress->total;
> >  		if (percent != progress->last_percent || progress_update) {
> >  			progress->last_percent = percent;
> > -			if (is_foreground_fd(fileno(stderr)) || done) {
> > -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> > -					progress->title, percent,
> > -					(uintmax_t)n, (uintmax_t)progress->total,
> > -					tp, eol);
> > -				fflush(stderr);
> > -			}
> > +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> > +				progress->title, percent,
> > +				(uintmax_t)n, (uintmax_t)progress->total,
> > +				tp, eol);
> > +			fflush(stderr);
> >  			progress_update = 0;
> >  			return;
> >  		}
> >  	} else if (progress_update) {
> > -		if (is_foreground_fd(fileno(stderr)) || done) {
> > -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> > -				progress->title, (uintmax_t)n, tp, eol);
> > -			fflush(stderr);
> > -		}
> > +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> > +			progress->title, (uintmax_t)n, tp, eol);
> > +		fflush(stderr);
> >  		progress_update = 0;
> >  		return;
> >  	}
> 
> This patch looks good, just notes for potential follow-up:
> 
>  * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
>    start_progress_delay() & setting a variable? It's a few C lib calls &
>    potential syscall (getpid(...)).

It depends on whether you consider the following case worth caring
about:

  $ git long-cmd
  <shows progress>
  Ctrl-Z!
  $ bg
  <silent>
  $ fg
  <shows progress>

Or:

  $ git long-cmd &
  <silent>
  $ fg
  <shows progress>

By moving the is_foreground_fd() check to start_progress_delay() and
caching its result, in the first case we would print progress even
after the process is sent to the background, while in the second we
wouldn't print progress even after the initially backgrounded process
is brought to the foreground.

I think the current behavior makes sense (though I'm not quite sure
about printing the final "done" line, as I think I would be annoyed if
it were printed from the background process while I was typing a
longer command... but I don't run git commands in the background in
the first place)

>  * Is that "|| done" part in the "progress_update" case something that
>    needs to happen? I.e. can we entirely skip the "setup signal handler"
>    part in start_progress_delay() if we detect that we're not in the
>    foreground, and then rely on the stop_progress() call to print the
>    "done"?

This, too, depends on how (or whether at all) we want to handle the
user sending the process to the background and bringing it back.

>    Although we set "progress_update = 1" in stop_progress_msg(), so it's
>    not *just* the signal handler but also us "faking" it, and we'd still
>    need to stash away "progress->last_value = n" in display() in that
>    backgrounding case.
> 
>    So maybe it's as simple as it's going to get.