ruby · nobu · Jun 26, 2012
diff --git a/internal.h b/internal.h
@@ -309,6 +309,8 @@ VALUE rb_execarg_init(int argc, VALUE *argv, int accept_shell, VALUE execarg_obj
 int rb_execarg_addopt(VALUE execarg_obj, VALUE key, VALUE val);
 void rb_execarg_fixup(VALUE execarg_obj);
 int rb_execarg_run_options(const struct rb_execarg *e, struct rb_execarg *s, char* errmsg, size_t errmsg_buflen);
+VALUE rb_execarg_extract_options(VALUE execarg_obj, VALUE opthash);
+void rb_execarg_setenv(VALUE execarg_obj, VALUE env);
 
 #if defined __GNUC__ && __GNUC__ >= 4
 #pragma GCC visibility pop

diff --git a/io.c b/io.c
@@ -5692,41 +5692,36 @@ pipe_open(VALUE execarg_obj, const char *modestr, int fmode, convconfig_t *convc
     return port;
 }
 
-static VALUE
-pipe_open_v(int argc, VALUE *argv, const char *modestr, int fmode, convconfig_t *convconfig)
+static int
+is_popen_fork(VALUE prog)
 {
-    VALUE execarg_obj, ret;
-    execarg_obj = rb_execarg_new(argc, argv, FALSE);
-    ret = pipe_open(execarg_obj, modestr, fmode, convconfig);
-    return ret;
+    if (RSTRING_LEN(prog) == 1 && RSTRING_PTR(prog)[0] == '-') {
+#if !defined(HAVE_FORK)
+	rb_raise(rb_eNotImpError,
+		 "fork() function is unimplemented on this machine");
+#else
+	return TRUE;
+#endif
+    }
+    return FALSE;
 }
 
 static VALUE
 pipe_open_s(VALUE prog, const char *modestr, int fmode, convconfig_t *convconfig)
 {
-    const char *cmd = RSTRING_PTR(prog);
     int argc = 1;
     VALUE *argv = &prog;
-    VALUE execarg_obj, ret;
+    VALUE execarg_obj = Qnil;
 
-    if (RSTRING_LEN(prog) == 1 && cmd[0] == '-') {
-#if !defined(HAVE_FORK)
-	rb_raise(rb_eNotImpError,
-		 "fork() function is unimplemented on this machine");
-#else
-        return pipe_open(Qnil, modestr, fmode, convconfig);
-#endif
-    }
-
-    execarg_obj = rb_execarg_new(argc, argv, TRUE);
-    ret = pipe_open(execarg_obj, modestr, fmode, convconfig);
-    return ret;
+    if (!is_popen_fork(prog))
+	execarg_obj = rb_execarg_new(argc, argv, TRUE);
+    return pipe_open(execarg_obj, modestr, fmode, convconfig);
 }
 
 /*
  *  call-seq:
- *     IO.popen(cmd, mode="r" [, opt])               -> io
- *     IO.popen(cmd, mode="r" [, opt]) {|io| block } -> obj
+ *     IO.popen([env,] cmd, mode="r" [, opt])               -> io
+ *     IO.popen([env,] cmd, mode="r" [, opt]) {|io| block } -> obj
  *
  *  Runs the specified command as a subprocess; the subprocess's
  *  standard input and output will be connected to the returned
@@ -5766,6 +5761,11 @@ pipe_open_s(VALUE prog, const char *modestr, int fmode, convconfig_t *convconfig
  *      ls_result_with_error = ls_io.read
  *    }
  *
+ *    # spawn options can be mixed with IO options
+ *    IO.popen(["ls", "/"], :err=>[:child, :out]) {|ls_io|
+ *      ls_result_with_error = ls_io.read
+ *    }
+ *
  *  Raises exceptions which <code>IO.pipe</code> and
  *  <code>Kernel.spawn</code> raise.
  *
@@ -5810,14 +5810,24 @@ static VALUE
 rb_io_s_popen(int argc, VALUE *argv, VALUE klass)
 {
     const char *modestr;
-    VALUE pname, pmode, port, tmp, opt;
+    VALUE pname, pmode = Qnil, port, tmp, opt = Qnil, env = Qnil, execarg_obj = Qnil;
     int oflags, fmode;
     convconfig_t convconfig;
 
-    argc = rb_scan_args(argc, argv, "11:", &pname, &pmode, &opt);
-
-    rb_io_extract_modeenc(&pmode, 0, opt, &oflags, &fmode, &convconfig);
-    modestr = rb_io_oflags_modestr(oflags);
+    if (argc > 1 && !NIL_P(opt = rb_check_hash_type(argv[argc-1]))) --argc;
+    if (argc > 1 && !NIL_P(env = rb_check_hash_type(argv[0]))) --argc, ++argv;
+    switch (argc) {
+      case 2:
+	pmode = argv[1];
+      case 1:
+	pname = argv[0];
+	break;
+      default:
+	{
+	    int ex = !NIL_P(opt);
+	    rb_error_arity(argc + ex, 1 + ex, 2 + ex);
+	}
+    }
 
     tmp = rb_check_array_type(pname);
     if (!NIL_P(tmp)) {
@@ -5829,13 +5839,25 @@ rb_io_s_popen(int argc, VALUE *argv, VALUE klass)
 #endif
 	tmp = rb_ary_dup(tmp);
 	RBASIC(tmp)->klass = 0;
-	port = pipe_open_v((int)len, RARRAY_PTR(tmp), modestr, fmode, &convconfig);
+	execarg_obj = rb_execarg_new((int)len, RARRAY_PTR(tmp), FALSE);
 	rb_ary_clear(tmp);
     }
     else {
 	SafeStringValue(pname);
-	port = pipe_open_s(pname, modestr, fmode, &convconfig);
+	execarg_obj = Qnil;
+	if (!is_popen_fork(pname))
+	    execarg_obj = rb_execarg_new(1, &pname, TRUE);
     }
+    if (!NIL_P(execarg_obj)) {
+	if (!NIL_P(opt))
+	    opt = rb_execarg_extract_options(execarg_obj, opt);
+	if (!NIL_P(env))
+	    rb_execarg_setenv(execarg_obj, env);
+    }
+    rb_io_extract_modeenc(&pmode, 0, opt, &oflags, &fmode, &convconfig);
+    modestr = rb_io_oflags_modestr(oflags);
+
+    port = pipe_open(execarg_obj, modestr, fmode, &convconfig);
     if (NIL_P(port)) {
 	/* child */
 	if (rb_block_given_p()) {

diff --git a/process.c b/process.c
@@ -1654,8 +1654,7 @@ rb_execarg_addopt(VALUE execarg_obj, VALUE key, VALUE val)
             goto redirect;
         }
         else {
-            rb_raise(rb_eArgError, "wrong exec option symbol: %s",
-                                   rb_id2name(id));
+	    return ST_STOP;
         }
         break;
 
@@ -1667,7 +1666,7 @@ rb_execarg_addopt(VALUE execarg_obj, VALUE key, VALUE val)
         break;
 
       default:
-        rb_raise(rb_eArgError, "wrong exec option");
+	return ST_STOP;
     }
 
     RB_GC_GUARD(execarg_obj);
