On Wed, Jun 23 2021, SZEDER Gábor wrote: > On Sun, Jun 20, 2021 at 10:02:56PM +0200, SZEDER Gábor wrote: >> It turned out that progress >> counters can be checked easily and transparently in case of progress >> lines that are shown in the tests, i.e. that are shown even when >> stderr is not a terminal or are forced with '--progress'. (In other >> cases it's still fairly easy but not quite transparent, as I think we >> need changes to the progress API; more on that later in a separate >> series.) > > So, the first patch in this WIP/POC series is my attempt at checking > even those progress counters that are not shown in our test suite, > either because stderr is not a terminal or because of an explicit > '--no-progress' option. There are no usable commit messages yet, I > just wanted to see whether it's possible to check all progress lines > and whether it uncovers any more bugs; and the answer is yes to both. > > Anyway, the basic idea is that instead of checking isatty(2) in the > caller, let's perform that check in start_progress() and let callers > override it through an extra function parameter (e.g. when > '--(no-)progress', '-v' or '--quiet' was given). This way > start_progress() will always be called and it would then return NULL > if the progress line should not be shown. Or, if > GIT_TEST_CHECK_PROGRESS=1, then it would return a valid non-NULL > progress instance even when the progress line should not be shown, but > with the new 'progress->hidden' flag set, so subsequent > display_progress() and stop_progress() calls won't print anything but > will be able to perform all the checks and trigger BUG() if one is > violated. > > However, after Ævar pointed out upthread that progress also generates > trace2 regions, I think that it would be better if start_progress() > always returned a valid progress instance, even without > GIT_TEST_CHECK_PROGRESS but with 'progress->hidden' set as necessary, > because that way we would always get that trace2 output, even with > '--no-progress' or 'git cmd 2>log'. > > The first patch also converts a good couple of progress lines to this > new approach, and the subsequent patches fix most of the uncovered > buggy progress lines. Thanks, I skimmed over it and this sort of approach is definitely what we'll need to address my "But we'll still have various untested for BUG()[...]" in https://lore.kernel.org/git/cover-00.25-00000000000-20210623T155626Z-avarab@xxxxxxxxx/ And as you point out we'll get the benefit of consistent trace2 regions, on the one hand it's a bit weird to have this UI code drive a trace2 region when we don't have a TTY, but I think it's useful. We could e.g. eventually record some stats about min/max/avg/percentile processing per-item while we're at it, that's unlikely to be worth it if we need another API like display_progress(), but since we have that one we can piggy-back on it quite easily. Just some implementation nits: I for one would prefer "static inline" wrappers instead of macros in progress.h, makes it easier to consistently set breakpoints in gdb. It's more work up-front, but I think Re Randall's question in https://lore.kernel.org/git/00fb01d76859$8a6ebc50$9f4c34f0$@nexbridge.com that instead of s/start_delayed_progress/start_delayed_progress_if_tty/ it would be better to just leave the "start_delayed_progress", and have it by default do the TTY check, and also check for --progress and/or --verbose/--quiet etc. itself. We'd probably have some special-cases left even then, but I think most of them can be handled with an isatty() check and the "standard" options of --progress etc. I.e. we have OPT__VERBOSE now, but no OPT__PROGRESS (we just use OPT_BOOL). If we made the various common parse-options flags that impact it callbacks that would munge a global variable we could then pick that up in progress.c, and handle the common case of "git some-command --no-progress" directly. It would also make it easy to just move that over to git.c, so we could have "git --no-progress some-command", which I think for --progress, --object-format and other "global-y" options it we should have them to "git" directly, not per-command, especially with us hopefully soon moving 100% away from dashed built-ins. Isn't the most common general rule just: int want_progress = progress ? 1 : verbose ? 1 : quiet ? 0 : isatty(2); Well, that and a version that handles --no-progress distinct from "did not provide it", so we need some "-1" checks in there. Maybe: /* Earlier */ if (quiet != -1 && verbose != -1) die("--quiet and --verbose?"); /* In progress.c after getopt */ int enable = -1; if (opt_progress != -1) enable = opt_progress; if (enable == -1 && opt_verbose != -1) enable = opt_verbose; if (enable == -1 && opt_quiet != -1) enable = !opt_quiet; if (enable == -1) enable = isatty(2); In any case, I think moving that to one place so it's consistently checked would make sense. Some things like builtin/multi-pack-index.c set "progress" as a bitflag for IMO (this was discussed on-list before) no good reason. I.e. the builtin should handle it with a bool, maybe the library wants a flag, but in any case if we can do what I proposed above such libraries won't need a flag at all.
