Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
Michael G Schwern wrote: > On 2012.7.26 10:18 PM, Junio C Hamano wrote: > > 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. > > I don't personally plan on doing any more about it, no. It isn't needed for > SVN 1.7, there's very little real code change (which you could see by looking > at my remote instead of waiting to be fed patches...) and its a very, very > minor problem in the grand scheme. I agree, its not worth it right now. > The first step toward that would be to change git-svn so it can be loaded as a > library using the standard "main() unless caller" trick. Then Git::SVN unit > tests can require git-svn as a library without executing it and get some tests > written with a minimum of Git::SVN code change. > None of which I plan to get into just now. That's fine. The modules were an afterthought and not intended at the time for standalone use, so it'd take a bit of work. I doubt the modules will be useful elsewhere, but will make code easier to maintain in the future. I also value functional/integration tests far more than unit tests. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
On 2012.7.26 10:18 PM, Junio C Hamano wrote: > Forgot to sign-off, or are you still unsure about this step? I just never think to do it. It's just a line in the commit message, right? There's no crypto involved like tag -s. Is it a blocker? I guess I can write a msg-filter if it's important. > 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. I don't personally plan on doing any more about it, no. It isn't needed for SVN 1.7, there's very little real code change (which you could see by looking at my remote instead of waiting to be fed patches...) and its a very, very minor problem in the grand scheme. How git-svn structures its switches needs a ton of work, and there are far deeper problems with Git::SVN. For one, it's completely undocumented. For another, Git::SVN can't instantiate an object without git-svn being loaded and so is very difficult to unit test. I wouldn't want to change the constructor interface until I could construct an object. The first step toward that would be to change git-svn so it can be loaded as a library using the standard "main() unless caller" trick. Then Git::SVN unit tests can require git-svn as a library without executing it and get some tests written with a minimum of Git::SVN code change. Step zero would be to allow Perl unit tests to either use or emulate the work done in lib-git-svn.sh. The major problem being how to communicate the location of the trash directory, currently done by environment variables. A simple trick would be for the Perl tests to execute a shell wrapper that outputs the relevant information. None of which I plan to get into just now. -- emacs -- THAT'S NO EDITOR... IT'S AN OPERATING SYSTEM! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
Junio C Hamano writes: > "Michael G. Schwern" writes: > >> From: "Michael G. Schwern" >> >> 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 s/is/should be/; sorry for the noise. > state after applying the whole series (not just these four but also > with the follow-up patches). > ... > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
"Michael G. Schwern" writes:
> From: "Michael G. Schwern"
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] Prepare Git::SVN for extraction into its own file.
From: "Michael G. Schwern"
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.
* Unqualify unnecessarily fully qualified globals like
$Git::SVN::default_repo_id.
* Lexically isolate the class just to make sure nothing is leaking out.
---
git-svn.perl | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 79fe4a4..9cdf6fc 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -89,7 +89,7 @@ BEGIN {
foreach (qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe
command_bidi_pipe command_close_bidi_pipe/) {
- for my $package ( qw(Git::SVN::Migration Git::SVN::Log
Git::SVN),
+ for my $package ( qw(Git::SVN::Migration Git::SVN::Log),
__PACKAGE__) {
*{"${package}::$_"} = \&{"Git::$_"};
}
@@ -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;
@@ -2038,6 +2042,7 @@ sub gc_directory {
}
}
+{
package Git::SVN;
use strict;
use warnings;
@@ -2056,6 +2061,13 @@ use Memoize; # core since 5.8.0, Jul 2002
use Memoize::Storable;
use POSIX qw(:signal_h);
+use Git qw(
+command
+command_oneline
+command_noisy
+command_output_pipe
+command_close_pipe
+);
use Git::SVN::Utils qw(fatal can_compress);
my $can_use_yaml;
@@ -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() : "";
$_[2] = $ref_id =
-"refs/remotes/$_prefix$Git::SVN::default_ref_id";
+"refs/remotes/$prefix$default_ref_id";
}
$_[1] = $repo_id;
my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
@@ -4347,6 +4360,7 @@ sub uri_decode {
sub remove_username {
$_[0] =~ s{^([^:]*://)[^@]+@}{$1};
}
+}
package Git::SVN::Log;
use strict;
--
1.7.11.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] Prepare Git::SVN for extraction into its own file.
From: "Michael G. Schwern"
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.
* Unqualify unnecessarily fully qualified globals like
$Git::SVN::default_repo_id.
* Lexically isolate the class just to make sure nothing is leaking out.
---
git-svn.perl | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 79fe4a4..9cdf6fc 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -89,7 +89,7 @@ BEGIN {
foreach (qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe
command_bidi_pipe command_close_bidi_pipe/) {
- for my $package ( qw(Git::SVN::Migration Git::SVN::Log
Git::SVN),
+ for my $package ( qw(Git::SVN::Migration Git::SVN::Log),
__PACKAGE__) {
*{"${package}::$_"} = \&{"Git::$_"};
}
@@ -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;
@@ -2038,6 +2042,7 @@ sub gc_directory {
}
}
+{
package Git::SVN;
use strict;
use warnings;
@@ -2056,6 +2061,13 @@ use Memoize; # core since 5.8.0, Jul 2002
use Memoize::Storable;
use POSIX qw(:signal_h);
+use Git qw(
+command
+command_oneline
+command_noisy
+command_output_pipe
+command_close_pipe
+);
use Git::SVN::Utils qw(fatal can_compress);
my $can_use_yaml;
@@ -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() : "";
$_[2] = $ref_id =
-"refs/remotes/$_prefix$Git::SVN::default_ref_id";
+"refs/remotes/$prefix$default_ref_id";
}
$_[1] = $repo_id;
my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
@@ -4347,6 +4360,7 @@ sub uri_decode {
sub remove_username {
$_[0] =~ s{^([^:]*://)[^@]+@}{$1};
}
+}
package Git::SVN::Log;
use strict;
--
1.7.11.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