@@ -1686,7 +1685,28 @@ check_exec_options_i(st_data_t st_key, st_data_t st_val, st_data_t arg)
     VALUE key = (VALUE)st_key;
     VALUE val = (VALUE)st_val;
     VALUE execarg_obj = (VALUE)arg;
-    return rb_execarg_addopt(execarg_obj, key, val);
+    if (rb_execarg_addopt(execarg_obj, key, val) != ST_CONTINUE) {
+	if (SYMBOL_P(key))
+	    rb_raise(rb_eArgError, "wrong exec option symbol: %"PRIsVALUE,
+		     key);
+	rb_raise(rb_eArgError, "wrong exec option");
+    }
+    return ST_CONTINUE;
+}
+
+static int
+check_exec_options_i_extract(st_data_t st_key, st_data_t st_val, st_data_t arg)
+{
+    VALUE key = (VALUE)st_key;
+    VALUE val = (VALUE)st_val;
+    VALUE *args = (VALUE *)arg;
+    VALUE execarg_obj = args[0];
+    if (rb_execarg_addopt(execarg_obj, key, val) != ST_CONTINUE) {
+	VALUE nonopts = args[1];
+	if (NIL_P(nonopts)) args[1] = nonopts = rb_hash_new();
+	rb_hash_aset(nonopts, key, val);
+    }
+    return ST_CONTINUE;
 }
 
 static int
