Add support for applying dependent patches when applying a patch

[loudonlune]

The DPDK project has defined syntax for a "Depends-on" tag. A contributor can add this label to their patch or cover letter if it depends on another patch or series that has not yet been applied to the target branch.

The syntax for this tag and where it appears in a patch file is as follows:

... MIME headers and such go here ...

Some commit message

Signed-off-by: someone
---
Depends-on: patch-##### ("title of patch")
Depends-on: series-##### ("title of series")

... git diff summary goes here ...

---
... diff goes here ...

There can be multiple of these tags defined with differing values. An implementation is intended to collect them into a list. When applying a patch which has these tags, they should be applied in the order listed in the patch file. This process is not recursive, as in if the dependents of a patch have additional dependents, those are not applied.

The ask is for, when running git pw patch apply <id> or git pw series apply <id>, the command will scan for these dependent patches or series and apply them automatically as described above.

Would this functionality be useful to other projects, and appropriate to add to git-pw?

[stephenfin]

I'm okay with implementing this client side but my preference would be to expose the via Patchwork server. That way we can expose via the API and support in other clients (like pwclient) also. It should be as simple as:

class Patch(SubmissionMixin):
    ...
    dependencies = models.ManyToManyField(
        "self",
        symmetrical=False,
        related_name="dependents",
        related_query_name="dependent",
    )

class Series(FilenameMixin, models.Model):
    ...
    dependencies = models.ManyToManyField(
        "self",
        symmetrical=False,
        related_name="dependents",
        related_query_name="dependent",
    )
    ...

plus the relevant parser and API changes. The parser would need to parse those Depends-on: markers (hint: I would suggest allowing use of Patchwork URLs also) and populated patch.dependencies or series.dependencies as required.

wdyt?

[loudonlune]

Yes, that approach sounds good to me and is definitely better than just implementing it in git-pw.

[loudonlune]

For next steps, should I create another issue on the main patchwork repository for these changes?
Would you like help from the DPDK community with this effort? We're willing to help with the work if needed.

Another comment I have about the approach above, how would we handle a series which is dependent on a patch, or vice versa? It seems like that solution would only handle a series being dependent on another series, or a patch being dependent on another patch.

[stephenfin]

For next steps, should I create another issue on the main patchwork repository for these changes?

Yes, that would be best.

Would you like help from the DPDK community with this effort? We're willing to help with the work if needed.

Absolutely. Patchwork has long been a spare-time activity for me (none of the projects I work on currently use a mailing list-driven development process) so the more I can leave to others the better for me/project throughput 😄

Another comment I have about the approach above, how would we handle a series which is dependent on a patch, or vice versa? It seems like that solution would only handle a series being dependent on another series, or a patch being dependent on another patch.

I've been thinking over this and have come to the conclusion that neither patch-series dependencies nor patch-patch dependencies are a good idea. Consider things from an end-user/UX perspective. Currently, if we run git pw patch apply, it will default to applying all preceding patches (the "dependencies") from the series. What will happen now if that patch has a dependency (an additional dependency), either on another patch or on another series? What gets applied first? What about if it has multiple dependencies, or even if different patches from the same series depend on different patches and series. It would be perfectly possible for e.g. patch [2/n] in series X depended on patch [1/M] from series Y while patch [1/n] of series X depended on patch [2/M] from series Y and we would need to resolve this. I suspect we're opening ourselves up to dependency hell and could easily envision problems like diamond dependencies conflicts popping up relatively quickly. I really don't want to write a dependency resolver 😄

fwiw I've also looked around and I don't think this is a problem that's really been solved elsewhere. The two examples of similar functionality that jump to mind are Gerrit and Mergify but both of them only allow mapping of the highest level "container" to other instances of same. Gerrit is focused entirely on individual commits and doesn't really have the concept of series, so you track dependencies by simply rebasing one or more commits on top of another commit [*]. Mergify, by comparison, only seems to care about Pull Requests (which are functionally similar to Series) and therefore you can only specify Depends-On trailers in PR messages ("cover letters").

Based on the above and considering that series are a thing and we already have the concept of patch dependencies (on preceding patches from a patch's series), I suspect it would be much easier if we only allowed allow series to depend on other series (that is, if we only respected Depends-On trailers in cover letters). I appreciate that we'd lose some granularity here, but it should be trivial for patch authors to structure series in such a manner that you have multiple smaller series rather than one giant series, allowing other authors to point to one of these smaller series.

[*] OpenDev Gerrit (i.e. what's used for OpenStack development), or more specifically Zuul, extends this slightly by supporting a Depends-On commit message trailer, which allows you to track dependencies across projects, but once again the value of that trailer resolves to a single commit/change rather than a "series".

[stephenfin]

btw, we could still infer series dependencies when applying patches. For example, if series Y depended on series X, then I would expect git pw patch apply <patch-from-series-y> to apply all patches from series X first. I don't know how likely a use case this is though.

[tmonjalo]

It may be interesting to keep the per-patch granularity for reviewers.
From an automation point of view, I agree it is enough to pull the whole series we depends on.
Can we keep the per-patch syntax and translate it into a series dependency?

[stephenfin]

It may be interesting to keep the per-patch granularity for reviewers. From an automation point of view, I agree it is enough to pull the whole series we depends on. Can we keep the per-patch syntax and translate it into a series dependency?

So to be clear, if patch 1686360 states:

Depends-on: https://patchwork.ozlabs.org/project/patchwork/series/259312

Can we infer every patch in the same series as patch 1686360 has the same dependency?

What about if patch 1686360 states:

Depends-on: https://patchwork.ozlabs.org/project/patchwork/patch/1519911

In this case, can we infer we depend on the series patch 1519911 depends on, rather than that patch specifically?

(note: we don't have to use that trailer syntax specifically - I'm more focused on how we form the associations)

[loudonlune]

So to be clear, if patch 1686360 states:

Depends-on: https://patchwork.ozlabs.org/project/patchwork/series/259312

Can we infer every patch in the same series as patch 1686360 has the same dependency?

Yes. If a single patch x in some series X references another series Y, then that should be interpreted as X depends on Y.
Then, all of Y will be applied before all of X.

What about if patch 1686360 states:

Depends-on: https://patchwork.ozlabs.org/project/patchwork/patch/1519911

In this case, can we infer we depend on the series patch 1519911 depends on, rather than that patch specifically?

We do the same thing here.
In a patch x in some series X, it declares a dependency on patch y which is a member of series Y.
In this case, we infer again that series X depends on series Y.

This way we can allow dependencies to be declared in any of the commit messages, while keeping the complexity of the dependency relations low. I think the reasoning behind this is so that developers can make it clear that, for example, code contained in patch 3/4 in a series is what depends on another series.

[stephenfin]

@loudonlune Okay, that makes sense to me and I've no issues with that. In effect, dependencies will still be between series (and only between series) but we can specify a dependency on a series from a patch in another series. We can also reference a patch from a series in place of the series itself. Both seem reasonable.

[stephenfin]

@loudonlune Could you create a new issue at https://github.com/getpatchwork/patchwork/issues and close this one out, since the work needs to happen there now. I'm happy to review patches if you have someone to work on the feature.

[loudonlune]

Sure thing. Here is the new issue: getpatchwork/patchwork#583