Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

History file being overidden if initially empty #388

Closed
krystof-k opened this issue Aug 5, 2022 · 14 comments · Fixed by #389
Closed

History file being overidden if initially empty #388

krystof-k opened this issue Aug 5, 2022 · 14 comments · Fixed by #389

Comments

@krystof-k
Copy link

krystof-k commented Aug 5, 2022

Description

I'm running multiple containers via Docker Compose with all of them mapped to a single .irb-history file which is placed on Docker volume. When they are launched, the (last?) one overrides the history.

Consider following example:

  • Dockerfile:

    FROM ruby:alpine
    RUN echo "IRB.conf[:HISTORY_FILE] = ENV['IRB_HISTORY_FILE'] if ENV['IRB_HISTORY_FILE']" >> ~/.irbrc
  • docker-compose.yaml:

    services:
      first:
        build:
          context: .
        volumes:
          - history:/root/.history
        environment:
          HISTFILE: /root/.history/.bash_history
          IRB_HISTORY_FILE: /root/.history/.irb-history
        command: tail -f /dev/null
      second:
        extends:
          service: first
    volumes:
      history:
  • run:

    docker compose up -d

    run in another terminal:

    docker compose exec first irb
    puts 'first'

    run in another terminal:

    docker compose exec second irb
    puts 'second'

    run in first terminal:

    docker compose down

    run in first terminal:

    docker compose run first cat /root/.history/.irb-history

    result:

    puts 'second'

Result of irb_info

irb(main):001:0> irb_info
=> 
Ruby version: 3.1.2                            
IRB version: irb 1.4.1 (2021-12-25)            
InputMethod: ReidlineInputMethod with Reline 0.3.0
.irbrc path: /root/.irbrc                      
RUBY_PLATFORM: aarch64-linux-musl              
LANG env: C.UTF-8                              
East Asian Ambiguous Width: 1 

Setting Files

~/.irbrc:

IRB.conf[:HISTORY_FILE] = ENV['IRB_HISTORY_FILE'] if ENV['IRB_HISTORY_FILE']
@krystof-k
Copy link
Author

krystof-k commented Aug 5, 2022

I understood that something that should probably handle this was done by @jeremyevans in #193, but it is definitely not working in this case.

I also tried "initializing" the file in the Dockerfile already (putting some content in it), but that didn't help.

@jeremyevans
Copy link
Contributor

If you can provide steps to reproduce this issue without docker, I will look into it.

@krystof-k
Copy link
Author

I have no idea how. However exactly the same approach for .bash_history is working with no issue.

@krystof-k
Copy link
Author

krystof-k commented Aug 5, 2022

OK, maybe I did.

First – ensure there is no history file existing.

Launch IRB in two windows, enter someting in the first, exit, enter something in the second, exit. You will have only the history from the second.

1st terminal:

  • cd ~
  • rm -f .irb_history
  • irb
  • puts 'first'
  • exit

2nd terminal:

  • irb
  • puts 'second'
  • exit

1st terminal:

  • cat .irb_history:
    puts 'second'
    exit
    

@jeremyevans
Copy link
Contributor

Thank you for that. I can reproduce the issue. I'll look into it, and hopefully have a fix today or next week.

@krystof-k
Copy link
Author

Thanks. Unfortunately I'm afraid that this is not the same issue I'm actually facing in the Docker Compose setup with Rails :(

@jeremyevans
Copy link
Contributor

Can you try this patch?:

diff --git a/lib/irb/ext/save-history.rb b/lib/irb/ext/save-history.rb
index 7acaebe..fd5db16 100644
--- a/lib/irb/ext/save-history.rb
+++ b/lib/irb/ext/save-history.rb
@@ -81,9 +81,9 @@ module IRB
             end
           }
         end
-        @loaded_history_lines = history.size
-        @loaded_history_mtime = File.mtime(history_file)
       end
+      @loaded_history_lines = history.size
+      nil
     end

     def save_history
@@ -107,21 +107,10 @@ module IRB
           raise
         end

-        if File.exist?(history_file) && @loaded_history_mtime &&
-           File.mtime(history_file) != @loaded_history_mtime
-          history = history[@loaded_history_lines..-1]
-          append_history = true
-        end
+        history = history[@loaded_history_lines..-1]

-        open(history_file, "#{append_history ? 'a' : 'w'}:#{IRB.conf[:LC_MESSAGES].encoding}", 0600) do |f|
+        open(history_file, "a:#{IRB.conf[:LC_MESSAGES].encoding}", 0600) do |f|
           hist = history.map{ |l| l.split("\n").join("\\\n") }
-          unless append_history
-            begin
-              hist = hist.last(num) if hist.size > num and num > 0
-            rescue RangeError # bignum too big to convert into `long'
-              # Do nothing because the bignum should be treated as inifinity
-            end
-          end
           f.puts(hist)
         end
       end

@krystof-k
Copy link
Author

Is it enough if I just edit the file in /usr/local/bundle/gems/irb-1.4.1?

@jeremyevans
Copy link
Contributor

That should work.

@krystof-k
Copy link
Author

Yes! That seems to be working (my original issue in Docker Compose no longer occurs).

@jeremyevans
Copy link
Contributor

OK. Let me update the tests and send a pull request for this.

@jeremyevans
Copy link
Contributor

Looks like my previous diff fixed one issue, but broke others. Here's a much simpler diff that doesn't break any tests:

diff --git a/lib/irb/ext/save-history.rb b/lib/irb/ext/save-history.rb
index 7acaebe..83d0710 100644
--- a/lib/irb/ext/save-history.rb
+++ b/lib/irb/ext/save-history.rb
@@ -107,7 +107,7 @@ module IRB
           raise
         end

-        if File.exist?(history_file) && @loaded_history_mtime &&
+        if File.exist?(history_file) &&
            File.mtime(history_file) != @loaded_history_mtime
           history = history[@loaded_history_lines..-1]
           append_history = true

If there is no history file originally, @loaded_history_mtime is not set, but we should still append in that case, as it is obvious the file has changed.

jeremyevans added a commit to jeremyevans/irb that referenced this issue Aug 5, 2022
…e doesn't exist

If history file didn't exist when irb was started, @loaded_history_mtime
would be nil.  However, if the history file didn't exist before, but it
exists when saving history, that means the history file was modified,
and we should handle it the same way as we handle the other case where
the history file was modified.

Fixes ruby#388
@jeremyevans
Copy link
Contributor

OK, I submitted #389 to fix this

@krystof-k
Copy link
Author

That seems to be working as well. Awesome, many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants