Code review comment for ~dviererbe/ubuntu/+source/unzip:fix-missing-documentation-and-add-autopkgtest

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

My first reaction was "have you submitted to Debian" but this is in fact about a Ubuntu-only delta.

Next I wanted to criticize how much whitespace damage this patch had, but then it is probably as taken from arch-linux. While we are changing it I wonder if we should not:
- find where it is from and mention it as origin if there is any link
- fix the whitespace damage (mostly tailing whitespace, mixed space/tab)
- combined with your fixes submit it back to archlinux

Looking at that I found that it is actually from https://src.fedoraproject.org/rpms/unzip/raw/rawhide/f/unzip-6.0-alt-iconv-utf8.patch

So - if nothing else - that should be mentioned in the dep-3 header.

But I've seen and appreciate that the last change (Updated 2015-02-11 by Marc Deslauriers <email address hidden> to fix buffer overflow in charset_to_intern()) was submitted there.

So I guess fedora is "the upstream" of that delta. To ease long term maintenance I think you should check if that evolved since then and submit your changes there - to then update both and in the future be able to pull their changes as well.

For example there is a follow on patch
https://src.fedoraproject.org/rpms/unzip/raw/rawhide/f/unzip-6.0-alt-iconv-utf8-print.patch

Also the signature changed massively
$ diffstat debian/patches/20-unzip60-alt-iconv-utf8.patch
 unix/unix.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 unix/unxcfg.h | 26 ++++++++++++++++++++++++++
 unzip.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 unzpriv.h | 2 +-
 zipinfo.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 279 insertions(+), 1 deletion(-)

vs
$ diffstat ~/unzip-6.0-alt-iconv-utf8-print.patch
 extract.c | 289 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------
 unzpriv.h | 7 ++++
 2 files changed, 233 insertions(+), 63 deletions(-)

I think this is worth a deeper revisit to patch it once and patch it right - unless you disagree.

review: Needs Information

« Back to merge proposal