Change behavior of cd - command

Since banister begged me to do that... completely rewrite `cd -`
command (implemetation is much simpler now).

This commit brings such changes:

  * completely rewrite behavior of `cd -` command;

  * implement ScratchPad aka Pad for unit testing purposes (by
    banister);

  * use Pad riches in the unit tests for `cd -` command;

  * remove verbose and clunky unit tests;

This commit brings new meaning to the `cd -` command. The main
difference is that the new command saves entire binding stack, not just
the last binding. Let me show you an example of the variance between
these two implemetations:

  * Old `cd -` implementation saves *only* last binding. With our next
    `cd -` invocation our interjacent results are lost:
      [1] pry(main)> cd 1/2/3/../4
      [2] pry(4):3> cd -
      [3] pry(main)> cd -
      [4] pry(4):1> nesting
      Nesting status:
      --
      0. main (Pry top level)
      1. 4

      Also, there are a few bugs in old `cd -` command:

        * you type `cd :foo`, `cd 1/2/3` and `cd -`. The last command
          relocates you to the scope of `3` (leaves you where you was),
          when `:foo` is expected;

        * you type `cd :foo`, `cd 1/2/3/../4`, `cd -`. The last command
          relocates you to the scope of `3`, when `:foo` is expected.

  * New and shiny `cd -` is devoid of those shortcomings:
      [1] pry(main)> cd 1/2/3/../4
      [2] pry(4):3> cd -
      [3] pry(main)> cd -
      [4] pry(4):3> nesting
      Nesting status:
      --
      0. main (Pry top level)
      1. 1
      2. 2
      3. 4

As I said before, this solution is *much* simpler and less error-prone.

Signed-off-by: Kyrylo Silin <kyrylosilin@gmail.com>

lib/pry.rb

@@ -103,16 +103,17 @@ class Pry
   # 3) In a nested session  - behave like `cd ..`       (pop a binding)
   DEFAULT_CONTROL_D_HANDLER = proc do |eval_string, _pry_|
     if !eval_string.empty?
-      # clear input buffer
+      # Clear input buffer.
       eval_string.replace("")
     elsif _pry_.binding_stack.one?
-      # ^D at top-level breaks out of a REPL loop
+      # ^D at top-level breaks out of a REPL loop.
       _pry_.binding_stack.clear
       throw(:breakout)
     else
-      # otherwise just pops a binding and stores it as old binding
+      # Otherwise, saves current binding stack as old stack and pops last
-      _pry_.command_state["cd"].old_binding = _pry_.binding_stack.pop
+      # binding out of binding stack (the old stack still has that binding).
-      _pry_.command_state["cd"].append = true
+      _pry_.command_state["cd"].old_stack = _pry_.binding_stack.dup
+      _pry_.binding_stack.pop
     end
   end
 

lib/pry/default_commands/cd.rb

@@ -24,48 +24,47 @@ class Pry
         def process
           # Extract command arguments. Delete blank arguments like " ", but
           # don't delete empty strings like "".
