Workflow questions

[simark]

Hi,

We are currently exploring the use of Patchwork for the GDB project, to be able to better track pending patches and reduce the number of patches that fall between the cracks. We actually had a Patchwork instance (an older version) in the past, but it hasn't been very helpful to us. Because of the amount of manual work required to change patches to the appropriate state (especially to mark them as closed), the patches would just stay open forever and Patchwork couldn't really be used anymore to track which patches needed attention.

So I am wondering how people use Patchwork to track patches in projects that have a large volume of patches, and looking for advice on how to do so efficiently.

For example, can Patchwork handle multiple versions of a patch or series, automatically marking previous versions as "superseded" when a new version is sent? My brief testing has found that it doesn't, the second series is treated as a completely separate series as the first one (which I expected, I don't see how Patchwork could magically know that they are related).

I have seen that there is a git hook that allows closing patches based on git commits. That sounds nice, but I am a bit skeptical on how well that works in practice, given that the hook hashes the diff of the patch and sends that hash to the server. If the patch was modified (even a simple rebase) before it was pushed, how can the server identify the patch to close properly? I'd be curious to get some feedback from people using it "in production".

I have tried to do some testing using the patchwork-update-commits script before asking, but I couldn't get it to work so far.

On the design page of the doc, we have well noted this design goal:

Don’t pollute the project’s changelogs with Patchwork poop

A project’s changelogs are valuable - we don’t want to add Patchwork-specific metadata.

We were a bit surprised to read that, since having a tag in the patch comment (that stays the same across versions), like Gerrit does with its Change-Id, is a very effective way to uniquely identify a patch. And it's not like it makes the change log less readable. It would surely make tracking versions of patches and automatically closing merged patches much more robust.

Is the door completely closed from allowing Patchwork to identify patches using something such as a Change-Id? If a project is fine with having this information in their patches and can benefit from better patch tracking, they could use it, but it wouldn't force other projects to use it.

Thanks!

[ajdlinux]

Hi @simark!

First up - for general discussion, please use the mailing list, patchwork@lists.ozlabs.org rather than GitHub issues, we generally use this for feature/bug tracking. You'll get a much better response there, as we've got a lot of Patchwork users on that list too whereas very few people look at the bug tracker.

I write this from the perspective of someone who's mainly a contributor to patchwork-using projects, rather than a maintainer, so take my views with a shaker full of salt.

I have seen that there is a git hook that allows closing patches based on git commits. That sounds nice, but I am a bit skeptical on how well that works in practice, given that the hook hashes the diff of the patch and sends that hash to the server. If the patch was modified (even a simple rebase) before it was pushed, how can the server identify the patch to close properly? I'd be curious to get some feedback from people using it "in production".

The API is flexible so you can implement your own automation as you want - for example we have https://github.com/stewartsmith/pwnm-sync from a maintainer that really likes notmuch. IIRC, over in LinuxPPC @mpe uses some scripts that use git notes to track the IDs when he pulls patches off Patchwork, and then updates patchwork state when he pushes.

(The tiny project I maintain using Patchwork is too small for manual updating to pose a problem, so I haven't thought too hard about this myself. I haven't used the sample git hook myself.)

For example, can Patchwork handle multiple versions of a patch or series, automatically marking previous versions as "superseded" when a new version is sent? My brief testing has found that it doesn't, the second series is treated as a completely separate series as the first one (which I expected, I don't see how Patchwork could magically know that they are related).

Not as yet. @HoneyByte is working on some of these problems.

We were a bit surprised to read that, since having a tag in the patch comment (that stays the same across versions), like Gerrit does with its Change-Id, is a very effective way to uniquely identify a patch. And it's not like it makes the change log less readable. It would surely make tracking versions of patches and automatically closing merged patches much more robust.

Patchwork is primarily designed around the needs of the Linux kernel more than any other project, and if you go take a look over at workflows@vger.kernel.org you'll find that the idea of using Change-Id in the kernel has both (a) been proposed and (b) triggered some quite intense flaming. Most Patchwork contributors come from a kernel background so I think it's unlikely that this will be high priority for anyone unless the kernel community comes to a consensus that we want this.

[simark]

Hi @simark!

First up - for general discussion, please use the mailing list, patchwork@lists.ozlabs.org rather than GitHub issues, we generally use this for feature/bug tracking. You'll get a much better response there, as we've got a lot of Patchwork users on that list too whereas very few people look at the bug tracker.

Ah indeed, my bad.

Thanks for your detailed response. By saying:

The API is flexible so you can implement your own automation as you want

You made me realize that we can easily prototype something that runs outside of Patchwork, that uses the Patchwork API.

Patchwork is primarily designed around the needs of the Linux kernel more than any other project, and if you go take a look over at workflows@vger.kernel.org you'll find that the idea of using Change-Id in the kernel has both (a) been proposed and (b) triggered some quite intense flaming. Most Patchwork contributors come from a kernel background so I think it's unlikely that this will be high priority for anyone unless the kernel community comes to a consensus that we want this.

Can you point to one particular thread where this is discussed? I'm curious to read what are the arguments against it. The archives on lore don't go way back so if the discussion predates the archive, would it be possible to forward it to me (simon.marchi at polymtl dot ca)?

If something good comes out of our experimentation, I might propose something that can leverage some kind of Change-Id to the Patchwork project at some point. If some people don't want to use it (like the kernel community), they don't have it, but other projects could benefit from it.

[ajdlinux]

