tar cat should return appropriate headers

[RichardLitt]

I've added application:x-tar and a sharness test, although I can't figure out how to run the test.

License: MIT
Signed-off-by: Richard Littauer richard.littauer@gmail.com

[RichardLitt]

So, the code works, but I am unhappy with it. The approach in the handler is too specific to tar cat; at times, we'll need to return other headers, like application/x-gzip in ipfs get (#2376). Also, the test doesn't work, and I'm not sure how to fix it.

This is blocking ipfs-inactive/http-api-spec#46 and needs to be incorporated into the tar Group in the API.

[RichardLitt]

@whyrusleeping Thoughts on this?

[whyrusleeping]

@keks In your commands lib refactor, will making this change be easy?

[keks]

I don't think I get the change. Richard's code checks whether the request path is /cat/cat and, if so, sets the application/x-tar header. The test does a request for /tar/cat though, which makes much more sense.

Also this is a rather unclean approach, if we want to add static checks like that I think adding a new field in the Command struct is a much better option.
However, if we want a solution that also fixes #2376 this is not enough. We will need to be able to set the content type from Run, maybe adding a Response.SetContentType() method. In my rewrite that would be ResponseEmitter.SetContentType() and work pretty much the same. What is important though is that SetContentType will need to be called before the first Emit().

If we take one of the approaches above, go-ipfs-cmds can handle this. If we choose to do it another way I will need to check again.

By the way: should /cat maybe return the content type of the served data? Not sure if that is important and/or a good idea, just wondering.

[whyrusleeping]

@keks cool, I like the approach with the emitter setting content type. I'll go ahead and close this in favor of your changes

…

On Thu, Mar 2, 2017, 05:09 keks ***@***.***> wrote: I don't think I get the change. Richard's code checks whether the request path is /cat/cat and, if so, sets the application/x-tar header. The test does a request for /tar/cat though, which makes much more sense. Also this is a rather ugly approach, if we want to add static checks like that I think adding a new field in the Command struct is a much better option. However, if we want a solution that also fixes #2376 <#2376> this is not enough. We will need to be able to set the content type from Run, maybe adding a Response.SetContentType() method. In my rewrite that would be ResponseEmitter.SetContentType() and work pretty much the same. What is important though is that SetContentType will need to be called before the first Emit(). If we take one of the approaches above, go-ipfs-cmds can handle this. If we choose to do it another way I will need to check again. By the way: should /cat maybe return the content type of the served data? Not sure if that is important and/or a good idea, just wondering. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2427 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL4HHB20HTJJjDHPfEXbOwgy0ZS0Lgsks5rhr9zgaJpZM4Hm9Qv> .