Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > On Fri, Dec 10 2021, Junio C Hamano wrote: > >> * ab/only-single-progress-at-once (2021-11-03) 8 commits >> - progress.c: add & assert a "global_progress" variable >> - various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) >> - pack-bitmap-write.c: don't return without stop_progress() >> - progress.c: add temporary variable from progress struct >> - progress.c tests: test some invalid usage >> - progress.c tests: make start/stop commands on stdin >> - progress.c test helper: add missing braces >> - leak tests: fix a memory leaks in "test-progress" helper >> >> Further tweaks on progress API. >> >> Needs review. >> The last three rounds has seen little reaction, even though earlier >> round saw a lot of responses. The latest round needs a serious >> review or at least Acks from past commentors. >> source: <cover-v6-0.8-00000000000-20211102T122507Z-avarab@xxxxxxxxx> > > I think less in "needs review" and more in needing a tiebreaker and/or > reply to my [1]. I.e. the ongoing discussion SZEDER and I have had about > the relative danger of adding this BUG() to this part of the code. I thought the BUG() thing has been already settled a few iteration ago, but now you bring it up back, I read that part again. In code that is directly about the correctness of the feature, I do agree with the statement you made (paraphrasing---use of BUG() is appropriate when we see a condition that should never trigger or our code is buggy and not behaving the way we intended, and it is better to stop before spreading the damage and eating user's data), but eye candy like progress bars, I do not think it is end-user friendly to kill the operation they started. Of course, when they report a bug, we may be able to say "please run with --no-progress to work it around in the meantime" after thanking them for reporting, so it may not be such a big deal either way. > Which is fair enough, but I'd think a good way forward with it would be > to give it wider exposure during this cycle. If it's triggered somehow > it's trivial to amend/revert the tip commit to remove the assertion. > > If we merge it down I promise I'll (and try to remember to, putting it > in my calendar if so...) start some discussion before the next release > about whether we'll want to cut the release with that BUG(), which if we > don't trigger it by then we'll probably feel confident about keeping. > > Does that sound like a good way forward? Otherwise the "one alternative > way forward[...]" mentioned in [1] is something we could do. I'm OK with marking it as "will cook in 'next'" to keep it longer than usual, but I do not think merging down iffy code to 'next' or even 'master' will give us better chance to expose the issue. You will not get a real-user feedback until it hits a release, which is unfortunately way too late X-<.
