Macのrubyのreadlineに不具合がある！？(8)

Ruby Mac

ruby の Readline へのパッチを作成した。

パッチの概要を次に述べる。
GNU Readline を使用した場合の挙動は一切変えない。
パッチの適用前には、 EditLine(libedit) を使用する場合、次の問題がある。

  • libedit のバージョン (2.9より上?) によっては、 history_get 関数で指定するインデックスは 0 から始まる。 history_base からではない。これが原因で、 Readline::HISTORY ではユーザからの最初の入力を取得できない。
  • history_get 関数で -1 を指定すると 0 番目の history を取得する。 GNU Readline では NULL である。これが原因で、 Readline::HISTORY[-100] では IndexError 例外が発生しない。
  • replace_history_entry 関数で -1 を指定すると 0 番目の history を操作する。GNU Readline では操作できない。これが原因で、 Readline::HISTORY[-100] = "abc" などにより 0 番目の history を操作してしまう。

上記を回避するため、次のことを行う。

  • Init_readline 関数で libedit かどうかを判定する。
  • libedit であれば、 history の操作時の最初のインデックスの番号が history_base か history_base - 1 かを調査する。
  • 上記の情報を hist_get (Readline::HISTORY[] -> string | nil) や hist_set (Readline::HISTORY[index] = string) で使用する。

以下、パッチ。

Index: ext/readline/readline.c
===================================================================
--- readline.c	(revision 18084)
+++ readline.c	(working copy)
@@ -29,6 +29,8 @@
 
 static VALUE mReadline;
 
+#define EDIT_LINE_LIBRARY_VERSION "EditLine wrapper"
+
 #define COMPLETION_PROC "completion_proc"
 #define COMPLETION_CASE_FOLD "completion_case_fold"
 static ID completion_proc, completion_case_fold;
@@ -43,6 +45,9 @@
 # define rl_completion_matches completion_matches
 #endif
 
+static int use_editline = 0;
+static int history_offset = 0;
+
 static char **readline_attempted_completion_function(const char *text,
                                                      int start, int end);
 
@@ -516,7 +521,10 @@
     if (i < 0) {
         i += history_length;
     }
-    entry = history_get(history_base + i);
+    if (use_editline && i < history_offset)
+	entry = NULL;
+    else
+	entry = history_get(i + history_offset);
     if (entry == NULL) {
 	rb_raise(rb_eIndexError, "invalid index");
     }
@@ -536,7 +544,10 @@
     if (i < 0) {
         i += history_length;
     }
-    entry = replace_history_entry(i, RSTRING_PTR(str), NULL);
+    if (use_editline && i < history_offset)
+	entry = NULL;
+    else
+	entry = replace_history_entry(i, RSTRING_PTR(str), NULL);
     if (entry == NULL) {
 	rb_raise(rb_eIndexError, "invalid index");
     }
@@ -624,7 +635,7 @@
 
     rb_secure(4);
     for (i = 0; i < history_length; i++) {
-        entry = history_get(history_base + i);
+        entry = history_get(i + history_offset);
         if (entry == NULL)
             break;
 	rb_yield(rb_tainted_str_new2(entry->line));
@@ -795,6 +806,25 @@
     rb_define_const(mReadline, "USERNAME_COMPLETION_PROC", ucomp);
 #if defined HAVE_RL_LIBRARY_VERSION
     rb_define_const(mReadline, "VERSION", rb_str_new2(rl_library_version));
+    if (strncmp(rl_library_version, EDIT_LINE_LIBRARY_VERSION, 
+		strlen(EDIT_LINE_LIBRARY_VERSION)) == 0) {
+	HIST_ENTRY *entry;
+
+	use_editline = 1;
+	add_history("1");
+	add_history("2");
+	entry = history_get(history_base);
+	if (entry == NULL)
+	    /* history is not available. */
+	    history_offset = history_base;
+	else if (strncmp("1", entry->line, strlen("1")) == 0)
+	    history_offset = history_base;
+	else
+	    history_offset = history_base - 1;
+	clear_history();
+    }
+    else
+	history_offset = history_base;
 #else
     rb_define_const(mReadline, "VERSION", rb_str_new2("2.0 or prior version"));
 #endif

パッチを前田さん (Readline のメンテナ) に確認いただいた。
そして、次のことを言われた。

  • GNU Readline や EditLine 用の history へのアクセス位置を返す関数を用意する。
  • history のオフセットを調査する。
  • 調査結果を元にアクセス位置を返す関数のポインタを設定する。
  • history へのアクセスは上記の関数ポインタを使って行う。

はい。意図はわかりました。 hist_get 関数や hist_set 関数での条件分岐はしたくないということですね。条件分岐は Init_readline 関数で行ってしまうわけですね。
この場合のオーバーヘッドは、 history へのアクセス時に関数呼び出しが 1 回増えてしまうことです。条件分岐よりも関数呼び出しの方が処理に時間がかかると思いますが、そのオーバーヘッドは無視できるほど小さいと考えています。
メンテナンス性が上がると思います。さすが前田さん。

ということで、関数ポインタを使った版を作りました。コードがすっきりしたと思います。
その他の修正として、

  • offset のチェック部分を短くする
  • libedit を使用している場合に、 free(entry->line) の部分のコンパイル時に警告が出力されないように修正しました。これを修正するため libedit の entry->line を free している箇所をみました。
Index: readline.c
===================================================================
--- readline.c	(revision 18084)
+++ readline.c	(working copy)
@@ -29,6 +29,8 @@
 
 static VALUE mReadline;
 
+#define EDIT_LINE_LIBRARY_VERSION "EditLine wrapper"
+
 #define COMPLETION_PROC "completion_proc"
 #define COMPLETION_CASE_FOLD "completion_case_fold"
 static ID completion_proc, completion_case_fold;
@@ -43,6 +45,8 @@
 # define rl_completion_matches completion_matches
 #endif
 
+static int (*history_get_offset_func)(int);
+
 static char **readline_attempted_completion_function(const char *text,
                                                      int start, int end);
 
@@ -505,10 +509,22 @@
     return rb_str_new2("HISTORY");
 }
 
