Add `ronin compress` command by moozzi · Pull Request #184 · ronin-rb/ronin

moozzi · 2024-01-03T18:21:49Z

moozzi · 2024-01-03T18:24:11Z

@postmodern 0.1 version of this PR.
Let me know if at least my idea is correct.
I'm not sure if I understand CommandKit correctly 😄

postmodern

Submitted a bunch of comments and suggestions. There's multiple ways we could implement this command. This is a good start!

postmodern · 2024-01-04T12:40:49Z

+      #
+      #     [FILE ...]                       Optional file(s) to compress
+      #
+      class Compress < StringMethodsCommand


If we only want to support processing files, then probably want to inherit from FileProcessorCommand, and define a process_input(io) method to compress the input File or from stdin.

If we do want to support accepting arbitrary Strings (ex: ronin compress --string "hello world"), then we should inherit from StringProcessorCommand.

StringMethodsCommand is more meant to be used by commands which simply map options to certain monkey-patch methods that are added to String (ex: ronin decode --base64 -> String#base64_decode).

postmodern · 2024-01-04T12:42:08Z

+        option :gzip, short: '-g',
+                      desc:  'gzip compresses the data' do
+                        @format = :gzip
+                      end


If we're going to support --gzip, we should also support a --zlib option. ronin-support also happens to have a Ronin::Support::Compression::Zlib module we can use.

postmodern · 2024-01-04T12:42:40Z

+        #
+        # The compression format.
+        #
+        # @return Symbol


I tend to explicitly list the possible values @return [:gzip, :tar, :zip].

postmodern · 2024-01-04T12:43:55Z

+        end
+
+        def extension
+          return "gz" if @format == :gzip


We could probably set an @ext instance variable in the option blocks, along with @format. This way we follow the "Tell, don't ask" principle.

postmodern · 2024-01-04T12:45:30Z

+        end
+
+        def process_file(file)
+          compression_class.send(@format, "#{file}.#{extension}") do |c|


Maybe rename @format to @method or @compression_method if we're going to send() it.

postmodern · 2024-01-04T12:46:48Z

+
+        def process_file(file)
+          compression_class.send(@format, "#{file}.#{extension}") do |c|
+            c.write File.read(file)


The files being compressed could be very very large. Probably more efficient to open the input file and read chunks of data from it using input.readpartial(4096) and write those to the output stream.

postmodern · 2024-01-04T12:48:39Z

+        end
+
+        def process_file(file)
+          compression_class.send(@format, "#{file}.#{extension}") do |c|


So here's a question, if you gave this command the --zip option and multiple files, would the user expect each file to be compressed into it's own .zip file, or all files to be compressed into one zip file?

postmodern · 2024-01-05T04:32:18Z

@AI-Mozi what do you think about adding separate ronin archive and ronin unarchive commands? This might help with the bifurcating logic that uses Ronin::Support::Compression or Ronin::Support::Archive. ronin archive/ronin unarchive would probably inherit from FileProcessorCommand, while ronin compress/ronin uncompress would inherit from StringProcessorCommand that could accept both --string values and FILEs to allow specifying a raw zlib/gzip compressed string or a .gz file.

moozzi · 2024-01-05T07:52:49Z

It's definitely a good idea!

moozzi · 2024-01-07T16:08:22Z

+        option :name, short: '-n',
+                      value: {
+                        type: String
+                      },
+                      desc: 'compressed file name' do |name|
+                        @compressed_file_name = name
+                      end


I think we should add an option for specifying compressed file name.

moozzi · 2024-01-07T16:09:10Z

+        #   The path to the file.
+        #
+        def process_file(file)
+          raise "Files can be compressed using gzip only" if @compression_method != :gzip


Should we raise an error, or just print some message and return?

Call print_error and exit(1), or call print_error and return if you think the error can be printed but the command should continue processing arguments.

moozzi · 2024-01-07T16:09:42Z

+          rescue EOFError
+          rescue IOError => e


Rubocop does not like empty rescue :/

Ideally, there should be no IO errors when reading the file. I suspect file.eof? should make the rescues unnecessary.

postmodern · 2024-01-07T20:53:47Z

+
+            Ronin::Support::Compression.gzip(compressed_file_name) do |gz|
+              files.each do |file|
+                File.open(file, 'rb') do |f|


Avoid single letter block arguments. Rename file to like path or file_path, and f to file or io.

postmodern · 2024-01-07T20:54:39Z

+          rescue EOFError
+          rescue IOError => e


Ideally, there should be no IO errors when reading the file. I suspect file.eof? should make the rescues unnecessary.

postmodern · 2024-01-07T20:55:54Z

+          if files.empty?
+            super(*files)
+          else
+            raise "Files can be compressed using gzip only" if @compression_method != :gzip


Do a print_error and call exit(1) if invalid combinations of options or arguments are given.

postmodern · 2024-01-07T20:56:51Z

+        #   The compressed string.
+        #
+        def process_string(string)
+          string.send(@compression_method)


I try to avoid calling the monkey-patched methods when possible. Might be better to do Support::Compression.send(@compression_method,string).

postmodern · 2024-01-07T20:57:53Z

+        # @return String
+        #
+        def compressed_file_name
+          @compressed_file_name || "ronin_compressed_#{Time.now.to_i}.gz"


I'd get rid of the timestamp, or maybe add an option for it. I think a ronin compress command should behave exactly like gzip and just append the .gz extension on the given file name.

postmodern · 2024-01-07T20:59:22Z

+        #   The path to the file.
+        #
+        def process_file(file)
+          raise "Files can be compressed using gzip only" if @compression_method != :gzip


Call print_error and exit(1), or call print_error and return if you think the error can be printed but the command should continue processing arguments.

postmodern · 2024-01-07T21:01:11Z

+          if files.empty?
+            super(*files)
+          else
+            raise "Files can be compressed using gzip only" if @compression_method != :gzip


I think we could support compressing files using SupportCompression.zlib_deflate. Just read the entire file using File.read, compress the String, and write it back out using File.binwrite.

postmodern · 2024-01-07T21:01:42Z

+          else
+            raise "Files can be compressed using gzip only" if @compression_method != :gzip
+
+            Ronin::Support::Compression.gzip(compressed_file_name) do |gz|


Also why are there two Ronin::Support::Compression.gzip(compressed_file_name) blocks? One in run and another in process_file?

postmodern · 2024-01-07T21:02:03Z

+        #   File arguments.
+        #
+        def run(*files)
+          if files.empty?


Shouldn't this be unless files.empty? or if !files.empty??

I thought about it like if there is file given as an argument, compress it, otherwise go to StringProcessorCommand#run else statement and process @input_values.

Ohh.. now that I read it, it works different than i thought 😅

Hmm maybe it might be easier to just write all files and strings into a common Support::Compression::GZip output stream? Although, we would have to have different logic for the Compression::Zlib code.

Another idea would be to define separate process_file and process_string methods. The process_file method would read the File and write the gziped/zlib'ed output to a .gz or .zlib file. The process_string method would instead gzip/zlib compress a String and write it to stdout; maybe also support a common --output option?

moozzi · 2024-04-18T15:14:30Z

@postmodern It's been a while. I did some changes. Let me know what do you thing and we can discuss that later :D

postmodern · 2024-04-18T21:56:04Z

@AI-Mozi I'm not sure about how the compression and uncompress commands should handle multiple files. Whether they should compress/uncompress each file individually, or concat the output into a single output file or print to stdout, etc. Unless we can come up with a useful and logical solution, maybe we should hold off on these commands.

I do however think there should be a single ronin archive command that has --list, --extract, --create options.

add ronin compress command

b50ad1d

postmodern reviewed Jan 4, 2024

View reviewed changes

code changes

e77a8cf

moozzi commented Jan 7, 2024

View reviewed changes

postmodern reviewed Jan 7, 2024

View reviewed changes

change process_string and process_file methods

ca1c621

Uh oh!

Conversation

moozzi commented Jan 3, 2024

Uh oh!

moozzi commented Jan 3, 2024

Uh oh!

postmodern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

postmodern commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moozzi commented Jan 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

postmodern Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moozzi commented Apr 18, 2024

Uh oh!

postmodern commented Apr 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

postmodern commented Jan 5, 2024 •

edited

Loading

postmodern Jan 8, 2024 •

edited

Loading