Subject: Re: [PATCH v3 10/10] progress.c: add & assert a "global_progress" variable

From: SZEDER Gábor <[email-masked]>
Date: Fri, 3 Dec 2021 00:14:21 +0100

On Mon, Oct 25, 2021 at 02:38:04AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <[email-masked]> writes:
> 
> > 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.
> 
> I too recall that we have fixed numerous bugs in the past year in
> the area, but weren't they kind of obvious ones _once_ they are
> pointed out at you (e.g. progress never reaching to 100%)?  Yet the
> developers have failed to catch them because their eyes would coast
> over without paying attention to them, exactly because the progress
> bar is merely a UI gimmick.
> 
> I haven't formed a firm opinion on this yet, but I think the idea
> behind these BUG() is to help such problems be caught while they are
> still in the lab.  You may not notice when your live progress bar
> behaved a bit funny, but if you hit a BUG(), that would be squarely
> in your face and you cannot ignore it.

The "outside of the test suite" part is important.  Running the test
suite with a GIT_TEST_CHECK_PROGRESS knob is sufficient to catch these
issues as my inital two patch series have shown at the very beginning
of this thread before Ævar hijacked them.  Interested Git developers
can even enable it in their .profile if they wish; I did so with
GIT_TEST_SPLIT_INDEX for a while to expose some of my patches to more
real-world use.

However, I think it's conceptually a bad idea to abort with a BUG() an
otherwise successful Git operation by default, when that bug happens
to be in such an ancillary component as the progress display, and find
the justifications given in the commit message unconvincing.