Don't use tar reader for '-C' flag #1354

rht · 2015-06-11T14:05:39Z

Currently ipfs get -C <hash> returns error even if is a file.
This PR is for the case when the compress flag is enabled, always
archive the output before the compression.

whyrusleeping · 2015-06-11T20:46:50Z

I think this LGTM.

jbenet · 2015-06-12T03:23:09Z

@rht i'm not sure about this. what if I want a gzipped raw file?

> echo hello | gzip | gzip -d
hello

is possible. so should:

> ipfs cat <file> | gzip
<compressed-file-contents>
> ipfs cat <file> | gzip | gzip -d
<file-contents>

and thus i would expect

> ipfs get -C <file>
<compressed-file-contents>

not

> ipfs get -C -o out <file> && cat out
<tarred-and-compressed-file-contents>

jbenet · 2015-06-12T03:23:18Z

other people's thoughts?

rht · 2015-06-12T05:26:22Z

Ok my starting point is to investigate why ipfs get -C <file> always returns error.
Then I figured from the stream that:

tarwriter and gzipwriter are soldered in (less buffer needed) in unixfs/tar/reader.go, turning the stream into a .tar.gz one.
The .tar.gz stream is un-gzipped in https://github.com/ipfs/go-ipfs/blob/master/core/commands/get.go#L139 into a .tar stream
The .tar stream is extracted in https://github.com/ipfs/go-ipfs/blob/master/core/commands/get.go#L148
Unexpected EOF because the tar header expects a compressed .tar.gz size.

To bypass the problem, since it is already a .tar.gz stream, why not just use .tar.gz extension?

Alternatively the only way for > ipfs get -C <file> to not use the .tar.gz stream is to use NewDAGReader directly like in ipfs cat and pipe it to a gzip writer. Where beforehand the node has to be identified as a file. And the output should have .gz extension to avoid ambiguity (if someone ipfs add this gzipped file then it will turn into a different hash).

jbenet · 2015-06-17T23:00:04Z

@rht ah, interesting.

To bypass the problem, since it is already a .tar.gz stream, why not just use .tar.gz extension?

that makes sense to me. if a .tar is compressed it should use .tar.gz

I'm still not sold on -C always meaning -a -C -- i may want a gzipped file. imagine:

ipfs get -o=file.gzip <huge-file>

or something.

the output should have .gz extension to avoid ambiguity

agree when -o is not specified. if the user specifies the filename directly with -o, then we should use what we're given exactly.

rht · 2015-06-22T14:31:44Z

This is now fixed. '-C' uses NewDagReader directly, then piped to gzip processor.

Though I don't know which one is more effective: bufio.NewReader + WriteTo, or just io.Copy(reader, gw). I use the former.

rht · 2015-06-22T14:39:01Z

I figure that in unixfs/tar/reader.go, NewDagReader -> syncCopy -> TarWriter -> GzipWriter -> buffer -> io.Reader, the syncCopy & buffer pattern are quite general (async read/write). I wonder if there is already a stdlib for this async glue between reader and writer into another reader?

So, NewDagReader -> syncCopy -> [a Writer] -> buffer -> io.Reader, can be reused where [a Writer] is DagArchiveWriter / 'TarWriter -> Bzip2Writer'

jbenet · 2015-06-25T21:41:35Z

Could someone else CR this? i'm behind. maybe @wking @whyrusleeping @cryptix ?

whyrusleeping · 2015-06-30T18:30:11Z

core/commands/get.go

