You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Emmanuel Bourg <eb...@apache.org> on 2008/06/17 11:02:57 UTC
[compress] A few comments
I looked a bit at commons compress, I'd like to make a couple suggestions:
- FileOutputStream and FileInputStream are used in several method
signatures, but a simple OutputStream or InputStream would do the job.
It's limiting the API to files only for no apparent reason.
- I believe it would make sense if the exception hierarchy extend
IOException. It could simplify some exception handling in the code, for
example in PackableObject instead of this:
public static PackableObject identifyByHeader(File file, List
packables) throws PackableObjectException {
...
try {
...
} catch (FileNotFoundException e) {
throw new PackableObjectException("File not found", e);
} catch (IOException e) {
throw new PackableObjectException("Internal factory
exception", e);
} finally {
try {
fis.close();
} catch (IOException e1) {
throw new PackableObjectException("Error while closing
InputStream to file", e1);
}
}
}
we could have this:
public static PackableObject identifyByHeader(File file, List
packables) throws IOException {
...
try {
...
} finally {
fis.close();
}
}
Any objection to change this ?
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Stefan Bodewig <bo...@apache.org>.
On Wed, 18 Jun 2008, Emmanuel Bourg <eb...@apache.org> wrote:
> Stefan Bodewig a écrit :
>
>> On Solaris a file with the extension .jar whose first ZIP extra
>> field has the ID 0xCAFE (and is otherwise empty) is considered
>> executable and automatically handed of to Java as a "shell".
>>
>> The jar tool in Sun's JDK adds this extra field and so does Ant's
>> jar task.
>
> Is it only added if the manifest has a Main-Class attribute ?
Not sure what the jar CLI tool does, but Ant always adds it (I just
looked into the code).
It has to be the first extra field of the directory entry of META-INF/
- which in turn has to be the very first entry IIRC.
<https://issues.apache.org/bugzilla/show_bug.cgi?id=32649> confirms
that.
If you try it on a jar without a main class you'll get the exact same
effect as running java -jar manually.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Stefan Bodewig a écrit :
> On Solaris a file with the extension .jar whose first ZIP extra field
> has the ID 0xCAFE (and is otherwise empty) is considered executable
> and automatically handed of to Java as a "shell".
>
> The jar tool in Sun's JDK adds this extra field and so does Ant's jar
> task.
Is it only added if the manifest has a Main-Class attribute ?
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Stefan Bodewig <bo...@apache.org>.
On Wed, 18 Jun 2008, Torsten Curdt <tc...@apache.org> wrote:
>> - Ant has a JarMarker class that extends ZipExtraField, is there an
>> equivalent ?
>
> Could you elaborate?
On Solaris a file with the extension .jar whose first ZIP extra field
has the ID 0xCAFE (and is otherwise empty) is considered executable
and automatically handed of to Java as a "shell".
The jar tool in Sun's JDK adds this extra field and so does Ant's jar
task.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Jörg Schaible <jo...@gmx.de>.
sebb wrote:
> On 18/06/2008, Jörg Schaible <jo...@gmx.de> wrote:
[snip]
>> >
>> > IOException is:
>> > "This class is the general class of exceptions produced by failed or
>> > interrupted I/O operations. "
>> > One can see wrong passwords as an interrupted IO exception. For me
>> > this are logical issues.
>>
>>
>> No, it's definitely not an InterruptedIOException. Such a beast is thrown
>> if
>> the thread is interrupted while it is performing an IO operation. Note,
>> that you will not see this on Windows, but on Solaris that exception can
>> cause major headaches ;-)
>>
>
> However, I think one could regard an incorrect password as much the
> same as a file protection error, which is surely an IOException?
IOException for sure, I just want to point out that it is a bad idea to use
*InterruptedIOException* for something different, since it is already a
very special case in the JDK ;-)
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by sebb <se...@gmail.com>.
On 18/06/2008, Jörg Schaible <jo...@gmx.de> wrote:
> Christian Grobmeier wrote:
>
> >>
> >>
> >> I agree with Torsten. There may occur other exceptions than plain
> >>> input/output errors.
> >>>
> >>
> >> For example ?
> >>
> >
> > hm, maybe those?
> > - putting the same file twice under same name in an archive
> > - putting different files twice under same name in an archive
> > - try to set unix permissions in an special archiver failed
> > - password to open zipfiles is wrong
> >
> > IOException is:
> > "This class is the general class of exceptions produced by failed or
> > interrupted I/O operations. "
> > One can see wrong passwords as an interrupted IO exception. For me this
> > are logical issues.
>
>
> No, it's definitely not an InterruptedIOException. Such a beast is thrown if
> the thread is interrupted while it is performing an IO operation. Note,
> that you will not see this on Windows, but on Solaris that exception can
> cause major headaches ;-)
>
However, I think one could regard an incorrect password as much the
same as a file protection error, which is surely an IOException?
> - Jörg
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Jörg Schaible <jo...@gmx.de>.
Christian Grobmeier wrote:
>>
>>
>> I agree with Torsten. There may occur other exceptions than plain
>>> input/output errors.
>>>
>>
>> For example ?
>>
>
> hm, maybe those?
> - putting the same file twice under same name in an archive
> - putting different files twice under same name in an archive
> - try to set unix permissions in an special archiver failed
> - password to open zipfiles is wrong
>
> IOException is:
> "This class is the general class of exceptions produced by failed or
> interrupted I/O operations. "
> One can see wrong passwords as an interrupted IO exception. For me this
> are logical issues.
No, it's definitely not an InterruptedIOException. Such a beast is thrown if
the thread is interrupted while it is performing an IO operation. Note,
that you will not see this on Windows, but on Solaris that exception can
cause major headaches ;-)
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
>
>
> I agree with Torsten. There may occur other exceptions than plain
>> input/output errors.
>>
>
> For example ?
>
hm, maybe those?
- putting the same file twice under same name in an archive
- putting different files twice under same name in an archive
- try to set unix permissions in an special archiver failed
- password to open zipfiles is wrong
IOException is:
"This class is the general class of exceptions produced by failed or
interrupted I/O operations. "
One can see wrong passwords as an interrupted IO exception. For me this are
logical issues.
Best,
Chris.
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Christian Grobmeier a écrit :
> I agree with Torsten. There may occur other exceptions than plain
> input/output errors.
For example ?
> We had a discussion at this list long time before. If you are interested,
> you may want to search the mailinglists.
I'll get a look, I haven't review all the past discussions yet.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
>
>
> +1 that would not be the place to throw a NPE IMO
>>
>
> You are right sorry, thought it was a method parameter.
>
No worries :-) Glad you jumped in and give so much feedback! That is
motivating!
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Torsten Curdt a écrit :
> +1 that would not be the place to throw a NPE IMO
You are right sorry, thought it was a method parameter.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Torsten Curdt <tc...@apache.org>.
>> Btw, regading this code in ArchiveStreamFactory :
>>
>> if (clazz == null) {
>> throw new ArchiveException("ArchiverFactory could not create
>> instance");
>> }
>>
>> Shouldn't this throw a NullPointerException instead ?
>>
>
> I would like to be carefully with throwing NullPointerException. In
> that
> case we simply
> wouldn't find a matching implementation - i think a better message
> but still
> archiveexception
> will do fine.
+1 that would not be the place to throw a NPE IMO
cheers
--
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
>
> input/output errors.
>>
> I agree with Torsten. There may occur other exceptions than plain
>
> I looked at the code, indeed the exceptions in the refactored compress are
> always related to configuration issue, whereas in the current svn code
> errors are often created on IO issues (path not writable, file is not a
> directory, etc).
>
ah right, the svn-version is more file oriented, that maybe would make sense
there.
>
> Btw, regading this code in ArchiveStreamFactory :
>
> if (clazz == null) {
> throw new ArchiveException("ArchiverFactory could not create
> instance");
> }
>
> Shouldn't this throw a NullPointerException instead ?
>
I would like to be carefully with throwing NullPointerException. In that
case we simply
wouldn't find a matching implementation - i think a better message but still
archiveexception
will do fine.
If that params:
final String archiverName, final InputStream out
would be null, i would agree with a NullPointerException. Thanks for the
hint.
Best,
chris
>
> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Christian Grobmeier a écrit :
> I agree with Torsten. There may occur other exceptions than plain
> input/output errors.
I looked at the code, indeed the exceptions in the refactored compress
are always related to configuration issue, whereas in the current svn
code errors are often created on IO issues (path not writable, file is
not a directory, etc).
Btw, regading this code in ArchiveStreamFactory :
if (clazz == null) {
throw new ArchiveException("ArchiverFactory could not create
instance");
}
Shouldn't this throw a NullPointerException instead ?
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
>
> Torsten Curdt a écrit :
>>
>>> - the exceptions could extend IOException
>>>>
>>> Could - but why restrict it that way? (composition over inheritance)
>>>
>>
>> I don't see this as a restriction.
>>
>
> Well, it means all ArchiveException necessarily need to be IOExceptions.
> That's restricts their type.
> I don't see the benefit.
>
I agree with Torsten. There may occur other exceptions than plain
input/output errors.
>
> An issue during a compression or archive operation is an IO exception to
>> me. The TrueZIP developers made the same assumption:
>>
>>
>> https://truezip.dev.java.net/nonav/javadoc-6/de/schlichtherle/io/ArchiveException.html
>>
>
> I don't realy see that being an argument :) ...they^^^^he did other very
> weird design decisions, too. :)
>
We had a discussion at this list long time before. If you are interested,
you may want to search the mailinglists.
>
>
> - Ant has a JarMarker class that extends ZipExtraField, is there an
>>>> equivalent ?
>>>>
>>> Could you elaborate?
>>>
>>
>> I don't know what it is, but it seems the Ant source is more recent than
>> the code in compress, it might be good to resync the code with Ant.
>>
>
> +1
>
Torsten: I will do that for the GIT repository. I also will clean up style
issues then, i don't like hungarian notation too.
Hm ...indeed. But IMO the changeset stuff is where it actually gets
> interesting :)
>
>> It's also the most debatable topic, and it will delay the release. The
other stuff is ready for a release.
About releasing: i like that "release often" principle. I am not sure about
why we didn't release compress in the past, but I guess that we hadn't
enough community about that. A release 0.1 with just the cleaned-up
streaming classes + testcases feels good to me. 0.2 could have that improved
ChangeSet implementation then.
Best
Chris
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Torsten Curdt a écrit :
> I don't realy see that being an argument :) ...they^^^^he did other very
> weird design decisions, too. :)
Well, at least I'm not the only one to think that :)
>> It's also the most debatable topic, and it will delay the release. The
>> other stuff is ready for a release.
>
> In what sense debatable?
There are different ways to do it (changeset, virtual file system style,
archive descriptor with add/remove/save operations), that's why I say
it's a debatable topic. That doesn't prevent supporting different
handling styles btw, and let the user pick the one he likes.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Torsten Curdt <tc...@apache.org>.
On Jun 18, 2008, at 10:32, Emmanuel Bourg wrote:
> Torsten Curdt a écrit :
>>> - the exceptions could extend IOException
>> Could - but why restrict it that way? (composition over inheritance)
>
> I don't see this as a restriction.
Well, it means all ArchiveException necessarily need to be
IOExceptions. That's restricts their type.
I don't see the benefit.
> An issue during a compression or archive operation is an IO
> exception to me. The TrueZIP developers made the same assumption:
>
> https://truezip.dev.java.net/nonav/javadoc-6/de/schlichtherle/io/ArchiveException.html
I don't realy see that being an argument :) ...they^^^^he did other
very weird design decisions, too. :)
>>> - IOUtils.copy(in, out) could call copy(in, out, size)
>> Where is that?
>
> In the org.apache.commons.compress.utils.IOUtils class from Chris
> archive.
Ah
>>> - the header in ArArchive*Stream could be factorized (ArConstants?
>>> along with the sizes of the entry fields)
>> Hm ...IMO only useful if used somewhere else. Otherwise it's just
>> work. Or where do you see the benefit?
>
> Just to have a cleaner code. PMD or Checkstyle will complain about
> "magic numbers" otherwise.
Cleaner code is debatable :) ...but if someone wants to do it. *shrug*
>>> - Ant has a JarMarker class that extends ZipExtraField, is there
>>> an equivalent ?
>> Could you elaborate?
>
> I don't know what it is, but it seems the Ant source is more recent
> than the code in compress, it might be good to resync the code with
> Ant.
+1
>> Hm ...indeed. But IMO the changeset stuff is where it actually gets
>> interesting :)
>
> It's also the most debatable topic, and it will delay the release.
> The other stuff is ready for a release.
In what sense debatable?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Torsten Curdt a écrit :
>> - the exceptions could extend IOException
>
> Could - but why restrict it that way? (composition over inheritance)
I don't see this as a restriction. An issue during a compression or
archive operation is an IO exception to me. The TrueZIP developers made
the same assumption:
https://truezip.dev.java.net/nonav/javadoc-6/de/schlichtherle/io/ArchiveException.html
>> - IOUtils.copy(in, out) could call copy(in, out, size)
>
> Where is that?
In the org.apache.commons.compress.utils.IOUtils class from Chris archive.
>> - the header in ArArchive*Stream could be factorized (ArConstants?
>> along with the sizes of the entry fields)
>
> Hm ...IMO only useful if used somewhere else. Otherwise it's just work.
> Or where do you see the benefit?
Just to have a cleaner code. PMD or Checkstyle will complain about
"magic numbers" otherwise.
>> - Ant has a JarMarker class that extends ZipExtraField, is there an
>> equivalent ?
>
> Could you elaborate?
I don't know what it is, but it seems the Ant source is more recent than
the code in compress, it might be good to resync the code with Ant.
> Hm ...indeed. But IMO the changeset stuff is where it actually gets
> interesting :)
It's also the most debatable topic, and it will delay the release. The
other stuff is ready for a release.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi
A higher level API to manipulate the archives would be nice, but I believe
> an initial release could be made with just the stream classes.
>
Hm ...indeed. But IMO the changeset stuff is where it actually gets
> interesting :)
>
changeset is great :-) however an initial release with the stream classes
could also help VFS a bit. And one could get rid of GIT which drives me nuts
sometimes :-)
> - what is missing to complete the API ? The implementation of the changes
>> package ? Or did you plan anything else ?
>>
>
> Mostly changeset support and cleaning up. Especially the testcases need
> some more love.
>
I have a bit time for cleaning up after my holidays. It would be nice to
move on with compress :-)
Best,
Chris
>
> cheers
> --
> Torsten
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
>
> >> OK, we've never been consistent with our notation and some
> >> committers liked HN You probably don't find any in the zip package.
> >
> > * @author <a href="stefan.bodewig@epost.de">Stefan Bodewig</a>
>
> If anybody could remove that, I'd be grateful. That email address
> died many years ago (not exaggerating) and in addition I've come to
> consider @author tags harmful by now.
>
> I know I technically can do that myself.
>
I will do that for the new implementation in the GIT repos.
Chris.
Re: [compress] A few comments
Posted by Stefan Bodewig <bo...@apache.org>.
On Wed, 18 Jun 2008, Emmanuel Bourg <eb...@apache.org> wrote:
> Stefan Bodewig a écrit :
>
>> OK, we've never been consistent with our notation and some
>> committers liked HN You probably don't find any in the zip package.
>
> * @author <a href="stefan.bodewig@epost.de">Stefan Bodewig</a>
If anybody could remove that, I'd be grateful. That email address
died many years ago (not exaggerating) and in addition I've come to
consider @author tags harmful by now.
I know I technically can do that myself.
> * @version $Revision: 155439 $
> */
> public class ZipEntry extends java.util.zip.ZipEntry
> {
> private static Method c_setCompressedSizeMethod;
>
>
> But that's commons compress code, Ant is clean.
And never had hungarian notation, I guess it was introduced somewhere
along the way (Avalon would be my best guess).
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Stefan Bodewig a écrit :
> OK, we've never been consistent with our notation and some committers
> liked HN You probably don't find any in the zip package.
* @author <a href="stefan.bodewig@epost.de">Stefan Bodewig</a>
* @version $Revision: 155439 $
*/
public class ZipEntry extends java.util.zip.ZipEntry
{
private static Method c_setCompressedSizeMethod;
But that's commons compress code, Ant is clean. It seems like we could
update our code from the latest Ant source for the zip package.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Stefan Bodewig <bo...@apache.org>.
On Wed, 18 Jun 2008, Torsten Curdt <tc...@apache.org> wrote:
>> - the exceptions could extend IOException
>
> Could - but why restrict it that way? (composition over inheritance)
>
>> - I'm not sure to understand the purpose of the jar classes, isn't
>> a jar just a renamed zip ?
>
> Essentially - yes. But the JDK also handles them differently because
> of meta data and so on.
The JDK also relies on the file names being UTF-8 encoded, for
example.
>> - hungarian notation must die :)
+1
> Yepp!! ....these classes are borrowed from ant :)
Impossible! 8-)
OK, we've never been consistent with our notation and some committers
liked HN You probably don't find any in the zip package.
I've probably said this before. The compress classes have been
extracted from Ant and found their way into commons via Avalon and
probably some other places without the Ant community being involved.
I'd double check whether any improvements/bug fixes that have been
made in Ant over the past five years or so are present in the compress
codebase as well.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Torsten Curdt <tc...@apache.org>.
> - the exceptions could extend IOException
Could - but why restrict it that way? (composition over inheritance)
> - I'm not sure to understand the purpose of the jar classes, isn't a
> jar just a renamed zip ?
Essentially - yes. But the JDK also handles them differently because
of meta data and so on.
> - what's the intent with the empty classes CompressorInputStream and
> CompressorOutputStream ? The GzipCompressor*Stream would become
> unnecessary without these.
I think it's a relict from the factory code. But I am not sure it
would be so bad to keep them (for now) for the symmetry of the API.
If we *really* don't need them - getting rid of that could be the last
thing we do before a release ;)
> - would it make sense to put the unix permissions at the
> ArchiveEntry level ? Or maybe have a base class (UnixEntry?) for the
> Tar/Ar/ZipEntry classes ?
Well, I think e.g. RAR carries slightly different permissions.
UnixEntry might be idea though.
> - hungarian notation must die :)
Yepp!! ....these classes are borrowed from ant :)
> - IOUtils.copy(in, out) could call copy(in, out, size)
Where is that?
> - the exception handling in the factories could be simplified
+1
> - the header in ArArchive*Stream could be factorized (ArConstants?
> along with the sizes of the entry fields)
Hm ...IMO only useful if used somewhere else. Otherwise it's just
work. Or where do you see the benefit?
> - it seems there are some duplicate classes in the zip package
> (ZipEntry vs ZipArchiveEntry)
Yepp ...ups!
> - Ant has a JarMarker class that extends ZipExtraField, is there an
> equivalent ?
Could you elaborate?
> - the zip package seems quite complicated, do we have to expose all
> these classes as public ?
Would actually be good to hide some more of that. Indeed.
> - I wonder if the changeset model is mature enough.
No - it's not :)
> A higher level API to manipulate the archives would be nice, but I
> believe an initial release could be made with just the stream classes.
Hm ...indeed. But IMO the changeset stuff is where it actually gets
interesting :)
> - for simplicity, I would rename the 2 factories as Compressors and
> Archives. The name of the methods could be shortened too, maybe
> something like Compressors.newStream("bzip2", in).
Not a big fan of static factory accessors. For the length - sure we
could rename them. On the other hand the current names are quite self
explanatory. Like that.
> - what is missing to complete the API ? The implementation of the
> changes package ? Or did you plan anything else ?
Mostly changeset support and cleaning up. Especially the testcases
need some more love.
cheers
--
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Christian Grobmeier a écrit :
> thanks for your comments below. They all make sense.
I committed the changes to get a hand on the code, but I won't go
farther since your code base is much more advanced.
> However, Thorsten and I currently develop a new and (hopefully) better
> implementation
> than that which is currently available. That implementation fixes some of
> the issues you mentioned.
> If you want to take a look at it, please check it out via GIT:
> http://projects.grobmeier.de/compress-redesign.git/
> Your comments and your help is highly appreciated.
My comments, in no specific order:
- the exceptions could extend IOException
- I'm not sure to understand the purpose of the jar classes, isn't a jar
just a renamed zip ?
- what's the intent with the empty classes CompressorInputStream and
CompressorOutputStream ? The GzipCompressor*Stream would become
unnecessary without these.
- would it make sense to put the unix permissions at the ArchiveEntry
level ? Or maybe have a base class (UnixEntry?) for the Tar/Ar/ZipEntry
classes ?
- hungarian notation must die :)
- IOUtils.copy(in, out) could call copy(in, out, size)
- the exception handling in the factories could be simplified
- the header in ArArchive*Stream could be factorized (ArConstants? along
with the sizes of the entry fields)
- it seems there are some duplicate classes in the zip package (ZipEntry
vs ZipArchiveEntry)
- Ant has a JarMarker class that extends ZipExtraField, is there an
equivalent ?
- the zip package seems quite complicated, do we have to expose all
these classes as public ?
- I wonder if the changeset model is mature enough. A higher level API
to manipulate the archives would be nice, but I believe an initial
release could be made with just the stream classes.
- for simplicity, I would rename the 2 factories as Compressors and
Archives. The name of the methods could be shortened too, maybe
something like Compressors.newStream("bzip2", in).
- what is missing to complete the API ? The implementation of the
changes package ? Or did you plan anything else ?
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
>
> I'll give it a look. I wanted to see how the Ar classes from jdeb could fit
> into commons compress.
Cool!
Thorsten allready included some ArArchivers code, you may want to .take a
look into:
src\main\java\org\apache\commons\compress\archivers\ar
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Christian Grobmeier a écrit :
> I use cygwin and could check out with:
> $ git clone http://projects.grobmeier.de/compress-redesign.gitcompress-redesign
>
> I created a zip file for you:
> http://projects.grobmeier.de/compress-redesign-draft1.zip
Thank you! I was about to clone it with Ubuntu on VMWare and transfer it
with ssh ;)
> the old implementation is "file based" as you allready have mentioned. The
> new implementation just uses input/output streams.
> We also introduced delete support with a changeset implementation. That
> stuff is very new
> not tested very well, but if you want to contribute, I would be happy. I was
> lazy the last weeks :-)
I'll give it a look. I wanted to see how the Ar classes from jdeb could
fit into commons compress.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi
> Git is problematic for Windows. I tried to clone the repository with
> msysgit but it failed with the following error :
>
> fatal: Couldn't find remote ref HEAD
>
I use cygwin and could check out with:
$ git clone http://projects.grobmeier.de/compress-redesign.gitcompress-redesign
I created a zip file for you:
http://projects.grobmeier.de/compress-redesign-draft1.zip
> What are the differences of the new implementation ?
>
the old implementation is "file based" as you allready have mentioned. The
new implementation just uses input/output streams.
We also introduced delete support with a changeset implementation. That
stuff is very new
not tested very well, but if you want to contribute, I would be happy. I was
lazy the last weeks :-)
Best regards,
Chris
>
>
> Emmanuel Bourg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [compress] A few comments
Posted by Emmanuel Bourg <eb...@apache.org>.
Christian Grobmeier a écrit :
> However, Thorsten and I currently develop a new and (hopefully) better
> implementation
> than that which is currently available. That implementation fixes some of
> the issues you mentioned.
> If you want to take a look at it, please check it out via GIT:
> http://projects.grobmeier.de/compress-redesign.git/
> Your comments and your help is highly appreciated.
Git is problematic for Windows. I tried to clone the repository with
msysgit but it failed with the following error :
fatal: Couldn't find remote ref HEAD
What are the differences of the new implementation ?
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] A few comments
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi Emmanuel,
thanks for your comments below. They all make sense.
However, Thorsten and I currently develop a new and (hopefully) better
implementation
than that which is currently available. That implementation fixes some of
the issues you mentioned.
If you want to take a look at it, please check it out via GIT:
http://projects.grobmeier.de/compress-redesign.git/
Your comments and your help is highly appreciated.
However, unfortunatly I cannot fix this issues in the repository,
cause I have no commit access.
Best regards,
Chris.
On Tue, Jun 17, 2008 at 11:02 AM, Emmanuel Bourg <eb...@apache.org> wrote:
> I looked a bit at commons compress, I'd like to make a couple suggestions:
>
> - FileOutputStream and FileInputStream are used in several method
> signatures, but a simple OutputStream or InputStream would do the job. It's
> limiting the API to files only for no apparent reason.
>
> - I believe it would make sense if the exception hierarchy extend
> IOException. It could simplify some exception handling in the code, for
> example in PackableObject instead of this:
>
> public static PackableObject identifyByHeader(File file, List packables)
> throws PackableObjectException {
> ...
> try {
> ...
> } catch (FileNotFoundException e) {
> throw new PackableObjectException("File not found", e);
> } catch (IOException e) {
> throw new PackableObjectException("Internal factory exception",
> e);
> } finally {
> try {
> fis.close();
> } catch (IOException e1) {
> throw new PackableObjectException("Error while closing
> InputStream to file", e1);
> }
> }
> }
>
>
> we could have this:
>
>
> public static PackableObject identifyByHeader(File file, List packables)
> throws IOException {
> ...
> try {
> ...
> } finally {
> fis.close();
> }
> }
>
>
> Any objection to change this ?
>
> Emmanuel Bourg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>