-          path  = arg_string.split(/\//).delete_if { |a| a =~ /\A\s+\z/ }
+          path      = arg_string.split(/\//).delete_if { |a| a =~ /\A\s+\z/ }
-          stack = _pry_.binding_stack.dup
+          stack     = _pry_.binding_stack.dup
-
+          old_stack = state.old_stack || []
-          # Save current state values for the sake of restoring them them later
-          # (for example, when an exception raised).
-          old_binding = state.old_binding
-          append      = state.append
 
           # Special case when we only get a single "/", return to root.
           if path.empty?
-            set_old_binding(stack.last, true) if old_binding
+            state.old_stack = stack.dup unless old_stack.empty?
             stack = [stack.first]
           end
 
-          path.each do |context|
+          path.each_with_index do |context, i|
             begin
               case context.chomp
               when ""
-                set_old_binding(stack.last, true)
+                state.old_stack = stack.dup
                 stack = [stack.first]
               when "::"
-                set_old_binding(stack.last, false)
+                state.old_stack = stack.dup
                 stack.push(TOPLEVEL_BINDING)
               when "."
                 next
               when ".."
                 unless stack.size == 1
-                  set_old_binding(stack.pop, true)
+                  # Don't rewrite old_stack if we're in complex expression
+                  # (e.g.: `cd 1/2/3/../4).
+                  state.old_stack = stack.dup if path.first == ".."
+                  stack.pop
                 end
               when "-"
-                if state.old_binding
+                unless old_stack.empty?
-                  toggle_old_binding(stack, old_binding, append)
+                  # Interchange current stack and old stack with each other.
+                  stack, state.old_stack = state.old_stack, stack
                 end
               else
-                unless path.length > 1
+                state.old_stack = stack.dup if i == 0
-                  set_old_binding(stack.last, false)
-                end
                 stack.push(Pry.binding_for(stack.last.eval(context)))
               end
 
             rescue RescuableException => e
-              set_old_binding(old_binding, append) # Restore previous values.
+              # Restore old stack to its initial values.
+              state.old_stack = old_stack
 
               output.puts "Bad object path: #{arg_string.chomp}. Failed trying to resolve: #{context}"
               output.puts e.inspect
@@ -76,42 +75,6 @@ class Pry
           _pry_.binding_stack = stack
         end
 
-        private
-
-        # Toggle old binding value by either appending it to the current stack
-        # (when `append` is `true`) or setting the new one (when `append` is
-        # `false`).
-        #
-        # @param [Array<Binding>] stack The current stack of bindings.
-        # @param [Binding] old_binding The old binding.
-        # @param [Boolean] append The adjunction flag.
-        #
-        # @return [Binding] The new old binding.
-        def toggle_old_binding(stack, old_binding, append)
-          if append
-            stack.push(old_binding)
-            old_binding = stack[-2]
-          else
-            old_binding = stack.pop
-          end
-          append = !append
-
-          set_old_binding(old_binding, append)
-
-          old_binding
-        end
-
-        # Set new old binding and adjunction flag.
-        #
-        # @param [Binding] binding The old binding.
-        # @param [Boolean] append The adjunction flag.
-        #
-        # @return [void]
-        def set_old_binding(binding, append)
-          state.old_binding = binding
-          state.append = append
-        end
-
       end
     end
   end

test/helper.rb

@@ -8,6 +8,12 @@ puts "Ruby v#{RUBY_VERSION} (#{defined?(RUBY_ENGINE) ? RUBY_ENGINE : "ruby"}), P
 require 'bacon'
 require 'open4'
 
+# A global space for storing temporary state during tests.
+ScratchPad = OpenStruct.new
+Pad = ScratchPad
+def ScratchPad.clear
+  @table = {}
+end
 
 # turn warnings off (esp for Pry::Hooks which will generate warnings
 # in tests)
@@ -56,7 +62,6 @@ class TestClassForShowSourceInstanceEval
   end
 end
 
-
 # in case the tests call reset_defaults, ensure we reset them to
 # amended (test friendly) values
 class << Pry

test/test_default_commands/test_cd.rb

@@ -1,272 +1,156 @@
 require 'helper'
 
 describe 'Pry::DefaultCommands::Cd' do
+  before do
+    @o = Object.new
+
+    @os1 = "Pad.os1 = _pry_.command_state['cd'].old_stack.dup"
+    @os2 = "Pad.os2 = _pry_.command_state['cd'].old_stack.dup"
+
+    @bs1 = "Pad.bs1 = _pry_.binding_stack.dup"
+    @bs2 = "Pad.bs2 = _pry_.binding_stack.dup"
+    @bs3 = "Pad.bs3 = _pry_.binding_stack.dup"
+  end
+
   after do
-    $obj = nil
+    Pad.clear
   end
 
   describe 'state' do
     it 'should not to be set up in fresh instance' do
-      instance = nil
+      redirect_pry_io(InputTester.new(@os1, "exit-all")) do
-      redirect_pry_io(InputTester.new("cd", "exit-all")) do
+        Pry.start(@o)
-        instance = Pry.new
-        instance.repl
       end
 
-      instance.command_state["cd"].old_binding.should == nil
+      Pad.os1.should == nil
-      instance.command_state["cd"].append.should == nil
     end
   end
 
-  describe 'old binding toggling with `cd -`' do
+  describe 'old stack toggling with `cd -`' do
+    describe 'in fresh pry instance' do
+      it 'should not toggle when there is no old stack' do
+        redirect_pry_io(InputTester.new("cd -", @bs1, "cd -", @bs2, "exit-all")) do
+          Pry.start(@o)
+        end
+
+        Pad.bs1.map { |v| v.eval("self") }.should == [@o]
+        Pad.bs2.map { |v| v.eval("self") }.should == [@o]
+      end
+    end
+
     describe 'when an error was raised' do
       it 'should ensure cd @ raises SyntaxError' do
         mock_pry("cd @").should =~ /SyntaxError/
       end
 
-      it 'should keep correct old binding' do
+      it 'should not toggle and should keep correct old stack' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd @", @os1, "cd -", @os2, "exit-all")) do
-        redirect_pry_io(InputTester.new("cd @", "cd -", "exit-all")) do
+          Pry.start(@o)
-          instance = Pry.new
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.should == nil
+        Pad.os1.should == []
-        instance.command_state["cd"].append.should == nil
+        Pad.os2.should == []
+      end
 
-        instance = nil
+      it 'should not toggle and should keep correct current binding stack' do
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd @", "exit-all")) do
+        redirect_pry_io(InputTester.new("cd @", @bs1, "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self")
+        Pad.bs1.map { |v| v.eval("self") }.should == [@o]
-        instance.command_state["cd"].append.should == false
+        Pad.bs2.map { |v| v.eval("self") }.should == [@o]
-
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd @", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :mon_dogg
-        instance.command_state["cd"].append.should == true
       end
     end
 
     describe 'when using simple cd syntax' do
-      it 'should keep correct old binding' do
+      it 'should toggle' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", @bs1,
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "exit-all")) do
+                                        "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self")
+        Pad.bs1.map { |v| v.eval("self") }.should == [@o]
-        instance.command_state["cd"].append.should == false
+        Pad.bs2.map { |v| v.eval("self") }.should == [@o, :mon_dogg]
+      end
+    end
+
+    describe "when using complex cd syntax" do
+      it 'should toggle with a complex path (simple case)' do
+        redirect_pry_io(InputTester.new("cd 1/2/3", "cd -", @bs1,
+                                        "cd -", @bs2, "exit-all")) do
+          Pry.start(@o)
+        end
+
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o]
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, 1, 2, 3]
       end
 
