Re: [PATCH 4/4] Move initialization of Git::SVN variables into Git::SVN.

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2012, 05:18 UTC
Message-ID: <7vhast3hpb.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1343344945-3717-5-git-send-email-schwern@pobox.com>

"Michael G. Schwern" <schwern@pobox.com> writes:

> From: "Michael G. Schwern" <schwern@pobox.com>
>
> Also it can compile on its own now, yay!

Hmmm.

If you swap the order of steps 3/4 and 4/4 by creating Git/SVN.pm that only has these variable definitions (i.e. "our $X" and "use vars $X") and make git-svn.perl use them from Git::SVN in the first step, and then do the bulk-moving (equivalent of your 3/4) in the second step, would it free you from having to say "it's doubtful it will compile by itself"?

In short:

 - I didn't see anything questionable in 1/4;

 - Calling up ::opt_prefix() from module in 2/4 looked ugly to me
   but I suspect it should be easy to fix;

 - 3/4 was a straight move and I didn't see anything questionable in
   it, but I think it would be nicer if intermediate steps can be
   made to still work by making 4/4 come first or something
   similarly simple.

If the issues in 2/4 and 3/4 are easily fixable by going the route I handwaved above, the result of doing so based on this round is ready to be applied, I think.

Eric, Jonathan, what do you think?

Show 70 quoted lines

> ---
>  git-svn.perl          | 4 ----
>  perl/Git/SVN.pm       | 9 +++++++--
>  t/Git-SVN/00compile.t | 3 ++-
>  3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 4c77f69..ef10f6f 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -20,10 +20,7 @@ my $cmd_dir_prefix = eval {
>  
>  my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};
>  $ENV{GIT_DIR} ||= '.git';
> -$Git::SVN::default_repo_id = 'svn';
> -$Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
>  $Git::SVN::Ra::_log_window_size = 100;
> -$Git::SVN::_minimize_url = 'unset';
>  
>  if (! exists $ENV{SVN_SSH} && exists $ENV{GIT_SSH}) {
>  	$ENV{SVN_SSH} = $ENV{GIT_SSH};
> @@ -114,7 +111,6 @@ my ($_stdin, $_help, $_edit,
>  # This is a refactoring artifact so Git::SVN can get at this git-svn switch.
>  sub opt_prefix { return $_prefix || '' }
>  
> -$Git::SVN::_follow_parent = 1;
>  $Git::SVN::Fetcher::_placeholder_filename = ".gitignore";
>  $_q ||= 0;
>  my %remote_opts = ( 'username=s' => \$Git::SVN::Prompt::_username,
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index c71c041..2e0d7f0 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -3,9 +3,9 @@ use strict;
>  use warnings;
>  use Fcntl qw/:DEFAULT :seek/;
>  use constant rev_map_fmt => 'NH40';
> -use vars qw/$default_repo_id $default_ref_id $_no_metadata $_follow_parent
> +use vars qw/$_no_metadata
>              $_repack $_repack_flags $_use_svm_props $_head
> -            $_use_svnsync_props $no_reuse_existing $_minimize_url
> +            $_use_svnsync_props $no_reuse_existing
>  	    $_use_log_author $_add_author_from $_localtime/;
>  use Carp qw/croak/;
>  use File::Path qw/mkpath/;
> @@ -30,6 +30,11 @@ BEGIN {
>  	$can_use_yaml = eval { require Git::SVN::Memoize::YAML; 1};
>  }
>  
> +our $_follow_parent  = 1;
> +our $_minimize_url   = 'unset';
> +our $default_repo_id = 'svn';
> +our $default_ref_id  = $ENV{GIT_SVN_ID} || 'git-svn';
> +
>  my ($_gc_nr, $_gc_period);
>  
>  # properties that we do not log:
> diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
> index a7aa85a..97475d9 100644
> --- a/t/Git-SVN/00compile.t
> +++ b/t/Git-SVN/00compile.t
> @@ -3,6 +3,7 @@
>  use strict;
>  use warnings;
>  
> -use Test::More tests => 1;
> +use Test::More tests => 2;
>  
>  require_ok 'Git::SVN::Utils';
> +require_ok 'Git::SVN';