[Scummvm-devel] Archive subclass inconsistencies (Was: [Scummvm-cvs-logs] SF.net SVN: scummvm:[52120] scummvm/trunk/common/fs.cpp (and r52121))

[Eugene Sandulenko]

On Mon, 16 Aug 2010 23:10:09 +0200
Max Horn <max at quendi.de> wrote:
> This is a violation of the Archive specification, i.e., a bug in
> ZipArchive. We specifically and on purpose designed Archive so as to
> not handle directories
Okay, I can live with that, since I can simulate what I need having
full member list.

> Actually, the Archive docs are not that clear right now :-(. We
> really need to write proper, good and complete documentation &
> specification for them. And then make sure the code conforms to this
> (and add test cases for this, either as unit tests or as part of the
> "testbed" engine).
Indeed.

> So ZipArchive needs fixing
Yes, it has to remove directories from the list. And probably
ArjArchive also has same misfeature.

> The directory entries are wrong and must go. What has to be debated
> now is which of these is supposed to be "correct": 1)
> m/foo/text.txt,  m/bar/text.txt, baz/1.dat, baz/2.dat 2)  text.txt,
> text.txt, 1.dat, 2.dat
> 
> In the original Archive design, the "flat" list in 2 would be the
> definite correct answer.
No, that's wrong. flat does not allow files with same name coexist in
different directories, and a warning is produced during load in such
cases. The reason is that the file name is used for hashmap key and
there is a clash occurs.

> Side note: Having multiple members with identical names *is*
> explicitly allowed because, well, it simply can happen in reality for
> many good (and some bad) reasons. Think of SearchSet, which can merge
> multiple Archives together. So this is a conscious design decision.
> When using Archive::getMember or Archive::createReadStreamForMember,
> the first match (ignoring case!), if any, is used. But by iterating
> over all members, one can find even the "hidden" duplicate entries.
If you're talking about flattered hierarchies, then it is not what is
implemented. SearchSet contains several Archives which individually can
have overlapping files, but they are used with definite priority, and
no single Archive can contain several files with same name (see above).
If it is a case, non-flat hierarchy has to be used.

> > But FSDirectory archive gives somewhat inconsistent results, and
> > moreover, they're weird for the first call and completely wrong for
> > the second call:
> > 
> >  listMembers() -> [ text.txt, text.txt, 1.dat, 2.dat] (4 elements)
> > 
> >  listMatchingMembers(m/*) -> [text.txt, text.txt] (2 elements)
> > 
> >  listMatchingMembers(baz/*.dat) -> [1.dat, 2.dat]
> 
> These look right for me if we assume a "flat" name hierarchy. But not
> if we use a non-flat hierarchy, then it would be wrong; and that is
> the default, isn't it?
That's the point. These are results of non-flat hierarchy and are wrong.

> Lastly, we need to discuss the supposed equality
> archive.getMember(name).getName() == name. Right now, FSDir violates
> this. We can fix that by changing FSDir to use GenericArchiveMember.
> Or, we can change the spec by saying that this equality may *not*
> hold
It has to be fixed, because currently the logic is flawed.

Also speaking of FSes which do not support subdirectories, in those
cases Archives will always be one level too, so no issue with nested
files. Also we do not support multi-trunk FSs (such as DOS/Windows), and
simulate devices/drives via artificial directories in backends, so to
the middleware there is always visible a single tree which could be
represented by hierarchy separated by slashes ('/') (of course,
top-level directory is non portable, but still backends have ability of
handling that and they do).


Eugene