Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.

[Junio C Hamano <gitster@pobox.com>]

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

> From: "Michael G. Schwern" <schwern@pobox.com>
>
> This means it should be able to load without git-svn being loaded.
>
> * Load Git.pm on its own and all the needed command functions.
>
> * It needs to grab at a git-svn lexical $_prefix representing the --prefix
>   option.  Provide opt_prefix() for that.  This is a refactoring artifact.
>   The prefix should really be passed into Git::SVN->new.

I agree that the prefix is part of SVN->new arguments in the final
state after applying the whole series (not just these four but also
with the follow-up patches).

> * Unqualify unnecessarily fully qualified globals like
>   $Git::SVN::default_repo_id.
>
> * Lexically isolate the class just to make sure nothing is leaking out.
> ---

Forgot to sign-off, or are you still unsure about this step?

> diff --git a/git-svn.perl b/git-svn.perl
> index 79fe4a4..9cdf6fc 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -109,6 +109,10 @@ my ($_stdin, $_help, $_edit,
>  	$_merge, $_strategy, $_preserve_merges, $_dry_run, $_local,
>  	$_prefix, $_no_checkout, $_url, $_verbose,
>  	$_git_format, $_commit_url, $_tag, $_merge_info, $_interactive);
> +
> +# 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;
> @@ -4280,12 +4292,13 @@ sub find_rev_after {
>  sub _new {
>  	my ($class, $repo_id, $ref_id, $path) = @_;
>  	unless (defined $repo_id && length $repo_id) {
> -		$repo_id = $Git::SVN::default_repo_id;
> +		$repo_id = $default_repo_id;
>  	}
>  	unless (defined $ref_id && length $ref_id) {
> -		$_prefix = '' unless defined($_prefix);
> +		# Access the prefix option from the git-svn main program if it's loaded.
> +		my $prefix = defined &::opt_prefix ? ::opt_prefix() : "";

Again, I agree with you that passing $prefix as one of the arguments
to ->new is the right thing to do in the final state after applying
the whole series.  I don't know if later steps in your patch series
will do so, but it _might_ make more sense to update ->new and its
callers to do so without doing anything else first, so that you do
not have to call out to the ::opt_prefix() when you split things
out.