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....@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.
pgp6DYjSE6TzP.pgp
Description: PGP signature