On Tue., 3 Dec. 2019, 08:04 Simon Marchi, ***@***.***> wrote: Hi @simark <https://github.com/simark>! First up - for general discussion, please use the mailing list, ***@***.*** rather than GitHub issues, we generally use this for feature/bug tracking. You'll get a much better response there, as we've got a lot of Patchwork users on that list too whereas very few people look at the bug tracker. Ah indeed, my bad. Thanks for your detailed response. By saying: The API is flexible so you can implement your own automation as you want You made me realize that we can easily prototype something that runs outside of Patchwork, that uses the Patchwork API. Patchwork is primarily designed around the needs of the Linux kernel more than any other project, and if you go take a look over at ***@***.*** you'll find that the idea of using Change-Id in the kernel has both (a) been proposed and (b) triggered some quite intense flaming. Most Patchwork contributors come from a kernel background so I think it's unlikely that this will be high priority for anyone unless the kernel community comes to a consensus that we want this. Can you point to one particular thread where this is discussed? I'm curious to read what are the arguments against it. The archives on lore <https://lore.kernel.org/workflows/> don't go way back so if the discussion predates the archive, would it be possible to forward it to me (simon.marchi at polymtl dot ca)?

Turns out I actually meant Ksummit-discuss. https://lore.kernel.org/ksummit-discuss/CAD=FV=UPjPpUyFTPjF-Ogzj_6LJLE4PTxMhCoCEDmH1LXSSmpQ@mail.gmail.com/ If something good comes out of our experimentation, I might propose

…

something that can leverage some kind of Change-Id to the Patchwork project at some point. If some people don't want to use it (like the kernel community), they don't have it, but other projects could benefit from it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#327?email_source=notifications&email_token=AAWELQRY5PU4GWL4JQPH753QWVZ6XA5CNFSM4JRDSVXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFU4LPY#issuecomment-560580031>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWELQRVFAZJZRE2D7LREZTQWVZ6XANCNFSM4JRDSVXA> .

[metp]

Hey @simark,

The API is flexible so you can implement your own automation as you want

You made me realize that we can easily prototype something that runs outside of Patchwork, that uses the Patchwork API.

As @ajdlinux said, that's exactly what we are trying to do right now! You can read more about this here:
https://lists.ozlabs.org/pipermail/patchwork/2019-June/005811.html
lfd/PaStA#9

Basically our approach is to find related patches by actually comparing their content, files they touch, etc.. This means we do not encourage to "pollute" any commit messages with UUIDs (change-ids) but instead make use of all the available information. This also means that we do not only find previous revisions of a patch, but also similar ones in general.
That's why we would like to introduce the concept of related submissions (patches and cover letters) in Patchwork: http://patchwork.ozlabs.org/cover/1180058/

Does our approach cover your uses-cases well?

[simark]

Our goal is to automate as much as possible the process of maintaining patches on Patchwork, so that at any given moment, the Patchwork instance gives an accurate picture of the state of patches. When I have a little bit of free time to do reviews, I'd like to be able to go on Patchwork and quickly find something I can review, without spending that time updating patch states.

Identifying the relationships between patch and patch series is a good first step that will surely be useful. But a few notes:

• In all the discussions I've had with other GDB maintainers, there was never any problem having a Change-Id of some kind in our patches, and we would be more than happy to use one in conjunction to Patchwork. I really have trouble understanding the resistance against them. If it allows to get the relationship between patch versions right 100% of the time using very dumb and easy to write tools, that's a very small price to pay (and personally I don't think it hinders my ability to read the commit log, it's at the bottom where there's already a bunch of tags). That said, having a tool like PaSta would still be useful, because not every contributor would include such Change-Id. So I think it would make sense to make use of the Change-Id if it's present, but otherwise resort to using an heuristic tool such as PaSta.
  Also, note that false positives (marking a patch as superseded by another) are probably worst than false negative (failing to mark a patch as superseded by another), because it will lead to patches that fall between the cracks.

• You mentioned that you also find related patches (so I understand, if they touch the same code, they can be considered as related), not only new versions. I think it's very interesting, but the distinction between a "related" patch and a "previous version" of the same patch would have to be clear. When we find that a patch is a new version of another, we would like to automatically change the state of the previous one to "superseded". We would not want to do that for two patches that look alike but are otherwise unrelated.

• We would like to also mark patches as "merged" the patch is pushed to the master branch of the upstream repository. Would your tool be able to process git commits (maybe using git-format-patch), to then be able to find and mark patches as merged on Patchwork?

[stephenfin]

• In all the discussions I've had with other GDB maintainers, there was never any problem having a Change-Id of some kind in our patches, and we would be more than happy to use one in conjunction to Patchwork. I really have trouble understanding the resistance against them. If it allows to get the relationship between patch versions right 100% of the time using very dumb and easy to write tools, that's a very small price to pay (and personally I don't think it hinders my ability to read the commit log, it's at the bottom where there's already a bunch of tags). That said, having a tool like PaSta would still be useful, because not every contributor would include such Change-Id. So I think it would make sense to make use of the Change-Id if it's present, but otherwise resort to using an heuristic tool such as PaSta.
  Also, note that false positives (marking a patch as superseded by another) are probably worst than false negative (failing to mark a patch as superseded by another), because it will lead to patches that fall between the cracks.

Just to chime in on this one point. FWIW, I have no objection to supporting the Change-ID trailer/footer and think it's a good idea for the reasons you outline above. I would happily support it in Patchwork, so long as it wasn't mandatory, and I don't see any issues we couldn't add support for the auto-installation of the hook to 'git-pw' like 'git-review' does. The only question I do have is who's going to do this work? 😄

[bulwahn]

I think the infrastructure of related patches as model and view in patchwork is quite simple and generic; it does assume or require any specific metadata in the patches, but rather assumes that you would implement any algorithm you like to determine the related patches and then update the relation with the REST API. That would allow you to follow incoming patches, parse for the identifier of your choice, and then update the model as you like, all with a project-specific script. So, now we just need to get Mete's extension (model and view) merged and then, you simply need to write your own 'backend' script.