New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for multiple file processing. Fixes Google code issue 151. #52

Merged
merged 1 commit into from Mar 3, 2015

Conversation

Projects
None yet
3 participants
@KyleJHarper
Copy link
Contributor

KyleJHarper commented Mar 2, 2015

Continuation of my previous PR, which accidentally had all changes in master and the PR targeted master. I'm not spectacular with git, but I think this PR is correct. If I did something wrong or you simply dislike the feature branch, you can just copy the lz4cli.c file out and add it to your local, then push; it's the only file changed.

Added support for multiple input files to act more like other compressors. For example: gzip file1 file2 file3. You can now do: lz4 [args] -m file1 file2 file3. Fixes 151.

Added support for multiple input files to act more like other compres…
…sors. For example: gzip file1 file2 file3. You can now do: lz4 [args] -m file1 file2 file3. Fixes 151.
@Cyan4973

This comment has been minimized.

Copy link
Member

Cyan4973 commented Mar 2, 2015

Thanks Kyle, your patch is correctly directed.

@Cyan4973

This comment has been minimized.

Copy link
Member

Cyan4973 commented Mar 2, 2015

I'm surprised that this line of code works :

char *input_filenames[argc];

argc is not a constant, so it looks like a kind of "dynamic size allocation on stack" .
I suspect that Visual will not like it.
Let's find out....

@Cyan4973

This comment has been minimized.

Copy link
Member

Cyan4973 commented Mar 2, 2015

Quick question about error case behavior :

Let's suppose we want to compress a list of 6 filenames.
In the middle of the list, say name 3, compression fails (typically because the file does not exist or is not valid).
How should the program behave ?
=> Exit immediatly with an error code ?
=> Proceed with the rest of list, trying to compress as much valid files as possible ?

@t-mat

This comment has been minimized.

Copy link
Contributor

t-mat commented Mar 2, 2015

@Cyan4973, it's C99 feature "VLA" (Variable Length Array). Since our CFLAGS has -std=c99, it's strictly valid code. But VLA has some problems :

  • As you suppose, VC++ and old C89 compilers could not compile it.
  • VLA becomes optional feature in C11 and greater (see __STDC_NO_VLA__). 😧

If we will support C89 compilers, intermingled declarations also should be removed.

How should the program behave ?

Since this PR's intention is mimicing gzip, gzip-like behaviour will be expected. In this case, latter is more "gzip-like" behaviour. The following log is my experiment :

# Initial state
$ ls -1
lz4.1
lz4c.1
lz4cat.1

# Normal compression
$ gzip -k lz4.1 lz4c.1 lz4cat.1
$ ls -1
lz4.1
lz4.1.gz
lz4c.1
lz4c.1.gz
lz4cat.1
lz4cat.1.gz

$ rm *.gz

# Compression with errors
$ gzip -k lz4.1 dummy1 lz4c.1 dummy2 lz4cat.1 dummy3
gzip: dummy1: No such file or directory
gzip: dummy2: No such file or directory
gzip: dummy3: No such file or directory
$ ls -1
lz4.1
lz4.1.gz
lz4c.1
lz4c.1.gz
lz4cat.1
lz4cat.1.gz
@Cyan4973

This comment has been minimized.

Copy link
Member

Cyan4973 commented Mar 2, 2015

Thanks Takayuki
It's an excellent reference.
Let's make it behave this way.

I'll also update Kyle contribution to make it C89 compatible.

Cyan4973 added a commit that referenced this pull request Mar 3, 2015

Merge pull request #52 from KyleJHarper/r128/multiple_inputs
Adding support for multiple file processing.  Fixes Google code issue 151.

@Cyan4973 Cyan4973 merged commit 046bd3a into lz4:dev Mar 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment