File::Find::find dies when an empty directory list is supplied. #15042
Comments
From Mohammed_ElAfifi@yahoo.comCreated by Mohammed_ElAfifi@yahoo.comFile::Find::find(and similarly its cousine finddepth) die when an find sub {}; will die with a message This makes it a little tedious to test an array before passing it find sub {}, @directories if @directories; or worse if more than one array is used, like this: find sub {}, @dirs1, @dirs2 if @dirs1 || @dirs2; I've created a trivial patch that would return early if the The problem is reproducible on different platforms(windows and Things I'm not sure I did right: Perl Info |
From Mohammed_ElAfifi@yahoo.comOn Tue Nov 10 12:03:03 2015, Mohammed_ElAfifi@yahoo.com wrote:
Forgot to attach the patches, sorry. |
From Mohammed_ElAfifi@yahoo.com0001-add-test-for-empty-directory-list-with-File-Find.patchFrom fc1698fb3cb67fe01774b7b83af8ec8460260d59 Mon Sep 17 00:00:00 2001
From: Mohammed El-Afifi <Mohammed_ElAfifi@yahoo.com>
Date: Mon, 9 Nov 2015 23:49:08 +0200
Subject: [PATCH 1/2] add test for empty directory list with File::Find
---
AUTHORS | 1 +
ext/File-Find/t/find.t | 12 +++++++++++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/AUTHORS b/AUTHORS
index ebd9222..d56db73 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -1271,3 +1271,4 @@ Zefram <zefram@fysh.org>
Zsbán Ambrus <ambrus@math.bme.hu>
Zbynek Vyskovsky <kvr@centrum.cz>
Ævar Arnfjörð Bjarmason <avar@cpan.org>
+Mohammed El-Afifi <mohammed_elafifi@yahoo.com>
diff --git a/ext/File-Find/t/find.t b/ext/File-Find/t/find.t
index 390f39d..b04f9d3 100644
--- a/ext/File-Find/t/find.t
+++ b/ext/File-Find/t/find.t
@@ -24,7 +24,7 @@ BEGIN {
}
my $symlink_exists = eval { symlink("",""); 1 };
-my $test_count = 109;
+my $test_count = 111;
$test_count += 127 if $symlink_exists;
$test_count += 26 if $^O eq 'MSWin32';
$test_count += 2 if $^O eq 'MSWin32' and $symlink_exists;
@@ -66,6 +66,16 @@ my $orig_dir = cwd();
cleanup();
##### Sanity checks #####
+# Do find() and finddepth() work correctly with an empty list of
+# directories?
+{
+ find(\&noop_wanted);
+ pass("'find' successfully returned for an empty list of directories");
+
+ finddepth(\&noop_wanted);
+ pass("'finddepth' successfully returned for an empty list of directories");
+}
+
# Do find() and finddepth() work correctly in the directory
# from which we start? (Test presumes the presence of 'taint.t' in same
# directory as this test file.)
--
2.4.3
|
From Mohammed_ElAfifi@yahoo.com0002-fix-File-Find-to-handle-empty-directory-list.patchFrom e4b57cf5a4a2fecdfd61ebe814ae3f9998641dc1 Mon Sep 17 00:00:00 2001
From: Mohammed El-Afifi <Mohammed_ElAfifi@yahoo.com>
Date: Tue, 10 Nov 2015 00:36:12 +0200
Subject: [PATCH 2/2] fix File::Find to handle empty directory list
---
ext/File-Find/lib/File/Find.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ext/File-Find/lib/File/Find.pm b/ext/File-Find/lib/File/Find.pm
index 094d5ea..36242d4 100644
--- a/ext/File-Find/lib/File/Find.pm
+++ b/ext/File-Find/lib/File/Find.pm
@@ -3,7 +3,7 @@ use 5.006;
use strict;
use warnings;
use warnings::register;
-our $VERSION = '1.31';
+our $VERSION = '1.32';
require Exporter;
require Cwd;
@@ -126,6 +126,7 @@ sub is_tainted_pp {
sub _find_opt {
my $wanted = shift;
+ return unless @_;
die "invalid top directory" unless defined $_[0];
# This function must local()ize everything because callbacks may
--
2.4.3
|
From @jkeenanOn Tue Nov 10 14:15:38 2015, Mohammed_ElAfifi@yahoo.com wrote:
The patches look fine as is. It's therefore a question as to whether we should change File::Find::find's behavior as you recommend. If I understand you correctly what you're saying is, "When find() (or finddepth) is presented with an empty list of directories to search, simply return without a warning or die-ing. Only die if the first item in a non-empty list of directories is undefined [the current behavior])." Thank you very much. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Tue Nov 10 14:15:38 2015, Mohammed_ElAfifi@yahoo.com wrote:
Thanks. It's generally preferred not to add tests that fail or crash without the fix. Adding the test as a TODO test is fine, so something like: +# Do find() and finddepth() work correctly with an empty list of Or if you prefer I can just squash the two patch together. Tony |
From @jkeenanOn Wed Nov 11 16:53:45 2015, tonyc wrote:
I applied each patch in a local branch and tested the first before the second. I got the expected test failures on the first and PASS on the second. So if you're accepting the thrust of the RT, I'd say: Go ahead and squash the patches and apply. Thank you very much. -- |
From Mohammed_ElAfifi@yahoo.comOn Wed Nov 11 16:53:45 2015, tonyc wrote:
Thanks everyone for taking the time to review the patch. It's of course perfectly fine with me to squash the patches; it was just that I wrote the test first in its own commit before I changed the code. @Tony: Thanks for refactoring the test; I didn't feel comfortable with my first tests but I didn't have anything else in mind at the time. Wrapping the call in eval would prevent the test from dying when the test is run against the old behavior. |
From Mohammed_ElAfifi@yahoo.comOn Fri Nov 13 09:26:44 2015, Mohammed_ElAfifi@yahoo.com wrote:
I consolidated the two patches, along with Tony's suggested refactored tests into this new patch for convenience. Not that I removed the TODO label from the tests since they're now in a single patch along with the fix. I also split the test lines to stay under the 80 chars/line limit. Regards, |
From Mohammed_ElAfifi@yahoo.com0001-handle-empty-directory-lists-in-File-Find.patchFrom c1d87a1c0f0742ede39dda1f96e42a71799c926a Mon Sep 17 00:00:00 2001
From: Mohammed El-Afifi <Mohammed_ElAfifi@yahoo.com>
Date: Mon, 9 Nov 2015 23:49:08 +0200
Subject: [PATCH] handle empty directory lists in File::Find
---
AUTHORS | 1 +
ext/File-Find/lib/File/Find.pm | 3 ++-
ext/File-Find/t/find.t | 12 +++++++++++-
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index ebd9222..d56db73 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -1271,3 +1271,4 @@ Zefram <zefram@fysh.org>
Zsbán Ambrus <ambrus@math.bme.hu>
Zbynek Vyskovsky <kvr@centrum.cz>
Ævar Arnfjörð Bjarmason <avar@cpan.org>
+Mohammed El-Afifi <mohammed_elafifi@yahoo.com>
diff --git a/ext/File-Find/lib/File/Find.pm b/ext/File-Find/lib/File/Find.pm
index 094d5ea..36242d4 100644
--- a/ext/File-Find/lib/File/Find.pm
+++ b/ext/File-Find/lib/File/Find.pm
@@ -3,7 +3,7 @@ use 5.006;
use strict;
use warnings;
use warnings::register;
-our $VERSION = '1.31';
+our $VERSION = '1.32';
require Exporter;
require Cwd;
@@ -126,6 +126,7 @@ sub is_tainted_pp {
sub _find_opt {
my $wanted = shift;
+ return unless @_;
die "invalid top directory" unless defined $_[0];
# This function must local()ize everything because callbacks may
diff --git a/ext/File-Find/t/find.t b/ext/File-Find/t/find.t
index 390f39d..b532752 100644
--- a/ext/File-Find/t/find.t
+++ b/ext/File-Find/t/find.t
@@ -24,7 +24,7 @@ BEGIN {
}
my $symlink_exists = eval { symlink("",""); 1 };
-my $test_count = 109;
+my $test_count = 111;
$test_count += 127 if $symlink_exists;
$test_count += 26 if $^O eq 'MSWin32';
$test_count += 2 if $^O eq 'MSWin32' and $symlink_exists;
@@ -66,6 +66,16 @@ my $orig_dir = cwd();
cleanup();
##### Sanity checks #####
+# Do find() and finddepth() work correctly with an empty list of
+# directories?
+{
+ ok(eval { find(\&noop_wanted); 1 },
+ "'find' successfully returned for an empty list of directories");
+
+ ok(eval { finddepth(\&noop_wanted); 1 },
+ "'finddepth' successfully returned for an empty list of directories");
+}
+
# Do find() and finddepth() work correctly in the directory
# from which we start? (Test presumes the presence of 'taint.t' in same
# directory as this test file.)
--
2.4.3
|
From Mohammed_ElAfifi@yahoo.comThanks everyone for taking the time to review the patch. It's of course perfectly fine with me to squash the patches; it was just that I wrote the test first in its own commit before I changed the code. On Thursday, November 12, 2015 4:06 AM, James E Keenan via RT <perlbug-followup@perl.org> wrote: On Wed Nov 11 16:53:45 2015, tonyc wrote:
I applied each patch in a local branch and tested the first before the second. I got the expected test failures on the first and PASS on the second. So if you're accepting the thrust of the RT, I'd say: Go ahead and squash the patches and apply. Thank you very much. --
|
From @tonycozOn Fri Nov 13 22:51:12 2015, Mohammed_ElAfifi@yahoo.com wrote:
Thanks, applied as 867fa9e. Tony |
|
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#126611 (status was 'resolved')
Searchable as RT126611$
The text was updated successfully, but these errors were encountered: