Re: [PATCH] Add git svn gc command

From: Eric Wong <normalperson@yhbt.net>
Date: Jul 20, 2009, 07:59 UTC
Message-ID: <20090720075929.GA5591@dcvr.yhbt.net>
In-Reply-To: <8E0EFA3B-E8C0-4030-8BAE-E8EF65694031@gmail.com>

Robert Zeh <robert.a.zeh@gmail.com> wrote:

Show 11 quoted lines

> Add a git svn gc command that gzips all unhandled.log files, and removes 
> all index files under .git/svn.
>
> Signed-off-by: Robert Allan Zeh <robert.a.zeh@gmail.com>
> ---
>  Documentation/git-svn.txt |    4 ++++
>  git-svn.perl              |   37 +++++++++++++++++++++++++++++++++++++
>  t/t9140-git-svn-gc.sh     |   45 ++++++++++++++++++++++++++++++++++++ 
> +++++++++
>  3 files changed, 86 insertions(+), 0 deletions(-)
>  create mode 100755 t/t9140-git-svn-gc.sh

Hi Robert,

Your mailer is mangling whitespace badly so the patch isn't applying at all. Make sure indents are real tabs like the rest of the code.

Some more comments below, but I think this will be a good addition to git svn.

Show 7 quoted lines

> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -35,11 +35,14 @@ push @Git::SVN::Ra::ISA, 'SVN::Ra';
>  push @SVN::Git::Editor::ISA, 'SVN::Delta::Editor';
>  push @SVN::Git::Fetcher::ISA, 'SVN::Delta::Editor';
>  use Carp qw/croak/;
> +use Compress::Zlib;

I'd "require" Compress::Zlib lazilly so it's not loaded at startup. It's not a stock component of Perl and not needed for the majority of commands.

Show 7 quoted lines

> +sub cmd_gc
> +{
> +    use Cwd;
> +       my $git_dir = $ENV{GIT_DIR};
> +    my $svn_dir = "$git_dir/svn";
> +    print getcwd;
> +    print "\n";

I'm not sure why you need to print getcwd there, leftover code from testing/debugging?

Show 8 quoted lines

> +sub gc_directory {
> +    if (-f $_ && basename($_) eq "unhandled.log") {
> +        my $out_filename = $_ . ".gz";
> +        print $out_filename, "\n";
> +        open my $in_fh, "<", $_ or die "Unable to open $_: $!\n";
> +        binmode $in_fh;
> +        my $gz = gzopen($out_filename, "wb") or die "Unable to open  
> $out_filename: $!\n";

gzopen with "ab" might be a better idea than "wb". I'm not sure if clobbering the existing log every time we run this command is a good idea.

-- 
Eric Wong