[dfvfs] Add support for TAR files with missing directory entries. #148

[dfvfs] Add support for TAR files with missing directory entries. #148

[Joachim Metz, 2016-07-04 04:43:11]

Overall look OK, a couple of nits. I have one concern in terms of file
system/entry separation I need to look closer into.

https://codereview.appspot.com/303920043/diff/1/dfvfs/file_io/tar_file_io.py
File dfvfs/file_io/tar_file_io.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/file_io/tar_file_io.py#...
dfvfs/file_io/tar_file_io.py:62: raise IOError(u'Attempted to open directory.')
Attempted => Unable

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py
File dfvfs/vfs/tar_file_entry.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:33: # Add ending path separator.
ending => trailing

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:35: location += self._file_system.PATH_SEPARATOR
Use format instead of +

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:195: path = self.path_spec.location
Use: location = getattr(self.path_spec, u'location', None)

[Joachim Metz, 2016-07-04 04:48:50]

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py
File dfvfs/vfs/tar_file_entry.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:211: yield
self._file_system.GetFileEntryByPathSpec(path_spec)
Note to self, double check if these changes don't have unwanted side effects.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_system.py
File dfvfs/vfs/tar_file_system.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_system.py#...
dfvfs/vfs/tar_file_system.py:107: else:
what is this else doing here?

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_system.py#...
dfvfs/vfs/tar_file_system.py:134: else:
what is this else doing here?

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py
File tests/vfs/tar_file_entry.py (right):

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py#n...
tests/vfs/tar_file_entry.py:22: file_entry (FileEntry): a file entry.
You can drop the "a" in "a file entry"

Was recently changed: https://github.com/log2timeline/plaso/wiki/Style-guide

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py#n...
tests/vfs/tar_file_entry.py:23: sub_file_entry_names ([string]): a list of sub
file entry names.
string => list[str]

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py#n...
tests/vfs/tar_file_entry.py:23: sub_file_entry_names ([string]): a list of sub
file entry names.
- "a list of"

[dc3.plaso, 2016-07-05 19:34:56]

https://codereview.appspot.com/303920043/diff/1/dfvfs/file_io/tar_file_io.py
File dfvfs/file_io/tar_file_io.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/file_io/tar_file_io.py#...
dfvfs/file_io/tar_file_io.py:62: raise IOError(u'Attempted to open directory.')
On 2016/07/04 04:43:11, Joachim Metz wrote:
> Attempted => Unable

Done.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py
File dfvfs/vfs/tar_file_entry.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:33: # Add ending path separator.
On 2016/07/04 04:43:11, Joachim Metz wrote:
> ending => trailing

Done.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:35: location += self._file_system.PATH_SEPARATOR
On 2016/07/04 04:43:11, Joachim Metz wrote:
> Use format instead of +

Done.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:195: path = self.path_spec.location
On 2016/07/04 04:43:11, Joachim Metz wrote:
> Use: location = getattr(self.path_spec, u'location', None)

Done.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_entry.py#n...
dfvfs/vfs/tar_file_entry.py:211: yield
self._file_system.GetFileEntryByPathSpec(path_spec)
On 2016/07/04 04:48:50, Joachim Metz wrote:
> Note to self, double check if these changes don't have unwanted side effects.

I had to switch it to this because of the added logic necessary to correctly
initialize the TARFileEntry.

The other file entry implementations also tend to create the entries manually
instead of using the helper function. This can cause issues if certain
implementations need to do more than just initialize.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_system.py
File dfvfs/vfs/tar_file_system.py (right):

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_system.py#...
dfvfs/vfs/tar_file_system.py:107: else:
On 2016/07/04 04:48:50, Joachim Metz wrote:
> what is this else doing here?

You can add an else in a try except which will get run if the exception was not
raised. 

In this situation, it is not necessary because we are returning right away, so
I'll remove it.

https://codereview.appspot.com/303920043/diff/1/dfvfs/vfs/tar_file_system.py#...
dfvfs/vfs/tar_file_system.py:134: else:
On 2016/07/04 04:48:50, Joachim Metz wrote:
> what is this else doing here?

See above comment.

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py
File tests/vfs/tar_file_entry.py (right):

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py#n...
tests/vfs/tar_file_entry.py:22: file_entry (FileEntry): a file entry.
On 2016/07/04 04:48:50, Joachim Metz wrote:
> You can drop the "a" in "a file entry"
> 
> Was recently changed: https://github.com/log2timeline/plaso/wiki/Style-guide

