From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_LOW, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 35F3F1F670 for ; Mon, 25 Oct 2021 05:02:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231940AbhJYFE3 (ORCPT ); Mon, 25 Oct 2021 01:04:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51408 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229489AbhJYFE2 (ORCPT ); Mon, 25 Oct 2021 01:04:28 -0400 Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A6C38C061745 for ; Sun, 24 Oct 2021 22:02:06 -0700 (PDT) Received: by mail-wr1-x436.google.com with SMTP id w15so6770604wra.3 for ; Sun, 24 Oct 2021 22:02:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=JmuhRHb9zVaozts7k2iba2TbJythbbmf/MnaVEdYKNQ=; b=IZ2Ixd82Nhr2tzlScYhaKVWRr0R7qJKuT4bgxf28wSmgyjfbBZ1ecBnVNHpzgTZrCP HSiAmXg5TW4YuwGdJmX5pWSljrnmMTrcS0+bqUzNuKj0pazHMPbIntl8+TT4wcTb79v3 CEfsyIwlW2jgjZCnsN1na03VFzeCD3CMLUPpMElrldo/3YoUBt2xJL+x2im0tbz13QI0 gg+7ppDUyQ8OZaoEcV+nvMP/HYDEUUcLoIvuYP4B42lxlbaj2MdCIDYrzhTqLbWmyw0j mlJ3Br78OXQ8IUBz8Cx58H2kdWKz6Wrh4Cg4X4IoK8MV1EI1aarmON3V9mn30rW/pr2e Ht3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=JmuhRHb9zVaozts7k2iba2TbJythbbmf/MnaVEdYKNQ=; b=Sv0OB2QIxbFmycZt9BP3bZTfdVjhtwUgXTPlYJRPo3/ibBRD0ybbuY1Q+LZVzDwqmj W9wty/KONVBkhu++cJ+ReG7BTaSg9vpfvSFCdOFuRimbe6zZWHaKlYWK028/mCWZ9jiz tRvG6hux+UW1D0sckcnjhxi/4hF8mC75gNUGZHMZWvCalCrRCSJwMqsmptnkkdwgVCuR LjKTWYvxfIJdUvts6jIw32FpQ9ea0q5IcCMJzri2W4D6ubJs7fp6QEjQCHVuRa4LxRjR PUnHbF5kLtPxMtJfX/cQDxZeVy285VUj7L4/SzJEfJIy0urlIXnimn4DhOCl+H1DME7U 9Ohw== X-Gm-Message-State: AOAM531eFtygzuZpCxZfM7VqCAaArvAumZRT+iOFR+gienHkM65s7ULa k+iKHRC/WRwks3BfEH+NDFs= X-Google-Smtp-Source: ABdhPJweXFfCGnpC2gQ3jMNUtnHfpmPeb6d020R/KxSM0ZfRqhckq4zqP+LGXb3yZKK1aEXXOjYY3Q== X-Received: by 2002:adf:f1d2:: with SMTP id z18mr11107743wro.160.1635138125105; Sun, 24 Oct 2021 22:02:05 -0700 (PDT) Received: from szeder.dev (94-21-23-225.pool.digikabel.hu. [94.21.23.225]) by smtp.gmail.com with ESMTPSA id s3sm16293392wmh.30.2021.10.24.22.02.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 24 Oct 2021 22:02:04 -0700 (PDT) Date: Mon, 25 Oct 2021 07:02:02 +0200 From: SZEDER =?utf-8?B?R8OhYm9y?= To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: git@vger.kernel.org, Junio C Hamano , =?utf-8?B?UmVuw6k=?= Scharfe , Emily Shaffer Subject: Re: [PATCH v3 10/10] progress.c: add & assert a "global_progress" variable Message-ID: <20211025050202.GC2101@szeder.dev> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Thu, Oct 14, 2021 at 12:28:26AM +0200, Ævar Arnfjörð Bjarmason wrote: > The progress.c code makes a hard assumption that only one progress bar > be active at a time (see [1] for a bug where this wasn't the > case). Add a BUG() that'll trigger if we ever regress on that promise > and have two progress bars active at the same time. I still very much dislike the idea of a BUG() in the progress code that can trigger outside of the test suite, because the progress line is only a UI gimmick and not a crucial part of any Git operation, and even though a progress line might be buggy, the underlying Git operation is not affected by it and would still finish successfully, as was the case with the dozen of so progress line bugs in the past. > There was an alternative test-only approach to doing the same > thing[2], but by doing this outside of a GIT_TEST_* mode we'll know > we've put a hard stop to this particular API misuse. > > It will also establish scaffolding to address current fundamental > limitations in the progress output: The current output must be > "driven" by calls to the likes of display_progress(). Please elaborate why that is a "fundamental limitation"; I don't see any drawback of the current approach. > Once we have a > global current progress object we'll be able to update that object via > SIGALRM. What are the supposed benefits of doing that? I do see its drawbacks, considering that we have progress lines that are updated from multiple threads. > See [3] for early code to do that. > > It's conceivable that this change will hit the BUG() condition in some > scenario that we don't currently have tests for, this would be very > bad. If that happened we'd die just because we couldn't emit some > pretty output. > > See [4] for a discussion of why our test coverage is lacking; our > progress display is hidden behind isatty(2) checks in many cases, so > the test suite doesn't cover it unless individual tests are run in > "--verbose" mode, we might also have multi-threaded use of the API, so > two progress bars stopping and starting would only be visible due to a > race condition. > > Despite that, I think that this change won't introduce such > regressions, because: > > 1. I've read all the code using the progress API (and have modified a > large part of it in some WIP code I have). Almost all of it is really > simple, the parts that aren't[5] are complex in the display_progress() part, > not in starting or stopping the progress bar. > > 2. The entire test suite passes when instrumented with an ad-hoc > Linux-specific mode (it uses gettid()) to die if progress bars are > ever started or stopped on anything but the main thread[6]. > > Extending that to die if display_progress() is called in a thread > reveals that we have exactly two users of the progress bar under > threaded conditions, "git index-pack" and "git pack-objects". Both > uses are straightforward, and they don't start/stop the progress > bar when threads are active. > > 3. I've likewise done an ad-hoc test to force progress bars to be > displayed with: > > perl -pi -e 's[isatty\(2\)][1]g' $(git grep -l -F 'isatty(2)') > > I.e. to replace all checks (not just for progress) of checking > whether STDERR is connected to a TTY, and then monkeypatching > is_foreground_fd() in progress.c to always "return 1". Running the > tests with those applied, interactively and under -V reveals via: > > $ grep -e set_progress_signal -e clear_progress_signal test-results/*out > > That nothing our tests cover hits the BUG conditions added here, > except the expected "BUG: start two concurrent progress bars" test > being added here. > > That isn't entirely true since we won't be getting 100% coverage > due to cascading failures from tests that expected no progress > output on stderr. To make sure I covered 100% I also tried making > the display() function in progress.c a NOOP on top of that (it's > the calls to start_progress_delay() and stop_progress()) that > matter. > > That doesn't hit the BUG() either. Some tests fail in that mode > due to a combination of the overzealous isatty(2) munging noted > above, and the tests that are testing that the progress output > itself is present (but for testing I'd made display() a NOOP). > > Between those three points I think it's safe to go ahead with this > change. The above analysis only considers _our_ _current_ codebase. However, even though this might be safe now, it doesn't mean that it will remain safe in the future, as we might add new progress lines that lack test coverage (though hopefully won't), and would hit that BUG() at a user. Furthermore, even though this might be safe in our codebase, it doesn't mean that it is safe in some 20+k forks of Git that exist on GitHub alone (I for one have a git command or two with in my fork which output progress lines but, sadly, have zero test coverage). But more importantly, even though it might be safe to do so, that doesn't mean that it's a good idea to do so. The commit message does little to justify why it is conceptually a good idea to add this BUG() to the progress code in a way that it can trigger outside of the test suite. > 1. 6f9d5f2fda1 (commit-graph: fix progress of reachable commits, 2020-07-09) > 2. https://lore.kernel.org/git/20210620200303.2328957-3-szeder.dev@gmail.com > 3. https://lore.kernel.org/git/patch-18.25-e21fc66623f-20210623T155626Z-avarab@gmail.com/ > 4. https://lore.kernel.org/git/cover-00.25-00000000000-20210623T155626Z-avarab@gmail.com/ > 5. b50c37aa44d (Merge branch 'ab/progress-users-adjust-counters' into > next, 2021-09-10) > 6. https://lore.kernel.org/git/877dffg37n.fsf@evledraar.gmail.com/ > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > progress.c | 18 ++++++++++++++---- > t/t0500-progress-display.sh | 11 +++++++++++ > 2 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/progress.c b/progress.c > index b9369e9a264..a31500f8b2b 100644 > --- a/progress.c > +++ b/progress.c > @@ -46,6 +46,7 @@ struct progress { > }; > > static volatile sig_atomic_t progress_update; > +static struct progress *global_progress; > > /* > * These are only intended for testing the progress output, i.e. exclusively > @@ -219,11 +220,16 @@ void progress_test_force_update(void) > progress_interval(SIGALRM); > } > > -static void set_progress_signal(void) > +static void set_progress_signal(struct progress *progress) > { > struct sigaction sa; > struct itimerval v; > > + if (global_progress) > + BUG("'%s' progress still active when trying to start '%s'", > + global_progress->title, progress->title); > + global_progress = progress; This function is called set_progress_signal(), so checking and setting 'global_progress' feels out of place here; it would be better to do that in start_progress_delay(). > + > if (progress_testing) > return; > > @@ -241,10 +247,14 @@ static void set_progress_signal(void) > setitimer(ITIMER_REAL, &v, NULL); > } > > -static void clear_progress_signal(void) > +static void clear_progress_signal(struct progress *progress) > { > struct itimerval v = {{0,},}; > > + if (!global_progress) > + BUG("should have active global_progress when cleaning up"); > + global_progress = NULL; Likewise. > if (progress_testing) > return; > > @@ -268,7 +278,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, > strbuf_init(&progress->counters_sb, 0); > progress->title_len = utf8_strwidth(title); > progress->split = 0; > - set_progress_signal(); > + set_progress_signal(progress); > trace2_region_enter("progress", title, the_repository); > return progress; > } > @@ -372,7 +382,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) > display(progress, progress->last_value, buf); > free(buf); > } > - clear_progress_signal(); > + clear_progress_signal(progress); > strbuf_release(&progress->counters_sb); > if (progress->throughput) > strbuf_release(&progress->throughput->display); > diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh > index 59e9f226ea4..867fdace3f2 100755 > --- a/t/t0500-progress-display.sh > +++ b/t/t0500-progress-display.sh > @@ -298,6 +298,17 @@ test_expect_success 'cover up after throughput shortens a lot' ' > test_cmp expect out > ' > > +test_expect_success 'BUG: start two concurrent progress bars' ' > + cat >in <<-\EOF && > + start 0 one > + start 0 two > + EOF > + > + test_must_fail test-tool progress \ > + stderr && > + grep "^BUG: .*'\''one'\'' progress still active when trying to start '\''two'\''$" stderr > +' > + > test_expect_success 'progress generates traces' ' > cat >in <<-\EOF && > start 40 > -- > 2.33.1.1346.g48288c3c089 >