Move Git::SVN into its own .pm file

Move Git::SVN into its own .pm file

[Michael G. Schwern]

This is a refactoring to move Git::SVN out of git-svn and into its own .pm file.
This will make it easier to work with and test.  This is just the extraction
with minimal work to keep all tests passing.

A couple of utility functions which were used by Git::SVN, git-svn and others
were also extracted from git-svn into a new Git::SVN::Utils.  Not the most
imaginitive name, but it's better than Git::SVN grabbing at git-svn internals
and it allows Git::SVN (and later other classes) to stand alone without git-svn.

Its was reworked to be done backwards (instead of extracting and then fixing the
resulting problems, the problems were fixed in place and then it's extracted) in
order to keep every commit passing tests and provide a useful history.  This was
something of a pain.

[PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Michael G. Schwern]

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

Put them in a new module called Git::SVN::Utils.  Yeah, not terribly
original and it will be a dumping ground.  But its better than having
them in the main git-svn program.  At least they can be documented
and tested.

* fatal() is used by many classes.
* Change the $can_compress lexical into a function.

This should be enough to extract Git::SVN.

Signed-off-by: Michael G. Schwern <schwern@pobox.com>
---
 git-svn.perl                   | 34 +++++++++++++-----------
 perl/Git/SVN/Utils.pm          | 59 ++++++++++++++++++++++++++++++++++++++++++
 perl/Makefile                  |  1 +
 t/Git-SVN/00compile.t          |  8 ++++++
 t/Git-SVN/Utils/can_compress.t | 11 ++++++++
 t/Git-SVN/Utils/fatal.t        | 34 ++++++++++++++++++++++++
 6 files changed, 132 insertions(+), 15 deletions(-)
 create mode 100644 perl/Git/SVN/Utils.pm
 create mode 100644 t/Git-SVN/00compile.t
 create mode 100644 t/Git-SVN/Utils/can_compress.t
 create mode 100644 t/Git-SVN/Utils/fatal.t

diff --git a/git-svn.perl b/git-svn.perl
index 0b074c4..79fe4a4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -10,6 +10,8 @@ use vars qw/	$AUTHOR $VERSION
 $AUTHOR = 'Eric Wong <normalperson@yhbt.net>';
 $VERSION = '@@GIT_VERSION@@';
 
+use Git::SVN::Utils qw(fatal can_compress);
+
 # From which subdir have we been invoked?
 my $cmd_dir_prefix = eval {
 	command_oneline([qw/rev-parse --show-prefix/], STDERR => 0)
@@ -35,8 +37,6 @@ $Git::SVN::Log::TZ = $ENV{TZ};
 $ENV{TZ} = 'UTC';
 $| = 1; # unbuffer STDOUT
 
-sub fatal (@) { print STDERR "@_\n"; exit 1 }
-
 # All SVN commands do it.  Otherwise we may die on SIGPIPE when the remote
 # repository decides to close the connection which we expect to be kept alive.
 $SIG{PIPE} = 'IGNORE';
@@ -66,7 +66,7 @@ sub _req_svn {
 		fatal "Need SVN::Core 1.1.0 or better (got $SVN::Core::VERSION)";
 	}
 }
-my $can_compress = eval { require Compress::Zlib; 1};
+
 use Carp qw/croak/;
 use Digest::MD5;
 use IO::File qw//;
@@ -1578,7 +1578,7 @@ sub cmd_reset {
 }
 
 sub cmd_gc {
-	if (!$can_compress) {
+	if (!can_compress()) {
 		warn "Compress::Zlib could not be found; unhandled.log " .
 		     "files will not be compressed.\n";
 	}
@@ -2014,13 +2014,13 @@ sub md5sum {
 	} elsif (!$ref) {
 		$md5->add($arg) or croak $!;
 	} else {
-		::fatal "Can't provide MD5 hash for unknown ref type: '", $ref, "'";
+		fatal "Can't provide MD5 hash for unknown ref type: '", $ref, "'";
 	}
 	return $md5->hexdigest();
 }
 
 sub gc_directory {
-	if ($can_compress && -f $_ && basename($_) eq "unhandled.log") {
+	if (can_compress() && -f $_ && basename($_) eq "unhandled.log") {
 		my $out_filename = $_ . ".gz";
 		open my $in_fh, "<", $_ or die "Unable to open $_: $!\n";
 		binmode $in_fh;
@@ -2055,6 +2055,9 @@ use Time::Local;
 use Memoize;  # core since 5.8.0, Jul 2002
 use Memoize::Storable;
 use POSIX qw(:signal_h);
+
+use Git::SVN::Utils qw(fatal can_compress);
+
 my $can_use_yaml;
 BEGIN {
 	$can_use_yaml = eval { require Git::SVN::Memoize::YAML; 1};
@@ -2880,8 +2883,8 @@ sub assert_index_clean {
 		command_noisy('read-tree', $treeish);
 		$x = command_oneline('write-tree');
 		if ($y ne $x) {
-			::fatal "trees ($treeish) $y != $x\n",
-			        "Something is seriously wrong...";
+			fatal "trees ($treeish) $y != $x\n",
+			      "Something is seriously wrong...";
 		}
 	});
 }
@@ -3236,7 +3239,7 @@ sub mkemptydirs {
 	my %empty_dirs = ();
 	my $gz_file = "$self->{dir}/unhandled.log.gz";
 	if (-f $gz_file) {
-		if (!$can_compress) {
+		if (!can_compress()) {
 			warn "Compress::Zlib could not be found; ",
 			     "empty directories in $gz_file will not be read\n";
 		} else {
@@ -3919,7 +3922,7 @@ sub set_tree {
 	my ($self, $tree) = (shift, shift);
 	my $log_entry = ::get_commit_entry($tree);
 	unless ($self->{last_rev}) {
-		::fatal("Must have an existing revision to commit");
+		fatal("Must have an existing revision to commit");
 	}
 	my %ed_opts = ( r => $self->{last_rev},
 	                log => $log_entry->{log},
@@ -4348,6 +4351,7 @@ sub remove_username {
 package Git::SVN::Log;
 use strict;
 use warnings;
+use Git::SVN::Utils qw(fatal);
 use POSIX qw/strftime/;
 use constant commit_log_separator => ('-' x 72) . "\n";
 use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
@@ -4446,15 +4450,15 @@ sub config_pager {
 sub run_pager {
 	return unless defined $pager;
 	pipe my ($rfd, $wfd) or return;
-	defined(my $pid = fork) or ::fatal "Can't fork: $!";
+	defined(my $pid = fork) or fatal "Can't fork: $!";
 	if (!$pid) {
 		open STDOUT, '>&', $wfd or
-		                     ::fatal "Can't redirect to stdout: $!";
+		                     fatal "Can't redirect to stdout: $!";
 		return;
 	}
-	open STDIN, '<&', $rfd or ::fatal "Can't redirect stdin: $!";
+	open STDIN, '<&', $rfd or fatal "Can't redirect stdin: $!";
 	$ENV{LESS} ||= 'FRSX';
-	exec $pager or ::fatal "Can't run pager: $! ($pager)";
+	exec $pager or fatal "Can't run pager: $! ($pager)";
 }
 
 sub format_svn_date {
@@ -4603,7 +4607,7 @@ sub cmd_show_log {
 		} elsif ($::_revision =~ /^\d+$/) {
 			$r_min = $r_max = $::_revision;
 		} else {
-			::fatal "-r$::_revision is not supported, use ",
+			fatal "-r$::_revision is not supported, use ",
 				"standard 'git log' arguments instead";
 		}
 	}
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
new file mode 100644
index 0000000..3d0bfa4
--- /dev/null
+++ b/perl/Git/SVN/Utils.pm
@@ -0,0 +1,59 @@
+package Git::SVN::Utils;
+
+use strict;
+use warnings;
+
+use base qw(Exporter);
+
+our @EXPORT_OK = qw(fatal can_compress);
+
+
+=head1 NAME
+
+Git::SVN::Utils - utility functions used across Git::SVN
+
+=head1 SYNOPSIS
+
+    use Git::SVN::Utils qw(functions to import);
+
+=head1 DESCRIPTION
+
+This module contains functions which are useful across many different
+parts of Git::SVN.  Mostly it's a place to put utility functions
+rather than duplicate the code or have classes grabbing at other
+classes.
+
+=head1 FUNCTIONS
+
+All functions can be imported only on request.
+
+=head3 fatal
+
+    fatal(@message);
+
+Display a message and exit with a fatal error code.
+
+=cut
+
+# Note: not certain why this is in use instead of die.  Probably because
+# the exit code of die is 255?  Doesn't appear to be used consistently.
+sub fatal (@) { print STDERR "@_\n"; exit 1 }
+
+
+=head3 can_compress
+
+    my $can_compress = can_compress;
+
+Returns true if Compress::Zlib is available, false otherwise.
+
+=cut
+
+my $can_compress;
+sub can_compress {
+    return $can_compress if defined $can_compress;
+
+    return $can_compress = eval { require Compress::Zlib; } ? 1 : 0;
+}
+
+
+1;
diff --git a/perl/Makefile b/perl/Makefile
index 6ca7d47..24a9f5a 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -31,6 +31,7 @@ modules += Git/SVN/Fetcher
 modules += Git/SVN/Editor
 modules += Git/SVN/Prompt
 modules += Git/SVN/Ra
+modules += Git/SVN/Utils
 
 $(makfile): ../GIT-CFLAGS Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
new file mode 100644
index 0000000..a7aa85a
--- /dev/null
+++ b/t/Git-SVN/00compile.t
@@ -0,0 +1,8 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More tests => 1;
+
+require_ok 'Git::SVN::Utils';
diff --git a/t/Git-SVN/Utils/can_compress.t b/t/Git-SVN/Utils/can_compress.t
new file mode 100644
index 0000000..d7b49b8
--- /dev/null
+++ b/t/Git-SVN/Utils/can_compress.t
@@ -0,0 +1,11 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+use Git::SVN::Utils qw(can_compress);
+
+# !! is the "convert this to boolean" operator.
+is !!can_compress(), !!eval { require Compress::Zlib };
diff --git a/t/Git-SVN/Utils/fatal.t b/t/Git-SVN/Utils/fatal.t
new file mode 100644
index 0000000..b90746c
--- /dev/null
+++ b/t/Git-SVN/Utils/fatal.t
@@ -0,0 +1,34 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+BEGIN {
+    # Override exit at BEGIN time before Git::SVN::Utils is loaded
+    # so it will see our local exit later.
+    *CORE::GLOBAL::exit = sub(;$) {
+        return @_ ? CORE::exit($_[0]) : CORE::exit();
+    };
+}
+
+use Git::SVN::Utils qw(fatal);
+
+# fatal()
+{
+    # Capture the exit code and prevent exit.
+    my $exit_status;
+    no warnings 'redefine';
+    local *CORE::GLOBAL::exit = sub { $exit_status = $_[0] || 0 };
+
+    # Trap fatal's message to STDERR
+    my $stderr;
+    close STDERR;
+    ok open STDERR, ">", \$stderr;
+
+    fatal "Some", "Stuff", "Happened";
+
+    is $stderr, "Some Stuff Happened\n";
+    is $exit_status, 1;
+}
-- 
1.7.11.1

Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Eric Wong]

"Michael G. Schwern" <schwern@pobox.com> wrote:
> From: "Michael G. Schwern" <schwern@pobox.com>
> 
> Put them in a new module called Git::SVN::Utils.  Yeah, not terribly
> original and it will be a dumping ground.  But its better than having
> them in the main git-svn program.  At least they can be documented
> and tested.
> 
> * fatal() is used by many classes.
> * Change the $can_compress lexical into a function.
> 
> This should be enough to extract Git::SVN.

Please keep Jonathan Cc:-ed, he's been very helpful with this series
(and very helpful in general :)

This series is mostly alright by me, a few minor comments inline.

> --- /dev/null
> +++ b/t/Git-SVN/00compile.t
> +
> +use Test::More tests => 1;

> +++ b/t/Git-SVN/Utils/fatal.t
> @@ -0,0 +1,34 @@
> +
> +use Test::More 'no_plan';

Didn't we agree to use done_testing()?   Perhaps (as you suggested) with
a private copy of Test::More?  It's probably easier to start using
done_testing() earlier rather than later.

> +BEGIN {
> +    # Override exit at BEGIN time before Git::SVN::Utils is loaded
> +    # so it will see our local exit later.
> +    *CORE::GLOBAL::exit = sub(;$) {
> +        return @_ ? CORE::exit($_[0]) : CORE::exit();
> +    };
> +}

For new code related to git-svn, please match the existing indentation
style (tabs) prevalent in git-svn.  Most of the Perl found in git also
uses tabs for indentation.

Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Michael G Schwern]

On 2012.7.25 2:24 PM, Eric Wong wrote:
> Please keep Jonathan Cc:-ed, he's been very helpful with this series
> (and very helpful in general :)

I will try.


>> +use Test::More 'no_plan';
> 
> Didn't we agree to use done_testing()?   Perhaps (as you suggested) with
> a private copy of Test::More?  It's probably easier to start using
> done_testing() earlier rather than later.

Yes, we agreed done_testing is the way forward.  Given how much work I've had
to do to get even basic patches in I decided to ditch anything extra.  That
includes adding a t/lib and I didn't want to make it silently depend on an
upgraded Test::More either.

There's not much difference if we do it later.  Switching to done_testing is
trivial.  I'd like to get the big class extractions in so code stops shifting
around, and worry about the minutia of test plans later.  If it happens before
I get to it, great!

PS  Those t/Git-SVN/ tests are not tied into the normal testing process.  I
felt writing the tests now was important and they could be integrated into the
test suite later.


>> +BEGIN {
>> +    # Override exit at BEGIN time before Git::SVN::Utils is loaded
>> +    # so it will see our local exit later.
>> +    *CORE::GLOBAL::exit = sub(;$) {
>> +        return @_ ? CORE::exit($_[0]) : CORE::exit();
>> +    };
>> +}
> 
> For new code related to git-svn, please match the existing indentation
> style (tabs) prevalent in git-svn.  Most of the Perl found in git also
> uses tabs for indentation.

About that.  I followed kernel style in existing code, but felt that new code
would do better to follow Perl style.  The existing Perl code mixes tabs and
spaces, so I felt it wasn't a strongly held style.  You'll get more Perl
programmers to work on the Perl code by following Perl style in the Perl code
rather than kernel style.

Alternatively, how about allowing emacs/vim configuration comments?  The
Kernel coding style doesn't allow them, how do you folks feel?  Then people
don't have to guess the style and reconfigure their editor, their editor will
do it for them.

The important thing is to have one less special thing a new-to-your-project
Perl programmer has to do.


-- 
ROCKS FALL! EVERYONE DIES!
	http://www.somethingpositive.net/sp05032002.shtml

Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> On 2012.7.25 2:24 PM, Eric Wong wrote:
> > Didn't we agree to use done_testing()?   Perhaps (as you suggested) with
> > a private copy of Test::More?  It's probably easier to start using
> > done_testing() earlier rather than later.
> 
> Yes, we agreed done_testing is the way forward.  Given how much work I've had
> to do to get even basic patches in I decided to ditch anything extra.  That
> includes adding a t/lib and I didn't want to make it silently depend on an
> upgraded Test::More either.

OK.

> >> +BEGIN {
> >> +    # Override exit at BEGIN time before Git::SVN::Utils is loaded
> >> +    # so it will see our local exit later.
> >> +    *CORE::GLOBAL::exit = sub(;$) {
> >> +        return @_ ? CORE::exit($_[0]) : CORE::exit();
> >> +    };
> >> +}
> > 
> > For new code related to git-svn, please match the existing indentation
> > style (tabs) prevalent in git-svn.  Most of the Perl found in git also
> > uses tabs for indentation.
> 
> About that.  I followed kernel style in existing code, but felt that new code
> would do better to follow Perl style.  The existing Perl code mixes tabs and
> spaces, so I felt it wasn't a strongly held style.  You'll get more Perl
> programmers to work on the Perl code by following Perl style in the Perl code
> rather than kernel style.

git-svn should be all tabs (though some spaces accidentally slipped in
over the years).  git maintainers are mostly C programmers used to tabs,
so non-C code should favor their expectations.

I also favor tabs since they're more space-efficient and leads to faster
"git grep" on large source trees (more important for bigger projects).
I remember many years ago "git grep" was shown to be ~20% faster on
a source tree with tabs.

(I'm neither a kernel hacker nor a regular Perl hacker)

> Alternatively, how about allowing emacs/vim configuration comments?  The
> Kernel coding style doesn't allow them, how do you folks feel?  Then people
> don't have to guess the style and reconfigure their editor, their editor will
> do it for them.

I don't like them, and I think others here frowns upon them, too.  They
take too much space and shows favor/preference towards certain editors.
It'll be a bigger problem if more editors become popular and we start
"supporting" them.

> The important thing is to have one less special thing a new-to-your-project
> Perl programmer has to do.

Historically git development has catered to C programmers used to tabs.
I think the mixing of indentation styles in current Perl files is the
most confusing.

Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Michael G Schwern]

This is rapidly getting into the weeds.  Regardless of the debate below, what
would you like me to do?  Switch indentation to tabs and resubmit?  AFAIK only
the new tests are affected.


On 2012.7.25 4:08 PM, Eric Wong wrote:
>>>> +BEGIN {
>>>> +    # Override exit at BEGIN time before Git::SVN::Utils is loaded
>>>> +    # so it will see our local exit later.
>>>> +    *CORE::GLOBAL::exit = sub(;$) {
>>>> +        return @_ ? CORE::exit($_[0]) : CORE::exit();
>>>> +    };
>>>> +}
>>>
>>> For new code related to git-svn, please match the existing indentation
>>> style (tabs) prevalent in git-svn.  Most of the Perl found in git also
>>> uses tabs for indentation.
>>
>> About that.  I followed kernel style in existing code, but felt that new code
>> would do better to follow Perl style.  The existing Perl code mixes tabs and
>> spaces, so I felt it wasn't a strongly held style.  You'll get more Perl
>> programmers to work on the Perl code by following Perl style in the Perl code
>> rather than kernel style.
> 
> git-svn should be all tabs (though some spaces accidentally slipped in
> over the years).  git maintainers are mostly C programmers used to tabs,
> so non-C code should favor their expectations.

Perhaps this is self fulfilling.  Would you like to attract more Perl programmers?


> I also favor tabs since they're more space-efficient and leads to faster
> "git grep" on large source trees (more important for bigger projects).
> I remember many years ago "git grep" was shown to be ~20% faster on
> a source tree with tabs.

Storage costs a dime a gig.  One extra music file negates your space savings.
 I have more CPU power on my phone than I had on my desktop "many years ago".
 It doesn't matter if tabs make git-grep 20% faster because it's going to be
200ms vs 240ms.

Regardless, you don't choose your coding style because it's easier for the
computer.  You choose it because it makes the code easier for humans to work with.


> (I'm neither a kernel hacker nor a regular Perl hacker)
> 
>> Alternatively, how about allowing emacs/vim configuration comments?  The
>> Kernel coding style doesn't allow them, how do you folks feel?  Then people
>> don't have to guess the style and reconfigure their editor, their editor will
>> do it for them.
> 
> I don't like them, and I think others here frowns upon them, too.  They
> take too much space and shows favor/preference towards certain editors.
> It'll be a bigger problem if more editors become popular and we start
> "supporting" them.

What you say above is correct, but the extra space is not wasted.  It would be
like complaining about all the space that Documentation takes up, and that it
shows preference towards English.  Its *useful* to somebody not already using
your style.  It's useful for new-to-your-project folks.  It's also useful for
somebody switching between the C and Perl code (though a good editor should
already be set up to do C and Perl differently).

Are the editor wars still going on here?  Is somebody going to be *offended*
if their editor isn't represented?  If so, shouldn't they grow up?


>> The important thing is to have one less special thing a new-to-your-project
>> Perl programmer has to do.
> 
> Historically git development has catered to C programmers used to tabs.
> I think the mixing of indentation styles in current Perl files is the
> most confusing.

I agree that mixing the style within the Perl code isn't good.  Perl code can
very quickly be reformatted.  A basic perltidy config can help keep it that way.


-- 
Alligator sandwich, and make it snappy!

Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> This is rapidly getting into the weeds.  Regardless of the debate below, what
> would you like me to do?  Switch indentation to tabs and resubmit?  AFAIK only
> the new tests are affected.

Yes, unless Jonathan (or anybody else) has more comments.

> On 2012.7.25 4:08 PM, Eric Wong wrote:
> >>>> +BEGIN {
> >>>> +    # Override exit at BEGIN time before Git::SVN::Utils is loaded
> >>>> +    # so it will see our local exit later.
> >>>> +    *CORE::GLOBAL::exit = sub(;$) {
> >>>> +        return @_ ? CORE::exit($_[0]) : CORE::exit();
> >>>> +    };
> >>>> +}
> >>>
> >>> For new code related to git-svn, please match the existing indentation
> >>> style (tabs) prevalent in git-svn.  Most of the Perl found in git also
> >>> uses tabs for indentation.
> >>
> >> About that.  I followed kernel style in existing code, but felt that new code
> >> would do better to follow Perl style.  The existing Perl code mixes tabs and
> >> spaces, so I felt it wasn't a strongly held style.  You'll get more Perl
> >> programmers to work on the Perl code by following Perl style in the Perl code
> >> rather than kernel style.
> > 
> > git-svn should be all tabs (though some spaces accidentally slipped in
> > over the years).  git maintainers are mostly C programmers used to tabs,
> > so non-C code should favor their expectations.
> 
> Perhaps this is self fulfilling.  Would you like to attract more Perl
> programmers?

I prefer programmers not tied to a particular language.

> > I also favor tabs since they're more space-efficient and leads to faster
> > "git grep" on large source trees (more important for bigger projects).
> > I remember many years ago "git grep" was shown to be ~20% faster on
> > a source tree with tabs.
> 
> Storage costs a dime a gig.

It's also kernel page cache overhead.

> Regardless, you don't choose your coding style because it's easier for the
> computer.  You choose it because it makes the code easier for humans to work with.

Hard tabs also happen to be the default indent for my editor (which
is also a popular editor) and slightly easier for me.

> >> Alternatively, how about allowing emacs/vim configuration comments?  The
> >> Kernel coding style doesn't allow them, how do you folks feel?  Then people
> >> don't have to guess the style and reconfigure their editor, their editor will
> >> do it for them.
> > 
> > I don't like them, and I think others here frowns upon them, too.  They
> > take too much space and shows favor/preference towards certain editors.
> > It'll be a bigger problem if more editors become popular and we start
> > "supporting" them.
> 
> What you say above is correct, but the extra space is not wasted.  It would be
> like complaining about all the space that Documentation takes up, and that it
> shows preference towards English.  Its *useful* to somebody not already using
> your style.  It's useful for new-to-your-project folks.  It's also useful for
> somebody switching between the C and Perl code (though a good editor should
> already be set up to do C and Perl differently).
> 
> Are the editor wars still going on here?  Is somebody going to be *offended*
> if their editor isn't represented?  If so, shouldn't they grow up?

It's not about editor wars, but unnecessary code churn to accept/review
patches to support new editors, making it a maintenance burden.  Picking
up (and following) existing conventions within a project ought to be
common sense.

Anyways I don't speak for others, but seeing how we've gone all these
years without editor-specific comments, I don't believe other git devs
want them, either.

Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.

[Jonathan Nieder]

Michael G Schwern wrote:

> This is rapidly getting into the weeds.  Regardless of the debate below, what
> would you like me to do?  Switch indentation to tabs and resubmit?  AFAIK only
> the new tests are affected.

Yes, please do so at some point before the final submission.  (If
there's more feedback coming for this generation of the patches, no
need to resubmit right away unless you want to.  In general, when
getting review a useful strategy can be to discuss proposed changes
and batch them until the discussion seems to have died down and only
then resubmit the next version.)

If we want to switch indentation styles (not a traditional way to
start working with an existing project, but whatever), that would be a
separate change, affecting all the code consistently.

Thanks,
Jonathan

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

[Michael G. Schwern]

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.

* 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

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

[Michael G. Schwern]

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

Also it can compile on its own now, yay!
---
 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';
-- 
1.7.11.1

Re: Move Git::SVN into its own .pm file

[Jonathan Nieder]

Hi,

Michael G. Schwern wrote:

> This is a refactoring to move Git::SVN out of git-svn and into its own .pm file.
> This will make it easier to work with and test.  This is just the extraction
> with minimal work to keep all tests passing.

Patch 3 doesn't seem to have hit the list archive.  I am not
subscribed and was not cc-ed for some reason, so I do not have it.
I'd appreciate a copy if that's not too inconvenient.

Thanks,
Jonathan

Re: Move Git::SVN into its own .pm file

[Michael G Schwern]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

On 2012.7.25 2:15 PM, Jonathan Nieder wrote:
> Michael G. Schwern wrote:
>> This is a refactoring to move Git::SVN out of git-svn and into its own .pm file.
>> This will make it easier to work with and test.  This is just the extraction
>> with minimal work to keep all tests passing.
> 
> Patch 3 doesn't seem to have hit the list archive.  I am not
> subscribed and was not cc-ed for some reason, so I do not have it.
> I'd appreciate a copy if that's not too inconvenient.

The patch was 136k.  I'm going to guess there's some old 50k filter somewhere
that blocked it.

<insert ironic statement about email based development here>

The patch is attached.


-- 
87. If the thought of something makes me giggle for longer than 15
    seconds, I am to assume that I am not allowed to do it.
    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
           http://skippyslist.com/list/

[-- Attachment #2: 0001-Extract-Git-SVN-from-git-svn-into-its-own-.pm-file.patch.gz --]
[-- Type: application/x-tar-gz, Size: 40861 bytes --]

Extract Git::SVN from git-svn, take 2.

[Michael G. Schwern]

Same as before, now with tab indentation in the new Perl tests.

As before, patch #3 is 132k and will be rejected by some of the lists.

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

[Michael G. Schwern]

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

Also it can compile on its own now, yay!
---
 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';
-- 
1.7.11.1

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

[Junio C Hamano]

"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?

> ---
>  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';

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

[Jonathan Nieder]

Junio C Hamano wrote:
> "Michael G. Schwern" <schwern@pobox.com> writes:

>> Also it can compile on its own now, yay!
>
> Hmmm.

I agree with Michael's "yay" and also think it's fine that after
patch 3 it isn't there yet.

That's because git-svn.perl doesn't use Git::SVN on its own but helps
it out a little.  So even if we only applied patches 1-3, git-svn
would still work (maybe it's worth testing "perl -MGit::SVN" by hand
to avoid the "it's doubtful" about whether Git::SVN is self-contained
and replace it with a more certain statement?), and patch 4 just makes
it even better.

[...]
> 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?

I think this is pretty good already, though I also like your
suggestion re 2/4.

I haven't reviewed the tests these introduce and assume Eric has that
covered.

Thanks,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> 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?
>
> I think this is pretty good already, though I also like your
> suggestion re 2/4.
>
> I haven't reviewed the tests these introduce and assume Eric has that
> covered.

I didn't mean to say "Unless you prove that the two suggestions are
not easy to implement, I will veto the series until they are fixed."
Especially, I consider that the ordering between 3 and 4 falls into
the "it would be nicer if this wart weren't there" category.

The result will be queued tentatively near the tip of 'pu', but as
this is primarily about git-svn, I would prefer a copy that is
vetted by Eric to be fed from him.

Thanks.

P.S.

t91XX series seem to fail in 'pu' with "Can't locate Git/SVN.pm in
@INC" for me.  I see perl/blib/lib/Git/SVN/ directory and files
under it, but there is no perl/blib/lib/Git/SVN.pm installed.  I see
Git/I18N.pm and Git/SVN/Ra.pm (and friends) mentioned in
perl/perl.mak generated by MakeMaker, but Git/SVN.pm does not appear
anywhere.

I think it is some interaction with other topics, as the tip of
ms/git-svn-pm topic that parks this series does not exhibit the
symptom, but it is getting late for me already, so I won't dig into
this further.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> t91XX series seem to fail in 'pu' with "Can't locate Git/SVN.pm in
> @INC" for me.  I see perl/blib/lib/Git/SVN/ directory and files
> under it, but there is no perl/blib/lib/Git/SVN.pm installed.  I see
> Git/I18N.pm and Git/SVN/Ra.pm (and friends) mentioned in
> perl/perl.mak generated by MakeMaker, but Git/SVN.pm does not appear
> anywhere.
>
> I think it is some interaction with other topics, as the tip of
> ms/git-svn-pm topic that parks this series does not exhibit the
> symptom, but it is getting late for me already, so I won't dig into
> this further.

Actually there is no difference between ms/git-svn-pm and pu in perl/
directory. I _think_ there is some dependency missing that makes
this sequence break:

	(in one repository)
	git checkout pu ;# older pu without ms/git-svn-pm
        make ; make test

	(in another repository that shares the refs)
	git checkout pu
        git merge ms/git-svn-pm

	(in the first repository)
        git reset --hard ;# update the working tree
        make ; make test

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> t91XX series seem to fail in 'pu' with "Can't locate Git/SVN.pm in
>> @INC" for me.  I see perl/blib/lib/Git/SVN/ directory and files
>> under it, but there is no perl/blib/lib/Git/SVN.pm installed.  I see
>> Git/I18N.pm and Git/SVN/Ra.pm (and friends) mentioned in
>> perl/perl.mak generated by MakeMaker, but Git/SVN.pm does not appear
>> anywhere.
>> 
>> I think it is some interaction with other topics, as the tip of
>> ms/git-svn-pm topic that parks this series does not exhibit the
>> symptom, but it is getting late for me already, so I won't dig into
>> this further.
>
> Actually there is no difference between ms/git-svn-pm and pu in perl/
> directory. I _think_ there is some dependency missing that makes
> this sequence break:
>
> 	(in one repository)
> 	git checkout pu ;# older pu without ms/git-svn-pm
>         make ; make test
>
> 	(in another repository that shares the refs)
> 	git checkout pu
>         git merge ms/git-svn-pm
>
> 	(in the first repository)
>         git reset --hard ;# update the working tree
>         make ; make test

What was happening was that originally, pu had ms/makefile-pl but
not ms/git-svn-pm.  Hence, perl/Git/SVN.pm did not exist.  I ran
"make" and it created perl/perl.mak that does not know about
Git/SVN.pm;

Then ms/git-svn-pm is merged to pu and now we have perl/Git/SVN.pm.
But there is nothing in ms/makefile-pl that says on what files
perl.mak depends on.

I think there needs to be a dependency in to recreate perl/perl.mak
when any of the *.pm files are changed, perhaps like this.

I am not sure why perl/perl.mak is built by the top-level Makefile,
instead of just using "$(MAKE) -C perl/", though...

 Makefile      | 7 +++++++
 perl/Makefile | 1 +
 2 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index b0b3493..e2a4ac7 100644
--- a/Makefile
+++ b/Makefile
@@ -2090,6 +2090,13 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
+perl/perl.mak: perl/PM.stamp
+
+perl/PM.stamp: FORCE
+	$(QUIET_GEN)find perl -type f -name '*.pm' | sort >$@+ && \
+	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
+	$(RM) $@+
+
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
diff --git a/perl/Makefile b/perl/Makefile
index 6ca7d47..d6f8478 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -20,6 +20,7 @@ clean:
 	$(RM) ppport.h
 	$(RM) $(makfile)
 	$(RM) $(makfile).old
+	$(RM) PM.stamp
 
 ifdef NO_PERL_MAKEMAKER
 instdir_SQ = $(subst ','\'',$(prefix)/lib)

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> What was happening was that originally, pu had ms/makefile-pl but
> not ms/git-svn-pm.  Hence, perl/Git/SVN.pm did not exist.  I ran
> "make" and it created perl/perl.mak that does not know about
> Git/SVN.pm;
> 
> Then ms/git-svn-pm is merged to pu and now we have perl/Git/SVN.pm.
> But there is nothing in ms/makefile-pl that says on what files
> perl.mak depends on.
> 
> I think there needs to be a dependency in to recreate perl/perl.mak
> when any of the *.pm files are changed, perhaps like this.
> 
> I am not sure why perl/perl.mak is built by the top-level Makefile,
> instead of just using "$(MAKE) -C perl/", though...

I'm not sure why perl/perl.mak is built by the top-level Makefile,
either.

I'll put the following after ms/makefile-pl but before ms/git-svn-pm:

>From a6ea2301d1bb6fd7c7415fed3aa7673542a563bd Mon Sep 17 00:00:00 2001
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 27 Jul 2012 20:04:20 +0000
Subject: [PATCH] perl: detect new files in MakeMaker builds

While Makefile.PL now finds .pm files on its own, it does not
detect new files after it generates perl/perl.mak.

[ew: commit message]

ref: http://mid.gmane.org/7vlii51xz4.fsf@alter.siamese.dyndns.org

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Makefile      | 7 +++++++
 perl/Makefile | 1 +
 2 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index b0b3493..e2a4ac7 100644
--- a/Makefile
+++ b/Makefile
@@ -2090,6 +2090,13 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
+perl/perl.mak: perl/PM.stamp
+
+perl/PM.stamp: FORCE
+	$(QUIET_GEN)find perl -type f -name '*.pm' | sort >$@+ && \
+	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
+	$(RM) $@+
+
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
diff --git a/perl/Makefile b/perl/Makefile
index 8493d76..4969ef8 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -20,6 +20,7 @@ clean:
 	$(RM) ppport.h
 	$(RM) $(makfile)
 	$(RM) $(makfile).old
+	$(RM) PM.stamp
 
 ifdef NO_PERL_MAKEMAKER
 instdir_SQ = $(subst ','\'',$(prefix)/lib)
-- 
Eric Wong

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

[Michael G Schwern]

On 2012.7.27 1:07 PM, Eric Wong wrote:
> While Makefile.PL now finds .pm files on its own, it does not
> detect new files after it generates perl/perl.mak.

Are you saying this doesn't work?

perl Makefile.PL
make -f perl.mak
touch Git/Foo.pm
perl Makefile.PL
make -f perl.mak

or this?

perl Makefile.PL
make -f perl.mak
touch Git/Foo.pm
make -f perl.mak

The former should work.  The latter is a MakeMaker limitation.  Makefile.PL
hard codes the list of .pm files into the Makefile.


-- 
Who invented the eponym?

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

[Eric Wong]

Michael G Schwern <schwern@pobox.com> wrote:
> On 2012.7.27 1:07 PM, Eric Wong wrote:
> > While Makefile.PL now finds .pm files on its own, it does not
> > detect new files after it generates perl/perl.mak.
> 
> Are you saying this doesn't work?
> 
> perl Makefile.PL
> make -f perl.mak
> touch Git/Foo.pm
> perl Makefile.PL
> make -f perl.mak

This works.

> or this?
> 
> perl Makefile.PL
> make -f perl.mak
> touch Git/Foo.pm
> make -f perl.mak
> 
> The former should work.  The latter is a MakeMaker limitation.  Makefile.PL
> hard codes the list of .pm files into the Makefile.

Yup, Junio's patch works around the MM limitation so the latter works.

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

[Junio C Hamano]

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

> On 2012.7.27 1:07 PM, Eric Wong wrote:
>> While Makefile.PL now finds .pm files on its own, it does not
>> detect new files after it generates perl/perl.mak.
>
> Are you saying this doesn't work?
>
> perl Makefile.PL
> make -f perl.mak
> touch Git/Foo.pm
> perl Makefile.PL
> make -f perl.mak
>
> or this?
>
> perl Makefile.PL
> make -f perl.mak
> touch Git/Foo.pm
> make -f perl.mak

Neither of the above.  Nobody should be typing "perl Makefile.PL"
inside our source tree unless he is trying to debug our Makefiles
anyway.

What does not work is this sequence:

	make
        >perl/Git/Foo.pm
        make

Makefile at the top-level, which builds perl/perl.mak by running
"perl Makefile.PL" in perl/ subdirectory, doesn't have dependencies
[*1*], so in the above sequence, the second invocation of "make"
fails to rebuild perl/perl.mak, which causes Git/Foo.pm forgotten
from the build/installation step.

And that is what happened to Git/SVN.pm.


[Footnote]

*1* I also suspect perl/Makefile lacks this dependency even though
it has its own rule to build perl/perl.mak---don't they need to be
cleaned-up and merged???

	

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> I'll put the following after ms/makefile-pl but before ms/git-svn-pm:

OK, it seems that you haven't pushed out the result yet (which is
fine); how do we want to proceed?

I generally prefer pulling from maintainer trees directly to my
'master' without staging them in 'next', so when the above is done
and you feel the tip of your tree is good for 1.7.12-rc1 without
regression, please throw me a "pull, now!".

Thanks.

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > I'll put the following after ms/makefile-pl but before ms/git-svn-pm:
> 
> OK, it seems that you haven't pushed out the result yet (which is
> fine); how do we want to proceed?

Oops, got sidetracked into something else.  Before I got sidetracked,
my application of another patch in Michael's 3rd series failed
(even with your updated Makefile patch):

  [PATCH 7/8] Extract Git::SVN::GlobSpec from git-svn

GlobSpec.pm did not get picked up and placed into blib/ and some tests
(t9118-git-svn-funky-branch-names.sh) failed as a result

So I'll hold off until we can fix the build regressions (working on it
now)

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

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Wong <normalperson@yhbt.net> writes:
> > 
> > > I'll put the following after ms/makefile-pl but before ms/git-svn-pm:
> > 
> > OK, it seems that you haven't pushed out the result yet (which is
> > fine); how do we want to proceed?

> So I'll hold off until we can fix the build regressions (working on it
> now)

OK, all fixed, all I needed was this (squashed in):

--- a/perl/Makefile
+++ b/perl/Makefile
@@ -22,6 +22,8 @@ clean:
 	$(RM) $(makfile).old
 	$(RM) PM.stamp
 
+$(makfile): PM.stamp
+
 ifdef NO_PERL_MAKEMAKER
 instdir_SQ = $(subst ','\'',$(prefix)/lib)

-------------
The redundant dependencies are biting us :<  I agree there presence in
the top-level Makefile needs to be reviewed.

Anyways, you can pull this now from my master:

The following changes since commit cdd159b2f56c9e69e37bbb8f5af301abd93e5407:

  Merge branch 'jc/test-lib-source-build-options-early' (2012-07-25 15:47:08 -0700)

are available in the git repository at:

  git://bogomips.org/git-svn master

for you to fetch changes up to 5c71028fced46d03bf81b8625680d9ac87c8f4f0:

  Move initialization of Git::SVN variables into Git::SVN. (2012-07-27 22:14:54 +0000)

----------------------------------------------------------------
Junio C Hamano (1):
      perl: detect new files in MakeMaker builds

Michael G. Schwern (7):
      Quiet warning if Makefile.PL is run with -w and no --localedir
      Don't lose Error.pm if $@ gets clobbered.
      The Makefile.PL will now find .pm files itself.
      Extract some utilities from git-svn to allow extracting Git::SVN.
      Prepare Git::SVN for extraction into its own file.
      Extract Git::SVN from git-svn into its own .pm file.
      Move initialization of Git::SVN variables into Git::SVN.

 Makefile                       |    7 +
 git-svn.perl                   | 2340 +---------------------------------------
 perl/.gitignore                |    1 +
 perl/Git/SVN.pm                | 2324 +++++++++++++++++++++++++++++++++++++++
 perl/Git/SVN/Utils.pm          |   59 +
 perl/Makefile                  |    5 +
 perl/Makefile.PL               |   35 +-
 t/Git-SVN/00compile.t          |    9 +
 t/Git-SVN/Utils/can_compress.t |   11 +
 t/Git-SVN/Utils/fatal.t        |   34 +
 10 files changed, 2487 insertions(+), 2338 deletions(-)
 create mode 100644 perl/Git/SVN.pm
 create mode 100644 perl/Git/SVN/Utils.pm
 create mode 100644 t/Git-SVN/00compile.t
 create mode 100644 t/Git-SVN/Utils/can_compress.t
 create mode 100644 t/Git-SVN/Utils/fatal.t

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> OK, all fixed, all I needed was this (squashed in):
>
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -22,6 +22,8 @@ clean:
>  	$(RM) $(makfile).old
>  	$(RM) PM.stamp
>  
> +$(makfile): PM.stamp
> +
>  ifdef NO_PERL_MAKEMAKER
>  instdir_SQ = $(subst ','\'',$(prefix)/lib)
>
> -------------
> The redundant dependencies are biting us :<  I agree there presence in
> the top-level Makefile needs to be reviewed.

Do you feel confident enough that we can leave that question hanging
around and still merge this before 1.7.12 safely?

I do not think it is a regression at the Makefile level per-se---we
didn't have right dependencies to keep perl.mak up to date, which
was the root cause of what we observed.

But the lack of dependencies did not matter before this series
because the list of *.pm files never changed, so in that sense the
series is what introduced the build regression, and I do not have a
solid feeling that we squashed it.

> Anyways, you can pull this now from my master:
>
> The following changes since commit cdd159b2f56c9e69e37bbb8f5af301abd93e5407:
>
>   Merge branch 'jc/test-lib-source-build-options-early' (2012-07-25 15:47:08 -0700)
>
> are available in the git repository at:
>
>   git://bogomips.org/git-svn master
>
> for you to fetch changes up to 5c71028fced46d03bf81b8625680d9ac87c8f4f0:
>
>   Move initialization of Git::SVN variables into Git::SVN. (2012-07-27 22:14:54 +0000)
>
> ----------------------------------------------------------------
> Junio C Hamano (1):
>       perl: detect new files in MakeMaker builds
>
> Michael G. Schwern (7):
>       Quiet warning if Makefile.PL is run with -w and no --localedir
>       Don't lose Error.pm if $@ gets clobbered.
>       The Makefile.PL will now find .pm files itself.
>       Extract some utilities from git-svn to allow extracting Git::SVN.
>       Prepare Git::SVN for extraction into its own file.
>       Extract Git::SVN from git-svn into its own .pm file.
>       Move initialization of Git::SVN variables into Git::SVN.
>
>  Makefile                       |    7 +
>  git-svn.perl                   | 2340 +---------------------------------------
>  perl/.gitignore                |    1 +
>  perl/Git/SVN.pm                | 2324 +++++++++++++++++++++++++++++++++++++++
>  perl/Git/SVN/Utils.pm          |   59 +
>  perl/Makefile                  |    5 +
>  perl/Makefile.PL               |   35 +-
>  t/Git-SVN/00compile.t          |    9 +
>  t/Git-SVN/Utils/can_compress.t |   11 +
>  t/Git-SVN/Utils/fatal.t        |   34 +
>  10 files changed, 2487 insertions(+), 2338 deletions(-)
>  create mode 100644 perl/Git/SVN.pm
>  create mode 100644 perl/Git/SVN/Utils.pm
>  create mode 100644 t/Git-SVN/00compile.t
>  create mode 100644 t/Git-SVN/Utils/can_compress.t
>  create mode 100644 t/Git-SVN/Utils/fatal.t

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> > The redundant dependencies are biting us :<  I agree there presence in
> > the top-level Makefile needs to be reviewed.
> 
> Do you feel confident enough that we can leave that question hanging
> around and still merge this before 1.7.12 safely?

Yes.

> I do not think it is a regression at the Makefile level per-se---we
> didn't have right dependencies to keep perl.mak up to date, which
> was the root cause of what we observed.
> 
> But the lack of dependencies did not matter before this series
> because the list of *.pm files never changed, so in that sense the
> series is what introduced the build regression, and I do not have a
> solid feeling that we squashed it.

Right, I agree the original dependencies are not good and it's not
a recent regression in the Makefile level.

I do feel our patch deals with the problem for now.  I've been going
between commits in Michael's 3rd series and haven't noticed new issues
when running the tests.

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Wong <normalperson@yhbt.net> writes:
>> > The redundant dependencies are biting us :<  I agree there presence in
>> > the top-level Makefile needs to be reviewed.
>> 
>> Do you feel confident enough that we can leave that question hanging
>> around and still merge this before 1.7.12 safely?
>
> Yes.
>
>> I do not think it is a regression at the Makefile level per-se---we
>> didn't have right dependencies to keep perl.mak up to date, which
>> was the root cause of what we observed.
>> 
>> But the lack of dependencies did not matter before this series
>> because the list of *.pm files never changed, so in that sense the
>> series is what introduced the build regression, and I do not have a
>> solid feeling that we squashed it.
>
> Right, I agree the original dependencies are not good and it's not
> a recent regression in the Makefile level.
>
> I do feel our patch deals with the problem for now.  I've been going
> between commits in Michael's 3rd series and haven't noticed new issues
> when running the tests.

Ok, please don't forget to add necessary .gitignore rule for the new
stamp file.

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Ok, please don't forget to add necessary .gitignore rule for the new
> stamp file.

I noticed/remembered that, but I forgot to mention I squashed that in,
too :)

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Eric Wong <normalperson@yhbt.net> wrote:
>> So I'll hold off until we can fix the build regressions (working on it
>> now)
>
> OK, all fixed, all I needed was this (squashed in):
>
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -22,6 +22,8 @@ clean:
>  	$(RM) $(makfile).old
>  	$(RM) PM.stamp
>  
> +$(makfile): PM.stamp
> +
>  ifdef NO_PERL_MAKEMAKER
>  instdir_SQ = $(subst ','\'',$(prefix)/lib)

Another thing I noticed but didn't say was that the top-level
Makefile seems to think without NO_PERL the way to regenerate
perl/perl.mak is to run perl/Makefile.PL, which is not true if the
build is done with NO_PERL_MAKEMAKER.

I do not offhand know why we even need to have dependency on
perl/perl.mak in the toplevel Makefile (other than "otherwise nobody
descends into perl/ and run make in it", which is a bogus
reason---there should be a rule to run "$(MAKE) -C perl/ $@" when
doing "make all" at the top-level if that is the case), but I think
at least the duplicated rule in the toplevel Makefile should read
something like:

	perl/perl.mak: ... (the dependencies) ...
		$(QUIET_SUBDIR0)perl ... (make variables) ... perl.mak

so that the real knowledge of how to rebuild it (with or without
NO_PERL_MAKEMAKER) should be in perl/Makefile.

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> The result will be queued tentatively near the tip of 'pu', but as
> this is primarily about git-svn, I would prefer a copy that is
> vetted by Eric to be fed from him.

OK.  I've signed-off on the 7 patches you have in pu.  Will look at the
other series in a few hours.

The following changes since commit cdd159b2f56c9e69e37bbb8f5af301abd93e5407:

  Merge branch 'jc/test-lib-source-build-options-early' (2012-07-25 15:47:08 -0700)

are available in the git repository at:

  git://bogomips.org/git-svn master

for you to fetch changes up to 8d1ddbdc877cf1f430ea8e79bf800ce806875565:

  Move initialization of Git::SVN variables into Git::SVN. (2012-07-27 11:29:21 +0000)

----------------------------------------------------------------
Michael G. Schwern (7):
      Quiet warning if Makefile.PL is run with -w and no --localedir
      Don't lose Error.pm if $@ gets clobbered.
      The Makefile.PL will now find .pm files itself.
      Extract some utilities from git-svn to allow extracting Git::SVN.
      Prepare Git::SVN for extraction into its own file.
      Extract Git::SVN from git-svn into its own .pm file.
      Move initialization of Git::SVN variables into Git::SVN.

 git-svn.perl                   | 2340 +---------------------------------------
 perl/Git/SVN.pm                | 2324 +++++++++++++++++++++++++++++++++++++++
 perl/Git/SVN/Utils.pm          |   59 +
 perl/Makefile                  |    2 +
 perl/Makefile.PL               |   35 +-
 t/Git-SVN/00compile.t          |    9 +
 t/Git-SVN/Utils/can_compress.t |   11 +
 t/Git-SVN/Utils/fatal.t        |   34 +
 8 files changed, 2476 insertions(+), 2338 deletions(-)
 create mode 100644 perl/Git/SVN.pm
 create mode 100644 perl/Git/SVN/Utils.pm
 create mode 100644 t/Git-SVN/00compile.t
 create mode 100644 t/Git-SVN/Utils/can_compress.t
 create mode 100644 t/Git-SVN/Utils/fatal.t

> Thanks.
> 
> P.S.
> 
> t91XX series seem to fail in 'pu' with "Can't locate Git/SVN.pm in
> @INC" for me.  I see perl/blib/lib/Git/SVN/ directory and files
> under it, but there is no perl/blib/lib/Git/SVN.pm installed.  I see
> Git/I18N.pm and Git/SVN/Ra.pm (and friends) mentioned in
> perl/perl.mak generated by MakeMaker, but Git/SVN.pm does not appear
> anywhere.
> 
> I think it is some interaction with other topics, as the tip of
> ms/git-svn-pm topic that parks this series does not exhibit the
> symptom, but it is getting late for me already, so I won't dig into
> this further.

I think your proposed patch in the followup should work.  We should
probably squash that into this series avoid breaking bisect in the
future.

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

[Michael G Schwern]

On 2012.7.26 10:18 PM, Junio C Hamano wrote:
> 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"?

If it wasn't clear, all tests pass with every patch using SVN 1.6.

"Compile on its own" wasn't entirely clear.  I meant that Git::SVN doesn't
depend on git-svn to set its defaults.  Git::SVN still depends on it for A LOT
of other things, and will likely remain that way for a long time, so it's
kinda splitting hairs to worry about it.

4/4 was done last to ensure the phase of git-svn when the Git::SVN globals are
initialized remains basically the same.  If they were moved into Git::SVN
before it was split out they'd be getting initialized *after* the git-svn
command has been executed.  I didn't want to expend the energy or risk the
bugs to get around that.


> 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;

Originally I tried to refactor new().  It rapidly turned into a lot of work on
undocumented code with no unit tests for no use to the SVN 1.7 issue for one
variable.  This is a very cheap way to let far more important work move
forward and it has a very narrow effect.  It could be made a Git::SVN global
that git-svn grabs at, but that's not really any better.  I'd rather leave it be.


-- 
91. I am not authorized to initiate Jihad.
    -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
           http://skippyslist.com/list/