@@ -1775,6 +1795,18 @@ rb_check_exec_options(VALUE opthash, VALUE execarg_obj)
     st_foreach(RHASH_TBL(opthash), check_exec_options_i, (st_data_t)execarg_obj);
 }
 
+VALUE
+rb_execarg_extract_options(VALUE execarg_obj, VALUE opthash)
+{
+    VALUE args[2];
+    if (RHASH_EMPTY_P(opthash))
+        return Qnil;
+    args[0] = execarg_obj;
+    args[1] = Qnil;
+    st_foreach(RHASH_TBL(opthash), check_exec_options_i_extract, (st_data_t)args);
+    return args[1];
+}
+
 static int
 check_exec_env_i(st_data_t st_key, st_data_t st_val, st_data_t arg)
 {
@@ -2093,6 +2125,14 @@ rb_exec_arg_init(int argc, VALUE *argv, int accept_shell, struct rb_exec_arg *e)
     return rb_execarg_init(argc, argv, accept_shell, e->execarg_obj);
 }
 
+void
+rb_execarg_setenv(VALUE execarg_obj, VALUE env)
+{
+    struct rb_execarg *eargp = rb_execarg_get(execarg_obj);
+    env = !NIL_P(env) ? rb_check_exec_env(env) : Qfalse;
+    eargp->env_modification = env;
+}
+
 static int
 fill_envp_buf_i(st_data_t st_key, st_data_t st_val, st_data_t arg)
 {

diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb
@@ -303,6 +303,47 @@ def test_execopts_env
     end
   end
 
+  def _test_execopts_env_popen(cmd)
+    message = cmd.inspect
+    IO.popen({"FOO"=>"BAR"}, cmd) {|io|
+      assert_equal('FOO=BAR', io.read[/^FOO=.*/], message)
+    }
+
+    old = ENV["hmm"]
+    begin
+      ENV["hmm"] = "fufu"
+      IO.popen(cmd) {|io| assert_match(/^hmm=fufu$/, io.read, message)}
+      IO.popen({"hmm"=>""}, cmd) {|io| assert_match(/^hmm=$/, io.read, message)}
+      IO.popen({"hmm"=>nil}, cmd) {|io| assert_not_match(/^hmm=/, io.read, message)}
+      ENV["hmm"] = ""
+      IO.popen(cmd) {|io| assert_match(/^hmm=$/, io.read, message)}
+      IO.popen({"hmm"=>""}, cmd) {|io| assert_match(/^hmm=$/, io.read, message)}
+      IO.popen({"hmm"=>nil}, cmd) {|io| assert_not_match(/^hmm=/, io.read, message)}
+      ENV["hmm"] = nil
+      IO.popen(cmd) {|io| assert_not_match(/^hmm=/, io.read, message)}
+      IO.popen({"hmm"=>""}, cmd) {|io| assert_match(/^hmm=$/, io.read, message)}
+      IO.popen({"hmm"=>nil}, cmd) {|io| assert_not_match(/^hmm=/, io.read, message)}
+    ensure
+      ENV["hmm"] = old
+    end
+  end
+
+  def test_execopts_env_popen_vector
+    _test_execopts_env_popen(ENVCOMMAND)
+  end
+
+  def test_execopts_env_popen_string
+    with_tmpchdir do |d|
+      open('test-script', 'w') do |f|
+        ENVCOMMAND.each_with_index do |cmd, i|
+          next if i.zero? or cmd == "-e"
+          f.puts cmd
+        end
+      end
+      _test_execopts_env_popen("#{RUBY} test-script")
+    end
+  end
+
   def test_execopts_preserve_env_on_exec_failure
     with_tmpchdir {|d|
       write_file 's', <<-"End"