Fix ln_sf with multiple sources and target_directory: false

In this case, an ArgumentError is now raised rather than ignoring the
option, just as GNU coreutils' `ln` would error on the command line.
Fixes #128 as well.

Author: nobu

lib/fileutils.rb

@@ -708,9 +708,10 @@ def ln_s(src, dest, force: nil, relative: false, target_directory: true, noop: n
     if relative
       return ln_sr(src, dest, force: force, noop: noop, verbose: verbose)
     end
-    fu_output_message "ln -s#{force ? 'f' : ''} #{[src,dest].flatten.join ' '}" if verbose
+    fu_output_message "ln -s#{force ? 'f' : ''}#{
+      target_directory ? '' : 'T'} #{[src,dest].flatten.join ' '}" if verbose
     return if noop
-    fu_each_src_dest0(src, dest) do |s,d|
+    fu_each_src_dest0(src, dest, target_directory) do |s,d|
       remove_file d, true if force
       File.symlink s, d
     end
@@ -730,17 +731,16 @@ def ln_sf(src, dest, noop: nil, verbose: nil)
   # Like FileUtils.ln_s, but create links relative to +dest+.
   #
   def ln_sr(src, dest, target_directory: true, force: nil, noop: nil, verbose: nil)
-    options = "#{force ? 'f' : ''}#{target_directory ? '' : 'T'}"
-    dest = File.path(dest)
-    srcs = Array(src)
-    link = proc do |s, target_dir_p = true|
-      s = File.path(s)
-      if target_dir_p
-        d = File.join(destdirs = dest, File.basename(s))
-      else
-        destdirs = File.dirname(d = dest)
+    fu_output_message "ln -sr#{force ? 'f' : ''}#{
+      target_directory ? '' : 'T'} #{[src,dest].flatten.join ' '}" if verbose
+    unless target_directory
+      destdirs = fu_split_path(File.realdirpath(dest))
+    end
+    fu_each_src_dest0(src, dest, target_directory) do |s,d|
+      if target_directory
+        destdirs = fu_split_path(File.realdirpath(File.dirname(d)))
+      # else d == dest
       end
-      destdirs = fu_split_path(File.realpath(destdirs))
       if fu_starting_path?(s)
         srcdirs = fu_split_path((File.realdirpath(s) rescue File.expand_path(s)))
         base = fu_relative_components_from(srcdirs, destdirs)
@@ -754,18 +754,10 @@ def ln_sr(src, dest, target_directory: true, force: nil, noop: nil, verbose: nil
         end
         s = File.join(*base, *srcdirs)
       end
-      fu_output_message "ln -s#{options} #{s} #{d}" if verbose
       next if noop
       remove_file d, true if force
       File.symlink s, d
     end
-    case srcs.size
-    when 0
-    when 1
-      link[srcs[0], target_directory && File.directory?(dest)]
-    else
-      srcs.each(&link)
-    end
   end
   module_function :ln_sr
 
@@ -2475,6 +2467,9 @@ def fu_each_src_dest(src, dest)   #:nodoc:
 
   def fu_each_src_dest0(src, dest, target_directory = true)   #:nodoc:
     if tmp = Array.try_convert(src)
+      unless target_directory or tmp.size <= 1
+        raise ArgumentError, "extra target #{tmp}"
+      end
       tmp.each do |s|
         s = File.path(s)
         yield s, (target_directory ? File.join(dest, File.basename(s)) : dest)

test/fileutils/test_fileutils.rb

@@ -955,16 +955,27 @@ def test_ln_pathname
   def test_ln_s
     check_singleton :ln_s
 
+    ln_s TARGETS, 'tmp'
+    each_srcdest do |fname, lnfname|
+      assert_equal fname, File.readlink(lnfname)
+    ensure
+      rm_f lnfname
+    end
+
+    lnfname = 'symlink'
+    assert_raise(Errno::ENOENT, "multiple targets need a destination directory") {
+      ln_s TARGETS, lnfname
+    }
+    assert_file.not_exist?(lnfname)
+
     TARGETS.each do |fname|
-      begin
-        fname = "../#{fname}"
-        lnfname = 'tmp/lnsdest'
-        ln_s fname, lnfname
-        assert FileTest.symlink?(lnfname), 'not symlink'
-        assert_equal fname, File.readlink(lnfname)
-      ensure
-        rm_f lnfname
-      end
+      fname = "../#{fname}"
+      lnfname = 'tmp/lnsdest'
+      ln_s fname, lnfname
+      assert_file.symlink?(lnfname)
+      assert_equal fname, File.readlink(lnfname)
+    ensure
+      rm_f lnfname
     end
   end if have_symlink? and !no_broken_symlink?
 
@@ -1017,22 +1028,52 @@ def test_ln_sf_pathname
   def test_ln_sr
     check_singleton :ln_sr
 
-    TARGETS.each do |fname|
-      begin
-        lnfname = 'tmp/lnsdest'
-        ln_sr fname, lnfname
-        assert FileTest.symlink?(lnfname), 'not symlink'
-        assert_equal "../#{fname}", File.readlink(lnfname), fname
+    assert_all_assertions_foreach(nil, *TARGETS) do |fname|
+      lnfname = 'tmp/lnsdest'
+      ln_sr fname, lnfname
+      assert FileTest.symlink?(lnfname), 'not symlink'
+      assert_equal "../#{fname}", File.readlink(lnfname)
+    ensure
+      rm_f lnfname
+    end
+
+    ln_sr TARGETS, 'tmp'
+    assert_all_assertions do |all|
+      each_srcdest do |fname, lnfname|
+        all.for(fname) do
+          assert_equal "../#{fname}", File.readlink(lnfname)
+        end
       ensure
         rm_f lnfname
       end
     end
+
     mkdir 'data/src'
     File.write('data/src/xxx', 'ok')
     File.symlink '../data/src', 'tmp/src'
     ln_sr 'tmp/src/xxx', 'data'
     assert File.symlink?('data/xxx')
     assert_equal 'ok', File.read('data/xxx')
+  end
+
+  def test_ln_sr_not_target_directory
+    assert_raise(ArgumentError) {
+      ln_sr TARGETS, 'tmp', target_directory: false
+    }
+    assert_empty(Dir.children('tmp'))
+
+    lnfname = 'symlink'
+    assert_raise(ArgumentError) {
+      ln_sr TARGETS, lnfname, target_directory: false
+    }
+    assert_file.not_exist?(lnfname)
+
+    assert_all_assertions_foreach(nil, *TARGETS) do |fname|
+      assert_raise(Errno::EEXIST, Errno::EACCES) {
+        ln_sr fname, 'tmp', target_directory: false
+      }
+      assert_file.not_exist? File.join('tmp/', File.basename(fname))
+    end
   end if have_symlink?
 
   def test_ln_sr_broken_symlink