-      it 'should toggle with a single `cd -` call' do
+      it 'should toggle with a complex path (more complex case)' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd 1/2/3", "cd ../4", "cd -",
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "exit-all")) do
+                                        @bs1, "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == :mon_dogg
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o, 1, 2, 3]
-        instance.command_state["cd"].append.should == true
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, 1, 2, 4]
-      end
-
-      it 'should toggle with multple `cd -` calls' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self")
-        instance.command_state["cd"].append.should == false
       end
     end
 
     describe 'series of cd calls' do
-      it 'should keep correct old binding' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd 42", "cd :john_dogg", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == 42
-        instance.command_state["cd"].append.should == false
-      end
-
-      it 'should toggle with a single `cd -` call' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd 42", "cd :john_dogg", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
-        instance.command_state["cd"].append.should == true
-      end
-
-      it 'should toggle with multple `cd -` calls' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd 42", "cd :john_dogg", "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == 42
-        instance.command_state["cd"].append.should == false
-      end
-
       it 'should toggle with fuzzy `cd -` calls' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "cd 42", "cd -",
-        redirect_pry_io(InputTester.new("cd :mon_dogg", "cd -", "cd 42", "cd -", "exit-all")) do
+                                        @bs1, "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == 42
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o]
-        instance.command_state["cd"].append.should == true
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, 42]
       end
     end
 
     describe 'when using cd ..' do
-      before do
+      it 'should toggle with a simple path' do
-        $obj = Object.new
+        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", @bs1,
-        $obj.instance_variable_set(:@x, 66)
+                                        "cd -", @bs2, "exit-all")) do
-        $obj.instance_variable_set(:@y, 79)
+          Pry.start(@o)
+        end
+
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o]
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, :john_dogg]
       end
 
