From 704cc6e859470a75370f04ba8f9dc750276ac2c5 Mon Sep 17 00:00:00 2001 From: Kyrylo Silin Date: Thu, 23 Aug 2012 02:27:02 +0300 Subject: [PATCH] Store `hist --replay ` calls in history Fix issue #484 (hist --replay N isn't stored in history). First, make some small amendments to existing code: * Make helper methods of "hist" command private; * What an irony! Amend the name of the duplicated test in `test_input.rb` ("should not contain duplicated lines" test). Secondly, resolve the issue. There is one notable moment in current implementation. Although `hist --replay` calls are being stored in history, you cannot "replay" entries of this kind (you cannot replay another call request to replay). Let me show an example: [1] pry(main)> hist --show 46894 46894: hist --replay 46675..46677 [2] pry(main)> hist --show 46675..46677 46675: 1+1 46676: a = 100 46677: hist --tail [3] pry(main)> hist --replay 46894 Error: Replay index 46894 points out to another replay call: `hist -r 46675..46677` [4] pry(main)> There are two reasons for that. Reason one or my incompetence ----------------------------- First of all, I simply failed to implement such behaviour. With current state of things (that are introduced in this commit), if you do not raise `Pry::CommandError`, you cannot guarantee that only user's input is getting stored in history. Here's an example when we get unwanted entry in history: [1] pry(main)> hist --show 46894 46894: hist --replay 46675..46677 [2] pry(main)> hist --show 46675..46677 46675: 1+1 46676: a = 100 46677: hist --tail 4 [3] pry(main)> hist --replay 46894 => 2 => 100 47021: hist --show 46894 47022: hist --show 46675..46677 47023: hist --replay 46894 47024: hist --replay 46675..46677 [8] pry(main)> Note that a user typed only `hist --replay 46894`. But the last saved entry in history is the entry to which user's input, actually, pointed out (`hist --replay 46675..46677`). So if you press up-arrow key, you will get not what you expected. Reason two or "Whoa, whoa, boy! There is a real reason" ------------------------------------------------------- But the main reason is that you can fall into a loop trap, when both "hist --replay" calls point to each other. Example of a loop trap: [31] pry(main)> hist --tail 4 47027: hist --tail 47028: hist --replay 47028 47029: hist --tail 47030: hist --replay 47032 [32] pry(main)> hist -r 47030 # We've just fallen into a loop trap. Let's break out of it. ^C [416] pry(main)> hist --tail 5 47409: hist --replay 47032 47410: hist --replay 47030 47411: hist --replay 47032 47412: hist --replay 47030 47413: hist --replay 47032 [417] pry(main)> Note the number of current line (417). Finally, add some unit tests for this commit. Signed-off-by: Kyrylo Silin --- lib/pry/commands/hist.rb | 44 +++++++++++++++++++++++- lib/pry/pry_instance.rb | 10 +++++- test/test_default_commands/test_input.rb | 43 ++++++++++++++++++++++- 3 files changed, 94 insertions(+), 3 deletions(-) diff --git a/lib/pry/commands/hist.rb b/lib/pry/commands/hist.rb index 055432ce..2ff5cbff 100644 --- a/lib/pry/commands/hist.rb +++ b/lib/pry/commands/hist.rb @@ -65,6 +65,8 @@ class Pry end end + private + def process_display unless opts.present?(:'no-numbers') @history = @history.with_line_numbers @@ -101,12 +103,52 @@ class Pry def process_replay @history = @history.between(opts[:r]) + replay_sequence = @history.raw + + check_for_juxtaposed_replay(replay_sequence) _pry_.input_stack.push _pry_.input - _pry_.input = StringIO.new(@history.raw) + _pry_.input = StringIO.new(replay_sequence) # eval_string << "#{@history.raw}\n" # run "show-input" unless _pry_.complete_expression?(eval_string) end + + # If we met another an extra "hist" call to be replayed, check for the + # "--replay" option presence. If "hist" command is called with other options + # then proceed further. Example: + # [1] pry(main)> hist --show 46894 + # 46894: hist --replay 46675..46677 + # [2] pry(main)> hist --show 46675..46677 + # 46675: 1+1 + # 46676: a = 100 + # 46677: hist --tail + # [3] pry(main)> hist --replay 46894 + # Error: Replay index 46894 points out to another replay call: `hist -r 46675..46677` + # [4] pry(main)> + # + # @raise [Pry::CommandError] If +replay_sequence+ contains another + # "hist --replay" call + # @param [String] replay_sequence The sequence of commands to be replayed + # (per saltum) + # @return [Boolean] `false` if +replay_sequence+ does not contain another + # "hist --replay" call + def check_for_juxtaposed_replay(replay_sequence) + if replay_sequence =~ /\Ahist(?:ory)?\b/ + # Create *fresh* instance of Slop for parsing of "hist" command. + _slop = self.slop + _slop.parse replay_sequence.split(" ")[1..-1] + + if _slop.present?(:r) + replay_sequence = replay_sequence.split("\n").join("; ") + index = opts[:r] + index = index.min if index.min == index.max + + raise CommandError, "Replay index #{ index } points out to another replay call: `#{ replay_sequence }`" + end + else + false + end + end end Pry::Commands.alias_command "history", "hist" diff --git a/lib/pry/pry_instance.rb b/lib/pry/pry_instance.rb index 5919beb9..f228a022 100644 --- a/lib/pry/pry_instance.rb +++ b/lib/pry/pry_instance.rb @@ -409,7 +409,15 @@ class Pry eval_string << "#{indented_val.chomp}\n" unless val.empty? end ensure - Pry.history << indented_val unless input.is_a?(StringIO) + if input.is_a?(StringIO) + # Add to history only those values that were typed by a user. Ignore + # programmatically created ones. + unless input.string.include?(indented_val) + Pry.history << indented_val.chomp + end + else + Pry.history << indented_val + end end end diff --git a/test/test_default_commands/test_input.rb b/test/test_default_commands/test_input.rb index f411e0ea..f2884f43 100644 --- a/test/test_default_commands/test_input.rb +++ b/test/test_default_commands/test_input.rb @@ -428,7 +428,7 @@ describe "Pry::DefaultCommands::Input" do @str_output.string.each_line.grep(/_ \+= 1/).count.should == 1 end - it "should not contain duplicated lines" do + it "should not contain empty lines" do redirect_pry_io(InputTester.new(":place_holder", "2 + 2", "", "", "3 + 3", "hist", "exit-all"), @str_output) do pry end @@ -438,5 +438,46 @@ describe "Pry::DefaultCommands::Input" do (a + 1).should == b end + + it "should store a call with `--replay` flag" do + redirect_pry_io(InputTester.new(":banzai", "hist --replay 1", + "hist", "exit-all"), @str_output) do + Pry.start + end + + @str_output.string.should =~ /hist --replay 1/ + end + + it "should not contain lines produced by `--replay` flag" do + redirect_pry_io(InputTester.new(":banzai", ":geronimo", ":huzzah", + "hist --replay 1..3", "hist", + "exit-all"), @str_output) do + Pry.start + end + + @str_output.string.each_line.to_a.reject { |line| line.start_with?("=>") }.size.should == 4 + @str_output.string.each_line.to_a.last.should =~ /hist --replay 1\.\.3/ + @str_output.string.each_line.to_a[-2].should =~ /:huzzah/ + end + + it "should raise CommandError when index of `--replay` points out to another `hist --replay`" do + redirect_pry_io(InputTester.new(":banzai", "hist --replay 1", + "hist --replay 2", "exit-all"), @str_output) do + Pry.start + end + + @str_output.string.should =~ /Replay index 2 points out to another replay call: `hist --replay 1`/ + end + + it "should disallow execution of `--replay ` when CommandError raised" do + redirect_pry_io(InputTester.new("a = 0", "a += 1", "hist --replay 2", + "hist --replay 3", "'a is ' + a.to_s", + "hist", "exit-all"), @str_output) do + Pry.start + end + + @str_output.string.each_line.to_a.reject { |line| line !~ /\A\d/ }.size.should == 5 + @str_output.string.should =~ /a is 2/ + end end end