Re: [PATCH 0/4] WIP/POC check isatty(2)-protected progress lines

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 24, 2021, 10:45 UTC
Message-ID: <877dij1pzv.fsf@evledraar.gmail.com>
In-Reply-To: <20210623215736.8279-1-szeder.dev@gmail.com>

On Wed, Jun 23 2021, SZEDER Gábor wrote:

Show 39 quoted lines

> 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@gmail.com/

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.