+
+// getZip is equivalent to `ipfs getdag $hash | gzip`
+func getZip(ctx context.Context, node *core.IpfsNode, p string, compression int) (io.Reader, error) {
+	pathToResolve := path.Path(p)


this should call path.ParsePath so we can validate the input path

jbenet · 2015-06-30T18:37:56Z

one comment above, otherwise LGTM.

rht · 2015-07-01T01:51:17Z

core/commands/get.go

+		// Validate path string
+		p, err := path.ParsePath(req.Arguments()[0])
+		if err != nil {
+			res.SetError(fmt.Errorf("failed to validate path: %v", err), cmds.ErrNormal)


Added path validation here.

whyrusleeping · 2015-07-01T01:57:48Z

LGTM

jbenet · 2015-07-01T02:17:40Z

@rht circleci didn't like it. have you rebased on new master? (it's always good to do so as keeps our branches closer)

Currently `ipfs get -C <hash>` returns error even if <hash> is a file. This PR is for the case when the compress flag is enabled, use the dagreader directly and pipe to a gzip processor. License: MIT Signed-off-by: rht <rhtbot@gmail.com>

rht · 2015-07-01T02:42:23Z

path.Path(pathString) is used in several other places as well. Put validations on those?

rht · 2015-07-01T02:53:24Z

Now it passed circleci, but failed travis go-test because of ctx issue. The PR doesn't change any ctx related code, so should be fine.

jbenet · 2015-07-01T03:02:32Z

Great, thanks! LGTM

Don't use tar reader for '-C' flag

rht · 2015-07-01T03:35:38Z

Want to add path.Path(pathString) validation everywhere? This validation stuff is in favor for making the struct private https://github.com/ipfs/go-ipfs/blob/master/path/path.go#L17

jbenet · 2015-07-08T06:42:49Z

Want to add path.Path(pathString) validation everywhere?

Probably?

rht · 2015-07-08T12:17:54Z

Already in dfde18e. All usage of path.Path(str) in the code will be validated later on.

jbenet added the backlog label Jun 11, 2015

rht force-pushed the get-flag branch from f170c7a to 1c35d8d Compare June 21, 2015 07:26

rht changed the title ~~Short-circuit '-C' flag to mean '-a -C'~~ Don't use tar reader for '-C' flag Jun 21, 2015

rht force-pushed the get-flag branch 3 times, most recently from 338a413 to 7508e1f Compare June 22, 2015 13:55

whyrusleeping reviewed Jun 30, 2015
View reviewed changes

rht force-pushed the get-flag branch from 7508e1f to 489ee24 Compare July 1, 2015 01:29

rht reviewed Jul 1, 2015
View reviewed changes

Don't use tar reader for '-C' only flag

3798178

Currently `ipfs get -C <hash>` returns error even if <hash> is a file. This PR is for the case when the compress flag is enabled, use the dagreader directly and pipe to a gzip processor. License: MIT Signed-off-by: rht <rhtbot@gmail.com>

rht force-pushed the get-flag branch from 489ee24 to 3798178 Compare July 1, 2015 02:36

jbenet added a commit that referenced this pull request Jul 1, 2015

Merge pull request #1354 from rht/get-flag

22c653c

Don't use tar reader for '-C' flag

jbenet merged commit 22c653c into ipfs:master Jul 1, 2015

jbenet removed the backlog label Jul 1, 2015

rht deleted the get-flag branch July 8, 2015 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use tar reader for '-C' flag #1354

Don't use tar reader for '-C' flag #1354

rht commented Jun 11, 2015

whyrusleeping commented Jun 11, 2015

jbenet commented Jun 12, 2015

jbenet commented Jun 12, 2015

rht commented Jun 12, 2015

jbenet commented Jun 17, 2015

rht commented Jun 22, 2015

rht commented Jun 22, 2015

jbenet commented Jun 25, 2015

whyrusleeping Jun 30, 2015

jbenet commented Jun 30, 2015

rht Jul 1, 2015

whyrusleeping commented Jul 1, 2015

jbenet commented Jul 1, 2015

rht commented Jul 1, 2015

rht commented Jul 1, 2015

jbenet commented Jul 1, 2015

rht commented Jul 1, 2015

jbenet commented Jul 8, 2015

rht commented Jul 8, 2015

Don't use tar reader for '-C' flag #1354

Don't use tar reader for '-C' flag #1354

Conversation

rht commented Jun 11, 2015

whyrusleeping commented Jun 11, 2015

jbenet commented Jun 12, 2015

jbenet commented Jun 12, 2015

rht commented Jun 12, 2015

jbenet commented Jun 17, 2015

rht commented Jun 22, 2015

rht commented Jun 22, 2015

jbenet commented Jun 25, 2015

whyrusleeping Jun 30, 2015

Choose a reason for hiding this comment

jbenet commented Jun 30, 2015

rht Jul 1, 2015

Choose a reason for hiding this comment

whyrusleeping commented Jul 1, 2015

jbenet commented Jul 1, 2015

rht commented Jul 1, 2015

rht commented Jul 1, 2015

jbenet commented Jul 1, 2015

rht commented Jul 1, 2015

jbenet commented Jul 8, 2015

rht commented Jul 8, 2015