Three things that I would change:

I wouldn't read the filenames into an array, I'd read them one at a time.

while ($_ = readdir(INDIR)) {
  ...
}
[download]

I'd use next to skip the loop for '.' and '..'.

while ($_ = readdir(INDIR)) {
  next if -d || $_ eq '.' || $_ eq '..';
  ...
}
[download]

Finally (and most importantly) using backticks for commands where you're not capturing the output is wasteful as Perl collects up all the output and returns it to you - only for you to throw it away. Better to use system in that case. Or to use Perl version of the command where they are available - like using move (from File::Copy instead of mv).

Update: One more thing I've just noticed.

unless (-d $srcdir && -d $destdir ) {die "Error: $!";}
[download]

Not sure why you're including the value of $! in that error message. It won't contain a value at that point.

Not sure why you're including the value of $! in that error message. It won't contain a value at that point.

I thought that, too. I tested it, however, and

$ perl -e '-e q{non-existent} or die $!;'
No such file or directory at -e line 1.
$
[download]

Cheers,

JohnGG

"Finally (and most importantly) using backticks for commands where you're not capturing the output is wasteful as Perl collects up all the output and returns it to you - only for you to throw it away. Better to use system in that case. Or to use Perl version of the command where they are available - like using move (from File::Copy instead of mv)."

Be thankful that Perl throws it away. If you do the same thing in (e.g.) ksh then the command for the shell to execute is evaluated as the return from the back-ticked command. i.e. it doesn't throw the output away it executes it!

