Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 27, 2012, 05:18 UTC
- Message-ID
- <7vvch93hpy.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1343344945-3717-3-git-send-email-schwern@pobox.com>
"Michael G. Schwern" <schwern@pobox.com> writes:
Show 9 quoted lines
> 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).
Show 5 quoted lines
> * 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?
Show 26 quoted lines
> 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.