<div dir="ltr">Hi Chris,<div><br></div><div>Thanks for the swift feedback, and for sharing. I have issued a merge request which can be found <a href="https://salsa.debian.org/reproducible-builds/diffoscope/merge_requests/10">here</a>. Have addressed your comments, including narrower exception, accompanied by a comment, and one test case trying to compare 2 encrypted zipfiles.</div><div><br></div><div><i><span style="color:rgb(33,33,33)">Isn't this a bit too "wide" an exception class to catch? How narrow can</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">we safely make it?</span><br></i></div><div><span style="color:rgb(33,33,33)"><br></span></div><div><span style="color:rgb(33,33,33)">You are right that is too wide, the narrowest I think we can go is catching a RuntimeError (which is not that narrow IMO) but that is what is thrown in this case by the <a href="https://github.com/python/cpython/blob/a2fe1e52eb94c41d9ebce1ab284180d7b1faa2a4/Lib/zipfile.py#L1495">open() method in zipfile.py</a></span></div><div><br></div><div>If we want to be more explicit we could just explicitly check if the archive is encrypted and raise a diffoscope exception accordingly:</div><div><br></div><div><div>+ try:</div><div>+ # Wrapped in a try block as exception may be raised due to archive </div><div>+ # being encrypted, already closed, or opened incorrectly see library </div><div>+ # zipfile.py line 1292</div><div>+ with self.archive.open(member_name) as source, open(targetpath, 'wb') as target:</div><div>+ shutil.copyfileobj(source, target)</div><div>+ return targetpath.decode(sys.getfilesystemencoding())</div><div>+ except RuntimeError as exc:</div><div>+ raise ContainerExtractionError(member_name, exc)</div></div><div><br></div><div>Not sure what is the best approach here. </div><div><br></div><div>We can move this discussion over to the MR if that suits better. Just let me know.</div><div><br></div><div>Thanks again,</div><div>Ricardo</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Sun, Aug 5, 2018 at 4:00 AM Chris Lamb <<a href="mailto:lamby@debian.org">lamby@debian.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">tags 904685 + patch<br>
thanks<br>
<br>
Dear Ricardo,<br>
<br>
> Based on this bug, please find attached a proposed patch for handling<br>
> this error gracefully [..]<br>
<br>
Wow, thank you so much for this :)<br>
<br>
> Additionally, I see that I could have also just submitted a merge request<br>
> via <a href="http://salsa.debian.org" rel="noreferrer" target="_blank">salsa.debian.org</a>. What is the usual workflow, email patches or merge<br>
> requests?<br>
<br>
It might depend more on the size of the patch and whether you were<br>
thinking of making more in the future. Using salsa also has the<br>
advantages of running the testsuite too, which may be useful here (see<br>
below). Please do join our group on salsa ...<br>
<br>
> I would gladly appreciate some feedback. I tried to update the changelog as<br>
> best as I understood here<br>
> <<a href="https://reproducible-builds.org/contribute/#Fixing_issues" rel="noreferrer" target="_blank">https://reproducible-builds.org/contribute/#Fixing_issues</a>>.<br>
<br>
Sure thing.<br>
<br>
So, the debian/changelog entries for diffoscope are generated<br>
automatically upon release so updating the changelog is not required.<br>
In your commit message please suffix with "(Closes: #904685)" so the<br>
versions/state is handled properly though.<br>
<br>
(Furthermore, the "~reproducibleX" suffix as outlined on that page is<br>
typically used when we fork packages from /elsewhere/ in Debian and, as<br>
this project is part of the reproducible builds effort, there is no<br>
naturally need to fork..)<br>
<br>
<br>
targetpath = os.path.join(dest_dir, os.path.basename(member_name)).encode(<br>
sys.getfilesystemencoding(), errors='replace')<br>
- with self.archive.open(member_name) as source, open(targetpath, 'wb') as target:<br>
- shutil.copyfileobj(source, target)<br>
- return targetpath.decode(sys.getfilesystemencoding())<br>
+ try:<br>
+ with self.archive.open(member_name) as source, open(targetpath, 'wb') as target:<br>
+ shutil.copyfileobj(source, target)<br>
+ return targetpath.decode(sys.getfilesystemencoding())<br>
+ except Exception as exc:<br>
^^^^^^^^^<br>
<br>
Isn't this a bit too "wide" an exception class to catch? How narrow can<br>
we safely make it?<br>
<br>
+ raise ContainerExtractionError(member_name, exc)<br>
<br>
I think I would also like to see:<br>
<br>
* A comment in the except block explaining why we might be seeing an<br>
exception in the first place.<br>
<br>
* A test :)<br>
<br>
<br>
Best wishes,<br>
<br>
-- <br>
,''`.<br>
: :' : Chris Lamb<br>
`. `'` <a href="mailto:lamby@debian.org" target="_blank">lamby@debian.org</a> / <a href="http://chris-lamb.co.uk" rel="noreferrer" target="_blank">chris-lamb.co.uk</a><br>
`-<br>
</blockquote></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Regards,<div>Ricardo Gaviria</div><div>Software Engineer, UniteLabs<br></div><div><div><b>M: </b>+41 77 956 2376</div><div><b>W: </b><a href="http://unitelabs.ch">http://unitelabs.ch</a></div><div><b>In: </b><a href="https://www.linkedin.com/in/ricardogaviria/">https://www.linkedin.com/in/ricardogaviria/</a></div><br class="inbox-inbox-inbox-inbox-inbox-inbox-inbox-inbox-inbox-inbox-Apple-interchange-newline"></div></div></div>