Re: [PATCH 0/9] ci: make Git's GitHub workflow output much more helpful

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 22, 2022, 13:31 UTC
Message-ID: <220222.86tucr6kz5.gmgdl@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2202221126450.4418@tvgsbejvaqbjf.bet>

On Tue, Feb 22 2022, Johannes Schindelin wrote:

Show 20 quoted lines

> Hi Ævar,
>
> On Sun, 20 Feb 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Sun, Feb 20 2022, Johannes Schindelin wrote:
>>
>> > I notice that you did not take this into `seen` yet. I find that a little
>> > sad because it would potentially have helped others to figure out the
>> > failure in the latest `seen`:
>> > https://github.com/git/git/runs/5255378056?check_suite_focus=true#step:5:162
>> >
>> > Essentially, a recent patch introduces hard-coded SHA-1 hashes in t3007.3.
>>
>> I left some feedback on your submission ~3 weeks ago that you haven't
>> responded to:
>> https://lore.kernel.org/git/220127.86ilu5cdnf.gmgdl@evledraar.gmail.com/
>
> You answered my goal of making it easier to figure out regressions by
> doubling down on hiding the logs even better. That's not feedback, that's
> just ignoring the goal.

I think it's clear to anyone reading my feedback that that's either a gross misreading of the feedback I provided or an intentional misrepresentation.

I don't mention the second one of those lightly, but I think after some months of that pattern now when commenting on various patches of yours it's not an unfair claim.

I.e. you generally seem to latch onto some very narrow interpretation or comment in some larger feedback pointing out various issues to you, and use that as a reason not to respond to or address any of the rest.

So just to make the point about one of those mentioned in my [1] with some further details (I won't go into the whole thing to avoid repeating myself):

I opened both of:

    https://github.com/git-for-windows/git/runs/4822802185?check_suite_focus=true
    https://github.com/dscho/git/runs/4840190622?check_suite_focus=true

Just now in Firefox 91.5.0esr-1. Both having been opened before, so they're in cache, and I've got a current 40MB/s real downlink speed etc.

The former fully loads in around 5100ms, with your series here that's just short of 18000ms.

So your CI changes are making the common case of just looking at a CI failure more than **3x as slow as before**.

That's according to the "performance" timeline, and not some abstract "some JS was still running in the background". It lines up with the time that the scroll bar on the side of the screen stops moving, and the viewport does the "zoom to end" thing in GitHub CI's UI, focusing on the error reported. It was really slow before, but it's SLOOOOOW now.

All of which (and I'm no webdev expert) seems to do with the browser engine struggling to keep up with a much larger set of log data being thrown at it, which despite the eliding you're adding can be seen in the ~1.7k lines of output growing to beyond ~33k now.

Once it loads the end result after all of that (re your "doubling down on hiding the logs even better") is that I need to (and I've got a sizable vertically mounted screen) scroll through around 6 pages of output, each of which takes around 3 seconds of Firefox churning on more than 100% CPU before it shows me the next page.

And even *if* it was instant the names of the failing tests are now spread across several pages of output, whereas in the "prove" output we have a quick overall summary separated from the "ci/print-test-failures.sh" output

Does that mean the current output is perfect and can't be improved? No, I also think it sucks. I just think that the current implementation you've proposed for improving it is making it worse overall.

Which doesn't mean that it couldn't be addressed, fixed, or that the core idea of using that "group" syntax to aggregate that output into sections is bad. I think we should use it, just not as it's currently implemented.

If that's "not feedback" I don't know what is. It's all relevant, and while I'm elaborating further here [1] sent almost a month ago notes the same issues.

> You answered my refactor of the Azure Pipelines support with the question
> "why?" that I had answered already a long time ago. That's not feedback,
> that's ignoring the answers I already provided.

I think it's clear what the gap between that answer is and what I was asking you was in the parallel follow-up discussion at [2].

But even your answer there of just wanting to keep it in place doesn't really answer that question for this series. You're not just keeping that stale code in place, but actively changing it.

I.e. even if you run with all that how are others meant to test and review the changes being proposed here?

I.e. is resurrecting Azure CI required to test this series, or should reviewers ignore those parts and just hope it all works etc?

> I don't know how to respond to that, therefore I didn't.

I think whatever differences in direction for this CI feature that we have, or troubles understanding one another, that your update after 3 weeks of not replying to that feedback [3] asking why Junio didn't pick up your patches being indistinguishable from there having been nothing said about your patches at all is, I think, not a good way to proceed with that.

I.e. we're not the only people talking here, there's presumably others who'll read these threads and will want to comment on the direction of any CI changes.

Knowing from you that you read outstanding feedback and didn't understand it, or read it and summarized but ultimately decided to change nothing etc. makes for much of a flow on the ML than just ignoring that feedback entirely.

1. https://lore.kernel.org/git/220126.86sftbfjl4.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/220222.86y2236ndp.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202200043590.26495@tvgsbejvaqbjf.bet/