Now I can see how this might be useful in some obscure circumstances.....however I have seen back-ticks used without the realization of the implications :-(

And yes, it did take me a while to work out where the obscure error messages were coming from!

Cheers

Dave.R

Nothing succeeds like a budgie with no teeth.

2006-10-03 Retitled by planetscape, as per Monastery guidelines: one-word (or module-only) titles hinder site navigation

Read more... view votes (24 Bytes)

Original title: 'Re^2: Backticks'

Some comments:

unless (-d $srcdir && -d $destdir )

It's better not to use unless, unless for simple conditionals ;^).

opendir(INDIR,$srcdir) || die

Better use lexical filehandles. Also, I don't like to use parentheses, it makes the code more perlish:

unless (-d || $_ eq "." || $_ eq "..") {

Same as above. Quick, what does this do?

`tar -cz $_ -f $_.tar.gz`;

Use backticks when you are going to use the output of the command. This is not the case, so you better use system.

Update: Oh, there's a recent thread about unless, be sure to check it out.

Quick, what does this do?

unless ($bar =~ /bar/ or $baz !~ /qux/ and defined $undef)
[download]

Exactly the same as

if( not ($bar =~ /bar/ or $baz !~ /qux/ and defined $undef) )
[download]

There. So much clearer now...not.

---

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?

"Science is about questioning the status quo. Questioning authority".

In the absence of evidence, opinion is indistinguishable from prejudice.

BrowserUk++

A not, a regex, an or and an and. Not only not clear but almost impossible for me to get my head round.

I resort to extreme lengths to try and get it straight.

if( 
  not (
    (
      $bar =~ /bar/ 
        or 
      (
        $baz !~ /qux/ 
          and 
        defined $undef
      ) 
    )
  )
)
[download]

But I find that almost always these constructions are the result of a poor algorithm. As the old saying goes "get your algorithm right and the code writes itself". While we're here, that is closely followed by "don't try to code yourself out of a problem, start again".

It works for me! :-)

There. So much clearer now...not.

Your ability to make something less clear doesn't negate everyone else's ability to make something more clear.

if(  ! -d  &&  $_ ne "."  &&  $_ ne ".."  ) {
[download]

Yes, that prevents a noticeable mental pause that the "unless" version induced. [ Even better, it avoids the mental tension between how "unless" is usually used in English vs. how it is used in Perl and the fact that the unexplicit "not" implied by "unless" is too easy to mentally drop. ]

- tye        

You made Hue-Bond's point. The statement was hard to understand without some major transformations. For example, the following is clearer:

if ( $bar !~ /bar/ and ( $baz =~ /qux/ or not defined $undef ) )
[download]

But let's not stop there. Reduce the number of things the reader has to keep in mind by breaking the statement into two.

if( $bar !~ /bar/ )
   if ( $baz =~ /qux/ or not defined $undef )
[download]

I would eliminate the use of chdir. Yes, the program as given works, but only because both of the directories are specified as absolute. I think you'd gain flexibility by allowing relative paths (for example, "." as the srcdir).

Here's how I might rewrite it:

#!/usr/bin/perl

use Getopt::Long;
use strict;
use warnings;
$|=1;

my $srcdir;
my $destdir;
my $mtime = 1;

GetOptions(
        'srcdir=s' => \$srcdir,
        'destdir=s' => \$destdir,
        'mtime|days=i' => \$mtime,
);

defined $srcdir && defined $destdir or die <<EOF;
Usage: $0 --srcdir SRCDIR --destdir DESTDIR [-days N]
EOF

opendir SRCDIR, $srcdir or die "opendir $srcdir - $!";

my @too_old;
my @files;

# only select non-hidden regular files:
for ( grep { -f "$srcdir/$_" } readdir SRCDIR )
{
        if ( -M "$srcdir/$_" > $mtime )
        {
                push @too_old, $_;
        }
        else
        {
                push @files, $_;
        }
}

closedir SRCDIR;

@too_old and
print "(Ignoring ".scalar(@too_old)." too-old files.)\n";

@files or die "No backup candidates in $srcdir.";

unless ( -d $destdir )
{
        warn "Destination directory $destdir does not exist; attemptin
+g to create.\n";
        mkdir $destdir or die "mkdir $destdir - $!";
}

print "Backing up ".scalar(@files)." files from $srcdir.\n";

for my $f ( @files )
{
        print "Archiving $f \n";
        system qq( cd "$srcdir" ; tar -cz "$f" -f "$f.tar.gz" );
        system qq( mv "$srcdir/$f.tar.gz" "$destdir" );
}
[download]

This doesn't entirely avoid chdir, but it puts it at the lowest, most constrained context possible. In fact, if you have gnu tar (which it appears you do), you can exploit the -C option to make tar itself do the directory change within itself.

#!/usr/bin/perl
use warnings;
use strict;

use File::Copy;

# change source backup dir, modification time, and destination backup 
+dir here
my $srcdir = '/var/local/somedir';
my $mtime = '1';
my $destdir = '/backup/somedir';

die "Error:$!" unless (-d $srcdir && -d $destdir );

print "Beginning backup...\n";

chdir $srcdir;
opendir(INDIR,$srcdir) || die "Can't open directory: $!";

while ($_ = readdir(INDIR)) {

        next if (-d || $_ eq "." || $_ eq "..");

        if (-M > $mtime) {
                print "$_ : Older than one day.\n";
        } else {
                system("tar -cz $_ -f $_.tar.gz");
                move(<*.gz>, $destdir) && print "$_ archived in $destd
+ir\n";
        }

}
closedir(INDIR);
print "Backup complete.\n";
[download]

die "Error:$!" unless (-d $srcdir && -d $destdir );

As I mentioned before, $! won't have anything useful in it at this point, so there's no reason to include it in the error message.

Why not make the error messages a little clearer.

die "$srcdir is not a valid directory\n" unless -d $srcdir;
die "$destdir is not a valid directory\n" unless -d $destdir;
[download]

Update: Looks like I'm wrong.

--
<http://dave.org.uk>

"The first rule of Perl club is you do not talk about Perl club."
-- Chip Salzenberg

Update: Looks like I'm wrong.

I disagree: just because someone tried it once and got a useful error message doesn't change the fact that $! is not documented to be defined at this point.

$! may well get set differently (or not at all) in different circumstances, when testing different files, or on a different O/S, or depending on phase of the moon for all we know.

Your suggested clearer error messages are definitively more correct; the only change I might make (at the risk of getting too clever) is to avoid the duplication with something like:

  die "$_ is not a valid directory\n" for grep !-d, $srcdir, $destdir;
[download]

Hugo

Try this on for size... Probably still not the way I would approach the problem, but at least it's a bit more elegant...

my $srcdir ='/var/local/some_dir';
my $mtime = 1;
my $destdir = '/backup/some_dir';

# Yours doesn't tell you which directory didn't exist...
die "$srcdir does not exist" unless -d $srcdir;
die "$destdir does not exist" unless -d $destdir;

print "Beginning backup...\n";

# Error checking on chdir is important too, the directory
# might exist, but not be readable by you
chdir $srcdir or die "Can't change to $srcdir: $!";

# Once your program gets bigger than a couple of lines,
# you really want to know which directories or files
# couldn't be opened
opendir ( INDIR, $srcdir ) or die "Can't open $srcdir: $!";

# if this is a big directory, slurping all the files into
# memory can be quite a waste of memory...
while ( my $file = readdir( INDIR ) ) {
    next if -d $file; # -d will be true for . and .. too
    if ( -M $file > $mtime ) {
        print "$file : Older than one day.\n";
        next;
    }
    # tar is for combining multiple things into one,
    # gzip is for compressing, since you are only doing
    # one file at a time, the tar is just wasted effort
    system( "gzip -c $file > $destdir/$file.gz" );
    if ( $? != 0 ) { die "Gzip failed" }
    print "$file archived in $destdir\n";
}
[download]

This is probably how I would actually approach the problem... Although this approach may not meet some requirements that couldn't really be inferred from your code...

use Path::Class qw( dir );
my $srcdir = dir( '/var/local/some_dir' );
my $destdir = dir( '/backup/some_dir' );

$destdir->mkpath unless -d $destdir;

die "$srcdir does not exist" unless -d $srcdir;
die "$destdir does not exist" unless -d $destdir;

print "Beginning backup...\n";

while ( my $file = $srcdir->next ) {
    next if $file->is_dir;
    my $target = $destdir->file( $file->basename.".gz" );
    if ( $file->stat->mtime <= $target->stat->mtime ) {
        print "backup is more recent than original\n";
        next;
    }
    system( "gzip -c $file > $target" );
    if ( $? != 0 ) { die "Gzip failed" }
    print "$file archived in $destdir\n";
}
[download]

- unless (-d $srcdir && -d $destdir ) {die "Error: $!";}
+ for($srcdir,$destdir) { ! -d $_ and die "Directory '$_' - $!\n" }
[download]

You wouldn't know which directory is missing.

- chdir $srcdir;
- opendir(INDIR,$srcdir) || die "Can't open directory: $!";
+ chdir $srcdir or die "Can't chdir to '$srcdir': $!\n";
+ open my $indir, '.' or die "Can't read '.' in $srcdir: $!\n";
[download]

Point is, you are already in $srcdir, so you are here->.

Instead of using the ugly and hard to grok

unless (-d || $_ eq "." || $_ eq "..") { # . and .. are always -d
[download]

in the loop, after having lumped all directory entries (files, directories, symlinks, sockets, device files) into @files (are you really looking for files only? and making tar.gz's containing a single file each?) it would be more elegant

my @files = grep { -f } readdir $indir;
[download]

to stuff only the interesting files into @files.

    `tar -cz $_ -f $_.tar.gz`;
    `mv *.gz $destdir`;
[download]

Really? what if your directory contains *.gz files? Your asking tar to complain. And use system, not backticks.

I would pack all files into one tar file:

chdir $srcdir or die "Can't chdir to '$srcdir': $!\n";
open my $indir, '.' or die "Can't read '.' in $srcdir: $!\n";

(my $tarfile = $srcdir) =~ s|.*/||;
$tarfile = "$destdir/$tarfile-" . time . '.tar';

my @files = grep { -f and -M <= 1 } readdir $indir;

# packing all files at once doesn't work with large directories
# (limit of command line length)

while(my $file = shift @files) {
    system ('tar', 'uf', $tarfile, $file)
        and die "Can't pack '$file' into '$tarfile' (exitcode $?)\n"; 
}

system ('gzip', $tarfile)
    and die "Couldn't gzip $tarfile (exitcode $?)\n";
[download]

--shmem

_($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                              /\_¯/(q    /
----------------------------  \__(m.====·.(_("always off the crowd"))."·
");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}

I hate to add anything that might be construed as fuel to the unless discussion, but aren't "." and ".." both directories anyway? (And, for that matter, if they weren't, would you want to skip them?)

   cd /var/local/some_dir
   find . -type -f -mtime 1 -maxdepth 1 \
          -exec tar cfz /backup/some_dir/{}.tar.gz {} \;
[download]