+static int
+history_get_offset_history_base(int offset)
+{
+    return history_base + offset;
+}
+
+static int
+history_get_offset_0(int offset)
+{
+    return offset;
+}
+
 static VALUE
 hist_get(VALUE self, VALUE index)
 {
-    HIST_ENTRY *entry;
+    HIST_ENTRY *entry = NULL;
     int i;
 
     rb_secure(4);
@@ -516,7 +532,9 @@
     if (i < 0) {
         i += history_length;
     }
-    entry = history_get(history_base + i);
+    if (i >= 0) {
+	entry = history_get(history_get_offset_func(i));
+    }
     if (entry == NULL) {
 	rb_raise(rb_eIndexError, "invalid index");
     }
@@ -527,7 +545,7 @@
 hist_set(VALUE self, VALUE index, VALUE str)
 {
 #ifdef HAVE_REPLACE_HISTORY_ENTRY
-    HIST_ENTRY *entry;
+    HIST_ENTRY *entry = NULL;
     int i;
 
     rb_secure(4);
@@ -536,7 +554,9 @@
     if (i < 0) {
         i += history_length;
     }
-    entry = replace_history_entry(i, RSTRING_PTR(str), NULL);
+    if (i >= 0) {
+	entry = replace_history_entry(i, RSTRING_PTR(str), NULL);
+    }
     if (entry == NULL) {
 	rb_raise(rb_eIndexError, "invalid index");
     }
@@ -581,7 +601,7 @@
     entry = remove_history(index);
     if (entry) {
         val = rb_tainted_str_new2(entry->line);
-        free(entry->line);
+        free((void *) entry->line);
         free(entry);
         return val;
     }
@@ -624,7 +644,7 @@
 
     rb_secure(4);
     for (i = 0; i < history_length; i++) {
-        entry = history_get(history_base + i);
+        entry = history_get(history_get_offset_func(i));
         if (entry == NULL)
             break;
 	rb_yield(rb_tainted_str_new2(entry->line));
@@ -793,8 +813,17 @@
     rb_define_singleton_method(ucomp, "call",
 			       username_completion_proc_call, 1);
     rb_define_const(mReadline, "USERNAME_COMPLETION_PROC", ucomp);
+    history_get_offset_func = history_get_offset_history_base;
 #if defined HAVE_RL_LIBRARY_VERSION
     rb_define_const(mReadline, "VERSION", rb_str_new2(rl_library_version));
+    if (strncmp(rl_library_version, EDIT_LINE_LIBRARY_VERSION, 
+		strlen(EDIT_LINE_LIBRARY_VERSION)) == 0) {
+	add_history("1");
+	if (history_get(history_get_offset_func(0)) == NULL) {
+	    history_get_offset_func = history_get_offset_0;
+	}
+	clear_history();
+    }
 #else
     rb_define_const(mReadline, "VERSION", rb_str_new2("2.0 or prior version"));
 #endif

これで、私がやりたかったことは実現できたので満足です。
あとは、報告があった Ubuntu 8.04 の libedit、Mac OSX 10.4 の libedit で試していただける方を募集し、その結果をもらって改善していくという感じですね。