Bug 30042 - patchworks should automatically notice a pushed patch
Summary: patchworks should automatically notice a pushed patch
Status: NEW
Alias: None
Product: sourceware
Classification: Unclassified
Component: Infrastructure (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: overseers mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-24 14:29 UTC by Tom Tromey
Modified: 2024-03-24 22:02 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2023-01-24 14:29:24 UTC
Consider this patch:

https://patchwork.sourceware.org/project/gdb/patch/20230122212801.1926063-1-tom@tromey.com/

I used git send-email to send this patch and patchworks dutifully
logged it.  Then I pushed the patch -- but patchworks did nothing,
I had to go into the web interface to explicitly mark it as committed.

I think patchworks ought to notice this situation.
Comment 1 Carlos O'Donell 2023-01-24 15:02:59 UTC
(In reply to Tom Tromey from comment #0)
> Consider this patch:
> 
> https://patchwork.sourceware.org/project/gdb/patch/20230122212801.1926063-1-
> tom@tromey.com/
> 
> I used git send-email to send this patch and patchworks dutifully
> logged it.  Then I pushed the patch -- but patchworks did nothing,
> I had to go into the web interface to explicitly mark it as committed.
> 
> I think patchworks ought to notice this situation.

This is easier said than done unfortunately.

(1) Out of the box feature.

This is not functionality that patchwork provides.

I would be an RFE for patchwork upstream (https://github.com/getpatchwork/patchwork/).

(2) Commit cleanup script.

Alternatively, in glibc we run a script to compare commits against patchwork patches and close those that have been committed.

The first problem we saw here was that it requires discipline from the community to commit *exactly* what was posted and agreed upon in order to make the checking less error prone.

In glibc we have adopted a stronger policy that you should only commit what the author provided, or ask the author to change it. My own opinion is that it is disrespectful to change what the author is submitting.

The only allowed changes are additions of Reviewed-by, Tested-by tags in the commit message.

In either (1) or (2) I think this is up to the projects to run the scripts that they need.
Comment 2 Siddhesh Poyarekar 2023-01-24 15:32:47 UTC
FWIW, glibc uses a variant of this script:

https://gist.github.com/siddhesh/b7be0970d8a70e5d68c36f3957ae19a4

I basically run it as a cron job.  I think this could be adapted by gdb too with the caveats that Carlos mentioned in comment 1.
Comment 3 Tom Tromey 2023-01-24 21:55:54 UTC
(In reply to Carlos O'Donell from comment #1)

> The first problem we saw here was that it requires discipline from the
> community to commit *exactly* what was posted and agreed upon in order to
> make the checking less error prone.

This would work fine for [pushed] patches, which seems good enough for
this ticket.

For other patches, this seems like more work because it means there
will have to be one final series sent, but patchworks is also bad about
removing old series.

> In either (1) or (2) I think this is up to the projects to run the scripts
> that they need.

Are you saying this shouldn't be a sourceware PR?
I can close it if so.
Comment 4 Mark Wielaard 2023-01-24 22:07:27 UTC
(In reply to Tom Tromey from comment #3)
> (In reply to Carlos O'Donell from comment #1)
> 
> > The first problem we saw here was that it requires discipline from the
> > community to commit *exactly* what was posted and agreed upon in order to
> > make the checking less error prone.
> 
> This would work fine for [pushed] patches, which seems good enough for
> this ticket.

yeah, one step at a time. Siddhesh script could be run under gdbadmin or we could do it from the buildbot since we build each gdb commit on push.

> For other patches, this seems like more work because it means there
> will have to be one final series sent, but patchworks is also bad about
> removing old series.

We could have a separate script or mail filter that checks for patches that supersede a previous patch/series (check subject is the same modulo vX+1).

> > In either (1) or (2) I think this is up to the projects to run the scripts
> > that they need.
> 
> Are you saying this shouldn't be a sourceware PR?
> I can close it if so.

We could move it to the gdb product, but lets keep it open here to document the steps we did so other projects can adopt them if they like.
Comment 5 Siddhesh Poyarekar 2023-01-25 17:22:51 UTC
(In reply to Tom Tromey from comment #3)
> (In reply to Carlos O'Donell from comment #1)
> 
> > The first problem we saw here was that it requires discipline from the
> > community to commit *exactly* what was posted and agreed upon in order to
> > make the checking less error prone.
> 
> This would work fine for [pushed] patches, which seems good enough for
> this ticket.
> 
> For other patches, this seems like more work because it means there
> will have to be one final series sent, but patchworks is also bad about
> removing old series.

Yeah I've been dragging my feet on this; I've been meaning to write a script that automatically marks outdated patches as superseded (e.g. with a matching subject line) but I never got to doing it.  If someone does, it would be great to use it for glibc too because right now we clean up manually.
Comment 6 Tom Tromey 2023-01-25 20:23:30 UTC
(In reply to Siddhesh Poyarekar from comment #5)

> Yeah I've been dragging my feet on this; I've been meaning to write a script
> that automatically marks outdated patches as superseded (e.g. with a
> matching subject line) but I never got to doing it.  If someone does, it
> would be great to use it for glibc too because right now we clean up
> manually.

For gdb it would be fine to require gerrit-like Change-Id tags.
We don't care about the noise (a bunch of patches already come with
these anyway), and at least I personally care a lot about automation.
I once tried patching patchworks for this, and I could maybe dig
up my patch, but I didn't have a good way to test it.
Comment 7 Pedro Alves 2023-01-30 14:19:12 UTC
> For gdb it would be fine to require gerrit-like Change-Id tags.

+1