This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Misc tarfile fixes
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.3, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, lars.gustaebel, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-02-14 16:10 by eric.araujo, last changed 2022-04-11 14:57 by admin.

Files
File name Uploaded Description Edit
tarfile-misc-bugs-3.2.diff eric.araujo, 2012-02-14 16:10
tarfile-misc-bugs-3.2-2.diff lars.gustaebel, 2012-02-22 14:26 review
tarfile-misc-bugs-3.2-3.diff eric.araujo, 2012-02-23 01:08
tarfile-misc-bugs-3.4.diff serhiy.storchaka, 2013-08-04 08:16 review
Messages (11)
msg153348 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-14 16:10
I found a few possible bugs in tarfile:

- “mode in 'raw'” can give false positives for '' or 'ra'.  Most of the code also checks for “len(mode) > 1”, but I find clearer and safer to just use “mode in ('r', 'a', 'w')”.

- To use the shadowed builtin “open”, tarfile uses both “import as” and a local alias “bltin_open = open”.  However, the second idiom would break if tarfile were reloaded.

- Error messages don’t say what the invalid mode was.  (Error messages are not part of the language, but nonetheless maybe it’s best not to commit that to stable branches.)
msg153734 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-19 23:39
Another one: now that shutil provides archiving operations, there is a circular dependency between tarfile and shutil.  It does not cause problems*, as both modules use qualified names, but it may be a good thing to avoid import cascades for performance reasons.  The single shutil function used by tarfile could be inlined, as I did in distutils2, or you may reject this idea.

* Except in distutils2’s backport, see http://hg.python.org/distutils2/rev/c1b1b537196d
msg153954 - (view) Author: Lars Gustäbel (lars.gustaebel) * (Python committer) Date: 2012-02-22 14:26
I updated your patch:

- I removed the "import as" bit completely and changed all occurrences of _open() to builtins.open() which is more readable and explanatory.

- I object to changing the error messages in the 3.2 branch due to backwards compatibility, although I left them in the patch for now. (I changed the style of %-formatting with a single item tuple in order to match the coding style of the rest of the module.)

- I inlined the shutil.copyfileobj() method to remove the shutil import.
msg154026 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-23 01:08
> I removed the "import as" bit completely and changed all occurrences of _open() to
> builtins.open() which is more readable and explanatory.
Truly.  tokenize got a similar fix in ea260d393cde (without a test, so I think that here we don’t need one either); locale has a similar bug.

> I object to changing the error messages in the 3.2 branch due to backwards compatibility,
> although I left them in the patch for now.
I removed these changes in the attached patch.  I’ll make another patch for 3.3 for that.

> (I changed the style of %-formatting with a single item tuple in order to match the coding
> style of the rest of the module.)
My reason was not style (I hate %-formatting with single-element tuples) but defensive coding, in case someone gives a tuple as argument.  OTOH, that will just change the type of error they get for the same line, and the doc clearly says what is allowed, so it does not matter.

> I inlined the shutil.copyfileobj() method to remove the shutil import.
Great, this will eliminate a circular dependency I had in a shutil refactoring (I need to access tarfile.compression_formats, but the tarfile module is not ready when shutil gets imported), and also reduce the diff with the tarfile backport we have in distutils2.

Tell me if you want me to commit.
msg181514 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-06 12:56
The patch is desynchronized from current sources.
msg186739 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-13 16:06
Éric, can you please update your patch?
msg186963 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2013-04-15 00:55
I should be able to do that but can’t say when.
msg194333 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-04 08:16
Here is updated for 3.4 patch.
msg195076 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2013-08-13 14:54
Thanks, LGTM.
msg195127 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-14 07:37
Lets push it. Lars?
msg228938 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-10 01:28
The patch no longer applies again.
History
Date User Action Args
2022-04-11 14:57:26adminsetgithub: 58220
2014-10-10 01:28:10r.david.murraysetnosy: + r.david.murray
messages: + msg228938

assignee: lars.gustaebel ->
stage: commit review -> needs patch
2013-08-14 07:37:42serhiy.storchakasetmessages: + msg195127
2013-08-13 14:54:47eric.araujosetmessages: + msg195076
2013-08-04 08:16:37serhiy.storchakasetfiles: + tarfile-misc-bugs-3.4.diff

messages: + msg194333
2013-04-15 00:55:26eric.araujosetmessages: + msg186963
versions: - Python 3.2
2013-04-13 16:06:42serhiy.storchakasetmessages: + msg186739
2013-02-06 12:56:29serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg181514
versions: + Python 3.4
2012-02-23 01:08:24eric.araujosetfiles: + tarfile-misc-bugs-3.2-3.diff

messages: + msg154026
2012-02-22 14:26:03lars.gustaebelsetfiles: + tarfile-misc-bugs-3.2-2.diff

messages: + msg153954
2012-02-19 23:39:33eric.araujosetmessages: + msg153734
2012-02-18 16:55:20lars.gustaebelsetassignee: lars.gustaebel
2012-02-14 16:10:36eric.araujocreate