From: Junio C Hamano <junkio@cox.net>
Date: Mon, 25 Jul 2005 06:21:14 GMT
Subject: Re: [PATCH] Add support for directories to git-rename-script.
Message-ID: <7vpst7z9r9.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20050725052646.GB6098@mythryan2.michonline.com>

Ryan Anderson <ryan@michonline.com> writes:

> +open(F,"-|","git-ls-files")
> +	or die "Failed to open pipe from git-ls-files: " . $!;
> +
> +@allfiles = <F>;
> +close(F);

We can afford to handle funny characters in the filename right
without pain in Perl, so let's do it right:

    use vars qw($/);
    $/ = "\0";
    open F, '-|', 'git-ls-files', '-z'
    	or die "Failed to open pipe from git-ls-files: " . $!;
    @allfiles = map { chomp; $_} <F>;

> +@srcfiles = grep /^$src/, @allfiles;
> +@dstfiles = @srcfiles;
> +s#^$src(/|$)#$dst$1# for @dstfiles;

Be careful with these regexps.  Perhaps something like this?

    @dstfiles = @srcfiles = grep /^\Q$src\E/, @allfiles;
    s#^\Q$src\E(/|$)#$dst$1# for @dstfiles;

or perhaps a bit more readable:

    my $qsrc = quotemeta($src);
    @dstfiles = @srcfiles = grep /^$qsrc/, @allfiles;
    for (@dstfiles) { s/^$qsrc(/|$)/$dst$1/; }

> +rename($src,$dst)
> +	or die "rename failed: $!";

Making sure leading directories for $dst exists, perhaps?

Otherwise looks good.

> +sub usage($) {

This is just a style thing, but do you really care to force a
scalar contect when "usage" is used?