Re: [workflow] Triaging issues (was Automatically close bug report when a patch is committed)

[Vagrant Cascadian <vagrant@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 8270 bytes --]

On 2023-09-07, Giovanni Biscuolo wrote:
> Christopher Baines <mail@cbaines.net> writes:
>
>> Giovanni Biscuolo <g@xelera.eu> writes:
>
> [...]
>
>>> 20 bugs with messages similar to this one:
>>>
>>>
>>>  rofi-wayland was added in:
>>>
>>>  04b5450ad852735dfa50961d3afc789b2e52b407 gnu: Add rofi-wayland.
>>>
>>>  And updated to a newer version in:
>>>
>>>  19c042ddf80533ba7a615b424dedf9647ca65b0f gnu: rofi-wayland: Update to 1.7.5+wayland2.
>>>
>>>  Marking as done.
>>>
>>> (https://yhetil.org/guix/87zg25r0id.fsf@wireframe/)
>>>
>>> IMO we need a way automatically close this kind of bug reports... or am
>>> I missing something?
>>
>> I think the example you give doesn't relate to what you're looking at
>> below (a post-receive hook).
>
> Oh I see, thanks!
>
> This is a complex case (see below), at least not one that can be solved
> by automatically closing bug reports upon commits :-O
>
> Sorry for the confusion I added by pointing out the wrong example, a
> quick look at many bug reports made by Vagrant Cascadian last Fri and
> Sat shows that many (all?) of the closed bug reports was some sort of
> "duplication" of others.  Vagrant please can you tell us?

Yes, I think a fair amount was from "duplicate" patches. From memory, I
would say many, if not most, of the bugs were due to one person
submitting a patch, and someone else independently committing the same
or newer version.

Part of the problem with duplicates may be the difficulty of reviewing a
very long poorly curated list of bugs... before issues.guix.gnu.org was
a thing, it was difficult to actually find prior submissions... and it
is still long enough that it is not easy.


> Probably /triaging/ is one of the most critical bug report management
> issue, it should be addressed properly:
>
> - by finding or developing triage helping tools to automate what is
>   possible
>   
> - by having more people do the (boring) task of triaging bugs
>
> Probably we should consider adding one more contributor "level": the
> triager; the triager is _not_ a reviewer (obviously not a committer),
> even if she could /also/ be a reviewer and/or committer.
>
> The point is that triaging is a (boring) activity that Someone™ should
> perform, sooner or later (as Vagrant did with the bug reports mentioned
> above).

I was definitely in the mood for "let me get some relatively easy, if
boring things done" the other day. :)


> Obviously a contrubutor could (should) also be a self-triager, if she
> wants help making the review process more efficient.

FWIW, I think I used the search:

  https://issues.guix.gnu.org/search?query=is%3Aopen+tag%3Apatch

Sorted by date, and searched for the phrase "update" in the subject, as
old bugs proposing to update to a newer version were and are, well,
likely candidates for culling. :)

Other tooling that could further help with the process would be
valuable.


>> There were at least two different issues with patches for adding
>> rofi-wayland [1] and [2].
>>
>> 1: https://issues.guix.gnu.org/53717
>
> This was to add (version "1.7.3+wayland1") and AFAIU was never committed

Right, I marked this (and several others) as done because it was
effectively superseded by newer versions in current guix.

There were sometimes some things that were not merged, and I made
judgement calls on weather they still needed to be, such as a tweak to
the packaging that was maybe only needed to get an older version to
build, but the newer version was building correctly.


>> 2: https://issues.guix.gnu.org/59241
>
> This issue have 2 patches:
>
> [PATCH 1/2] gnu: rofi: Update to 1.7.5.
>
> [PATCH 2/2] gnu: Add rofi-wayland.
>
> A (self-)triager should have noted two problems in that patch set
> submisison:
>
> 1. patch contains two set of unrelated changes (?)
>
> Point 12. of the "check list" in 22.6 Submitting Patches
> https://guix.gnu.org/en/manual/devel/en/html_node/Submitting-Patches.html says:
>
> --8<---------------cut here---------------start------------->8---
>
> Verify that your patch contains only one set of related changes. Bundling unrelated changes together makes reviewing harder and slower.
>
> Examples of unrelated changes include the addition of several packages, or a package update along with fixes to that package.
>
> --8<---------------cut here---------------end--------------->8---

That is a good reminder, there was at least one bug that seemed like a
collection of "music" packages, and over time more packages were added,
even after the original packages in the bug title were merged.


> Is the addition of rofi-wayland related to the upgrade of rofi?
>
> ...probably yes, but...
>
> 2. multiple patches without cover letter
>
> https://guix.gnu.org/en/manual/devel/en/html_node/Sending-a-Patch-Series.html#Multiple-Patches-1
>
> --8<---------------cut here---------------start------------->8---
>
> When sending a series of patches, it’s best to send a Git “cover letter” first, to give reviewers an overview of the patch series.
>
> --8<---------------cut here---------------end--------------->8---
>
> Missing a cover letter means that triaging is harder.

Indeed. Retitling can be used to help after the fact, at least.


> The issue title is from the first patch (gnu: rofi: Update to 1.7.5.)
> and IMO is somewhat confusing because the title is what appears in
> search results (Mumi, Debbugs, Emacs Debbugs).

I retitled several bugs as well, to at least update the current status,
as a few had some patches merged or superseded, but there were
outstanding issues not yet merged.


> If the contrubutor sent a cover letter with subject "gnu: Update rofi
> and Add rofi-wayland (inherinting)", possibly with a little bit of
> explanation in the message body, the (now undone) early triaging would
> have been easier.

Yes, cover letters would help significantly with triaging these more
complicated cases.

That said, sometimes over the course of review, it becomes clear that
additional changes are needed, and it would be helpful to retitle the
bug in these cases.

I saw at least one bug which started out as "Add XYZ" and evolved into
"Update ZXY, ... Add ABC, Add XYZ" and it would not have made sense to
make them separate patch submissions.


> How do we solve such bug management class of problems? WDYT?
>
>> One improvement I can think of here is that QA should highlight that
>> some of the changes in each of those patch series can be found in
>> another patch series.
>
> ...and tag both bugs as related on Debbugs?

Not sure how to mark related, but bugs can be marked as blocking other
bugs... this would make some sense in splitting patch series into
multiple bugs, marking blocking bugs for patches that are dependent on
others. But I suspect that would be painful in practice in many cases.


> This would be very helful for triagers, a very helping tool.
>
> ...but we need triagers, IMO
>
>> That would then make it easier to both issues to be closed if that's
>> appropriate.
>
> I guess you mean that a (human) triager can find related bugs with the
> help of such a tool.
>
> I doubt that related issues should be closed without human intervention,
> false positives are very dangerous in this case.

With old patches, honestly, it might bring attention back to an issue to
close it. When I get a bug closed notification, I definitely check to
make sure the issue is actually fixed, or did not introduce other
surprises...

I am not saying I think we should just blindly close old bugs with
patches, but processes that err slightly on the side of closing old
ones, perhaps with a message "please verify if the issue is actually
closed, and reopen in case we have made a mistake." might be reasonable.

Obviously, if an automated process can automatically rebase and it still
applies, this might be an indication that it still requires human review
and tag it as such... although occasionally a patch might propose adding
something to gnu/packages/abc.scm but actually gets merged into
gnu/packages/xyz.scm, and this might rebase "fine".


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]