Suggestion for some changes in box.rb

[rrepen]

I would like to suggest the following changes in the run_command method in box.rb. The purpose of that is to always get a coherent output of the command run, in any circonstances, specially when connections problems occurs.

1. cmd_clean need to be declare at the beginning so in case of exception the rescue block gets it set to the real value and not nil
2. move the begin statement right at the beginning so failure in "connect" is captured by the rescue block and then we can go through the "@rye_exception_hook" to treat it which is a real nice feature
3. Move the connect after the the command has been "cleaned", I mean after line "rap.cmd = cmd_clean", so in case of connect problem the cmd_clean is already set
4. Move the line "rap = Rye::Rap.new(self)" rigth at the beginning before the begin block, so it is available on return
5. I would also see the begin... rescue block be put in a timeout so the command will not hung for ever.

Below is the run_command I suggest:

def run_command(*args, &blk)
  debug "run_command"
  cmd_clean = nil
  rap = Rye::Rap.new(self)

  cmd, args = prep_args(*args)

  begin
    Timeout::timeout(@timeout) do |the_timeout|
      cmd_clean = Rye.escape(@rye_safe, cmd, args)

      # This following is the command we'll actually execute. cmd_clean
      # can be used for logging, otherwise the output is confusing.
      cmd_internal = prepend_env(cmd_clean)

      # Add the current working directory before the command if supplied. 
      # The command will otherwise run in the user's home directory.
      if @rye_current_working_directory
        cwd = Rye.escape(@rye_safe, 'cd', @rye_current_working_directory)
        cmd_internal = '(%s; %s)' % [cwd, cmd_internal]
      end

      # ditto (same explanation as cwd)
      if @rye_current_umask
        cwd = Rye.escape(@rye_safe, 'umask', @rye_current_umask)
        cmd_internal = [cwd, cmd_internal].join(' && ')
      end

      ## NOTE: Do not raise a CommandNotFound exception in this method.
      # We want it to be possible to define methods to a single instance
      # of Rye::Box. i.e. def rbox.rm()...
      # can? returns the methods in Rye::Cmd so it would incorrectly
      # return false. We could use self.respond_to? but it's possible
      # to get a name collision. I could write a work around but I think
      # this is good enough for now. 
      ## raise Rye::CommandNotFound unless self.can?(cmd)

      # begin
      debug "COMMAND: #{cmd_internal}"

      if !@rye_quiet && @rye_pre_command_hook.is_a?(Proc)
        @rye_pre_command_hook.call(cmd_clean, user, host, nickname) 
      end

      #rap = Rye::Rap.new(self)
      rap.cmd = cmd_clean

      connect if !@rye_ssh || @rye_ssh.closed?

      raise Rye::NotConnected, @rye_host unless @rye_ssh && !@rye_ssh.closed?

      channel = net_ssh_exec!(cmd_internal, &blk)
      channel[:stderr].position = 0
      channel[:stdout].position = 0

      if channel[:exception]
        rap = channel[:exception].rap
      else
        rap.add_stdout(channel[:stdout].read || '')
        rap.add_stderr(channel[:stderr].read || '')
        rap.add_exit_status(channel[:exit_status])
        rap.exit_signal = channel[:exit_signal]
      end

      debug "RESULT: %s " % [rap.inspect]
      # It seems a convention for various commands to return -1
      # when something only mildly concerning happens. (ls even 
      # returns -1 for apparently no reason sometimes). Anyway,
      # the real errors are the ones that are greater than zero.
      raise Rye::Err.new(rap) if rap.exit_status != 0
    end
  rescue Exception => ex
    return rap if @rye_quiet
    choice = nil
    @rye_exception_hook.each_pair do |klass,act|
      next unless ex.kind_of? klass
      choice = act.call(ex, cmd_clean, user, host, nickname)
      break
    end
    if choice == :retry
      retry
    elsif choice == :skip
      # do nothing
    elsif choice == :interactive && !@rye_shell
      @rye_shell = true
      previous_state = @rye_sudo
      disable_sudo
      bash
      @rye_sudo = previous_state
      @rye_shell = false
    elsif !ex.is_a?(Interrupt)
      raise ex, ex.message,ex.backtrace
    end
  end

  if !@rye_quiet && @rye_post_command_hook.is_a?(Proc)
    @rye_post_command_hook.call(rap)
  end
  rap
end

[delano]

Thanks for this code review. I'll take a look soon and get back to you.