-      it 'should keep correct old binding' do
+      it 'should toggle with a complex path' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd 1/2/3/../4", "cd -", @bs1,
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", "exit-all")) do
+                                        "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o]
-        instance.command_state["cd"].append.should == true
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, 1, 2, 4]
-
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd $obj/@x/../@y", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == 66
-        instance.command_state["cd"].append.should == true
-      end
-
-      it 'should toggle with a single `cd -` call' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self")
-        instance.command_state["cd"].append.should == false
-
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd $obj/@x/../@y", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == 79
-        instance.command_state["cd"].append.should == false
-      end
-
-      it 'should toggle with multiple `cd -` calls' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ..", "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
-        instance.command_state["cd"].append.should == true
-
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd $obj/@x/../@y", "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == 66
-        instance.command_state["cd"].append.should == true
       end
     end
 
     describe 'when using cd ::' do
-      it 'should keep correct old binding' do
+      it 'should toggle' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd ::", "cd -", @bs1,
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ::", "exit-all")) do
+                                        "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o]
-        instance.command_state["cd"].append.should == false
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, TOPLEVEL_BINDING.eval("self")]
-      end
-
-      it 'should toggle with a single `cd -` call' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ::", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self")
-        instance.command_state["cd"].append.should == true
-      end
-
-      it 'should toggle with multiple `cd -` calls' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd ::", "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
-        instance.command_state["cd"].append.should == false
       end
     end
 
     describe 'when using cd /' do
-      it 'should keep correct old binding' do
+      it 'should toggle' do
-        instance = nil
+        redirect_pry_io(InputTester.new("cd /", "cd -", @bs1, "cd :john_dogg",
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd /", "exit-all")) do
+                                        "cd /", "cd -", @bs2, "exit-all")) do
-          instance = Pry.new
+          Pry.start(@o)
-          instance.repl
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o]
-        instance.command_state["cd"].append.should == true
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, :john_dogg]
-      end
-
-      it 'should toggle with a single `cd -` call' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd /", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == TOPLEVEL_BINDING.eval("self")
-        instance.command_state["cd"].append.should == false
-      end
-
-      it 'should toggle with multiple `cd -` calls' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd /", "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :john_dogg
-        instance.command_state["cd"].append.should == true
       end
     end
 
@@ -276,59 +160,16 @@ describe 'Pry::DefaultCommands::Cd' do
       end
 
       it 'should keep correct old binding' do
-        instance = nil
         redirect_pry_io(InputTester.new("cd :john_dogg", "cd :mon_dogg",
-                                        "cd :kyr_dogg", @control_d, "exit-all")) do
+                                        "cd :kyr_dogg", @control_d, @bs1, "cd -",
-          instance = Pry.new
+                                        @bs2, "cd -", @bs3, "exit-all")) do
-          instance.repl
+          Pry.start(@o)
         end
 
-        instance.command_state["cd"].old_binding.eval("self").should == :kyr_dogg
+        Pad.bs1.map { |v| v.eval('self') }.should == [@o, :john_dogg, :mon_dogg]
-        instance.command_state["cd"].append.should == true
+        Pad.bs2.map { |v| v.eval('self') }.should == [@o, :john_dogg, :mon_dogg, :kyr_dogg]
+        Pad.bs3.map { |v| v.eval('self') }.should == [@o, :john_dogg, :mon_dogg]
       end
-
-      it 'should toggle with a single `cd -` call' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd :mon_dogg",
-                                        "cd :kyr_dogg", @control_d, "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :mon_dogg
-        instance.command_state["cd"].append.should == false
-      end
-
-      it 'should toggle with multiple `cd -` calls' do
-        instance = nil
-        redirect_pry_io(InputTester.new("cd :john_dogg", "cd :mon_dogg",
-                                        "cd :kyr_dogg", @control_d, "cd -", "cd -", "exit-all")) do
-          instance = Pry.new
-          instance.repl
-        end
-
-        instance.command_state["cd"].old_binding.eval("self").should == :kyr_dogg
-        instance.command_state["cd"].append.should == true
-      end
-    end
-
-    it 'should not toggle when there is no old binding' do
-      instance = nil
-      redirect_pry_io(InputTester.new("cd -", "exit-all")) do
-        instance = Pry.new
-        instance.repl
-      end
-
-      instance.command_state["cd"].old_binding.should == nil
-      instance.command_state["cd"].append.should == nil
-
-      redirect_pry_io(InputTester.new("cd -", "cd -", "exit-all")) do
-        instance = Pry.new
-        instance.repl
-      end
-
-      instance.command_state["cd"].old_binding.should == nil
-      instance.command_state["cd"].append.should == nil
     end
   end