Done.

https://codereview.appspot.com/303920043/diff/1/tests/vfs/tar_file_entry.py#n...
tests/vfs/tar_file_entry.py:23: sub_file_entry_names ([string]): a list of sub
file entry names.
On 2016/07/04 04:48:50, Joachim Metz wrote:
> string => list[str]

Done.

[dc3.plaso, 2016-07-05 19:41:15]

Code updated.

[Joachim Metz, 2016-07-06 06:42:43]

A short reply for now, still need to evaluate the latest changes and the note to
self.

https://codereview.appspot.com/303920043/diff/20001/dfvfs/vfs/tar_file_system.py
File dfvfs/vfs/tar_file_system.py (right):

https://codereview.appspot.com/303920043/diff/20001/dfvfs/vfs/tar_file_system...
dfvfs/vfs/tar_file_system.py:99: try:
I see try-except-else is one of those Python code constructs where I doubt it
improves readability. 

I opt to keep the try statement compact (if possible):

try:
  self._tar_file.getmember(location[1:])
  return True
except KeyError:
  pass

https://codereview.appspot.com/303920043/diff/20001/dfvfs/vfs/tar_file_system...
dfvfs/vfs/tar_file_system.py:130: try:
I opt:

kwargs = {}

try:
  kwargs[u'tar_info'] = self._tar_file.getmember(location[1:])
except KeyError:
  kwargs[u'is_virtual'] = True
  
return dfvfs.vfs.tar_file_entry.TARFileEntry(
   self._resolver_context, self, path_spec, **kwargs)

[dc3.plaso, 2016-07-06 12:51:54]

Code updated.

[dc3.plaso, 2016-07-06 12:52:03]

https://codereview.appspot.com/303920043/diff/20001/dfvfs/vfs/tar_file_system.py
File dfvfs/vfs/tar_file_system.py (right):

https://codereview.appspot.com/303920043/diff/20001/dfvfs/vfs/tar_file_system...
dfvfs/vfs/tar_file_system.py:99: try:
On 2016/07/06 06:42:43, Joachim Metz wrote:
> I see try-except-else is one of those Python code constructs where I doubt it
> improves readability. 
> 

In Python, using exceptions for flow control is a common and normal thing to do.
Therefore, the else statement makes perfect sense.


> I opt to keep the try statement compact (if possible):
> 
> try:
>   self._tar_file.getmember(location[1:])
>   return True
> except KeyError:
>   pass

I would argue that in this situation, that would make it less readable. It is
not as clear that the code below it only gets run if the KeyError exception gets
raised.

https://codereview.appspot.com/303920043/diff/20001/dfvfs/vfs/tar_file_system...
dfvfs/vfs/tar_file_system.py:130: try:
On 2016/07/06 06:42:43, Joachim Metz wrote:
> I opt:
> 
> kwargs = {}
> 
> try:
>   kwargs[u'tar_info'] = self._tar_file.getmember(location[1:])
> except KeyError:
>   kwargs[u'is_virtual'] = True
>   
> return dfvfs.vfs.tar_file_entry.TARFileEntry(
>    self._resolver_context, self, path_spec, **kwargs)

Agreed

[Joachim Metz, 2016-07-15 06:56:31]

I've manually tweaked this CL and merged the changes in. 

https://github.com/log2timeline/dfvfs/commits/master

You can close this now.

Sorry for the delay on my side.

https://codereview.appspot.com/303920043/diff/40001/dfvfs/file_io/tar_file_io.py
File dfvfs/file_io/tar_file_io.py (right):

https://codereview.appspot.com/303920043/diff/40001/dfvfs/file_io/tar_file_io...
dfvfs/file_io/tar_file_io.py:60: if file_entry.IsDirectory():
I changed this to: if not file_entry.IsFile():

https://codereview.appspot.com/303920043/diff/40001/dfvfs/vfs/tar_file_entry.py
File dfvfs/vfs/tar_file_entry.py (right):

https://codereview.appspot.com/303920043/diff/40001/dfvfs/vfs/tar_file_entry....
dfvfs/vfs/tar_file_entry.py:33: # Add trailing path separator.
This seems not to be needed.

https://codereview.appspot.com/303920043/diff/40001/dfvfs/vfs/tar_file_entry....
dfvfs/vfs/tar_file_entry.py:212: yield
self._file_system.GetFileEntryByPathSpec(path_spec)
I've changes this a bit to have less overhead.