homepage

tarfile.py compared to GNU tar doesn't overwrite existing symlink with directory of same name if such directory exists in the extracted tarball.

Here is the preliminary path. It works and tested on Linux. I'll check the behaviour on Windows later.

Here is the patch that works both on Windows and Linux.

Testing passed on OSX Mavericks.

Ah, thanks for the review, Serhiy. My bad. There is no underlying bug of tar. I was confused by the behaviour of tar which is converting the absolute path to relative path.
So, adding '/home/user/dir/file' to tar when you are in '/home/user/dir' then extracting it in the same place, you'll get:
'/home/user/dir/home/user/dir/file'.
I thought it was a bug. But this is what it is supposed to be. I see that tarfile module is mimicking GNU tar behaviour.
This is the updated patch.

Serhiy commented, "I think we should remove targetpath in all cases. Not only when softlink is
extracted."
I already did that in my latest patch but I am a little bit wary of this behaviour.

Yeah, you are right, Serhiy. I check the behaviour of GNU tar command line. It always replaces the target no matter what kind of file source and target are.

There are several issues with last patch.
* Fails to extract tarfiles containing the ./ directory (very common case).
* And even more common case -- fails to extract non-empty directory.

Here is the preliminary patch to address Serhiy's concern. I added some regression tests as well. Give me a time to think how to refactor the code (especially the test).

I’m not sure if this should be considered a bug fix, but if it goes into 2.7 it would overlap with Issue 10761 and Issue 12088. In 2.7 existing directory entries (including broken symlinks, but not including subdirectories) may be replaced by symbolic and hard links.

Martin: you review a patch written 4 years ago at https://bugs.python.org/review/19974/diff/11106/Lib/tarfile.py
Oh wow, I didn't know that Rietveld was still working :-D It's maybe time to convert the old patch to a proper pull request on GitHub, no? :-)

Martin Panter, thank you for reviewing my patch. Let me rework it. It has been a while (4 years!!!).

Martin Panter,
I have modernized the patch.
About your suggestion:
1. "import errno" -> Yes, this is unnecessary. I have removed it.
2. Use os.path.lexists instead of os.path.islink for broken symlink -> The thing is os.path.islink returns True also for broken symlink. I could not find a case where these methods return different result.

"I could not find a case where these methods return different result." -> This is wrong. My mistake. os.lexists returns True for non symbolic link file while os.islink returns False.

Sorry, Martin. I just understood what you suggested. I thought you were saying lexists to replace islink, but it is to replace the complex if condition. Let me work on it.

About "lexists", I meant using it instead of "os.path.exits" (not "islink"). On Linux:
>>> targetpath = 'target'
>>> os.symlink('nonexistant', dst=targetpath) # Make a broken symlink
>>> os.system('ls -l')
total 0
lrwxrwxrwx 1 vadmium vadmium 11 Jan 6 09:28 target -> nonexistant
0
>>> os.path.exists(targetpath) # Doesn't register broken symlinks
False
>>> os.path.lexists(targetpath) # Does register it
True
Did you try extracting a tar file over a broken link? (I haven’t tried your code; I’m just going on theory.)

After experimenting with lexists, I don't think I can simplify it with lexists.

I have added test case for broken symlinks. I am not sure whether this is necessary or not. But anyway I have added it.

Martin, thank you for your advice. I have added additional two test cases for broken symlink case.
1. broken symlink overwrites ordinary file,
2. ordinary file overwrites broken symlink.
I hope that is enough. Let me know if you have another concern.

Correction:
1. broken symlink overwrites valid symlink,
2. ordinary file overwrites broken symlink.