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

[Max Horn]

Am 16.08.2010 um 23:52 schrieb Eugene Sandulenko:
> 
> 
>> 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.

This is not a contradiction to what I wrote :). Yes, the internal hashmap of course can map every key (= file name) to only one value. And generates a warning. But in general, an Archive definitely *can* contain multiple members with the same name, you just cannot access them via getMember and hasFile. You still can access them by iterating over the result of listMembers, though. And it can't be different either (at least not with lots of extra work): Just think of SearchSet, which aggregates multiple heterogenous Archives together. It would have to perform expensive testing to detect inter-archive name clashes. Which we currently don't do, any I believe this is the right thing.




> 
>> 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.

This is not correct, see above. You *can* access all members, including those with duplicate names, if you use listMembers. This is so because SearchSet::listMembers simply invokes all its children's listMember methods in turn, concatenating their results into one huge archive member list.


[...]

> 
> 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).

I am not sure what this has to do with the rest of the discussion, did I miss something? Note: The filesystem abstraction as implemented by FSNode is one thing; the Common::Archive stuff is another thing. There are Common::Archive classes which make use of FSNode, true, but Common::Archive is *not* intended to be a fake file system, ala PhysFS. It has some overlap with what things like PhysFS do, granted, but that is only partial; it was created to solve a different set of problems.

If you want to have a way to merge multiple "file systems" together (e.g. the content of two directories, of a ZIP file, and two ARJ files), while retaining the full file and directory structure, then Common::Archive does *not* do that, and never was intended for this.

Bye,
Max