Thursday 24 September 2020

Re: SRU shift report: 2020-09-23

On Wed, Sep 23, 2020 at 1:41 PM Robie Basak <robie.basak@ubuntu.com> wrote:
>
> On my SRU shift today, I'm disappointed to report that out of sixteen
> packages I looked at, I didn't end up accepting a single one. Some of
> this is because I ran out of time, but I am writing this report because
> I think that with some help from you, we can do much better.

Thank you for this email. I do understand that review is difficult, and I
also really appreciate Robie's careful approach to SRU.

I think that some better tools for SRU team members would really help
make things easier.

In looking at the change myself, I think that tools to generate a
patches/applied branch of the SRU upload would be very helpful here.
If you could have done something like:

$ git-ubuntu add-dsc --name=candidate cloud-utils-0.27-0ubuntu25.2.dsc
...
$ git diff pkg/ubuntu/xenial-devel candidate/unapplied
...
+++ b/debian/patches/series
@@ -1,3 +1,5 @@
sync-to-trunk.patch
lp-1741300-fix-qcow-nbd.patch
lp-1762748-growpart-grow-past-2TB-disks.patch
+lp-1493188-support-overlay-filesystem
+lp-1630274-mount-overlay-first

$ git show
candidate/unapplied:debian/patches/lp-1493188-support-overlay-filesystem
\
candidate/unapplied:debian/patches/lp-1630274-mount-overlay-first
| diffstat
bin/mount-image-callback | 69 ++++++++-------
cloud-utils/bin/mount-image-callback | 34 ++++++-
cloud-utils/test/test-mic | 157
+++++++++++++++++++++++++++++++++++
3 files changed, 223 insertions(+), 37 deletions(-)

That is somewhat daunting. But if you can look at the applied change
its much less so.

$ git diff pkg/applied/ubuntu/xenial-devel..candidate/applied |
filterdiff --exclude "*/debian/*" | diffstat -p1
bin/mount-image-callback | 59 +++++++++++++----
test/test-mic | 157
+++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 201 insertions(+), 15 deletions(-)

$ git diff pkg/applied/ubuntu/xenial-devel..candidate/applied
bin/mount-image-callback | pastebinit -fdiff
https://paste.ubuntu.com/p/9K4f5YNBXf/

> ## cloud-utils (Xenial)
>
> Substantial diff (non-trivial change) will take considerable time to
> review; postponed for a subsequent pass through the queue.
>
> Outcome: deferred for a second pass over the queue[2].

This change is not as big as it looks. 157 of the lines of diff are an
added test (test/test-mic) runs as part of autopackage tests in newer releases.
For better or worse, xenial will not run autopackage tests (no
debian/tests) so this
is ignorable.

That leaves us with 1 file changed, and when applied, the diff against
pkg/applied/ubuntu/xenial-devel that looks like
https://paste.ubuntu.com/p/9K4f5YNBXf/

Again, I respect the difficulty in reviewing large numbers of
very different source code changes. But I disagree that this
specific change should qualify as "Substantial diff (non-trivial change)".

I'm willing to remove the test file if that makes this more clear, but
personally I think the ease and usefullness cherry picking upstream
changes into debian/patches justifies the added cost.

--
ubuntu-devel mailing list
ubuntu-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel