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

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

On Sun, Jun 20, 2021 at 10:02:56PM +0200, SZEDER Gábor wrote:

quoted

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/ (local)

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 (local)
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.