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

On 19-03-25 12:39, SZEDER Gábor wrote:
  | 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)

You've described the current behaviour as I intended it
in the original patch. I.e,:
- Display progress if foreground.
- Suppress output if background.
- Check the foreground/background state each update in case it changed.

I based that on other tools that also dynamically change their output/progress behaviour whether in the foreground or background.

Regarding the final "done" line; I think that's a matter of personal preference. I'm not too fussed if that was changed so that "done" isn't printed if in the background.

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

regards, Luke.