[PATCH v5 0/7] Add series support

[Stephen Finucane]

On 2016-10-11 09:14, Stephen Finucane wrote:
> On 2016-10-11 00:51, Russell Currey wrote:

[snip]

>> - no easy way to download a mbox file of all patches in a series, 
>> which would be
>> very useful
> 
> Yup, this is an easy follow up.
> 
>> - no series in the patch API at all.  This could probably be 
>> implemented a few
>> different ways, but having no series at all is limiting
> 
> I think Andrew was working on this so I halted my own efforts here.
> Again, this is a separate series though.

[snip]

>> - some indication that not all patches in the series had been 
>> received, i.e.
>> patch 7/7 arrived but there are only 6 patches
> 
> We have this in the admin interface alright, but presenting this in
> some real-time manner to the user will require tracking of events. I'd
> been hoping to integrate an '/events' API using something like
> 'django-activity-stream' [1]. This would signal events for things like
> "patch has all dependencies met" or "patch state has been changed" via
> the API. Once again, though, this seems very much like a separate,
> follow-up series.

So I've given the three points above a little more thought and decided 
it might be reiterating my ideas for how I feel series should function 
before proceeding.

One of the key differences between how I've approached this and how 
dlespiau approached it was avoiding his idea of series being "the main 
object tracked" [1]. I really don't agree with this approach. In my 
eyes, patches are the one, true thing we should care about and series 
are simply a way of indicating relationships between said patches [*]. 
This is why you'll note that there's no new "Series" tab in the UX and 
it also means that we shouldn't be adding a '/series' endpoint in the 
API, but rather modifying '/patches' like so:


   GET /patches/?series=1
   [
     "https://example.com/api/v1/patches/1",
     "https://example.com/api/v1/patches/3",
     "https://example.com/api/v1/patches/4"
   ]

   GET /patches/4/
   {
     "url": "https://example.com/api/v1/patches/4",
     ...
     "dependencies": [
       "https://example.com/api/v1/patches/1",
       null,
       "https://example.com/api/v1/patches/3"
     ],
     "series": "https://example.com/api/v1/patches?series=1",
     "series_revisions": [
       "https://example.com/api/v1/patches?series=2"
     ]
   }

This means my answers to the above three points are actually subtly 
different to what I said earlier:

* no easy way to download a mbox file of all patches in a series, which 
would be very useful

   We need to expose a method to download any patch with its dependencies 
in a mbox file - not the entire series. Something like so?

     GET /patches/4/mbox?include_dependencies=true

   This could return an error code if dependencies were missing, I guess? 
Maybe:

    GET /patches/4/mbox?include_dependencies=true&skip_broken=true

* no series in the patch API at all. This could probably be implemented 
a few different ways, but having no series at all is limiting

   See above. This definitely needs to happen and I'd love help 
(Daniel?), but it should all happen in '/patches'

* some indication that not all patches in the series had been received, 
i.e. patch 7/7 arrived but there are only 6 patches

   For the dependencies field of '/patch', we should return 'null' for 
any missing patches (as seen above). If anything is null, series != 
complete. The '/events' API I previously described would still be 
helpful to prevent polling though.

I've given this a lot of thought and am more than happy to defend my 
position, but there's surely stuff I've missed? Whatever the case, let 
me know what you think and how you would like to proceed here :)

Cheers,
Stephen

[*] There's a few reasons for this, but it ultimately boils down to a 
series of "-ability" words: "bisectability", "testability", and 
"backportability", to name a few. Coming from a testing background, I 
strongly believe in testing each and every patch independently before 
merging. In addition, I think it should be possible to download patch N 
or M without having to download N+1 to M at the same time. FWIW, GitHub 
follow a similar methodology of hiding the patches in favour of exposing 
a simple "pull request", and this exact model has incurred the wrath of 
Linus Torvalds at least once [2] :)

[1] 
http://damien.lespiau.name/2016/02/augmenting-mailing-lists-with-patchwork.html
[2] https://www.wired.com/2012/05/torvalds_github/