From 9b46379f75f384c86e42046ab03ce55231197c92 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Fri, 24 Dec 2010 13:23:32 -0800
Subject: accept a new :path argument in initialize

This lessens confusion for people configuring Clogger in
config.ru, since "File" could be mistaken for Rack::File
and "::File" needs to be specified.
---
 README                    | 10 ++++++++--
 ext/clogger_ext/clogger.c | 35 +++++++++++++++++++++++++----------
 lib/clogger/pure.rb       |  9 +++++++--
 test/test_clogger.rb      | 18 ++++++++++++++++++
 4 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/README b/README
index 59a6ced..c61e0da 100644
--- a/README
+++ b/README
@@ -36,7 +36,7 @@ is customizable so you can specify exactly which fields to log.
   require "clogger"
   use Clogger,
       :format => Clogger::Format::Combined,
-      :logger => ::File.open("/path/to/log", "ab"),
+      :path => "/path/to/log",
       :reentrant => true
   run YourApplication.new
 
@@ -45,9 +45,15 @@ somewhere inside the "Rails::Initializer.run do |config|" block:
 
   config.middleware.use 'Clogger',
       :format => Clogger::Format::Combined,
-      :logger => ::File.open("/path/to/log", "ab"),
+      :path => "/path/to/log",
       :reentrant => false
 
+Instead of specifying a :path, you may also specify a :logger object
+that receives a "<<" method:
+
+  use Clogger, :logger=> $stdout, :reentrant => true
+  run YourApplication.new
+
 == VARIABLES
 
 * $http_* - HTTP request headers (e.g. $http_user_agent)
diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index ad5bbb9..09eeffd 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -213,11 +213,6 @@ static VALUE obj_fileno(VALUE obj)
 	return rb_funcall(obj, rb_intern("fileno"), 0);
 }
 
-static VALUE obj_enable_sync(VALUE obj)
-{
-	return rb_funcall(obj, rb_intern("sync="), 1, Qtrue);
-}
-
 /* only for writing to regular files, not stupid crap like NFS  */
 static void write_full(int fd, const void *buf, size_t count)
 {
@@ -588,12 +583,35 @@ static VALUE cwrite(struct clogger *c)
 	return Qnil;
 }
 
+static void init_logger(struct clogger *c, VALUE path)
+{
+	ID id;
+
+	if (!NIL_P(path) && !NIL_P(c->logger))
+		rb_raise(rb_eArgError, ":logger and :path are independent");
+	if (!NIL_P(path)) {
+		VALUE ab = rb_str_new2("ab");
+		id = rb_intern("open");
+		c->logger = rb_funcall(rb_cFile, id, 2, path, ab);
+	}
+
+	id = rb_intern("sync=");
+	if (rb_respond_to(c->logger, id))
+		rb_funcall(c->logger, id, 1, Qtrue);
+
+	id = rb_intern("fileno");
+	if (rb_respond_to(c->logger, id))
+		c->fd = raw_fd(rb_funcall(c->logger, id, 0));
+}
+
 /**
  * call-seq:
  *   Clogger.new(app, :logger => $stderr, :format => string) => obj
  *
  * Creates a new Clogger object that wraps +app+.  +:logger+ may
  * be any object that responds to the "<<" method with a string argument.
+ * If +:logger:+ is a string, it will be treated as a path to a
+ * File that will be opened in append mode.
  */
 static VALUE clogger_init(int argc, VALUE *argv, VALUE self)
 {
@@ -609,12 +627,9 @@ static VALUE clogger_init(int argc, VALUE *argv, VALUE self)
 	if (TYPE(o) == T_HASH) {
 		VALUE tmp;
 
+		tmp = rb_hash_aref(o, ID2SYM(rb_intern("path")));
 		c->logger = rb_hash_aref(o, ID2SYM(rb_intern("logger")));
-		if (!NIL_P(c->logger)) {
-			rb_rescue(obj_enable_sync, c->logger, NULL, 0);
-			tmp = rb_rescue(obj_fileno, c->logger, NULL, 0);
-			c->fd = raw_fd(tmp);
-		}
+		init_logger(c, tmp);
 
 		tmp = rb_hash_aref(o, ID2SYM(rb_intern("format")));
 		if (!NIL_P(tmp))
diff --git a/lib/clogger/pure.rb b/lib/clogger/pure.rb
index 0dd5021..452e232 100644
--- a/lib/clogger/pure.rb
+++ b/lib/clogger/pure.rb
@@ -14,7 +14,12 @@ class Clogger
 
     @app = app
     @logger = opts[:logger]
-    (@logger.sync = true) rescue nil
+    path = opts[:path]
+    path && @logger and
+      raise ArgumentError, ":logger and :path are independent"
+    path and @logger = File.open(path, "ab")
+
+    @logger.sync = true if @logger.respond_to?(:sync=)
     @fmt_ops = compile_format(opts[:format] || Format::Common, opts)
     @wrap_body = need_wrap_body?(@fmt_ops)
     @reentrant = opts[:reentrant]
@@ -69,7 +74,7 @@ class Clogger
   end
 
   def fileno
-    @logger.fileno rescue nil
+    @logger.respond_to?(:fileno) ? @logger.fileno : nil
   end
 
 private
diff --git a/test/test_clogger.rb b/test/test_clogger.rb
index 1311017..9252a42 100644
--- a/test/test_clogger.rb
+++ b/test/test_clogger.rb
@@ -3,6 +3,7 @@ $stderr.sync = $stdout.sync = true
 require "test/unit"
 require "date"
 require "stringio"
+require "tempfile"
 
 require "rack"
 
@@ -629,4 +630,21 @@ class TestClogger < Test::Unit::TestCase
     end
   end if RUBY_PLATFORM =~ /linux/ && File.readable?(LINUX_PROC_PID_STATUS)
 
+  def test_path_open_file
+    tmp = Tempfile.new('test_clogger')
+    app = lambda { |env| [ 200, {}, [] ] }
+    app = Clogger.new(app, :format => '$status', :path => tmp.path)
+    assert_kind_of Integer, app.fileno
+    assert app.fileno != tmp.fileno
+    status, headers, body = app.call(@req)
+    assert_equal "200\n", tmp.read
+  end
+
+  def test_path_logger_conflict
+    tmp = Tempfile.new('test_clogger')
+    app = lambda { |env| [ 200, {}, [] ] }
+    assert_raises(ArgumentError) {
+      Clogger.new(app, :logger=> $stderr, :path => tmp.path)
+    }
+  end
 end
-- 
cgit v1.2.3-24-ge0c7