Fix git-svn tests for SVN 1.7.5.
Hi, I've fixed the git-svn tests for SVN 1.7 and tested with SVN 1.7.5. SVN 1.7 changed its expectations of path and URL formats and git-svn did not comply with them. The new code uses SVN's own canonicalization routines where available. This has been reported in several places... https://bugs.gentoo.org/show_bug.cgi?id=418431 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=678764 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661094 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=678137 https://trac.macports.org/ticket/32753 It also split the internal classes out of git-svn.perl and into their own modules in perl/Git/ to make them easier to work on. They compile alone, but remain heavily intertwined with each other and git-svn. I didn't want to go very far down that rabbit hole. This makes the tests pass, but I'm pretty sure plenty of canonicalization problems remain untested. Hopefully by attacking the problem at the root (ie. in the Git::SVN and Git::SVN::Ra accessors) it will wipe out a range of problems. t9100-git-svn-basic.sh tests 11-13 continue to fail for what look like unrelated reasons to do with SVN and symlinks. There's a lot of work in this change, so I felt it better to submit the patches as a link to a git repository rather than attach a pile of patches. Here is my repository, the work is in the fix-canonical branch. https://github.com/schwern/git Here's a summary of what was done. * Changed git-svn's main canonicalization routines to use SVN's API. * Replaced other ad-hoc canonicalization routines with git-svn's single routine. * Moved all the Git:: classes inside git-svn into their own .pm files in perl/Git. They compile, but don't do much more than that alone. They're still heavily dependent on git-svn. It's a start. * Added Git::SVN-url, Git::SVN-path and Git::SVN::Ra-url to replace code grabbing at hash keys. * Made the above automatically canonicalize their path or url. * Found some key locations which were not canonicalizing. * Made the process of adding a new Perl module easier by having the Makefile.PL scan for .pm files. -- Alligator sandwich, and make it snappy! -- 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: Fix git-svn tests for SVN 1.7.5.
On 2012.7.17 10:44 AM, Jonathan Nieder wrote: Michael G Schwern wrote: I've fixed the git-svn tests for SVN 1.7 and tested with SVN 1.7.5. Thanks. git-svn is not maintained by Junio but by Eric and others on the list. I'm cc-ing Eric and Ben Walton so they can benefit from your work. Thanks. There's a lot of work in this change, so I felt it better to submit the patches as a link to a git repository rather than attach a pile of patches. Here is my repository, the work is in the fix-canonical branch. https://github.com/schwern/git It is indeed quite the intimidating pile of patches, so I do not think we will be able to apply it all in one chunk as-is. :( My advice would be to send five or so of the patches that you would like to be reviewed first, inline, one per message, in reply to this message so we can start to work on that. Presumably the patches do not regress git-svn's behavior but only make it saner, so even if this is not a complete fix it should allow us to get started. See Documentation/SubmittingPatches for more hints. Yes, the refactorings are all as rote as I could make them and only lightly touch the code enough to make the canonicalization possible... with a bit more work than was strictly necessary around the Perl build system. Let me do a bit of rebase work to make things work better as a series of submissions and I'll get back to you. I'm new here, and I'll play nice, but let me go on record to state that Git asking for individual emails with inline patches feels like Sendmail Corp asking to be faxed an email thread. I was kinda hoping SubmittingPatches wasn't serious about that and it was some sort of policy artifact that was never updated. :-/ -- 151. The proper way to report to my Commander is Specialist Schwarz, reporting as ordered, Sir not You can't prove a thing! -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/ -- 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
Extract Git classes from git-svn (2/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 683a230e439f1d5ac2727ce4c2a74e93804fc72b Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Wed, 11 Jul 2012 22:16:01 -0700
Subject: [PATCH 03/11] Fix Git::SVN so it can at least compile alone.
It's still very intertwined with git-svn, but that's a lot of work. This
gets things working and tests passing again (as well as they were).
This required some parallel refactorings...
* fatal() moved out of git-svn into a new Git::SVN::Utils
* The $can_compress lexical moved into Git::SVN::Utils::can_compress()
* The $_prefix variable which stores the --prefix option is wrapped
in a function (rather than made global) so access to it can be
controlled. Git::SVN does not rely on this function being
available so it can work without git-svn loaded. In general,
the options should be put back together into a hash and accessed
via an options() function.
* A new tree of unit tests for the Git::SVN modules has been created.
It doesn't work with the existing Makefile, that can be worried
about later.
* Move initialization of Git::SVN globals into Git::SVN
* Have Git::SVN load the Git command* functions on its own
---
git-svn.perl | 33 ++---
perl/Git/SVN.pm| 29 -
perl/Git/SVN/Utils.pm | 19 +++
perl/Makefile | 2 ++
t/Git-SVN/00compile.t | 9 +
t/Git-SVN/Utils/can_compress.t | 11 +++
t/Git-SVN/Utils/fatal.t| 34 ++
7 files changed, 113 insertions(+), 24 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 59db0a4..8a02d1c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -10,6 +10,9 @@ use vars qw/ $AUTHOR $VERSION
$AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
+use Git::SVN;
+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)
@@ -17,10 +20,8 @@ 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};
@@ -35,8 +36,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 +65,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//;
@@ -89,7 +88,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,7 +108,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);
-$Git::SVN::_follow_parent = 1;
+
+# This is a refactoring artifact so Git::SVN can get at this variable.
+sub opt_prefix { return $_prefix || '' }
+
$Git::SVN::Fetcher::_placeholder_filename = .gitignore;
$_q ||= 0;
my %remote_opts = ( 'username=s' = \$Git::SVN::Prompt::_username,
@@ -1578,7 +1580,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;
}
@@ -2020,7 +2022,7 @@ sub md5sum {
}
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;
@@ -2042,6 +2044,7 @@ sub gc_directory {
package Git::SVN::Log;
use strict;
use warnings;
+use Git::SVN::Utils qw(fatal);
use POSIX qw/strftime/;
use constant
Extract Git classes from git-svn (3/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 5f0b609e9b0a70c86c46b48f0b180c96c3355a14 Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Tue, 17 Jul 2012 15:40:03 -0700
Subject: [PATCH 04/11] Extract Git::SVN::Log from git-svn.
This is a straight cut paste. Next commit will make it work. This will
make it easier to see the differences in Git::SVN::Log.
---
git-svn.perl| 387 ---
perl/Git/SVN/Log.pm | 388
2 files changed, 388 insertions(+), 387 deletions(-)
create mode 100644 perl/Git/SVN/Log.pm
diff --git a/git-svn.perl b/git-svn.perl
index 8a02d1c..5e6e3b5 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2041,393 +2041,6 @@ sub gc_directory {
}
-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
-%rusers $show_commit $incremental/;
-my $l_fmt;
-
-sub cmt_showable {
- my ($c) = @_;
- return 1 if defined $c-{r};
-
- # big commit message got truncated by the 16k pretty buffer in rev-list
- if ($c-{l} $c-{l}-[-1] eq ...\n
- $c-{a_raw} =~ /\@([a-f\d\-]+)$/) {
- @{$c-{l}} = ();
- my @log = command(qw/cat-file commit/, $c-{c});
-
- # shift off the headers
- shift @log while ($log[0] ne '');
- shift @log;
-
- # TODO: make $c-{l} not have a trailing newline in the future
- @{$c-{l}} = map { $_\n } grep !/^git-svn-id: /, @log;
-
- (undef, $c-{r}, undef) = ::extract_metadata(
- (grep(/^git-svn-id: /, @log))[-1]);
- }
- return defined $c-{r};
-}
-
-sub log_use_color {
- return $color || Git-repository-get_colorbool('color.diff');
-}
-
-sub git_svn_log_cmd {
- my ($r_min, $r_max, @args) = @_;
- my $head = 'HEAD';
- my (@files, @log_opts);
- foreach my $x (@args) {
- if ($x eq '--' || @files) {
- push @files, $x;
- } else {
- if (::verify_ref($x^0)) {
- $head = $x;
- } else {
- push @log_opts, $x;
- }
- }
- }
-
- my ($url, $rev, $uuid, $gs) = ::working_head_info($head);
- $gs ||= Git::SVN-_new;
- my @cmd = (qw/log --abbrev-commit --pretty=raw --default/,
- $gs-refname);
- push @cmd, '-r' unless $non_recursive;
- push @cmd, qw/--raw --name-status/ if $verbose;
- push @cmd, '--color' if log_use_color();
- push @cmd, @log_opts;
- if (defined $r_max $r_max == $r_min) {
- push @cmd, '--max-count=1';
- if (my $c = $gs-rev_map_get($r_max)) {
- push @cmd, $c;
- }
- } elsif (defined $r_max) {
- if ($r_max $r_min) {
- ($r_min, $r_max) = ($r_max, $r_min);
- }
- my (undef, $c_max) = $gs-find_rev_before($r_max, 1, $r_min);
- my (undef, $c_min) = $gs-find_rev_after($r_min, 1, $r_max);
- # If there are no commits in the range, both $c_max and $c_min
- # will be undefined. If there is at least 1 commit in the
- # range, both will be defined.
- return () if !defined $c_min || !defined $c_max;
- if ($c_min eq $c_max) {
- push @cmd, '--max-count=1', $c_min;
- } else {
- push @cmd, '--boundary', $c_min..$c_max;
- }
- }
- return (@cmd, @files);
-}
-
-# adapted from pager.c
-sub config_pager {
- if (! -t *STDOUT) {
- $ENV{GIT_PAGER_IN_USE} = 'false';
- $pager = undef;
- return;
- }
- chomp($pager = command_oneline(qw(var GIT_PAGER)));
- if ($pager eq 'cat') {
- $pager = undef;
- }
- $ENV{GIT_PAGER_IN_USE} = defined($pager);
-}
-
-sub run_pager {
- return unless defined $pager;
- pipe my ($rfd, $wfd) or return;
- defined(my $pid = fork) or fatal Can't fork: $!;
- if (!$pid) {
- open STDOUT, '', $wfd or
-fatal Can't redirect to stdout: $!;
- return;
- }
- open STDIN, '', $rfd or fatal Can't redirect stdin: $!;
- $ENV{LESS} ||= 'FRSX';
- exec $pager or fatal Can't run pager: $! ($pager);
-}
-
-sub format_svn_date {
- my $t = shift || time;
- my $gmoff = Git::SVN::get_tz($t);
- return strftime(%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y),
localtime($t));
-}
-
-sub parse_git_date {
- my ($t, $tz) = @_;
- # Date::Parse isn't
Extract Git classes from git-svn (4/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 8f70be0424a770c299b6a0c5bf99e4030e5e4d92 Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Thu, 12 Jul 2012 16:58:53 -0700
Subject: [PATCH 05/11] Make Git::SVN::Log work.
Changes to Git::SVN::Log to make it compile
* Change the $_git_format lexical only used by Git::SVN::Log into a
Git::SVN::Log global
* Have it load the Git command functions itself
---
git-svn.perl | 8 +---
perl/Git/SVN/Log.pm | 10 +-
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 4 +++-
4 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 5e6e3b5..7c8ca49 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -11,6 +11,8 @@ $AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
use Git::SVN;
+use Git::SVN::Log;
+
use Git::SVN::Utils qw(fatal can_compress);
# From which subdir have we been invoked?
@@ -88,7 +90,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),
+ for my $package ( qw(Git::SVN::Migration),
__PACKAGE__) {
*{${package}::$_} = \{Git::$_};
}
@@ -107,7 +109,7 @@ my ($_stdin, $_help, $_edit,
$_version, $_fetch_all, $_no_rebase, $_fetch_parent,
$_merge, $_strategy, $_preserve_merges, $_dry_run, $_local,
$_prefix, $_no_checkout, $_url, $_verbose,
- $_git_format, $_commit_url, $_tag, $_merge_info, $_interactive);
+ $_commit_url, $_tag, $_merge_info, $_interactive);
# This is a refactoring artifact so Git::SVN can get at this variable.
sub opt_prefix { return $_prefix || '' }
@@ -271,7 +273,7 @@ my %cmd = (
{ 'url' = \$_url, } ],
'blame' = [ \Git::SVN::Log::cmd_blame,
Show what revision and author last modified each line of a
file,
- { 'git-format' = \$_git_format } ],
+ { 'git-format' = \$Git::SVN::Log::_git_format } ],
'reset' = [ \cmd_reset,
Undo fetches back to the specified SVN revision,
{ 'revision|r=s' = \$_revision,
diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
index bbec3b0..7f3cb87 100644
--- a/perl/Git/SVN/Log.pm
+++ b/perl/Git/SVN/Log.pm
@@ -1,12 +1,17 @@
package Git::SVN::Log;
+
use strict;
use warnings;
+
+use Git qw(command command_oneline command_output_pipe command_close_pipe);
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
%rusers $show_commit $incremental/;
-my $l_fmt;
+
+# Options set in git-svn
+our $_git_format;
sub cmt_showable {
my ($c) = @_;
@@ -52,6 +57,7 @@ sub git_svn_log_cmd {
}
my ($url, $rev, $uuid, $gs) = ::working_head_info($head);
+ require Git::SVN;
$gs ||= Git::SVN-_new;
my @cmd = (qw/log --abbrev-commit --pretty=raw --default/,
$gs-refname);
@@ -113,6 +119,7 @@ sub run_pager {
sub format_svn_date {
my $t = shift || time;
+ require Git::SVN;
my $gmoff = Git::SVN::get_tz($t);
return strftime(%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y),
localtime($t));
}
@@ -183,6 +190,7 @@ sub process_commit {
return 1;
}
+my $l_fmt;
sub show_commit {
my $c = shift;
if ($oneline) {
diff --git a/perl/Makefile b/perl/Makefile
index d0a0c5c..2a4ca57 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -30,6 +30,7 @@ modules += Git/SVN
modules += Git/SVN/Memoize/YAML
modules += Git/SVN/Fetcher
modules += Git/SVN/Editor
+modules += Git/SVN/Log
modules += Git/SVN/Prompt
modules += Git/SVN/Ra
modules += Git/SVN/Utils
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
index c32ee4b..37626f4 100644
--- a/t/Git-SVN/00compile.t
+++ b/t/Git-SVN/00compile.t
@@ -3,7 +3,9 @@
use strict;
use warnings;
-use Test::More tests = 2;
+use Test::More tests = 4;
require_ok 'Git::SVN';
require_ok 'Git::SVN::Utils';
+require_ok 'Git::SVN::Ra';
+require_ok 'Git::SVN::Log';
--
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
Extract Git classes from git-svn (5/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From ab67ab421dbfd248b9a198b8cc1cd9944ba6178d Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Tue, 17 Jul 2012 15:46:44 -0700
Subject: [PATCH 06/11] Move Git::SVN::Migration into its own file.
Just a straight cut paste, the fixes come next commit.
---
git-svn.perl | 246 -
perl/Git/SVN/Migration.pm | 247 ++
2 files changed, 247 insertions(+), 246 deletions(-)
create mode 100644 perl/Git/SVN/Migration.pm
diff --git a/git-svn.perl b/git-svn.perl
index 7c8ca49..f2bf759 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2043,252 +2043,6 @@ sub gc_directory {
}
-package Git::SVN::Migration;
-# these version numbers do NOT correspond to actual version numbers
-# of git nor git-svn. They are just relative.
-#
-# v0 layout: .git/$id/info/url, refs/heads/$id-HEAD
-#
-# v1 layout: .git/$id/info/url, refs/remotes/$id
-#
-# v2 layout: .git/svn/$id/info/url, refs/remotes/$id
-#
-# v3 layout: .git/svn/$id, refs/remotes/$id
-#- info/url may remain for backwards compatibility
-#- this is what we migrate up to this layout automatically,
-#- this will be used by git svn init on single branches
-# v3.1 layout (auto migrated):
-#- .rev_db = .rev_db.$UUID, .rev_db will remain as a symlink
-# for backwards compatibility
-#
-# v4 layout: .git/svn/$repo_id/$id, refs/remotes/$repo_id/$id
-#- this is only created for newly multi-init-ed
-# repositories. Similar in spirit to the
-# --use-separate-remotes option in git-clone (now default)
-#- we do not automatically migrate to this (following
-# the example set by core git)
-#
-# v5 layout: .rev_db.$UUID = .rev_map.$UUID
-#- newer, more-efficient format that uses 24-bytes per record
-# with no filler space.
-#- use xxd -c24 .rev_map.$UUID to view and debug
-#- This is a one-way migration, repositories updated to the
-# new format will not be able to use old git-svn without
-# rebuilding the .rev_db. Rebuilding the rev_db is not
-# possible if noMetadata or useSvmProps are set; but should
-# be no problem for users that use the (sensible) defaults.
-use strict;
-use warnings;
-use Carp qw/croak/;
-use File::Path qw/mkpath/;
-use File::Basename qw/dirname basename/;
-use vars qw/$_minimize/;
-
-sub migrate_from_v0 {
- my $git_dir = $ENV{GIT_DIR};
- return undef unless -d $git_dir;
- my ($fh, $ctx) = command_output_pipe(qw/rev-parse --symbolic --all/);
- my $migrated = 0;
- while ($fh) {
- chomp;
- my ($id, $orig_ref) = ($_, $_);
- next unless $id =~ s#^refs/heads/(.+)-HEAD$#$1#;
- next unless -f $git_dir/$id/info/url;
- my $new_ref = refs/remotes/$id;
- if (::verify_ref($new_ref^0)) {
- print STDERR W: $orig_ref is probably an old ,
-branch used by an ancient version of ,
-git-svn.\n,
-However, $new_ref also exists.\n,
-We will not be able ,
-to use this branch until this ,
-ambiguity is resolved.\n;
- next;
- }
- print STDERR Migrating from v0 layout...\n if !$migrated;
- print STDERR Renaming ref: $orig_ref = $new_ref\n;
- command_noisy('update-ref', $new_ref, $orig_ref);
- command_noisy('update-ref', '-d', $orig_ref, $orig_ref);
- $migrated++;
- }
- command_close_pipe($fh, $ctx);
- print STDERR Done migrating from v0 layout...\n if $migrated;
- $migrated;
-}
-
-sub migrate_from_v1 {
- my $git_dir = $ENV{GIT_DIR};
- my $migrated = 0;
- return $migrated unless -d $git_dir;
- my $svn_dir = $git_dir/svn;
-
- # just in case somebody used 'svn' as their $id at some point...
- return $migrated if -d $svn_dir ! -f $svn_dir/info/url;
-
- print STDERR Migrating from a git-svn v1 layout...\n;
- mkpath([$svn_dir]);
- print STDERR Data from a previous version of git-svn exists, but\n\t,
-$svn_dir\n\t(required for this version ,
-($::VERSION) of git-svn) does not exist.\n;
- my ($fh, $ctx) = command_output_pipe(qw/rev-parse --symbolic --all/);
- while ($fh) {
- my $x = $_;
- next unless $x =~ s#^refs/remotes/##;
- chomp $x;
- next unless -f $git_dir/$x/info/url;
- my $u = eval { ::file_to_s($git_dir/$x/info/url) };
- next unless $u;
- my $dn = dirname($git_dir/svn
Extract Git classes from git-svn (6/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From cb1a73929da15e87fa3dcc41c4cfa9ca592081fa Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Thu, 12 Jul 2012 17:14:24 -0700
Subject: [PATCH 07/11] Fix Git::SVN::Migration after its move.
Also...
* eliminate the big import all the Git command functions loop, nothing needs
it
any more
* only load Git::SVN::Migration if we need it
---
git-svn.perl | 28 +++-
perl/Git/SVN/Migration.pm | 16 +++-
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 3 ++-
4 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index f2bf759..8b8607d 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -11,7 +11,6 @@ $AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
use Git::SVN;
-use Git::SVN::Log;
use Git::SVN::Utils qw(fatal can_compress);
@@ -77,24 +76,26 @@ use File::Spec;
use File::Find;
use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
use IPC::Open3;
-use Git;
+
+use Git qw(
+git_cmd_try
+command
+command_oneline
+command_noisy
+command_output_pipe
+command_close_pipe
+command_bidi_pipe
+command_close_bidi_pipe
+);
+
use Git::SVN::Editor qw//;
use Git::SVN::Fetcher qw//;
-use Git::SVN::Ra qw//;
+use Git::SVN::Log;
use Git::SVN::Prompt qw//;
+use Git::SVN::Ra qw//;
use Memoize; # core since 5.8.0, Jul 2002
BEGIN {
- # import functions from Git into our packages, en masse
- no strict 'refs';
- 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),
- __PACKAGE__) {
- *{${package}::$_} = \{Git::$_};
- }
- }
Memoize::memoize 'Git::config';
Memoize::memoize 'Git::config_bool';
}
@@ -365,6 +366,7 @@ if (defined $_authors_prog) {
}
unless ($cmd =~ /^(?:clone|init|multi-init|commit-diff)$/) {
+ require Git::SVN::Migration;
Git::SVN::Migration::migration_check();
}
Git::SVN::init_vars();
diff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm
index 082a788..b17fe00 100644
--- a/perl/Git/SVN/Migration.pm
+++ b/perl/Git/SVN/Migration.pm
@@ -32,12 +32,22 @@ package Git::SVN::Migration;
# rebuilding the .rev_db. Rebuilding the rev_db is not
# possible if noMetadata or useSvmProps are set; but should
# be no problem for users that use the (sensible) defaults.
+
use strict;
use warnings;
+
use Carp qw/croak/;
use File::Path qw/mkpath/;
use File::Basename qw/dirname basename/;
-use vars qw/$_minimize/;
+
+our $_minimize;
+
+use Git qw(
+ command
+ command_noisy
+ command_output_pipe
+ command_close_pipe
+);
sub migrate_from_v0 {
my $git_dir = $ENV{GIT_DIR};
@@ -146,6 +156,7 @@ sub migrate_from_v2 {
read_old_urls(\%l_map, '', $ENV{GIT_DIR}/svn);
my $migrated = 0;
+ require Git::SVN;
foreach my $ref_id (sort keys %l_map) {
eval { Git::SVN-init($l_map{$ref_id}, '', undef, $ref_id) };
if ($@) {
@@ -157,6 +168,9 @@ sub migrate_from_v2 {
}
sub minimize_connections {
+ require Git::SVN;
+ require Git::SVN::Ra;
+
my $r = Git::SVN::read_all_remotes();
my $new_urls = {};
my $root_repos = {};
diff --git a/perl/Makefile b/perl/Makefile
index 2a4ca57..d6a0e84 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -31,6 +31,7 @@ modules += Git/SVN/Memoize/YAML
modules += Git/SVN/Fetcher
modules += Git/SVN/Editor
modules += Git/SVN/Log
+modules += Git/SVN/Migration
modules += Git/SVN/Prompt
modules += Git/SVN/Ra
modules += Git/SVN/Utils
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
index 37626f4..1307b65 100644
--- a/t/Git-SVN/00compile.t
+++ b/t/Git-SVN/00compile.t
@@ -3,9 +3,10 @@
use strict;
use warnings;
-use Test::More tests = 4;
+use Test::More tests = 5;
require_ok 'Git::SVN';
require_ok 'Git::SVN::Utils';
require_ok 'Git::SVN::Ra';
require_ok 'Git::SVN::Log';
+require_ok 'Git::SVN::Migration';
--
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
Extract Git classes from git-svn (7/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 9ff49d9e91c9741d501620ac47f78d8ff8ef9983 Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Tue, 17 Jul 2012 15:51:53 -0700
Subject: [PATCH 08/11] Cut paste Git::IndexInfo into its own file.
No other changes, those are next commit so they can be seen in the diff.
---
git-svn.perl | 32
perl/Git/IndexInfo.pm | 33 +
2 files changed, 33 insertions(+), 32 deletions(-)
create mode 100644 perl/Git/IndexInfo.pm
diff --git a/git-svn.perl b/git-svn.perl
index 8b8607d..6632cfb 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2045,38 +2045,6 @@ sub gc_directory {
}
-package Git::IndexInfo;
-use strict;
-use warnings;
-use Git qw/command_input_pipe command_close_pipe/;
-
-sub new {
- my ($class) = @_;
- my ($gui, $ctx) = command_input_pipe(qw/update-index -z --index-info/);
- bless { gui = $gui, ctx = $ctx, nr = 0}, $class;
-}
-
-sub remove {
- my ($self, $path) = @_;
- if (print { $self-{gui} } '0 ', 0 x 40, \t, $path, \0) {
- return ++$self-{nr};
- }
- undef;
-}
-
-sub update {
- my ($self, $mode, $hash, $path) = @_;
- if (print { $self-{gui} } $mode, ' ', $hash, \t, $path, \0) {
- return ++$self-{nr};
- }
- undef;
-}
-
-sub DESTROY {
- my ($self) = @_;
- command_close_pipe($self-{gui}, $self-{ctx});
-}
-
package Git::SVN::GlobSpec;
use strict;
use warnings;
diff --git a/perl/Git/IndexInfo.pm b/perl/Git/IndexInfo.pm
new file mode 100644
index 000..a43108c
--- /dev/null
+++ b/perl/Git/IndexInfo.pm
@@ -0,0 +1,33 @@
+package Git::IndexInfo;
+use strict;
+use warnings;
+use Git qw/command_input_pipe command_close_pipe/;
+
+sub new {
+ my ($class) = @_;
+ my ($gui, $ctx) = command_input_pipe(qw/update-index -z --index-info/);
+ bless { gui = $gui, ctx = $ctx, nr = 0}, $class;
+}
+
+sub remove {
+ my ($self, $path) = @_;
+ if (print { $self-{gui} } '0 ', 0 x 40, \t, $path, \0) {
+ return ++$self-{nr};
+ }
+ undef;
+}
+
+sub update {
+ my ($self, $mode, $hash, $path) = @_;
+ if (print { $self-{gui} } $mode, ' ', $hash, \t, $path, \0) {
+ return ++$self-{nr};
+ }
+ undef;
+}
+
+sub DESTROY {
+ my ($self) = @_;
+ command_close_pipe($self-{gui}, $self-{ctx});
+}
+
+1;
--
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
Extract Git classes from git-svn (8/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 4fd7b8574b32753dcf22ec0a592f13586b938689 Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Thu, 12 Jul 2012 17:20:02 -0700
Subject: [PATCH 09/11] Fix Git::IndexInfo so it compiles.
Only used by Git::SVN::Fetcher.
---
perl/Git/IndexInfo.pm | 2 ++
perl/Git/SVN/Fetcher.pm | 2 ++
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 3 ++-
4 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/perl/Git/IndexInfo.pm b/perl/Git/IndexInfo.pm
index a43108c..9e454be 100644
--- a/perl/Git/IndexInfo.pm
+++ b/perl/Git/IndexInfo.pm
@@ -1,6 +1,8 @@
package Git::IndexInfo;
+
use strict;
use warnings;
+
use Git qw/command_input_pipe command_close_pipe/;
sub new {
diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index ef8e9ed..69486ef 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -10,6 +10,8 @@ use IO::File qw//;
use Git qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe
command_bidi_pipe command_close_bidi_pipe/;
+use Git::IndexInfo;
+
BEGIN {
@ISA = qw(SVN::Delta::Editor);
}
diff --git a/perl/Makefile b/perl/Makefile
index d6a0e84..6c32918 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -26,6 +26,7 @@ instdir_SQ = $(subst ','\'',$(prefix)/lib)
modules += Git
modules += Git/I18N
+modules += Git/IndexInfo
modules += Git/SVN
modules += Git/SVN/Memoize/YAML
modules += Git/SVN/Fetcher
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
index 1307b65..5419438 100644
--- a/t/Git-SVN/00compile.t
+++ b/t/Git-SVN/00compile.t
@@ -3,10 +3,11 @@
use strict;
use warnings;
-use Test::More tests = 5;
+use Test::More tests = 6;
require_ok 'Git::SVN';
require_ok 'Git::SVN::Utils';
require_ok 'Git::SVN::Ra';
require_ok 'Git::SVN::Log';
require_ok 'Git::SVN::Migration';
+require_ok 'Git::IndexInfo';
--
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
Extract Git classes from git-svn (9/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 368d6c7883080d85f82b1eae1815834e3d59ef5e Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Tue, 17 Jul 2012 15:54:33 -0700
Subject: [PATCH 10/11] Cut paste Git::SVN::GlobSpec into its own file.
Fixes to make it work come next commit.
---
git-svn.perl | 58 ---
perl/Git/SVN/GlobSpec.pm | 59
2 files changed, 59 insertions(+), 58 deletions(-)
create mode 100644 perl/Git/SVN/GlobSpec.pm
diff --git a/git-svn.perl b/git-svn.perl
index 6632cfb..7b54f43 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2045,64 +2045,6 @@ sub gc_directory {
}
-package Git::SVN::GlobSpec;
-use strict;
-use warnings;
-
-sub new {
- my ($class, $glob, $pattern_ok) = @_;
- my $re = $glob;
- $re =~ s!/+$!!g; # no need for trailing slashes
- my (@left, @right, @patterns);
- my $state = left;
- my $die_msg = Only one set of wildcard directories .
- (e.g. '*' or '*/*/*') is supported: '$glob'\n;
- for my $part (split(m|/|, $glob)) {
- if ($part =~ /\*/ $part ne *) {
- die Invalid pattern in '$glob': $part\n;
- } elsif ($pattern_ok $part =~ /[{}]/
-$part !~ /^\{[^{}]+\}/) {
- die Invalid pattern in '$glob': $part\n;
- }
- if ($part eq *) {
- die $die_msg if $state eq right;
- $state = pattern;
- push(@patterns, [^/]*);
- } elsif ($pattern_ok $part =~ /^\{(.*)\}$/) {
- die $die_msg if $state eq right;
- $state = pattern;
- my $p = quotemeta($1);
- $p =~ s/\\,/|/g;
- push(@patterns, (?:$p));
- } else {
- if ($state eq left) {
- push(@left, $part);
- } else {
- push(@right, $part);
- $state = right;
- }
- }
- }
- my $depth = @patterns;
- if ($depth == 0) {
- die One '*' is needed in glob: '$glob'\n;
- }
- my $left = join('/', @left);
- my $right = join('/', @right);
- $re = join('/', @patterns);
- $re = join('\/',
- grep(length, quotemeta($left), ($re), quotemeta($right)));
- my $left_re = qr/^\/\Q$left\E(\/|$)/;
- bless { left = $left, right = $right, left_regex = $left_re,
- regex = qr/$re/, glob = $glob, depth = $depth }, $class;
-}
-
-sub full_path {
- my ($self, $path) = @_;
- return (length $self-{left} ? $self-{left}/ : '') .
- $path . (length $self-{right} ? /$self-{right} : '');
-}
-
__END__
Data structures:
diff --git a/perl/Git/SVN/GlobSpec.pm b/perl/Git/SVN/GlobSpec.pm
new file mode 100644
index 000..96cfd98
--- /dev/null
+++ b/perl/Git/SVN/GlobSpec.pm
@@ -0,0 +1,59 @@
+package Git::SVN::GlobSpec;
+use strict;
+use warnings;
+
+sub new {
+ my ($class, $glob, $pattern_ok) = @_;
+ my $re = $glob;
+ $re =~ s!/+$!!g; # no need for trailing slashes
+ my (@left, @right, @patterns);
+ my $state = left;
+ my $die_msg = Only one set of wildcard directories .
+ (e.g. '*' or '*/*/*') is supported: '$glob'\n;
+ for my $part (split(m|/|, $glob)) {
+ if ($part =~ /\*/ $part ne *) {
+ die Invalid pattern in '$glob': $part\n;
+ } elsif ($pattern_ok $part =~ /[{}]/
+$part !~ /^\{[^{}]+\}/) {
+ die Invalid pattern in '$glob': $part\n;
+ }
+ if ($part eq *) {
+ die $die_msg if $state eq right;
+ $state = pattern;
+ push(@patterns, [^/]*);
+ } elsif ($pattern_ok $part =~ /^\{(.*)\}$/) {
+ die $die_msg if $state eq right;
+ $state = pattern;
+ my $p = quotemeta($1);
+ $p =~ s/\\,/|/g;
+ push(@patterns, (?:$p));
+ } else {
+ if ($state eq left) {
+ push(@left, $part);
+ } else {
+ push(@right, $part);
+ $state = right;
+ }
+ }
+ }
+ my $depth = @patterns;
+ if ($depth == 0) {
+ die One '*' is needed in glob: '$glob'\n;
+ }
+ my $left = join('/', @left);
+ my $right = join('/', @right);
+ $re = join('/', @patterns);
+ $re = join('\/',
+ grep(length, quotemeta($left), ($re
Extract Git classes from git-svn (10/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
From 5152b76800f076ba0bd528664f62d3c67966fa4e Mon Sep 17 00:00:00 2001
From: Michael G. Schwern schw...@pobox.com
Date: Thu, 12 Jul 2012 17:25:25 -0700
Subject: [PATCH 11/11] Fix Git::SVN::GlobSpec so it works.
Only used in one place in Git::SVN, load it on demand.
That should be all the Git classes out of git-svn.
---
perl/Git/SVN.pm | 5 -
perl/Git/SVN/GlobSpec.pm | 1 +
perl/Makefile| 1 +
t/Git-SVN/00compile.t| 3 ++-
4 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 02983d6..247ee1d 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -202,11 +202,14 @@ sub read_all_remotes {
. must start with 'refs/'\n)
unless $remote_ref =~ m{^refs/};
$local_ref = uri_decode($local_ref);
+
+ require Git::SVN::GlobSpec;
my $rs = {
t = $t,
remote = $remote,
path = Git::SVN::GlobSpec-new($local_ref, 1),
- ref = Git::SVN::GlobSpec-new($remote_ref, 0) };
+ ref = Git::SVN::GlobSpec-new($remote_ref, 0)
+ };
if (length($rs-{ref}-{right}) != 0) {
die The '*' glob character must be the last ,
character of '$remote_ref'\n;
diff --git a/perl/Git/SVN/GlobSpec.pm b/perl/Git/SVN/GlobSpec.pm
index 96cfd98..fede3af 100644
--- a/perl/Git/SVN/GlobSpec.pm
+++ b/perl/Git/SVN/GlobSpec.pm
@@ -1,4 +1,5 @@
package Git::SVN::GlobSpec;
+
use strict;
use warnings;
diff --git a/perl/Makefile b/perl/Makefile
index 6c32918..aa4a28b 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -31,6 +31,7 @@ modules += Git/SVN
modules += Git/SVN/Memoize/YAML
modules += Git/SVN/Fetcher
modules += Git/SVN/Editor
+modules += Git/SVN/GlobSpec
modules += Git/SVN/Log
modules += Git/SVN/Migration
modules += Git/SVN/Prompt
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
index 5419438..c92fee4 100644
--- a/t/Git-SVN/00compile.t
+++ b/t/Git-SVN/00compile.t
@@ -3,7 +3,7 @@
use strict;
use warnings;
-use Test::More tests = 6;
+use Test::More tests = 7;
require_ok 'Git::SVN';
require_ok 'Git::SVN::Utils';
@@ -11,3 +11,4 @@ require_ok 'Git::SVN::Ra';
require_ok 'Git::SVN::Log';
require_ok 'Git::SVN::Migration';
require_ok 'Git::IndexInfo';
+require_ok 'Git::SVN::GlobSpec';
--
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
Re: Find .pm files automatically (was Re: Fix git-svn tests for SVN 1.7.5.)
On 2012.7.17 5:01 PM, Jonathan Nieder wrote: It also moves Error.pm into a bundle directory. This both makes it just another directory to scan (or not scan), but it also makes it possible to bundle additional modules in the future. ExtUtils::MakeMaker uses this technique itself. This is not so much also as as an example to demonstrate the technique, no? I guess I'd prefer it to be in a separate patch, but this way's fine, too. I wrote the MakeMaker system so I was just cribbing off that. It made more sense to build a list of directories to scan and then scan them than to add individual file exceptions later. I could put it in a separate patch, but it would require some bending. You'll probably hate this. Because we have a bunch of patches to incorporate, I think it's worth spending the time to make that go as smoothly as possible for later patches. Sorry. I have lots of experience with git but very little with the email submission tools. I've always either just done everything via repositories or used Github. It sounds like I should figure out the git-send-email tool and do this very slowly. The word bundles/ left me a little nervous, because I (ignorantly) imagined that this might be some specialized facility like Python eggs or Ruby gems. Nope, just copy .pm files in. Is the intent that this directory contains CPAN modules we want to be able to depend on? Yes. Is there really any intention of having more of them than Error.pm? No idea, this is my first look at the code, but now it's possible. In my experience, if there's a barrier to using CPAN modules then people won't use them. They'll rewrite the functionality poorly. Before this patch, in the default case (with MakeMaker), make install wrote a manpage in mandir/man3/private-Error.3pm. Does it still do so after the patch? Will people who have installation scripts that expected that manpage have to change them, and if so, is the new behavior better to make up for that effort? The man page is now man3/bundles::Error::Error.3 which is equally as incorrect as man3/private-Error.3. It is possible to correct that so it's man3/Error.3, but that's going to require some effort. Basically its in the same boat as PM. Once you have to change one you have to change them all. Why do install scripts have specific code to look for that man page? If it's going to be trouble I can put Error.pm back. It's just something I did in passing. +# Don't forget to update the perl/Makefile, too. +# Don't forget to test with NO_PERL_MAKEMAKER=YesPlease Now the reader will have no reason to be looking at this file, so these comments are pretty much useless. In an ideal world, make test in the MakeMaker build would automatically grep perl/Makefile to catch modules that are not listed there, but that can wait, I imagine. Alternatively, maybe there could be a perl/modules.list that both makefiles read? That way, if I drop in an unrelated .pm file for reference while coding the build system would not be confused by it, and since both build systems would use the same module list there would be no risk of it falling out of date. Ideally, that second Makefile would go away. Parallel build systems are extra work and generate bugs. The log suggests it might have something to do with people wanting to build with an ActiveState Perl on Cygwin or something? MakeMaker builds different Makefiles depending on the OS, so it may be as simple as telling Makefile.PL what flavor of make you're using. -- 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: Extract Git classes from git-svn (2/10) (was Re: Fix git-svn tests for SVN 1.7.5.)
On 2012.7.18 3:58 AM, Eric Wong wrote: I agree with everything Jonathan said (and thank him for taking the time to point you in the right direction). Thanks, you guys have been very nice to my flailing and failing. I'm going to back off and send out a sort of overview email so we can figure out how best to chunk this up. +++ b/t/Git-SVN/00compile.t +use Test::More tests = 2; I prefer not declaring test counts and using done_testing() instead. done_testing() is favorable to me in at least 2 ways: * done_testing() closely matches the behavior of the existing sh-based test suite in git (which calls test_done) * maintaining test counts leads to unnecessary merge conflicts Yes, I concur 100%. So much that I went back in my time machine and added done_testing() to Test::More! Also I killed Hitler, so now WWII ends in 1945. Things seem to have turned out for the better. I love it when people advocate my features back to me. :) I didn't use done_testing because I didn't know your stance on using non-5.8 core versions of modules. Skipping the tests on old versions of Test::More ( 0.88) is acceptable to me (especially since integration tests provide the real coverage already). It is very easy to bundle an uninstalled copy of Test::More, probably easier than putting in the code necessary to check for it and skip it. A lot of Perl modules do it. The usual thing is to put it into t/lib/ and add use lib 't/lib' to the tests. I don't see any reason why that basic technique wouldn't work here, with some minor changes to match the Git test suite. I can help you with that, but I'd like to get through this SVN 1.7 fix first. -- Being faith-based doesn't trump reality. -- Bruce Sterling -- 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
git-svn SVN 1.7 fix, take 2
It's post OSCON so I can take another crack at this again. I'm struggling with how best to present all this to you folks. There's etiquette for how one presents a git pull request... but there's conflicting etiquette about how one presents patches to a mailing list. I'm not sure which bit of which applies when here. Documentation/SubmittingPatches focuses on single patches and basic commit etiquette. A big one is do not blast 10 emails to a mailing list but I gather that's ok here if a submission needs 10 commits to be well expressed and its done via git-send-email? And then if patch #3 needs revision I'm to do it in a rebase and resend the whole 10 commits? Am I to think of git-send-email less as a means of sending patches to a mailing list and more as a git transport mechanism? I'm trying to bust it up into easier to digest pieces. I came into this cold without much knowledge of the problem (something to do with canonicalization) and no knowledge of the code. While each commit is sharp, the work as a whole is mixed up. Here's the first pieces, as I see them, along with their branches. The whole work is in https://github.com/schwern/git/tree/git-svn/fix-canonical * Change the Makefile.PL so it automatically finds the .pm files. https://github.com/schwern/git/tree/git-svn/easier_modules (Going to remove the Error.pm movement as off-topic) * Extract each of the internal Git::* packages from inside git-svn. https://github.com/schwern/git/tree/git-svn/extract_classes There's five classes, and I did each in at least two commits. First is a straight cut paste with no further changes. Second (or more) fixes it so things work again. This is better for review (if it were done in a single commit the real change would be lost in the cut paste), but it means you have a commit that breaks thing which will be a problem for bisecting. I'm inclined to stick with two commits and you folks can squash them if you decide bisecting is more important. The Git::SVN extraction is more complicated than the rest, so I'll probably do that separately and bust it up into a few commits. Next I'm going to... 1) Submit easier_modules. 2) Break up the Git::SVN fix into more commits. 3) Submit the Git::SVN extraction. -- Reality is that which, when you stop believing in it, doesn't go away. -- Phillip K. Dick -- 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: Extract Git classes from git-svn (1/10)
On 2012.7.17 10:49 PM, Junio C Hamano wrote: By allowing people to easily publish a completed work, and making it easier for them to let others peek at their work, Git hosting services like GitHub are wonderful. But I am not conviced that quality code reviews like we do on the mailing list can be done with existing Web based interface to a satisfactory degree. In this instance, I was just using Github for repository storage. I was hoping I could just submit a remote git repository and people would look at it from there. No Github required. I understand this makes things very convenient for you to review patches, let me convey my POV... After I'm exhausted from volunteering all the coding work, rather than submitting a URL to a remote repository I find I have to learn new specialized tools. It's extra learning and work, an extra step to screw up, and foreign to me (even as a experienced git user). It is of little benefit to me as a casual volunteer submitter. I can see if you've been on the git mailing list for a while and have git-am and all that set up, this system is great. But it comes at a cost which is offloaded onto new and casual contributors. Patches with proposed commit log messages are sent via e-mail, people can review them and comment on them with quotes from the relevant part of the patch. The review can even be made offline, yet at the end, the list archive is an easy one-stop location you need to go to see how the changes progressed, what the background thinking was, etc. for all the changes that matter. Look at recent ones (randomly, $gmane/199492, $gmane/199497, $gmane/200750, $gmane/201477, $gmane/201434), and their re-rolls, and admire how well the process works. I've played with GitHub's in-line code comment interface, but honestly, it is cumbersome to use, for one thing, but more importantly, you have to click around various repositories of pull requestors, dig around to see in-line comments, and I do not see how we can keep a coherent discussion like we do on the mailing list. There may be a hosting site with better code review features, but all the code review of Git happens on this mailing list, and that is not likely to change in the near future. Everything you've said above is correct... but it creates a procedure optimized for the convenience of the long time reviewers at the expense of new and casual submissions. I'm going to guess you live in email and have your client setup to do fancy things to extract patches from your mailbox and the like. This sort of specialized setup makes people bounce right off the submission process. At OSCON I was asking around for help getting things setup so I could submit patches here properly. As soon as they said which mail daemon are you running?, I said stop! I don't want to know any more. I have too many things to do to be fiddling with my mailer configuration just to submit volunteer work in the right form (that said, I'm pleased as punch that git-send-email now has instructions for sending via GMail). You're volunteers, too. We're all volunteers, so a more balanced submission process would be nice. But since you brought Github up... (I get the impression its kind of a dirty word around here) As somebody who doesn't live in email anymore (once upon a time I did), I find the Github Pull Request model to be an excellent... I'm not even going to use the word compromise because I don't feel like either side has been compromised... it's an excellent enhancement. The commits and conversations about the commits are all there on one page. Looking at a commit is a click away (I usually open them all in tabs at once, much faster). You can comment on them as a whole or inline. Those comments appear BOTH in the commit AND in the larger conversation on the pull request keeping it coherent, no clicking around. And it has email mirroring. All that and its tracked and organized in an issue tracker. Here's an example that includes commits, discussion about the larger issue, comments on commits, more commits based on those comments, and so on. As you can see, the conversation is complete and coherent. It wasn't always this way, but they're constantly improving. https://github.com/schwern/test-more/pull/319 A concrete downside it is that it does not work offline. I agree that's a problem. I don't think it's a veto. There are various work arounds which are less complicated than your typical git-to-email-to-git setup. We can talk about that you're interested. I've gone all in on Github Pull Requests such that most of my projects don't even have mailing lists (issues are used for discussion). This avoids a split community between Github and the mailing list. And they have email mirroring, so issue discussion can be done all in email (I prefer email for things that involve push notification and replies). Github has a nice API and it would be possible to create a Github Pull Request - Mailing
Re: git-svn SVN 1.7 fix, take 2
On 2012.7.24 2:51 PM, Junio C Hamano wrote: Michael G Schwern schw...@pobox.com writes: A big one is do not blast 10 emails to a mailing list but I gather that's ok here if a submission needs 10 commits to be well expressed and its done via git-send-email? And then if patch #3 needs revision I'm to do it in a rebase and resend the whole 10 commits? Am I to think of git-send-email less as a means of sending patches to a mailing list and more as a git transport mechanism? Yes, yes and whatever (even though I think send-email is just a better MUA/MSA when you want to send patches and isn't restricted for a _git_ transport, I do not think it matters how you look at it). #3 was not intended as a dig. If I can think about git-send-email like a funny way to do a git-push then that fits better in my head. I worry about sending too many emails to a list at once. I don't worry about sending too many commits in one push. I'm trying to bust it up into easier to digest pieces. I came into this cold without much knowledge of the problem (something to do with canonicalization) and no knowledge of the code. Perhaps it is a good idea to lurk and see how others submit their topics first? While I use git heavily I'm not invested in working on it. I work on a lot of projects. I'd like to be able to do the work, submit it, work through review, and get out without joining another mailing list and studying their culture. Is there a document I could look at for submitting a large body of work, or could I help improve SubmittingPatches to document the process better? -- I do have a cause though. It's obscenity. I'm for it. - Tom Lehrer -- 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: git-svn SVN 1.7 fix, take 2
On 2012.7.24 4:45 PM, Junio C Hamano wrote:
git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical
That should give you the information you need...
I guess so. May we have your sign-off on these changes? (A simple
reply of yes is enough, no need to resend patches to do this.)
Here it is in patch form for reviewers. If I understand correctly,
the idea is to replace accesses to $gs-{path} with calls to a
$gs-path function that canonicalizes (and likewise for s/path/url/).
There are probably other subtleties, but that seems to be the gist.
The impression I am getting is that the updated code wants to handle
URL and paths without any funny encoding, but it is unclear from my
cursory read (e.g. what goes on with escape_url?).
No, now it's just canonicalizing as early as possible. Preferably within the
object accessor rather than at the point of use. So in the code below,
$full_url is already escaped/canonicalized.
In general this blob patch isn't going to make a lot of overall sense. I'm
working with Jonathan to get it submitted in manageable pieces.
if ($old_url =~ m#^svn(\+ssh)?://# ||
($full_url =~ m#^https?://#
- escape_url($full_url) ne $full_url)) {
+ $full_url ne $full_url)) {
How can the latter part of this conditional be true?
Good point. More importantly, what was it trying to accomplish before and
does it need to be preserved?
If the URL is svn OR its http and needs to be escaped... do something
special. I don't really understand what the special stuff in the following
block is. Anything that undef's the invocant (ie. $self) is probably broken.
a51cdb0c0420ee3bef26bbd1a9aa75e1d464e5b7 and
2a679c7a3148978a3f58f1c12100383638e744c5 shed some light. 2a679 looks like it
specifically holds off on escaping $full_url. It would be very nice if that
was not necessary. It would be helpful if the bug mentioned in 2a679 could be
reproduced to see if it still applies or can be dealt with in another way.
--
185. My name is not a killing word.
-- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
http://skippyslist.com/list/
--
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
Teach Makefile.PL to find .pm files on its own
This makes it so you no longer must edit the Makefile.PL every time you add, rename or delete a Perl module. This is convenient, and I'm about to extract a bunch of .pm files out of git-svn. You still have to edit the Makefile. That parallel build system should be able to be removed at a later date and replaced with the right Makefile.PL flags. Patch 1 and 2 are just things I noticed in the Makefile.PL along the way. Patch 3 is the meat. It doesn't depend on 1 2 but I figured it would be silly to send them separately. -- 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 1/3] Quiet warning if Makefile.PL is run with -w and no --localedir
From: Michael G. Schwern schw...@pobox.com
Usually it isn't, but its nice if it can be run with warnings on.
Signed-off-by: Michael G Schwern schw...@pobox.com
---
perl/Makefile.PL | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/perl/Makefile.PL b/perl/Makefile.PL
index b54b04a..87e1f62 100644
--- a/perl/Makefile.PL
+++ b/perl/Makefile.PL
@@ -6,7 +6,8 @@ use Getopt::Long;
# Sanity: die at first unknown option
Getopt::Long::Configure qw/ pass_through /;
-GetOptions(localedir=s = \my $localedir);
+my $localedir = '';
+GetOptions(localedir=s = \$localedir);
sub MY::postamble {
return 'MAKE_FRAG';
--
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/3] Don't lose Error.pm if $@ gets clobbered.
From: Michael G. Schwern schw...@pobox.com
In older Perls, sometimes $@ can become unset between the eval and
checking $@. Its safer to check the eval directly.
Signed-off-by: Michael G Schwern schw...@pobox.com
---
perl/Makefile.PL | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/perl/Makefile.PL b/perl/Makefile.PL
index 87e1f62..887fa1b 100644
--- a/perl/Makefile.PL
+++ b/perl/Makefile.PL
@@ -41,8 +41,7 @@ my %pm = (
# We come with our own bundled Error.pm. It's not in the set of default
# Perl modules so install it if it's not available on the system yet.
-eval { require Error };
-if ($@ || $Error::VERSION 0.15009) {
+if ( !eval { require Error } || $Error::VERSION 0.15009) {
$pm{'private-Error.pm'} = '$(INST_LIBDIR)/Error.pm';
}
--
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 3/3] The Makefile.PL will now find .pm files itself.
From: Michael G. Schwern schw...@pobox.com
It is no longer necessary to manually add new .pm files to the
Makefile.PL. This makes it easier to add modules.
It is still necessary to add them to the Makefile, but that extra work
should be removed at a future date.
Signed-off-by: Michael G Schwern schw...@pobox.com
---
perl/Makefile.PL | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/perl/Makefile.PL b/perl/Makefile.PL
index 887fa1b..3f29ba9 100644
--- a/perl/Makefile.PL
+++ b/perl/Makefile.PL
@@ -2,6 +2,10 @@ use strict;
use warnings;
use ExtUtils::MakeMaker;
use Getopt::Long;
+use File::Find;
+
+# Don't forget to update the perl/Makefile, too.
+# Don't forget to test with NO_PERL_MAKEMAKER=YesPlease
# Sanity: die at first unknown option
Getopt::Long::Configure qw/ pass_through /;
@@ -25,19 +29,18 @@ endif
MAKE_FRAG
}
-# XXX. When editing this list:
-#
-# * Please update perl/Makefile, too.
-# * Don't forget to test with NO_PERL_MAKEMAKER=YesPlease
-my %pm = (
- 'Git.pm' = '$(INST_LIBDIR)/Git.pm',
- 'Git/I18N.pm' = '$(INST_LIBDIR)/Git/I18N.pm',
- 'Git/SVN/Memoize/YAML.pm' = '$(INST_LIBDIR)/Git/SVN/Memoize/YAML.pm',
- 'Git/SVN/Fetcher.pm' = '$(INST_LIBDIR)/Git/SVN/Fetcher.pm',
- 'Git/SVN/Editor.pm' = '$(INST_LIBDIR)/Git/SVN/Editor.pm',
- 'Git/SVN/Prompt.pm' = '$(INST_LIBDIR)/Git/SVN/Prompt.pm',
- 'Git/SVN/Ra.pm' = '$(INST_LIBDIR)/Git/SVN/Ra.pm',
-);
+# Find all the .pm files in Git/ and Git.pm
+my %pm;
+find sub {
+ return unless /\.pm$/;
+
+ # sometimes File::Find prepends a ./ Strip it.
+ my $pm_path = $File::Find::name;
+ $pm_path =~ s{^\./}{};
+
+ $pm{$pm_path} = '$(INST_LIBDIR)/'.$pm_path;
+}, Git, Git.pm;
+
# We come with our own bundled Error.pm. It's not in the set of default
# Perl modules so install it if it's not available on the system yet.
--
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
Re: git-svn SVN 1.7 fix, take 2
On 2012.7.24 9:53 PM, Jonathan Nieder wrote: Michael G Schwern wrote: No, now it's just canonicalizing as early as possible. Preferably within the object accessor rather than at the point of use. So in the code below, $full_url is already escaped/canonicalized. Let's start with this. Is svn_path_canonicalize() idempotent? What does it do when it encounters a percent-sign? Nothing, because paths are not URI escaped. :) You probably meant svn_uri_canonicalize(). And no, it does not double escape, so its safe to escape as early as possible. use SVN::Core; my $uri = http://www.example.com/ foo; print SVN::_Core::svn_uri_canonicalize( SVN::_Core::svn_uri_canonicalize($uri) ); That produces http://www.example.com/%20foo;. The API docs don't say it specifically, but if it were otherwise it would be impossible to use. You'd have to check first if anything were escaped before canonicalizing. And a user couldn't pass in an escaped URL without risking it being double escaped. http://subversion.apache.org/docs/api/latest/svn__dirent__uri_8h.html#a8bae33a2fbf86857869f7b0e39a553e7 -- 'All anyone gets in a mirror is themselves,' she said. 'But what you gets in a good gumbo is everything.' -- Witches Abroad by Terry Prachett -- 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: Extract Git classes from git-svn (1/10)
On 2012.7.24 7:55 PM, Eric Wong wrote: After I'm exhausted from volunteering all the coding work, rather than submitting a URL to a remote repository I find I have to learn new specialized tools. It's extra learning and work, an extra step to screw up, and foreign to me (even as a experienced git user). It is of little benefit to me as a casual volunteer submitter. Except git is also a new specialized tool. Your examples are exactly why I'm saddened many projects only adopted git, but not the workflow which _built_ git (and Linux). There is an important difference between a tool which is useful for one or two projects and one which is useful for a broad spectrum of projects. I learn git once (or diff or bash or Perl or whatever) and I'm going to use it again and again all over the place. I learn git-send-email and (if I'm not a kernel developer) I'm going to use it on a handful of projects maybe. It's O(1) vs O(n) effort. Github also has broad spectrum utility. I learn how to fork and work with a Github pull request once, and I can repeat that on thousands of different projects of all different sorts of things. This commonality of tools and techniques is very important to easing the on ramp for new (to-your-project) developers. I can see if you've been on the git mailing list for a while and have git-am and all that set up, this system is great. But it comes at a cost which is offloaded onto new and casual contributors. Email is integral to Free/Open Source development and remains one of the few things on the Internet not (yet) controlled by any central entity. Once setup, the same email setup can work across all projects which use email. These projects need not be hosted on the same websites/servers at all. While I hear your concern about being centrally controlled, it is largely irrelevant to the new user experience. And remaining independent does not mean you can't use web tools. Be wary of a false dichotomy between Free and web. We use a mailing list is by no means an indication of commonality. Every project of the send patches to the list form has their own quirks and ways of doing it. Usually they're not written down. This is what I've been struggling with. I've been sending patches to mailing lists for decades and I can tell you everybody does it differently. Send a patch to the list is one of the steeper project on-ramps. This sort of specialized setup makes people bounce right off the submission process. At OSCON I was asking around for help getting things setup so I could submit patches here properly. As soon as they said which mail daemon are you running?, I said stop! I don't want to know any more. I have too many things to do to be fiddling with my mailer configuration just to submit volunteer work in the right form (that said, I'm pleased as punch that git-send-email now has instructions for sending via GMail). You're volunteers, too. We're all volunteers, so a more balanced submission process would be nice. How about we educate users about a proper email setup instead? If they're capable of learning git, they're surely capable of setting up an email client properly, and perhaps more projects can adopt an email-centric workflow. SubmittingPatches would be helped by that, particularly with a clear step-by-step example of using git-send-email and all its numerous command line switches. I was showing Jonathan the guide I have for releasing Perl modules which is both A) step-by-step and also B) covers the numerous little problems that are usually only in somebody's head or scattered around the docs. It was built in order to allow a person who had never released a module to release a module. Then we watched just such a person follow the directions. As they asked questions, struggled or made mistakes we filled in the gaps in the docs. https://github.com/scrottie/autobox-Core/wiki/How-To-Make-A-Release But this is also not about capability. Yes, people are capable of figuring out git-send-email, but its Yet Another Special Thing to learn before they can submit a patch and call their work done. Volunteers, especially brand new ones, have only so much volunteerism to burn before they'll walk away. You want them burning that on productive patching and contributions, not learning specialty tools. And, finally, the last thing most people want is more email. Seriously. It sounds like you live in your mailer, but fewer and fewer people do that. Me? I don't want to join another mailing list. My email management is a disaster! What it comes down to is this: is it enough to contribute to git.git to know how to work on git.git? Or do you also need to live in your mailer? Bolt on that extra requirement and you lose a large swath of contributors. But since you brought Github up... (I get the impression its kind of a dirty word around here) (Not speaking for the git project) I'm entirely against the way GitHub (or
Move Git::SVN into its own .pm file
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. -- 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 schw...@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
--
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 4/4] Move initialization of Git::SVN variables into Git::SVN.
From: Michael G. Schwern schw...@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
--
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: OT: mail-based interfaces and web-based interfaces (Re: Extract Git classes from git-svn (1/10))
On 2012.7.24 10:54 PM, Jonathan Nieder wrote: And again, it *does not have to be zero sum*. It doesn't have to be email VS GUI. You can have your cake and eat it too. I assume you're talking about web-based interfaces that have gateways to email, that produce inboxes like this: 24 Jul 02:46 GitHub [github] msysgit/msysgit was forked by peters 23 Jul 10:27 GitHub [msysgit/git] ce8ebc: vcs-svn: rename check_o 23 Jul 10:01 GitHub [github] Comment created on issue 44 (new git 23 Jul 09:50 GitHub [github] Comment created on issue 44 (new git 23 Jul 09:33 GitHub [github] Comment created on issue 44 (new git 23 Jul 09:39 GitHub [github] Comment created on issue 24 (Long fi 23 Jul 09:31 GitHub [github] Comment created on issue 44 (new git 23 Jul 09:30 GitHub [github] Comment created on issue 24 (Long fi 22 Jul 23:57 GitHub [github] Comment created on issue 44 (new git I call that pretending to have my cake, rather than having it. :) That's kind of like pointing at RCS and saying version control sucks and its pointless to try and make it better! Mail gateways built by web sites suck because they live in the web browser and email is an after thought. Sound familiar? Here is a much better example of the RT mail gateway that Perl 5 development uses. They're a dev community still centered around email, so it has to integrate well. http://www.nntp.perl.org/group/perl.perl5.porters/2012/07/msg189716.html And the corresponding ticket in the tracker. https://rt.perl.org/rt3/Public/Bug/Display.html?id=113684 The initial report comes in either via the web tracker or via a command line program (perlbug) that sends an email to the list. Replies on either the tracker or the mailing list are mirrored. Duplicates are detected etc... That's the sort of mail gateways I'm used to. Maybe some day someone will prove me wrong and make a nice web-based tool that I don't even need to know about that mines project mailing lists. If I have to tweak my subject lines a little to help it out, that's fine with me. I think patchwork is supposed to work this way. But unless we're talking about splitting the mailing list into a bunch of mini mailing lists (like some bug trackers do), it doesn't change anything fundamental, so I'm not sure why we're discussing this. I don't follow the bit about splitting the mailing list. -- 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: git-svn SVN 1.7 fix, take 2
On 2012.7.25 12:14 AM, Junio C Hamano wrote: Nothing, because paths are not URI escaped. :) You probably meant svn_uri_canonicalize(). And no, it does not double escape, so its safe to escape as early as possible. Are you saying that the function assumes that a local pathname would URI and path canonicalization are done differently and by different functions. svn_uri_canonicalize() vs svn_dirent_canonicalize(). Or maybe you're referring to the path portion of the URL? I don't think that makes a difference for what you're asking, but its important to keep in mind. not have '%' in it, returns its input as-is when it sees one, and if the caller really needs to express a path with '%' in it, it is the responsibility of the caller to escape it? It appears that if the % is followed by hex it assumes its an escape. Otherwise it escapes it. Thus... http://www.google.com/per%%nt - http://www.google.com/per%25%25nt http://www.google.com/per%ant - http://www.google.com/per%25ant http://www.google.com/per%cent - http://www.google.com/per%CEnt Which makes sense if the idea is to not double escape. That makes it even more confusing Straight out of the RFC. http://pretty-rfc.herokuapp.com/RFC3986#when-to-percent-encode Implementations must not percent-encode or decode the same string more than once, as decoding an already decoded string might lead to misinterpreting a percent data octet as the beginning of a percent- encoding, or vice versa in the case of percent-encoding an already percent-encoded string. It makes it far simpler to use. You can't read the mind of the user, but its a fair guess that they're not really thinking too deeply about how escaping works. It makes URI and path canonicalization safer and simpler. Otherwise you'd need to keep track of whether a thing was already escaped or not! Just begging for loads of bugs. (If SVN were using URI and path objects they'd just take care of it and none of this would be a problem in the first place). This way you have no double escaping concerns. No need to track if a thing is already canonicalized. Do it as often and as early as you like. Making a corner case a little harder is a small price to pay for making the common case much, much easier. This also appears to be what Firefox does. my $uri = http://www.example.com/ foo; print SVN::_Core::svn_uri_canonicalize( SVN::_Core::svn_uri_canonicalize($uri) ); That produces http://www.example.com/%20foo;. In other words, if your DocumentRoot was /var/www and you have a directory /var/www/per%cent you want to expose to the outside world, you have to say http://www.example.com/per%25cent; yourself and the canonicalize function will be an identity function? Yes. It can be made to work better. There's a number of places in the code which effectively do this: my $full_url = $url . '/' . $path; And I was canonicalizing them like this: my $full_url = canonicalize_url($url . '/' . $path); I'd been pondering whether it would be worthwhile to have a function which added a path to a base URL and canonicalized. Now I see that yes, it would be to deal with this corner case. my $full_url = append_path_to_url($url, $path); That would properly URI encode any % in $path before appending and then canonicalizing the whole thing as a URI. I'm pretty sure the code in master doesn't handle this at all. I have this vague suspicion that Jonathan was asking about what your Git::SVN::Utils::canonicalize_path() sub does, so all of the above might be moot, though... Its just a pass through to the SVN API. -- 44. I am not the atheist chaplain. -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/ -- 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: Teach Makefile.PL to find .pm files on its own
On 2012.7.25 9:56 AM, Junio C Hamano wrote: Looks sensible. Will queue. Thanks. Thanks! What's the lag time on it showing up in the repo, and which branch will it appear in? Also I just realized I've been basing my work on master. Should I move to maint? -- If you want the truth to stand clear before you, never be for or against. The struggle between for and against is the mind's worst disease. -- Sent-ts'an -- 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: Move Git::SVN into its own .pm file
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/ 0001-Extract-Git-SVN-from-git-svn-into-its-own-.pm-file.patch.gz Description: application/tar-gz
Re: Teach Makefile.PL to find .pm files on its own
On 2012.7.25 3:19 PM, Junio C Hamano wrote: Michael G Schwern schw...@pobox.com writes: How about the git-svn SVN 1.7 fix in general? All of these patch sets I'm sending build on one another, is that going to be a problem? It's going to come in about six parts. Judging from the rate of the discussion this is progressing, I was imagining that this series would be ready by 1.7.13 at the earliest, possibly back-merged to 1.7.12.X maintenance series, and 1.7.11.X maintenance series is no longer relevant by then. But I certainly do not mind seeing the series based on earlier maintenance releases, e.g. maint-1.7.9. There however are tons of other git-svn.perl and perl/ updates since then, so basing the series on the current maint branch to abandon 1.7.10.X and earlier but still leaving the door open to downmerge to 1.7.11.X may be a good trade-off. So... is that master or maint? Just let me know which one. -- 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/ -- 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 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.
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!
--
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
Extract Git::SVN from git-svn, take 2.
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. -- 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 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.
From: Michael G. Schwern schw...@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 schw...@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 normalper...@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
[PATCH 4/4] Move initialization of Git::SVN variables into Git::SVN.
From: Michael G. Schwern schw...@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
--
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
Extract remaining classes from git-svn
This series of patches extracts the remaining classes from git-svn. They're all simple extractions and functionally have no change. -- 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 1/8] Prepare Git::SVN::Log for extraction from git-svn.
From: Michael G. Schwern schw...@pobox.com
* Load Git command functions itself.
* Can't access the git-svn switch lexical any more, but its only used by
Git::SVN::Log so turn it into a Git::SVN::Log global.
* Load Git::SVN as needed. No need to load it always, its only used twice.
* Moved a state variable to the routine it's used for. (Drive by refactoring)
---
git-svn.perl | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index ef10f6f..e16475b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -87,7 +87,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),
+ for my $package ( qw(Git::SVN::Migration),
__PACKAGE__) {
*{${package}::$_} = \{Git::$_};
}
@@ -106,7 +106,7 @@ my ($_stdin, $_help, $_edit,
$_version, $_fetch_all, $_no_rebase, $_fetch_parent,
$_merge, $_strategy, $_preserve_merges, $_dry_run, $_local,
$_prefix, $_no_checkout, $_url, $_verbose,
- $_git_format, $_commit_url, $_tag, $_merge_info, $_interactive);
+ $_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 || '' }
@@ -270,7 +270,7 @@ my %cmd = (
{ 'url' = \$_url, } ],
'blame' = [ \Git::SVN::Log::cmd_blame,
Show what revision and author last modified each line of a
file,
- { 'git-format' = \$_git_format } ],
+ { 'git-format' = \$Git::SVN::Log::_git_format } ],
'reset' = [ \cmd_reset,
Undo fetches back to the specified SVN revision,
{ 'revision|r=s' = \$_revision,
@@ -2044,11 +2044,14 @@ package Git::SVN::Log;
use strict;
use warnings;
use Git::SVN::Utils qw(fatal);
+use Git qw(command command_oneline command_output_pipe command_close_pipe);
use POSIX qw/strftime/;
use constant commit_log_separator = ('-' x 72) . \n;
use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
%rusers $show_commit $incremental/;
-my $l_fmt;
+
+# Option set in git-svn
+our $_git_format;
sub cmt_showable {
my ($c) = @_;
@@ -2094,6 +2097,8 @@ sub git_svn_log_cmd {
}
my ($url, $rev, $uuid, $gs) = ::working_head_info($head);
+
+ require Git::SVN;
$gs ||= Git::SVN-_new;
my @cmd = (qw/log --abbrev-commit --pretty=raw --default/,
$gs-refname);
@@ -2155,6 +2160,7 @@ sub run_pager {
sub format_svn_date {
my $t = shift || time;
+ require Git::SVN;
my $gmoff = Git::SVN::get_tz($t);
return strftime(%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y),
localtime($t));
}
@@ -2225,6 +2231,7 @@ sub process_commit {
return 1;
}
+my $l_fmt;
sub show_commit {
my $c = shift;
if ($oneline) {
--
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/8] Extract Git::SVN::Log from git-svn.
From: Michael G. Schwern schw...@pobox.com
Straight cut paste.
Also noticed Git::SVN::Ra wasn't in the compile test. It is now.
---
git-svn.perl | 395 +-
perl/Git/SVN/Log.pm | 395 ++
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 6 +-
4 files changed, 401 insertions(+), 396 deletions(-)
create mode 100644 perl/Git/SVN/Log.pm
diff --git a/git-svn.perl b/git-svn.perl
index e16475b..7c8da44 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -11,6 +11,7 @@ $AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
use Git::SVN;
+use Git::SVN::Log;
use Git::SVN::Utils qw(fatal can_compress);
# From which subdir have we been invoked?
@@ -2040,400 +2041,6 @@ sub gc_directory {
}
-package Git::SVN::Log;
-use strict;
-use warnings;
-use Git::SVN::Utils qw(fatal);
-use Git qw(command command_oneline command_output_pipe command_close_pipe);
-use POSIX qw/strftime/;
-use constant commit_log_separator = ('-' x 72) . \n;
-use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
-%rusers $show_commit $incremental/;
-
-# Option set in git-svn
-our $_git_format;
-
-sub cmt_showable {
- my ($c) = @_;
- return 1 if defined $c-{r};
-
- # big commit message got truncated by the 16k pretty buffer in rev-list
- if ($c-{l} $c-{l}-[-1] eq ...\n
- $c-{a_raw} =~ /\@([a-f\d\-]+)$/) {
- @{$c-{l}} = ();
- my @log = command(qw/cat-file commit/, $c-{c});
-
- # shift off the headers
- shift @log while ($log[0] ne '');
- shift @log;
-
- # TODO: make $c-{l} not have a trailing newline in the future
- @{$c-{l}} = map { $_\n } grep !/^git-svn-id: /, @log;
-
- (undef, $c-{r}, undef) = ::extract_metadata(
- (grep(/^git-svn-id: /, @log))[-1]);
- }
- return defined $c-{r};
-}
-
-sub log_use_color {
- return $color || Git-repository-get_colorbool('color.diff');
-}
-
-sub git_svn_log_cmd {
- my ($r_min, $r_max, @args) = @_;
- my $head = 'HEAD';
- my (@files, @log_opts);
- foreach my $x (@args) {
- if ($x eq '--' || @files) {
- push @files, $x;
- } else {
- if (::verify_ref($x^0)) {
- $head = $x;
- } else {
- push @log_opts, $x;
- }
- }
- }
-
- my ($url, $rev, $uuid, $gs) = ::working_head_info($head);
-
- require Git::SVN;
- $gs ||= Git::SVN-_new;
- my @cmd = (qw/log --abbrev-commit --pretty=raw --default/,
- $gs-refname);
- push @cmd, '-r' unless $non_recursive;
- push @cmd, qw/--raw --name-status/ if $verbose;
- push @cmd, '--color' if log_use_color();
- push @cmd, @log_opts;
- if (defined $r_max $r_max == $r_min) {
- push @cmd, '--max-count=1';
- if (my $c = $gs-rev_map_get($r_max)) {
- push @cmd, $c;
- }
- } elsif (defined $r_max) {
- if ($r_max $r_min) {
- ($r_min, $r_max) = ($r_max, $r_min);
- }
- my (undef, $c_max) = $gs-find_rev_before($r_max, 1, $r_min);
- my (undef, $c_min) = $gs-find_rev_after($r_min, 1, $r_max);
- # If there are no commits in the range, both $c_max and $c_min
- # will be undefined. If there is at least 1 commit in the
- # range, both will be defined.
- return () if !defined $c_min || !defined $c_max;
- if ($c_min eq $c_max) {
- push @cmd, '--max-count=1', $c_min;
- } else {
- push @cmd, '--boundary', $c_min..$c_max;
- }
- }
- return (@cmd, @files);
-}
-
-# adapted from pager.c
-sub config_pager {
- if (! -t *STDOUT) {
- $ENV{GIT_PAGER_IN_USE} = 'false';
- $pager = undef;
- return;
- }
- chomp($pager = command_oneline(qw(var GIT_PAGER)));
- if ($pager eq 'cat') {
- $pager = undef;
- }
- $ENV{GIT_PAGER_IN_USE} = defined($pager);
-}
-
-sub run_pager {
- return unless defined $pager;
- pipe my ($rfd, $wfd) or return;
- defined(my $pid = fork) or fatal Can't fork: $!;
- if (!$pid) {
- open STDOUT, '', $wfd or
-fatal Can't redirect to stdout: $!;
- return;
- }
- open STDIN, '', $rfd or fatal Can't redirect stdin: $!;
- $ENV{LESS} ||= 'FRSX';
- exec $pager or fatal Can't run pager: $! ($pager);
-}
-
-sub format_svn_date {
- my $t = shift || time
[PATCH 8/8] Fix indents to match style.
From: Michael G. Schwern schw...@pobox.com
---
git-svn.perl | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 584e93a..4d173d4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -31,14 +31,14 @@ use Git::SVN::Migration;
use Git::SVN::Utils qw(fatal can_compress);
use Git qw(
-git_cmd_try
-command
-command_oneline
-command_noisy
-command_output_pipe
-command_close_pipe
-command_bidi_pipe
-command_close_bidi_pipe
+ git_cmd_try
+ command
+ command_oneline
+ command_noisy
+ command_output_pipe
+ command_close_pipe
+ command_bidi_pipe
+ command_close_bidi_pipe
);
BEGIN {
--
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 7/8] Extract Git::SVN::GlobSpec from git-svn.
From: Michael G. Schwern schw...@pobox.com
Straight cut paste. That's the last class.
* Make Git::SVN load it on its own, its the only thing that needs it.
---
git-svn.perl | 59
perl/Git/SVN.pm | 2 ++
perl/Git/SVN/GlobSpec.pm | 59
perl/Makefile| 1 +
t/Git-SVN/00compile.t| 3 ++-
5 files changed, 64 insertions(+), 60 deletions(-)
create mode 100644 perl/Git/SVN/GlobSpec.pm
diff --git a/git-svn.perl b/git-svn.perl
index 0856a77..584e93a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2040,65 +2040,6 @@ sub gc_directory {
}
}
-
-package Git::SVN::GlobSpec;
-use strict;
-use warnings;
-
-sub new {
- my ($class, $glob, $pattern_ok) = @_;
- my $re = $glob;
- $re =~ s!/+$!!g; # no need for trailing slashes
- my (@left, @right, @patterns);
- my $state = left;
- my $die_msg = Only one set of wildcard directories .
- (e.g. '*' or '*/*/*') is supported: '$glob'\n;
- for my $part (split(m|/|, $glob)) {
- if ($part =~ /\*/ $part ne *) {
- die Invalid pattern in '$glob': $part\n;
- } elsif ($pattern_ok $part =~ /[{}]/
-$part !~ /^\{[^{}]+\}/) {
- die Invalid pattern in '$glob': $part\n;
- }
- if ($part eq *) {
- die $die_msg if $state eq right;
- $state = pattern;
- push(@patterns, [^/]*);
- } elsif ($pattern_ok $part =~ /^\{(.*)\}$/) {
- die $die_msg if $state eq right;
- $state = pattern;
- my $p = quotemeta($1);
- $p =~ s/\\,/|/g;
- push(@patterns, (?:$p));
- } else {
- if ($state eq left) {
- push(@left, $part);
- } else {
- push(@right, $part);
- $state = right;
- }
- }
- }
- my $depth = @patterns;
- if ($depth == 0) {
- die One '*' is needed in glob: '$glob'\n;
- }
- my $left = join('/', @left);
- my $right = join('/', @right);
- $re = join('/', @patterns);
- $re = join('\/',
- grep(length, quotemeta($left), ($re), quotemeta($right)));
- my $left_re = qr/^\/\Q$left\E(\/|$)/;
- bless { left = $left, right = $right, left_regex = $left_re,
- regex = qr/$re/, glob = $glob, depth = $depth }, $class;
-}
-
-sub full_path {
- my ($self, $path) = @_;
- return (length $self-{left} ? $self-{left}/ : '') .
- $path . (length $self-{right} ? /$self-{right} : '');
-}
-
__END__
Data structures:
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 2e0d7f0..b8b3474 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -207,6 +207,8 @@ sub read_all_remotes {
. must start with 'refs/'\n)
unless $remote_ref =~ m{^refs/};
$local_ref = uri_decode($local_ref);
+
+ require Git::SVN::GlobSpec;
my $rs = {
t = $t,
remote = $remote,
diff --git a/perl/Git/SVN/GlobSpec.pm b/perl/Git/SVN/GlobSpec.pm
new file mode 100644
index 000..96cfd98
--- /dev/null
+++ b/perl/Git/SVN/GlobSpec.pm
@@ -0,0 +1,59 @@
+package Git::SVN::GlobSpec;
+use strict;
+use warnings;
+
+sub new {
+ my ($class, $glob, $pattern_ok) = @_;
+ my $re = $glob;
+ $re =~ s!/+$!!g; # no need for trailing slashes
+ my (@left, @right, @patterns);
+ my $state = left;
+ my $die_msg = Only one set of wildcard directories .
+ (e.g. '*' or '*/*/*') is supported: '$glob'\n;
+ for my $part (split(m|/|, $glob)) {
+ if ($part =~ /\*/ $part ne *) {
+ die Invalid pattern in '$glob': $part\n;
+ } elsif ($pattern_ok $part =~ /[{}]/
+$part !~ /^\{[^{}]+\}/) {
+ die Invalid pattern in '$glob': $part\n;
+ }
+ if ($part eq *) {
+ die $die_msg if $state eq right;
+ $state = pattern;
+ push(@patterns, [^/]*);
+ } elsif ($pattern_ok $part =~ /^\{(.*)\}$/) {
+ die $die_msg if $state eq right;
+ $state = pattern;
+ my $p = quotemeta($1);
+ $p =~ s/\\,/|/g;
+ push(@patterns, (?:$p));
+ } else {
+ if ($state eq left
[PATCH 4/8] Extract Git::SVN::Migration from git-svn.
From: Michael G. Schwern schw...@pobox.com
Straight cut paste.
---
git-svn.perl | 258 +-
perl/Git/SVN/Migration.pm | 258 ++
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 3 +-
4 files changed, 262 insertions(+), 258 deletions(-)
create mode 100644 perl/Git/SVN/Migration.pm
diff --git a/git-svn.perl b/git-svn.perl
index db60984..3741e2e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -12,6 +12,7 @@ $VERSION = '@@GIT_VERSION@@';
use Git::SVN;
use Git::SVN::Log;
+use Git::SVN::Migration;
use Git::SVN::Utils qw(fatal can_compress);
use Git qw(
@@ -2042,263 +2043,6 @@ sub gc_directory {
}
-package Git::SVN::Migration;
-# these version numbers do NOT correspond to actual version numbers
-# of git nor git-svn. They are just relative.
-#
-# v0 layout: .git/$id/info/url, refs/heads/$id-HEAD
-#
-# v1 layout: .git/$id/info/url, refs/remotes/$id
-#
-# v2 layout: .git/svn/$id/info/url, refs/remotes/$id
-#
-# v3 layout: .git/svn/$id, refs/remotes/$id
-#- info/url may remain for backwards compatibility
-#- this is what we migrate up to this layout automatically,
-#- this will be used by git svn init on single branches
-# v3.1 layout (auto migrated):
-#- .rev_db = .rev_db.$UUID, .rev_db will remain as a symlink
-# for backwards compatibility
-#
-# v4 layout: .git/svn/$repo_id/$id, refs/remotes/$repo_id/$id
-#- this is only created for newly multi-init-ed
-# repositories. Similar in spirit to the
-# --use-separate-remotes option in git-clone (now default)
-#- we do not automatically migrate to this (following
-# the example set by core git)
-#
-# v5 layout: .rev_db.$UUID = .rev_map.$UUID
-#- newer, more-efficient format that uses 24-bytes per record
-# with no filler space.
-#- use xxd -c24 .rev_map.$UUID to view and debug
-#- This is a one-way migration, repositories updated to the
-# new format will not be able to use old git-svn without
-# rebuilding the .rev_db. Rebuilding the rev_db is not
-# possible if noMetadata or useSvmProps are set; but should
-# be no problem for users that use the (sensible) defaults.
-use strict;
-use warnings;
-use Carp qw/croak/;
-use File::Path qw/mkpath/;
-use File::Basename qw/dirname basename/;
-
-our $_minimize;
-use Git qw(
- command
- command_noisy
- command_output_pipe
- command_close_pipe
-);
-
-sub migrate_from_v0 {
- my $git_dir = $ENV{GIT_DIR};
- return undef unless -d $git_dir;
- my ($fh, $ctx) = command_output_pipe(qw/rev-parse --symbolic --all/);
- my $migrated = 0;
- while ($fh) {
- chomp;
- my ($id, $orig_ref) = ($_, $_);
- next unless $id =~ s#^refs/heads/(.+)-HEAD$#$1#;
- next unless -f $git_dir/$id/info/url;
- my $new_ref = refs/remotes/$id;
- if (::verify_ref($new_ref^0)) {
- print STDERR W: $orig_ref is probably an old ,
-branch used by an ancient version of ,
-git-svn.\n,
-However, $new_ref also exists.\n,
-We will not be able ,
-to use this branch until this ,
-ambiguity is resolved.\n;
- next;
- }
- print STDERR Migrating from v0 layout...\n if !$migrated;
- print STDERR Renaming ref: $orig_ref = $new_ref\n;
- command_noisy('update-ref', $new_ref, $orig_ref);
- command_noisy('update-ref', '-d', $orig_ref, $orig_ref);
- $migrated++;
- }
- command_close_pipe($fh, $ctx);
- print STDERR Done migrating from v0 layout...\n if $migrated;
- $migrated;
-}
-
-sub migrate_from_v1 {
- my $git_dir = $ENV{GIT_DIR};
- my $migrated = 0;
- return $migrated unless -d $git_dir;
- my $svn_dir = $git_dir/svn;
-
- # just in case somebody used 'svn' as their $id at some point...
- return $migrated if -d $svn_dir ! -f $svn_dir/info/url;
-
- print STDERR Migrating from a git-svn v1 layout...\n;
- mkpath([$svn_dir]);
- print STDERR Data from a previous version of git-svn exists, but\n\t,
-$svn_dir\n\t(required for this version ,
-($::VERSION) of git-svn) does not exist.\n;
- my ($fh, $ctx) = command_output_pipe(qw/rev-parse --symbolic --all/);
- while ($fh) {
- my $x = $_;
- next unless $x =~ s#^refs/remotes/##;
- chomp $x;
- next unless -f $git_dir/$x/info/url
[PATCH 5/8] Load all the modules in one place and before running code.
From: Michael G. Schwern schw...@pobox.com
Just makes the code easier to follow. No functional change.
Also eliminate an unused lexical $SVN.
---
git-svn.perl | 44 +---
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 3741e2e..fc49ad6 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -10,11 +10,26 @@ use vars qw/$AUTHOR $VERSION
$AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
+use Carp qw/croak/;
+use Digest::MD5;
+use IO::File qw//;
+use File::Basename qw/dirname basename/;
+use File::Path qw/mkpath/;
+use File::Spec;
+use File::Find;
+use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
+use IPC::Open3;
+use Memoize;
+
use Git::SVN;
+use Git::SVN::Editor;
+use Git::SVN::Fetcher;
+use Git::SVN::Ra;
+use Git::SVN::Prompt;
use Git::SVN::Log;
use Git::SVN::Migration;
-use Git::SVN::Utils qw(fatal can_compress);
+use Git::SVN::Utils qw(fatal can_compress);
use Git qw(
git_cmd_try
command
@@ -26,6 +41,11 @@ use Git qw(
command_close_bidi_pipe
);
+BEGIN {
+ Memoize::memoize 'Git::config';
+ Memoize::memoize 'Git::config_bool';
+}
+
# From which subdir have we been invoked?
my $cmd_dir_prefix = eval {
@@ -79,28 +99,6 @@ sub _req_svn {
}
}
-use Carp qw/croak/;
-use Digest::MD5;
-use IO::File qw//;
-use File::Basename qw/dirname basename/;
-use File::Path qw/mkpath/;
-use File::Spec;
-use File::Find;
-use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
-use IPC::Open3;
-use Git::SVN::Editor qw//;
-use Git::SVN::Fetcher qw//;
-use Git::SVN::Ra qw//;
-use Git::SVN::Prompt qw//;
-use Memoize; # core since 5.8.0, Jul 2002
-
-BEGIN {
- Memoize::memoize 'Git::config';
- Memoize::memoize 'Git::config_bool';
-}
-
-my ($SVN);
-
$sha1 = qr/[a-f\d]{40}/;
$sha1_short = qr/[a-f\d]{4,40}/;
my ($_stdin, $_help, $_edit,
--
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
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 4/4] Move initialization of Git::SVN variables into Git::SVN.
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/ -- 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
Make git-svn Use accessors for paths and urls
This patch series gives Git::SVN and Git::SVN::Ra accessors for
path and url and then makes the rest of the code use them, rather
than grab at $obj-{path} and $obj-{url}. This then will give
us the control necessary to canonicalize them as early as
possible (done in the next patch series).
There are plenty of other places in the code which will benefit
from accessors and functions, but those will come later. path
and url were the most obvious.
This is a refactoring and has no functional change. All git-svn
tests pass with SVN 1.6 for each patch.
This goes on top of my previous patch series to extract other
classes. That hasn't been reviewed yet, but both that and this are
a simple patch series and I figure we can review a bit ahead.
This is the last refactoring patch series. After this bugs, start
getting fixed.
--
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 1/5] Make Git::SVN use accessors internally for path.
From: Michael G. Schwern schw...@pobox.com
Then later it can be canonicalized automatically rather than everywhere
its used.
Later patch will make other things use it.
---
perl/Git/SVN.pm | 87 +
1 file changed, 56 insertions(+), 31 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index b8b3474..0aff9d0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -314,12 +314,12 @@ sub init_remote_config {
print STDERR Using higher level of URL: ,
$url = $min_url\n;
}
- my $old_path = $self-{path};
- $self-{path} = $url;
- $self-{path} =~ s!^\Q$min_url\E(/|$)!!;
+ my $old_path = $self-path;
+ $url =~ s!^\Q$min_url\E(/|$)!!;
if (length $old_path) {
- $self-{path} .= /$old_path;
+ $url .= /$old_path;
}
+ $self-path($url);
$url = $min_url;
}
}
@@ -343,11 +343,13 @@ sub init_remote_config {
unless ($no_write) {
command_noisy('config',
svn-remote.$self-{repo_id}.url, $url);
- $self-{path} =~ s{^/}{};
- $self-{path} =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
+ my $path = $self-path;
+ $path =~ s{^/}{};
+ $path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
+ $self-path($path);
command_noisy('config', '--add',
svn-remote.$self-{repo_id}.fetch,
- $self-{path}:.$self-refname);
+ $self-path.:.$self-refname);
}
$self-{url} = $url;
}
@@ -435,17 +437,22 @@ sub new {
}
}
my $self = _new($class, $repo_id, $ref_id, $path);
- if (!defined $self-{path} || !length $self-{path}) {
+ if (!defined $self-path || !length $self-path) {
my $fetch = command_oneline('config', '--get',
svn-remote.$repo_id.fetch,
:$ref_id\$) or
die Failed to read \svn-remote.$repo_id.fetch\ ,
\:$ref_id\$\ in config\n;
- ($self-{path}, undef) = split(/\s*:\s*/, $fetch);
+ my($path) = split(/\s*:\s*/, $fetch);
+ $self-path($path);
+ }
+ {
+ my $path = $self-path;
+ $path =~ s{/+}{/}g;
+ $path =~ s{\A/}{};
+ $path =~ s{/\z}{};
+ $self-path($path);
}
- $self-{path} =~ s{/+}{/}g;
- $self-{path} =~ s{\A/}{};
- $self-{path} =~ s{/\z}{};
$self-{url} = command_oneline('config', '--get',
svn-remote.$repo_id.url) or
die Failed to read \svn-remote.$repo_id.url\ in config\n;
@@ -567,7 +574,7 @@ sub _set_svm_vars {
}
my $r = $ra-get_latest_revnum;
- my $path = $self-{path};
+ my $path = $self-path;
my %tried;
while (length $path) {
unless ($tried{$self-{url}/$path}) {
@@ -728,7 +735,7 @@ sub prop_walk {
$path =~ s#^/*#/#g;
my $p = $path;
# Strip the irrelevant part of the path.
- $p =~ s#^/+\Q$self-{path}\E(/|$)#/#;
+ $p =~ s#^/+\Q@{[$self-path]}\E(/|$)#/#;
# Ensure the path is terminated by a `/'.
$p =~ s#/*$#/#;
@@ -749,7 +756,7 @@ sub prop_walk {
foreach (sort keys %$dirent) {
next if $dirent-{$_}-{kind} != $SVN::Node::dir;
- $self-prop_walk($self-{path} . $p . $_, $rev, $sub);
+ $self-prop_walk($self-path . $p . $_, $rev, $sub);
}
}
@@ -920,19 +927,19 @@ sub rewrite_uuid {
sub metadata_url {
my ($self) = @_;
($self-rewrite_root || $self-{url}) .
- (length $self-{path} ? '/' . $self-{path} : '');
+ (length $self-path ? '/' . $self-path : '');
}
sub full_url {
my ($self) = @_;
- $self-{url} . (length $self-{path} ? '/' . $self-{path} : '');
+ $self-{url} . (length $self-path ? '/' . $self-path : '');
}
sub full_pushurl {
my ($self) = @_;
if ($self-{pushurl}) {
- return $self-{pushurl} . (length $self-{path} ? '/' .
- $self-{path} : '');
+ return $self-{pushurl} . (length $self-path ? '/' .
+ $self-path : '');
} else {
return $self-full_url;
}
@@ -1048,20 +1055,20 @@ sub do_git_commit {
sub match_paths {
my ($self, $paths, $r) = @_;
- return 1 if $self-{path} eq '';
- if (my $path = $paths-{/$self
[PATCH 2/5] Make Git::SVN use an accessor for URLs internally.
From: Michael G. Schwern schw...@pobox.com
So later it can do automatic canonicalization.
A later patch will make other things use the accessor.
No functional change here.
---
perl/Git/SVN.pm | 44 ++--
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 0aff9d0..59bca51 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -351,7 +351,7 @@ sub init_remote_config {
svn-remote.$self-{repo_id}.fetch,
$self-path.:.$self-refname);
}
- $self-{url} = $url;
+ $self-url($url);
}
sub find_by_url { # repos_root and, path are optional
@@ -453,9 +453,10 @@ sub new {
$path =~ s{/\z}{};
$self-path($path);
}
- $self-{url} = command_oneline('config', '--get',
- svn-remote.$repo_id.url) or
+ my $url = command_oneline('config', '--get',
+ svn-remote.$repo_id.url) or
die Failed to read \svn-remote.$repo_id.url\ in config\n;
+ $self-url($url);
$self-{pushurl} = eval { command_oneline('config', '--get',
svn-remote.$repo_id.pushurl) };
$self-rebuild;
@@ -577,17 +578,18 @@ sub _set_svm_vars {
my $path = $self-path;
my %tried;
while (length $path) {
- unless ($tried{$self-{url}/$path}) {
+ my $try = $self-url . /$path;
+ unless ($tried{$try}) {
return $ra if $self-read_svm_props($ra, $path, $r);
- $tried{$self-{url}/$path} = 1;
+ $tried{$try} = 1;
}
$path =~ s#/?[^/]+$##;
}
die Path: '$path' should be ''\n if $path ne '';
return $ra if $self-read_svm_props($ra, $path, $r);
- $tried{$self-{url}/$path} = 1;
+ $tried{$self-url./$path} = 1;
- if ($ra-{repos_root} eq $self-{url}) {
+ if ($ra-{repos_root} eq $self-url) {
die @err, (map { $_\n } keys %tried), \n;
}
@@ -610,7 +612,7 @@ sub _set_svm_vars {
if (!$ok) {
die @err, (map { $_\n } keys %tried), \n;
}
- Git::SVN::Ra-new($self-{url});
+ Git::SVN::Ra-new($self-url);
}
sub svnsync {
@@ -677,7 +679,7 @@ sub ra_uuid {
if (!$@ $uuid $uuid =~ /^([a-f\d\-]{30,})$/i) {
$self-{ra_uuid} = $uuid;
} else {
- die ra_uuid called without URL\n unless $self-{url};
+ die ra_uuid called without URL\n unless $self-url;
$self-{ra_uuid} = $self-ra-get_uuid;
tmp_config('--add', $key, $self-{ra_uuid});
}
@@ -701,7 +703,7 @@ sub repos_root {
sub ra {
my ($self) = shift;
- my $ra = Git::SVN::Ra-new($self-{url});
+ my $ra = Git::SVN::Ra-new($self-url);
$self-_set_repos_root($ra-{repos_root});
if ($self-use_svm_props !$self-{svm}) {
if ($self-no_metadata) {
@@ -926,13 +928,13 @@ sub rewrite_uuid {
sub metadata_url {
my ($self) = @_;
- ($self-rewrite_root || $self-{url}) .
+ ($self-rewrite_root || $self-url) .
(length $self-path ? '/' . $self-path : '');
}
sub full_url {
my ($self) = @_;
- $self-{url} . (length $self-path ? '/' . $self-path : '');
+ $self-url . (length $self-path ? '/' . $self-path : '');
}
sub full_pushurl {
@@ -1436,7 +1438,7 @@ sub find_extra_svk_parents {
for my $ticket ( @tickets ) {
my ($uuid, $path, $rev) = split /:/, $ticket;
if ( $uuid eq $self-ra_uuid ) {
- my $url = $self-{url};
+ my $url = $self-url;
my $repos_root = $url;
my $branch_from = $path;
$branch_from =~ s{^/}{};
@@ -1682,7 +1684,7 @@ sub find_extra_svn_parents {
# are now marked as merge, we can add the tip as a parent.
my @merges = split \n, $mergeinfo;
my @merge_tips;
- my $url = $self-{url};
+ my $url = $self-url;
my $uuid = $self-ra_uuid;
my %ranges;
for my $merge ( @merges ) {
@@ -2307,6 +2309,20 @@ sub path {
return $self-{path};
}
+
+sub url {
+my $self = shift;
+
+if( @_ ) {
+my $url = shift;
+$self-{url} = $url;
+return;
+}
+
+return $self-{url};
+}
+
+
# for read-only access of old .rev_db formats
sub unlink_rev_db_symlink {
my ($self) = @_;
--
1.7.11.3
--
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 4/5] Change the rest of the code to use Git::SVN-path instead of the hash directly.
From: Michael G. Schwern schw...@pobox.com
No functional change.
---
git-svn.perl| 12 +++-
perl/Git/SVN.pm | 4 ++--
perl/Git/SVN/Fetcher.pm | 2 +-
perl/Git/SVN/Ra.pm | 6 +++---
4 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 5711c57..039623e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1195,7 +1195,7 @@ sub cmd_show_ignore {
my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
$gs ||= Git::SVN-new;
my $r = (defined $_revision ? $_revision : $gs-ra-get_latest_revnum);
- $gs-prop_walk($gs-{path}, $r, sub {
+ $gs-prop_walk($gs-path, $r, sub {
my ($gs, $path, $props) = @_;
print STDOUT \n# $path\n;
my $s = $props-{'svn:ignore'} or return;
@@ -1211,7 +1211,7 @@ sub cmd_show_externals {
my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
$gs ||= Git::SVN-new;
my $r = (defined $_revision ? $_revision : $gs-ra-get_latest_revnum);
- $gs-prop_walk($gs-{path}, $r, sub {
+ $gs-prop_walk($gs-path, $r, sub {
my ($gs, $path, $props) = @_;
print STDOUT \n# $path\n;
my $s = $props-{'svn:externals'} or return;
@@ -1226,7 +1226,7 @@ sub cmd_create_ignore {
my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
$gs ||= Git::SVN-new;
my $r = (defined $_revision ? $_revision : $gs-ra-get_latest_revnum);
- $gs-prop_walk($gs-{path}, $r, sub {
+ $gs-prop_walk($gs-path, $r, sub {
my ($gs, $path, $props) = @_;
# $path is of the form /path/to/dir/
$path = '.' . $path;
@@ -1396,7 +1396,7 @@ sub cmd_commit_diff {
the command-line\n, $usage);
}
$url = $gs-{url};
- $svn_path = $gs-{path};
+ $svn_path = $gs-path;
}
unless (defined $_revision) {
fatal(-r|--revision is a required argument\n, $usage);
@@ -1634,6 +1634,8 @@ sub post_fetch_checkout {
sub complete_svn_url {
my ($url, $path) = @_;
$path =~ s#/+$##;
+
+ # If the path is not a URL...
if ($path !~ m#^[a-z\+]+://#) {
if (!defined $url || $url !~ m#^[a-z\+]+://#) {
fatal(E: '$path' is not a complete URL ,
@@ -1670,7 +1672,7 @@ sub complete_url_ls_init {
wanted to set to: $gs-{url}\n;
}
command_oneline('config', $k, $gs-{url}) unless $orig_url;
- my $remote_path = $gs-{path}/$repo_path;
+ my $remote_path = $gs-path . /$repo_path;
$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
$remote_path =~ s#/+#/#g;
$remote_path =~ s#^/##g;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 59bca51..fc907a0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1123,7 +1123,7 @@ sub find_parent_branch {
($base, $head) = parse_revision_argument(0, $r);
} else {
if ($r0 $r) {
- $gs-ra-get_log([$gs-{path}], $r0 + 1, $r, 1,
+ $gs-ra-get_log([$gs-path], $r0 + 1, $r, 1,
0, 1, sub { $base = $_[1] - 1 });
}
}
@@ -1145,7 +1145,7 @@ sub find_parent_branch {
# at the moment), so we can't rely on it
$self-{last_rev} = $r0;
$self-{last_commit} = $parent;
- $ed = Git::SVN::Fetcher-new($self, $gs-{path});
+ $ed = Git::SVN::Fetcher-new($self, $gs-path);
$gs-ra-gs_do_switch($r0, $rev, $gs,
$self-full_url, $ed)
or die SVN connection failed somewhere...\n;
diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 76fae9b..046a7a2 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -83,7 +83,7 @@ sub _mark_empty_symlinks {
chomp(my $empty_blob = `git hash-object -t blob --stdin /dev/null`);
my ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);
local $/ = \0;
- my $pfx = defined($switch_path) ? $switch_path : $git_svn-{path};
+ my $pfx = defined($switch_path) ? $switch_path : $git_svn-path;
$pfx .= '/' if length($pfx);
while ($ls) {
chomp;
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 329f855..27dcdd5 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -264,7 +264,7 @@ sub get_commit_editor {
sub gs_do_update {
my ($self, $rev_a, $rev_b, $gs, $editor) = @_;
my $new = ($rev_a == $rev_b);
- my $path = $gs-{path};
+ my $path = $gs-path;
if ($new -e $gs-{index}) {
unlink $gs-{index} or die
@@ -300,7 +300,7 @@ sub
[PATCH 5/5] Change the rest of the code to use the Git::SVN and Git::SVN::Ra url accessors.
From: Michael G. Schwern schw...@pobox.com
Note: The structure returned from Git::SVN-read_all_remotes() does not appear
to
contain objects, so I'm leaving them alone.
That's everything converted over to the url and path accessors.
No functional change.
---
git-svn.perl | 11 ++-
perl/Git/SVN.pm | 11 ++-
perl/Git/SVN/Migration.pm | 6 +++---
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 039623e..de1ddd1 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1395,7 +1395,7 @@ sub cmd_commit_diff {
fatal(Needed URL or usable git-svn --id in ,
the command-line\n, $usage);
}
- $url = $gs-{url};
+ $url = $gs-url;
$svn_path = $gs-path;
}
unless (defined $_revision) {
@@ -1663,15 +1663,16 @@ sub complete_url_ls_init {
and a separate URL is not specified);
}
}
- my $url = $ra-{url};
+ my $url = $ra-url;
my $gs = Git::SVN-init($url, undef, undef, undef, 1);
my $k = svn-remote.$gs-{repo_id}.url;
my $orig_url = eval { command_oneline(qw/config --get/, $k) };
- if ($orig_url ($orig_url ne $gs-{url})) {
+ if ($orig_url ($orig_url ne $gs-url)) {
die $k already set: $orig_url\n,
- wanted to set to: $gs-{url}\n;
+ wanted to set to: $gs-url\n;
}
- command_oneline('config', $k, $gs-{url}) unless $orig_url;
+ command_oneline('config', $k, $gs-url) unless $orig_url;
+
my $remote_path = $gs-path . /$repo_path;
$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
$remote_path =~ s#/+#/#g;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index fc907a0..7913d8f 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -560,7 +560,7 @@ sub _set_svm_vars {
# username is of no interest
$src =~ s{(^[a-z\+]*://)[^/@]*@}{$1};
- my $replace = $ra-{url};
+ my $replace = $ra-url;
$replace .= /$path if length $path;
my $section = svn-remote.$self-{repo_id};
@@ -599,16 +599,17 @@ sub _set_svm_vars {
$path = $ra-{svn_path};
$ra = Git::SVN::Ra-new($ra-{repos_root});
while (length $path) {
- unless ($tried{$ra-{url}/$path}) {
+ my $try = $ra-url ./$path;
+ unless ($tried{$try}) {
$ok = $self-read_svm_props($ra, $path, $r);
last if $ok;
- $tried{$ra-{url}/$path} = 1;
+ $tried{$try} = 1;
}
$path =~ s#/?[^/]+$##;
}
die Path: '$path' should be ''\n if $path ne '';
$ok ||= $self-read_svm_props($ra, $path, $r);
- $tried{$ra-{url}/$path} = 1;
+ $tried{$ra-url ./$path} = 1;
if (!$ok) {
die @err, (map { $_\n } keys %tried), \n;
}
@@ -1108,7 +1109,7 @@ sub find_parent_branch {
}
my $r = $i-{copyfrom_rev};
my $repos_root = $self-ra-{repos_root};
- my $url = $self-ra-{url};
+ my $url = $self-ra-url;
my $new_url = $url . $branch_from;
print STDERR Found possible branch point: ,
$new_url = , $self-full_url, , $r\n
diff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm
index 75d7429..30daf35 100644
--- a/perl/Git/SVN/Migration.pm
+++ b/perl/Git/SVN/Migration.pm
@@ -177,14 +177,14 @@ sub minimize_connections {
my $ra = Git::SVN::Ra-new($url);
# skip existing cases where we already connect to the root
- if (($ra-{url} eq $ra-{repos_root}) ||
+ if (($ra-url eq $ra-{repos_root}) ||
($ra-{repos_root} eq $repo_id)) {
- $root_repos-{$ra-{url}} = $repo_id;
+ $root_repos-{$ra-url} = $repo_id;
next;
}
my $root_ra = Git::SVN::Ra-new($ra-{repos_root});
- my $root_path = $ra-{url};
+ my $root_path = $ra-url;
$root_path =~ s#^\Q$ra-{repos_root}\E(/|$)##;
foreach my $path (keys %$fetch) {
my $ref_id = $fetch-{$path};
--
1.7.11.3
--
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 4/4] Move initialization of Git::SVN variables into Git::SVN.
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? -- 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: Make git-svn Use accessors for paths and urls
On 2012.7.27 8:10 PM, Jonathan Nieder wrote: This is the last refactoring patch series. After this bugs, start getting fixed. I just wanted to say thanks for your thoughtful presentation of this code. I was worried before, but these have been pleasantly submitted. You're welcome. I've gained at least three levels in rebasing in the process. If you have a chance at some point to offer advice, I'd love to add the information to Documentation/SubmittingPatches that was missing. Proposed text is ideal, but outline form or a list of missing aspects and confusing existing coverage would be fine, too. Remind me when I'm done with the 1.7 fix please? -- You are wicked and wrong to have broken inside and peeked at the implementation and then relied upon it. -- tchrist in 31832.969261130@chthon -- 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 1/7] Move the canonicalization functions to Git::SVN::Utils
From: Michael G. Schwern schw...@pobox.com
So they can be used by others.
I'd like to test them, but they're going to become SVN API wrappers shortly
and those aren't predictable.
No functional change.
---
git-svn.perl | 33 +++-
perl/Git/SVN/Utils.pm | 52 ++-
2 files changed, 58 insertions(+), 27 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index de1ddd1..a857484 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -29,7 +29,13 @@ use Git::SVN::Prompt;
use Git::SVN::Log;
use Git::SVN::Migration;
-use Git::SVN::Utils qw(fatal can_compress);
+use Git::SVN::Utils qw(
+ fatal
+ can_compress
+ canonicalize_path
+ canonicalize_url
+);
+
use Git qw(
git_cmd_try
command
@@ -1256,31 +1262,6 @@ sub cmd_mkdirs {
$gs-mkemptydirs($_revision);
}
-sub canonicalize_path {
- my ($path) = @_;
- my $dot_slash_added = 0;
- if (substr($path, 0, 1) ne /) {
- $path = ./ . $path;
- $dot_slash_added = 1;
- }
- # File::Spec-canonpath doesn't collapse x/../y into y (for a
- # good reason), so let's do this manually.
- $path =~ s#/+#/#g;
- $path =~ s#/\.(?:/|$)#/#g;
- $path =~ s#/[^/]+/\.\.##g;
- $path =~ s#/$##g;
- $path =~ s#^\./## if $dot_slash_added;
- $path =~ s#^/##;
- $path =~ s#^\.$##;
- return $path;
-}
-
-sub canonicalize_url {
- my ($url) = @_;
- $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
- return $url;
-}
-
# get_svnprops(PATH)
# --
# Helper for cmd_propget and cmd_proplist below.
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index 3d0bfa4..c842d00 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -5,7 +5,12 @@ use warnings;
use base qw(Exporter);
-our @EXPORT_OK = qw(fatal can_compress);
+our @EXPORT_OK = qw(
+ fatal
+ can_compress
+ canonicalize_path
+ canonicalize_url
+);
=head1 NAME
@@ -56,4 +61,49 @@ sub can_compress {
}
+=head3 canonicalize_path
+
+my $canoncalized_path = canonicalize_path($path);
+
+Converts $path into a canonical form which is safe to pass to the SVN
+API as a file path.
+
+=cut
+
+sub canonicalize_path {
+ my ($path) = @_;
+ my $dot_slash_added = 0;
+ if (substr($path, 0, 1) ne /) {
+ $path = ./ . $path;
+ $dot_slash_added = 1;
+ }
+ # File::Spec-canonpath doesn't collapse x/../y into y (for a
+ # good reason), so let's do this manually.
+ $path =~ s#/+#/#g;
+ $path =~ s#/\.(?:/|$)#/#g;
+ $path =~ s#/[^/]+/\.\.##g;
+ $path =~ s#/$##g;
+ $path =~ s#^\./## if $dot_slash_added;
+ $path =~ s#^/##;
+ $path =~ s#^\.$##;
+ return $path;
+}
+
+
+=head3 canonicalize_url
+
+my $canonicalized_url = canonicalize_url($url);
+
+Converts $url into a canonical form which is safe to pass to the SVN
+API as a URL.
+
+=cut
+
+sub canonicalize_url {
+ my ($url) = @_;
+ $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
+ return $url;
+}
+
+
1;
--
1.7.11.3
--
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 3/7] Extract, test and enhance the logic to collapse ../foo paths.
From: Michael G. Schwern schw...@pobox.com
The SVN API functions will not accept ../foo but their canonicalization
functions will not collapse it. So we'll have to do it ourselves.
_collapse_dotdot() works better than the existing regex did.
This will be used shortly when canonicalize_path() starts using the
SVN API.
---
perl/Git/SVN/Utils.pm | 14 +-
t/Git-SVN/Utils/collapse_dotdot.t | 23 +++
2 files changed, 36 insertions(+), 1 deletion(-)
create mode 100644 t/Git-SVN/Utils/collapse_dotdot.t
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index 9d5d3c5..7314e52 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -72,6 +72,18 @@ API as a file path.
=cut
+# Turn foo/../bar into bar
+sub _collapse_dotdot {
+ my $path = shift;
+
+ 1 while $path =~ s{/[^/]+/+\.\.}{};
+ 1 while $path =~ s{[^/]+/+\.\./}{};
+ 1 while $path =~ s{[^/]+/+\.\.}{};
+
+ return $path;
+}
+
+
sub canonicalize_path {
my ($path) = @_;
my $dot_slash_added = 0;
@@ -83,7 +95,7 @@ sub canonicalize_path {
# good reason), so let's do this manually.
$path =~ s#/+#/#g;
$path =~ s#/\.(?:/|$)#/#g;
- $path =~ s#/[^/]+/\.\.##g;
+ $path = _collapse_dotdot($path);
$path =~ s#/$##g;
$path =~ s#^\./## if $dot_slash_added;
$path =~ s#^/##;
diff --git a/t/Git-SVN/Utils/collapse_dotdot.t
b/t/Git-SVN/Utils/collapse_dotdot.t
new file mode 100644
index 000..1da1cce
--- /dev/null
+++ b/t/Git-SVN/Utils/collapse_dotdot.t
@@ -0,0 +1,23 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+use Git::SVN::Utils;
+my $collapse_dotdot = \Git::SVN::Utils::_collapse_dotdot;
+
+my %tests = (
+ foo/bar/baz = foo/bar/baz,
+ ..= ..,
+ foo/..= ,
+ /foo/bar/../../baz= /baz,
+ deeply/.././deeply/nested = ./deeply/nested,
+);
+
+for my $arg (keys %tests) {
+ my $want = $tests{$arg};
+
+ is $collapse_dotdot-($arg), $want, _collapse_dotdot('$arg') = $want;
+}
--
1.7.11.3
--
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 5/7] Remove irrelevant comment.
From: Michael G. Schwern schw...@pobox.com
The code doesn't use File::Spec.
---
perl/Git/SVN/Utils.pm | 2 --
1 file changed, 2 deletions(-)
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index deade07..6c8ae53 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -92,8 +92,6 @@ sub canonicalize_path {
$path = ./ . $path;
$dot_slash_added = 1;
}
- # File::Spec-canonpath doesn't collapse x/../y into y (for a
- # good reason), so let's do this manually.
$path =~ s#/+#/#g;
$path =~ s#/\.(?:/|$)#/#g;
$path = _collapse_dotdot($path);
--
1.7.11.3
--
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 6/7] Switch path canonicalization to use the SVN API.
From: Michael G. Schwern schw...@pobox.com
All tests pass with SVN 1.6. SVN 1.7 remains broken, not worrying
about it yet.
SVN changed its path canonicalization API between 1.6 and 1.7.
http://svnbook.red-bean.com/en/1.6/svn.developer.usingapi.html#svn.developer.usingapi.urlpath
http://svnbook.red-bean.com/en/1.7/svn.developer.usingapi.html#svn.developer.usingapi.urlpath
The SVN API does not accept foo/.. but it also doesn't canonicalize
it. We have to do it ourselves.
---
perl/Git/SVN/Utils.pm | 21 +
1 file changed, 21 insertions(+)
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index 6c8ae53..7ae6fac 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -86,6 +86,27 @@ sub _collapse_dotdot {
sub canonicalize_path {
+ my $path = shift;
+
+ # The 1.7 way to do it
+ if ( defined SVN::_Core::svn_dirent_canonicalize ) {
+ $path = _collapse_dotdot($path);
+ return SVN::_Core::svn_dirent_canonicalize($path);
+ }
+ # The 1.6 way to do it
+ elsif ( defined SVN::_Core::svn_path_canonicalize ) {
+ $path = _collapse_dotdot($path);
+ return SVN::_Core::svn_path_canonicalize($path);
+ }
+ # No SVN API canonicalization is available, do it ourselves
+ else {
+ $path = _canonicalize_path_ourselves($path);
+ return $path;
+ }
+}
+
+
+sub _canonicalize_path_ourselves {
my ($path) = @_;
my $dot_slash_added = 0;
if (substr($path, 0, 1) ne /) {
--
1.7.11.3
--
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 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.
From: Michael G. Schwern schw...@pobox.com
This canonicalizes paths and urls as early as possible so we don't
have to remember to do it at the point of use. It will fix a swath
of SVN 1.7 problems in one go.
Its ok to double canonicalize things.
SVN 1.7 still fails, still not worrying about that.
---
perl/Git/SVN.pm| 6 --
perl/Git/SVN/Ra.pm | 6 +-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index b0ed3ea..798f6c4 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -27,6 +27,8 @@ use Git::SVN::Utils qw(
fatal
can_compress
join_paths
+ canonicalize_path
+ canonicalize_url
);
my $can_use_yaml;
@@ -2305,7 +2307,7 @@ sub path {
if( @_ ) {
my $path = shift;
-$self-{path} = $path;
+$self-{path} = canonicalize_path($path);
return;
}
@@ -2318,7 +2320,7 @@ sub url {
if( @_ ) {
my $url = shift;
-$self-{url} = $url;
+$self-{url} = canonicalize_url($url);
return;
}
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 27dcdd5..ef7b3dd 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -3,6 +3,10 @@ use vars qw/@ISA $config_dir $_ignore_refs_regex
$_log_window_size/;
use strict;
use warnings;
use SVN::Client;
+use Git::SVN::Utils qw(
+ canonicalize_url
+);
+
use SVN::Ra;
BEGIN {
@ISA = qw(SVN::Ra);
@@ -138,7 +142,7 @@ sub url {
if( @_ ) {
my $url = shift;
-$self-{url} = $url;
+$self-{url} = canonicalize_url($url);
return;
}
--
1.7.11.3
--
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
Fix git-svn for SVN 1.7
This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and 1.6.18. Patch 7/8 is where SVN 1.7 starts passing. There is one exception. t9100-git-svn-basic.sh fails 11-13. This appears to be due to a bug in SVN to do with symlinks. Leave that for somebody else, this is the final submission in the series. The work was difficult because the code relies on simple string equalty when comparing URLs and paths. Turning on canonicalization in one part of the code would cause another part to fail if it also did not canonicalize. There's likely still issues. A better solution would be to have path and URL objects which overload the eq operator and automatically stringify canonicalized and escaped. This patch series should be placed on top of the previous which made accessors canonicalize. I'm getting a bit ahead of things by submitting it now, but I'd like to get the review process rolling. -- 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/8] Fix typo in test
From: Michael G. Schwern schw...@pobox.com
Test to check that the migration got rid of the old style git-svn directory.
It wasn't failing, just throwing a message to STDERR.
---
t/t9107-git-svn-migrate.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 289fc31..cfb4453 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -32,7 +32,7 @@ test_expect_success 'initialize old-style (v0) git svn
layout' '
echo $svnrepo $GIT_DIR/git-svn/info/url
echo $svnrepo $GIT_DIR/svn/info/url
git svn migrate
- ! test -d $GIT_DIR/git svn
+ ! test -d $GIT_DIR/git-svn
git rev-parse --verify refs/${remotes_git_svn}^0
git rev-parse --verify refs/remotes/svn^0
test $(git config --get svn-remote.svn.url) = $svnrepo
--
1.7.11.3
--
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 4/8] Replace hand rolled URL escapes with canonicalization
From: Michael G. Schwern schw...@pobox.com
Continuing to move towards getting everything canonicalizing the same way.
* Git::SVN-init_remote_config and Git::SVN::Ra-minimize_url both
have to canonicalize the same way else init_remote_config
will incorrectly think they're different URLs causing
t9107-git-svn-migrate.sh to fail.
---
git-svn.perl | 24 +++-
perl/Git/SVN.pm| 2 +-
perl/Git/SVN/Ra.pm | 27 +--
3 files changed, 9 insertions(+), 44 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 6e3e240..6e97545 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1412,24 +1412,6 @@ sub cmd_commit_diff {
}
}
-sub escape_uri_only {
- my ($uri) = @_;
- my @tmp;
- foreach (split m{/}, $uri) {
- s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf(%%%02X,ord($1))/eg;
- push @tmp, $_;
- }
- join('/', @tmp);
-}
-
-sub escape_url {
- my ($url) = @_;
- if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {
- my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
- $url = $scheme://$domain$uri;
- }
- $url;
-}
sub cmd_info {
my $path = canonicalize_path(defined($_[0]) ? $_[0] : .);
@@ -1457,18 +1439,18 @@ sub cmd_info {
my $full_url = $url . ($fullpath eq ? : /$fullpath);
if ($_url) {
- print escape_url($full_url), \n;
+ print canonicalize_url($full_url), \n;
return;
}
my $result = Path: $path\n;
$result .= Name: . basename($path) . \n if $file_type ne dir;
- $result .= URL: . escape_url($full_url) . \n;
+ $result .= URL: . canonicalize_url($full_url) . \n;
eval {
my $repos_root = $gs-repos_root;
Git::SVN::remove_username($repos_root);
- $result .= Repository Root: . escape_url($repos_root) . \n;
+ $result .= Repository Root: . canonicalize_url($repos_root)
. \n;
};
if ($@) {
$result .= Repository Root: (offline)\n;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index cb6d83a..4219e5b 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -296,7 +296,7 @@ sub find_existing_remote {
sub init_remote_config {
my ($self, $url, $no_write) = @_;
- $url =~ s!/+$!!; # strip trailing slash
+ $url = canonicalize_url($url);
my $r = read_all_remotes();
my $existing = find_existing_remote($url, $r);
if ($existing) {
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index ef7b3dd..ed9dbe9 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -66,24 +66,6 @@ sub _auth_providers () {
\@rv;
}
-sub escape_uri_only {
- my ($uri) = @_;
- my @tmp;
- foreach (split m{/}, $uri) {
- s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf(%%%02X,ord($1))/eg;
- push @tmp, $_;
- }
- join('/', @tmp);
-}
-
-sub escape_url {
- my ($url) = @_;
- if ($url =~ m#^(https?)://([^/]+)(.*)$#) {
- my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
- $url = $scheme://$domain$uri;
- }
- $url;
-}
sub new {
my ($class, $url) = @_;
@@ -119,7 +101,7 @@ sub new {
$Git::SVN::Prompt::_no_auth_cache = 1;
}
} # no warnings 'once'
- my $self = SVN::Ra-new(url = escape_url($url), auth = $baton,
+ my $self = SVN::Ra-new(url = canonicalize_url($url), auth = $baton,
config = $config,
pool = SVN::Pool-new,
auth_provider_callbacks = $callbacks);
@@ -314,7 +296,7 @@ sub gs_do_switch {
if ($old_url =~ m#^svn(\+ssh)?://# ||
($full_url =~ m#^https?://#
-escape_url($full_url) ne $full_url)) {
+canonicalize_url($full_url) ne $full_url)) {
$_[0] = undef;
$self = undef;
$RA = undef;
@@ -327,7 +309,7 @@ sub gs_do_switch {
}
$ra ||= $self;
- $url_b = escape_url($url_b);
+ $url_b = canonicalize_url($url_b);
my $reporter = $ra-do_switch($rev_b, '', 1, $url_b, $editor, $pool);
my @lock = (::compare_svn_version('1.2.0') = 0) ? (undef) : ();
$reporter-set_path('', $rev_a, 0, @lock, $pool);
@@ -582,7 +564,8 @@ sub minimize_url {
$ra-get_log(, $latest, 0, 1, 0, 1, sub {});
};
} while ($@ ($c = shift @components));
- $url;
+
+ return canonicalize_url($url);
}
sub can_do_switch {
--
1.7.11.3
--
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 1/8] SVN 1.7 will truncate not-a%40{0} to just not-a.
From: Michael G. Schwern schw...@pobox.com
Rather than guess what SVN is going to do for each version, make the test use
the branch name that was actually created.
---
t/t9118-git-svn-funky-branch-names.sh | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/t/t9118-git-svn-funky-branch-names.sh
b/t/t9118-git-svn-funky-branch-names.sh
index 63fc982..193d3ca 100755
--- a/t/t9118-git-svn-funky-branch-names.sh
+++ b/t/t9118-git-svn-funky-branch-names.sh
@@ -32,6 +32,11 @@ test_expect_success 'setup svnrepo' '
start_httpd
'
+# SVN 1.7 will truncate not-a%40{0] to just not-a.
+# Look at what SVN wound up naming the branch and use that.
+# Be sure to escape the @ if it shows up.
+non_reflog=`svn_cmd ls $svnrepo/pr ject/branches | grep not-a | sed 's/\///'
| sed 's/@/%40/'`
+
test_expect_success 'test clone with funky branch names' '
git svn clone -s $svnrepo/pr ject project
(
@@ -42,7 +47,7 @@ test_expect_success 'test clone with funky branch names' '
git rev-parse refs/remotes/%2Eleading_dot
git rev-parse refs/remotes/trailing_dot%2E
git rev-parse refs/remotes/trailing_dotlock%2Elock
- git rev-parse refs/remotes/not-a%40{0}reflog
+ git rev-parse refs/remotes/$non_reflog
)
'
--
1.7.11.3
--
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 8/8] Remove some ad hoc canonicalizations.
From: Michael G. Schwern schw...@pobox.com
---
git-svn.perl| 8
perl/Git/SVN.pm | 1 -
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 3d120d5..56d1ba7 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -36,6 +36,7 @@ use Git::SVN::Utils qw(
canonicalize_url
join_paths
add_path_to_url
+ join_paths
);
use Git qw(
@@ -1598,7 +1599,7 @@ sub post_fetch_checkout {
sub complete_svn_url {
my ($url, $path) = @_;
- $path =~ s#/+$##;
+ $path = canonicalize_path($path);
# If the path is not a URL...
if ($path !~ m#^[a-z\+]+://#) {
@@ -1617,7 +1618,7 @@ sub complete_url_ls_init {
print STDERR W: $switch not specified\n;
return;
}
- $repo_path =~ s#/+$##;
+ $repo_path = canonicalize_path($repo_path);
if ($repo_path =~ m#^[a-z\+]+://#) {
$ra = Git::SVN::Ra-new($repo_path);
$repo_path = '';
@@ -1638,9 +1639,8 @@ sub complete_url_ls_init {
}
command_oneline('config', $k, $gs-url) unless $orig_url;
- my $remote_path = $gs-path . /$repo_path;
+ my $remote_path = join_paths( $gs-path, $repo_path );
$remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg;
- $remote_path =~ s#/+#/#g;
$remote_path =~ s#^/##g;
$remote_path .= /* if $remote_path !~ /\*/;
my ($n) = ($switch =~ /^--(\w+)/);
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index e5f7acc..3c68c09 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -460,7 +460,6 @@ sub new {
}
{
my $path = $self-path;
- $path =~ s{/+}{/}g;
$path =~ s{\A/}{};
$path =~ s{/\z}{};
$self-path($path);
--
1.7.11.3
--
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 6/8] Add function to append a path to a URL.
From: Michael G. Schwern schw...@pobox.com
Remove the ad-hoc versions.
This is mostly to normalize the process and ensure the URLs produced
don't have double slashes or anything.
Also provides a place to fix the corner case where a file path
contains a percent sign.
---
git-svn.perl | 3 ++-
perl/Git/SVN.pm | 33 +++--
perl/Git/SVN/Ra.pm| 8
perl/Git/SVN/Utils.pm | 27 +++
t/Git-SVN/Utils/add_path_to_url.t | 27 +++
5 files changed, 75 insertions(+), 23 deletions(-)
create mode 100644 t/Git-SVN/Utils/add_path_to_url.t
diff --git a/git-svn.perl b/git-svn.perl
index 6b90765..3d120d5 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -35,6 +35,7 @@ use Git::SVN::Utils qw(
canonicalize_path
canonicalize_url
join_paths
+ add_path_to_url
);
use Git qw(
@@ -1436,7 +1437,7 @@ sub cmd_info {
# canonicalize_path() will return to make libsvn 1.5.x happy,
$path = . if $path eq ;
- my $full_url = canonicalize_url( $url . ($fullpath eq ? :
/$fullpath) );
+ my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) );
if ($_url) {
print $full_url\n;
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 4219e5b..22bf207 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -29,6 +29,7 @@ use Git::SVN::Utils qw(
join_paths
canonicalize_path
canonicalize_url
+ add_path_to_url
);
my $can_use_yaml;
@@ -564,8 +565,7 @@ sub _set_svm_vars {
# username is of no interest
$src =~ s{(^[a-z\+]*://)[^/@]*@}{$1};
- my $replace = $ra-url;
- $replace .= /$path if length $path;
+ my $replace = add_path_to_url($ra-url, $path);
my $section = svn-remote.$self-{repo_id};
tmp_config($section.svm-source, $src);
@@ -582,7 +582,7 @@ sub _set_svm_vars {
my $path = $self-path;
my %tried;
while (length $path) {
- my $try = $self-url . /$path;
+ my $try = add_path_to_url($self-url, $path);
unless ($tried{$try}) {
return $ra if $self-read_svm_props($ra, $path, $r);
$tried{$try} = 1;
@@ -591,7 +591,7 @@ sub _set_svm_vars {
}
die Path: '$path' should be ''\n if $path ne '';
return $ra if $self-read_svm_props($ra, $path, $r);
- $tried{$self-url./$path} = 1;
+ $tried{ add_path_to_url($self-url, $path) } = 1;
if ($ra-{repos_root} eq $self-url) {
die @err, (map { $_\n } keys %tried), \n;
@@ -603,7 +603,7 @@ sub _set_svm_vars {
$path = $ra-{svn_path};
$ra = Git::SVN::Ra-new($ra-{repos_root});
while (length $path) {
- my $try = $ra-url ./$path;
+ my $try = add_path_to_url($ra-url, $path);
unless ($tried{$try}) {
$ok = $self-read_svm_props($ra, $path, $r);
last if $ok;
@@ -613,7 +613,7 @@ sub _set_svm_vars {
}
die Path: '$path' should be ''\n if $path ne '';
$ok ||= $self-read_svm_props($ra, $path, $r);
- $tried{$ra-url ./$path} = 1;
+ $tried{ add_path_to_url($ra-url, $path) } = 1;
if (!$ok) {
die @err, (map { $_\n } keys %tried), \n;
}
@@ -933,20 +933,19 @@ sub rewrite_uuid {
sub metadata_url {
my ($self) = @_;
- ($self-rewrite_root || $self-url) .
- (length $self-path ? '/' . $self-path : '');
+ my $url = $self-rewrite_root || $self-url;
+ return add_path_to_url( $url, $self-path );
}
sub full_url {
my ($self) = @_;
- $self-url . (length $self-path ? '/' . $self-path : '');
+ return add_path_to_url( $self-url, $self-path );
}
sub full_pushurl {
my ($self) = @_;
if ($self-{pushurl}) {
- return $self-{pushurl} . (length $self-path ? '/' .
- $self-path : '');
+ return add_path_to_url( $self-{pushurl}, $self-path );
} else {
return $self-full_url;
}
@@ -1114,7 +1113,7 @@ sub find_parent_branch {
my $r = $i-{copyfrom_rev};
my $repos_root = $self-ra-{repos_root};
my $url = $self-ra-url;
- my $new_url = $url . $branch_from;
+ my $new_url = add_path_to_url( $url, $branch_from );
print STDERR Found possible branch point: ,
$new_url = , $self-full_url, , $r\n
unless $::_q 1;
@@ -1443,12 +1442,11 @@ sub find_extra_svk_parents {
for my $ticket ( @tickets ) {
my ($uuid, $path, $rev) = split /:/, $ticket;
if ( $uuid eq $self-ra_uuid ) {
- my $url = $self-url;
- my $repos_root = $url
[PATCH 7/8] Turn on canonicalization on newly minted URLs.
From: Michael G. Schwern schw...@pobox.com
Go through all the spots that use the new add_path_to_url() to
make a new URL and canonicalize them.
* copyfrom_path has to be canonicalized else find_parent_branch
will get confused
* due to the `canonicalize_url($full_url) ne $full_url)` line of
logic in gs_do_switch(), $full_url is left alone until after.
At this point SVN 1.7 passes except for 3 tests in
t9100-git-svn-basic.sh that look like an SVN bug to do with
symlinks.
---
perl/Git/SVN.pm| 19 ++-
perl/Git/SVN/Ra.pm | 9 -
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 22bf207..e5f7acc 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -362,6 +362,8 @@ sub init_remote_config {
sub find_by_url { # repos_root and, path are optional
my ($class, $full_url, $repos_root, $path) = @_;
+ $full_url = canonicalize_url($full_url);
+
return undef unless defined $full_url;
remove_username($full_url);
remove_username($repos_root) if defined $repos_root;
@@ -400,6 +402,11 @@ sub find_by_url { # repos_root and, path are optional
}
$p =~ s#^\Q$z\E(?:/|$)#$prefix# or next;
}
+
+ # remote fetch paths are not URI escaped. Decode ours
+ # so they match
+ $p = uri_decode($p);
+
foreach my $f (keys %$fetch) {
next if $f ne $p;
return Git::SVN-new($fetch-{$f}, $repo_id, $f);
@@ -934,18 +941,18 @@ sub rewrite_uuid {
sub metadata_url {
my ($self) = @_;
my $url = $self-rewrite_root || $self-url;
- return add_path_to_url( $url, $self-path );
+ return canonicalize_url( add_path_to_url( $url, $self-path ) );
}
sub full_url {
my ($self) = @_;
- return add_path_to_url( $self-url, $self-path );
+ return canonicalize_url( add_path_to_url( $self-url, $self-path ) );
}
sub full_pushurl {
my ($self) = @_;
if ($self-{pushurl}) {
- return add_path_to_url( $self-{pushurl}, $self-path );
+ return canonicalize_url( add_path_to_url( $self-{pushurl},
$self-path ) );
} else {
return $self-full_url;
}
@@ -1113,7 +1120,7 @@ sub find_parent_branch {
my $r = $i-{copyfrom_rev};
my $repos_root = $self-ra-{repos_root};
my $url = $self-ra-url;
- my $new_url = add_path_to_url( $url, $branch_from );
+ my $new_url = canonicalize_url( add_path_to_url( $url, $branch_from ) );
print STDERR Found possible branch point: ,
$new_url = , $self-full_url, , $r\n
unless $::_q 1;
@@ -1869,7 +1876,9 @@ sub make_log_entry {
$email ||= $author\@$uuid;
$commit_email ||= $author\@$uuid;
} elsif ($self-use_svnsync_props) {
- my $full_url = add_path_to_url( $self-svnsync-{url},
$self-path );
+ my $full_url = canonicalize_url(
+ add_path_to_url( $self-svnsync-{url}, $self-path )
+ );
remove_username($full_url);
my $uuid = $self-svnsync-{uuid};
$log_entry{metadata} = $full_url\@$rev $uuid;
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 788e642..29b46e8 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -5,6 +5,7 @@ use warnings;
use SVN::Client;
use Git::SVN::Utils qw(
canonicalize_url
+ canonicalize_path
add_path_to_url
);
@@ -102,6 +103,7 @@ sub new {
$Git::SVN::Prompt::_no_auth_cache = 1;
}
} # no warnings 'once'
+
my $self = SVN::Ra-new(url = $url, auth = $baton,
config = $config,
pool = SVN::Pool-new,
@@ -200,6 +202,7 @@ sub get_log {
qw/copyfrom_path copyfrom_rev action/;
if ($s{'copyfrom_path'}) {
$s{'copyfrom_path'} =~ s/$prefix_regex//;
+ $s{'copyfrom_path'} =
canonicalize_path($s{'copyfrom_path'});
}
$_[0]{$p} = \%s;
}
@@ -303,7 +306,11 @@ sub gs_do_switch {
$ra = Git::SVN::Ra-new($full_url);
$ra_invalid = 1;
} elsif ($old_url ne $full_url) {
- SVN::_Ra::svn_ra_reparent($self-{session}, $full_url, $pool);
+ SVN::_Ra::svn_ra_reparent(
+ $self-{session},
+ canonicalize_url($full_url),
+ $pool
+ );
$self-url($full_url);
$reparented = 1;
}
--
1.7.11.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
[PATCH 3/8] Improve our URL canonicalization to be more like SVN 1.7's.
From: Michael G. Schwern schw...@pobox.com
Previously, our URL canonicalization didn't do much of anything.
Now it actually escapes and collapses slashes. This is mostly a cut paste
of escape_url from git-svn.
This is closer to how SVN 1.7's canonicalization behaves. Doing it with
1.6 lets us chase down some problems caused by more effective canonicalization
without having to deal with all the other 1.7 issues on top of that.
* Remote URLs have to be canonicalized otherwise Git::SVN-find_existing_remote
will think they're different.
* The SVN remote is now written to the git config canonicalized. That
should be ok. Adjust a test to account for that.
---
perl/Git/SVN.pm| 4 ++--
perl/Git/SVN/Utils.pm | 19 +--
t/Git-SVN/Utils/canonicalize_url.t | 26 ++
t/t9107-git-svn-migrate.sh | 4 +++-
4 files changed, 48 insertions(+), 5 deletions(-)
create mode 100644 t/Git-SVN/Utils/canonicalize_url.t
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 798f6c4..cb6d83a 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -201,9 +201,9 @@ sub read_all_remotes {
} elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) {
$r-{$1}-{svm} = {};
} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
- $r-{$1}-{url} = $2;
+ $r-{$1}-{url} = canonicalize_url($2);
} elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) {
- $r-{$1}-{pushurl} = $2;
+ $r-{$1}-{pushurl} = canonicalize_url($2);
} elsif (m!^(.+)\.ignore-refs=\s*(.*)\s*$!) {
$r-{$1}-{ignore_refs_regex} = $2;
} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index 7ae6fac..dab6e4d 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -147,10 +147,25 @@ sub canonicalize_url {
}
+sub _canonicalize_url_path {
+ my ($uri_path) = @_;
+
+ my @parts;
+ foreach my $part (split m{/+}, $uri_path) {
+ $part =~
s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf(%%%02X,ord($1))/eg;
+ push @parts, $part;
+ }
+
+ return join('/', @parts);
+}
+
sub _canonicalize_url_ourselves {
my ($url) = @_;
- $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
- return $url;
+ if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {
+ my ($scheme, $domain, $uri) = ($1, $2,
_canonicalize_url_path(canonicalize_path($3)));
+ $url = $scheme://$domain$uri;
+ }
+ $url;
}
diff --git a/t/Git-SVN/Utils/canonicalize_url.t
b/t/Git-SVN/Utils/canonicalize_url.t
new file mode 100644
index 000..05795ab
--- /dev/null
+++ b/t/Git-SVN/Utils/canonicalize_url.t
@@ -0,0 +1,26 @@
+#!/usr/bin/env perl
+
+# Test our own home rolled URL canonicalizer. Test the private one
+# directly because we can't predict what the SVN API is doing to do.
+
+use strict;
+use warnings;
+
+use Test::More 'no_plan';
+
+use Git::SVN::Utils;
+my $canonicalize_url = \Git::SVN::Utils::_canonicalize_url_ourselves;
+
+my %tests = (
+ http://x.com; = http://x.com;,
+ http://x.com/; = http://x.com;,
+ http://x.com/foo/bar; = http://x.com/foo/bar;,
+ http://x.com//foo//bar//; = http://x.com/foo/bar;,
+ http://x.com/ /%/= http://x.com/%20%20/%25;,
+);
+
+for my $arg (keys %tests) {
+ my $want = $tests{$arg};
+
+ is $canonicalize_url-($arg), $want, canonicalize_url('$arg') =
$want;
+}
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index cfb4453..ee73013 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -27,6 +27,8 @@ test_expect_success 'setup old-looking metadata' '
head=`git rev-parse --verify refs/heads/git-svn-HEAD^0`
test_expect_success 'git-svn-HEAD is a real HEAD' test -n '$head'
+svnrepo_escaped=`echo $svnrepo | sed 's/ /%20/'`
+
test_expect_success 'initialize old-style (v0) git svn layout' '
mkdir -p $GIT_DIR/git-svn/info $GIT_DIR/svn/info
echo $svnrepo $GIT_DIR/git-svn/info/url
@@ -35,7 +37,7 @@ test_expect_success 'initialize old-style (v0) git svn
layout' '
! test -d $GIT_DIR/git-svn
git rev-parse --verify refs/${remotes_git_svn}^0
git rev-parse --verify refs/remotes/svn^0
- test $(git config --get svn-remote.svn.url) = $svnrepo
+ test $(git config --get svn-remote.svn.url) = $svnrepo_escaped
test `git config --get svn-remote.svn.fetch` = \
:refs/${remotes_git_svn}
'
--
1.7.11.3
--
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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 6:50 AM, Jonathan Nieder wrote:
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
[...]
@@ -100,6 +102,20 @@ API as a URL.
=cut
sub canonicalize_url {
+my $url = shift;
+
+# The 1.7 way to do it
+if ( defined SVN::_Core::svn_uri_canonicalize ) {
+return SVN::_Core::svn_uri_canonicalize($url);
+}
+# There wasn't a 1.6 way to do it, so we do it ourself.
+else {
+return _canonicalize_url_ourselves($url);
+}
+}
+
+
+sub _canonicalize_url_ourselves {
my ($url) = @_;
$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;
Leaves me a bit nervous.
As it should, SVN dumped a mess on us.
What effect should we expect this change to have? Is our emulation
of svn_uri_canonicalize already perfect and this change just a little
futureproofing in case svn_uri_canonicalize gets even better, or is
this a trap waiting to happen when new callers of canonicalize_url
start relying on, e.g., %-encoding of special characters?
This change is *just* about sliding in the SVN API call and seeing if git-svn
still works with SVN 1.6. It should have no effect on SVN 1.6. These patches
are a very slow and careful refactoring doing just one thing at a time. Every
time I tried to do too many things at once, tests broke and I had to tease the
patch apart.
At this point in the patch series the code is not ready for canonicalization.
Until 3/8 in the next patch series, canonicalize_url() basically does nothing
on SVN 1.6 so the code has never had to deal with the problem. 3/8 deals with
improving _canonicalize_url_ourselves() to work more like
svn_uri_canonicalize() and thus turns on canonicalization for SVN 1.6 and
deals with the breakage.
If I am reading Subversion r873487 correctly, in ancient times,
svn_path_canonicalize() did the appropriate tweaking for URIs. Today
its implementation is comforting:
const char *
svn_path_canonicalize(const char *path, apr_pool_t *pool)
{
if (svn_path_is_url(path))
return svn_uri_canonicalize(path, pool);
else
return svn_dirent_canonicalize(path, pool);
}
It might be easier to rely on that on pre-1.7 systems.
I didn't know about that. I don't know what your SVN backwards compat
requirements are, but if that behavior goes back far enough in SVN to satisfy
you folks, then canonicalize_url() should fall back to
SVN::_Core::svn_path_canonicalize(). But try it at the end of the patch
series. The code has to be prepared for canonicalization first. Then how it
actually does it can be improved.
--
Defender of Lexical Encapsulation
--
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 6/7] Switch path canonicalization to use the SVN API.
On 2012.7.28 6:55 AM, Jonathan Nieder wrote:
Michael G. Schwern wrote:
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm
@@ -86,6 +86,27 @@ sub _collapse_dotdot {
sub canonicalize_path {
+my $path = shift;
+
+# The 1.7 way to do it
+if ( defined SVN::_Core::svn_dirent_canonicalize ) {
+$path = _collapse_dotdot($path);
+return SVN::_Core::svn_dirent_canonicalize($path);
+}
+# The 1.6 way to do it
+elsif ( defined SVN::_Core::svn_path_canonicalize ) {
+$path = _collapse_dotdot($path);
+return SVN::_Core::svn_path_canonicalize($path);
+}
+# No SVN API canonicalization is available, do it ourselves
+else {
When would this else case trip?
When svn_path_canonicalize() does not exist in the SVN API, presumably because
their SVN is too old.
Would it be safe to make it
return an error message, or even to do something like the following?
I don't know what your SVN backwards compat requirements are, or when
svn_path_canonicalize() appears in the API, so I left it as is. git-svn's
home rolled path canonicalization worked and its no work to leave it working.
No reason to break it IMO.
sub canonicalize_path {
my $path = shift;
$path = _collapse_dotdot($path);
# Subversion 1.7 split svn_path_canonicalize() into
# svn_dirent_canonicalize() and svn_uri_canonicalize().
if (!defined SVN::_Core::svn_dirent_canonicalize) {
return SVN::_Core::svn_path_canonicalize($path);
}
return SVN::_Core::svn_dirent_canonicalize($path);
}
As a side note...
If they don't have Mars bar, get me a Twix. Else get me a Mars bar.
If they have a Mars bar, get me one. Else get me a Twix.
--
Look at me talking when there's science to do.
When I look out there it makes me glad I'm not you.
I've experiments to be run.
There is research to be done
On the people who are still alive.
-- Jonathan Coulton, Still Alive
--
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 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.
On 2012.7.28 7:11 AM, Jonathan Nieder wrote: Yay! Am I correct in imagining this makes the following sequence of commands[1] no longer trip an assertion failure in svn_path_join[2] with SVN 1.6? git svn init -Thttp://trac-hacks.org/svn/tagsplugin/trunk \ -thttp://trac-hacks.org/svn/tagsplugin/tags \ -bhttp://trac-hacks.org/svn/tagsplugin/branches git svn fetch Works For Me™! ... Checked out HEAD: http://trac-hacks.org/svn/tagsplugin/trunk r11776 -- Insulting our readers is part of our business model. http://somethingpositive.net/sp07122005.shtml -- 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 1/8] SVN 1.7 will truncate not-a%40{0} to just not-a.
On 2012.7.28 7:16 AM, Jonathan Nieder wrote:
Michael G. Schwern wrote:
Rather than guess what SVN is going to do for each version, make the test use
the branch name that was actually created.
[...]
-git rev-parse refs/remotes/not-a%40{0}reflog
+git rev-parse refs/remotes/$non_reflog
Doesn't this defeat the point of the testcase (checking that git-svn
is able to avoid creating git refs containing @{, following the rules
from git-check-ref-format(1))?
Unless I messed up, entirely possible as I'm not a shell programmer, the test
is still useful for testing SVN 1.6. Under SVN 1.6 $non_reflog should be
'not-a%40{0}reflog' as before.
Do you know when SVN truncates the directory name?
IIRC its silently does it during the svn cp.
Would historical
SVN repositories or historical SVN servers be able to have a directory
named with a %40 in it, or has this been disallowed completely,
leaving problematic historical repositories to be dumped with old SVN,
tweaked, and reloaded with new SVN?
Dunno, lemme check...
$ source ~/bin/svn16
$ svnadmin --version
svnadmin, version 1.6.18 (r1303927)
...
$ svnadmin create svnrepo
$ mkdir project project/trunk project/branches project/tags
$ echo foo project/trunk/foo
$ svn import -m 'test import' project
file:///Users/schwern/tmp/test/svnrepo/project
Adding project/tags
Adding project/trunk
Adding project/trunk/foo
Adding project/branches
Committed revision 1.
$ rm -rf project/
$ svn cp -m 'reflog' file:///Users/schwern/tmp/test/svnrepo/project/trunk
'file:///Users/schwern/tmp/test/svnrepo/project/branches/not-a%40{0}reflog'
Committed revision 2.
$ svn ls file:///Users/schwern/tmp/test/svnrepo/project/branches
not-a@{0}reflog/
$ source ~/bin/svn17
$ svn --version
svn, version 1.7.5 (r1336830)
...
$ svn ls file:///Users/schwern/tmp/test/svnrepo/project/branches
not-a@{0}reflog/
If you make it with SVN 1.6 its still there with SVN 1.7. That's good, it
means you can ship a prebuilt repository and check it against SVN 1.7.
The bad news is the new code segfaults on it. I don't know if that's the SVN
1.7 API choking on its own stuff or because of my changes or both. If you set
up the test I can try and fix it. Otherwise I'll just flounder in shell.
--
I went to college, which is a lot like being in the Army, except when
stupid people yell at me for stupid things, I can hit them.
-- Jonathan Schwarz
--
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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 12:30 PM, Jonathan Nieder wrote: I didn't know about that. I don't know what your SVN backwards compat requirements are, but if that behavior goes back far enough in SVN to satisfy you folks, then canonicalize_url() should fall back to SVN::_Core::svn_path_canonicalize(). svn_path_canonicalize() has been usable for this kind of thing since SVN 1.1, possibly earlier. Great! Then _canonicalize_url_ourselves() can probably be replaced with that. Just take my advice and do it after 1.7 is working and the code is ready for canonicalization. But try it at the end of the patch series. The code has to be prepared for canonicalization first. Then how it actually does it can be improved. Since this part of the series is not tested with SVN 1.7, this is basically adding dead code, right? That could be avoided by reordering the changes to keep canonicalize_url as-is until later in the series when the switchover is safe. I would suggest that worrying whether a few lines of code are introduced now or 10 patches later in the same branch which is all going to be merged in one go (and retesting the patches after it) is not the most important thing. The code needs humans looking over it and deciding if canonicalizations were missed or applied inappropriately. Or hey, work on that path and url object idea that makes a lot of real code mess go away. -- ROCKS FALL! EVERYONE DIES! http://www.somethingpositive.net/sp05032002.shtml -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 1:02 PM, Jonathan Nieder wrote: Jonathan Nieder wrote: Michael G Schwern wrote: I would suggest that worrying whether a few lines of code are introduced now or 10 patches later in the same branch which is all going to be merged in one go (and retesting the patches after it) is not the most important thing. [...] In that case they should be one patch, I'd think. The advantage of introducing changes gradually is that (1) the changes can be examined and tested one at a time, and (2) if later a change proves to be problematic, it can be isolated, understood, and fixed more easily. The strategy you are suggesting would have neither of those advantages. (To avoid confusion: by The strategy you are suggesting I mean introducing dead code first and activating it later, not the path and url object idea. The path and url object approach would be very nice. :)) If this is all a topic branch then it doesn't matter much whether a couple lines of code is introduced at patch 8 of a branch or patch 13. Sure, it matters a little, but... https://secure.wikimedia.org/wikipedia/en/wiki/Opportunity_cost If it *isn't* going in a topic branch, if its not visible as a collected work in history, if its going to be rebased on top of master, then yeah I can see why you're so concerned. -- Alligator sandwich, and make it snappy! -- 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 3/7] Extract, test and enhance the logic to collapse ../foo paths.
On 2012.7.30 12:51 PM, Eric Wong wrote:
The SVN API functions will not accept ../foo but their canonicalization
functions will not collapse it. So we'll have to do it ourselves.
_collapse_dotdot() works better than the existing regex did.
I don't dispute it's better, but it's worth explaining in the commit
message to reviewers why something is better.
Yeah. I figured the tests covered that.
+# Turn foo/../bar into bar
+sub _collapse_dotdot {
+my $path = shift;
+
+1 while $path =~ s{/[^/]+/+\.\.}{};
+1 while $path =~ s{[^/]+/+\.\./}{};
+1 while $path =~ s{[^/]+/+\.\.}{};
This is a bug that's gone unnoticed[1] for over 5 years now,
but I've just noticed this doesn' handle foo/..bar or foo/...bar
cases correctly.
Good catch. Woo unit tests! :) You could add them as TODO tests.
A more accurate way to do it would be to split the path, collapse using the
resulting list, and rejoin it.
[1] - I doubt anybody uses paths like these, though...
Not for an svnroot or branch name, no.
--
Hating the web since 1994.
--
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: Fix git-svn for SVN 1.7
On 2012.7.30 1:38 PM, Eric Wong wrote: A better solution would be to have path and URL objects which overload the eq operator and automatically stringify canonicalized and escaped. Perhaps we can depend on the URI.pm module? It seems to be widely-available and not be a significant barrier to installation. On the other hand, I don't know its history, either (especially since we're now dealing with SVN changes...). If you want to go down the road of having CPAN dependencies, then it should definitely be used rather than rolling our own and generating our own bugs. It's a very commonly needed Perl module. You'd make a subclass and put any special work arounds for SVN in there. Anyways, I don't like relying on operator overloading, it makes code harder to read and review. Right now, canonicalization is a bug generator. Paths and URLs have to be in the same form when they're compared. This requires meticulous care on the part of the coder and reviewer to check every comparison. It scatters the logic for proper comparison all over the code. Redundant logic scattered around the code is a Bad Thing. It makes it more likely a coder will forget the logic, or get it wrong, and a human reviewer must be far more vigilant. Right now I'm pretty sure there's still a ton of bugs. It also slows things down. As strings, URLs and paths have to be canonicalized every time they're used or compared. An object representing the URI or path can cache the canonicalization. With string comparison overloaded, you'd no longer have to meticulously check that URLs and paths are always in the same form when they're compared. It just does it. The logic is in one place. We don't even have to care if one of them is a string (or which one), it works even if only one half of the comparison is an object. A new coder to the project doesn't need to know anything special about URIs and paths, they just treat them as strings. Finally, they can be slipped into existing code without having to rewrite everything. Overloaded comparison and stringification can even be used as a tool to find all the places in the code where URLs and paths are being used, where they're being turned into strings, and where URL and path manipulation is being done ad-hoc. For example, if comparison sees one of its arguments as a string, or if concatenation is used. The only downside is when chasing down a bug related to canonicalization one might have to realize that eq is overloaded. But we'd have far less bugs due to canonicalization. So worth it. -- Being faith-based doesn't trump reality. -- Bruce Sterling -- 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: Fix git-svn for SVN 1.7
On 2012.7.30 3:15 PM, Eric Wong wrote: Right now, canonicalization is a bug generator. Paths and URLs have to be in the same form when they're compared. This requires meticulous care on the part of the coder and reviewer to check every comparison. It scatters the logic for proper comparison all over the code. Redundant logic scattered around the code is a Bad Thing. It makes it more likely a coder will forget the logic, or get it wrong, and a human reviewer must be far more vigilant. snip I agree completely with canonicalization. Sorry, I'm not sure what you're agreeing with. The only downside is when chasing down a bug related to canonicalization one might have to realize that eq is overloaded. Having to realize eq is overloaded is a huge downside to me. Presumably you'd be reviewing the change which implements the overloaded objects, so you'd know about it. And it would be documented. I've listed a bunch of concrete positives for using comparison overloaded URI/path objects vs how it's currently being done. How about you voice some of the downsides in concrete terms? Or an alternative that solves the current problems? -- Ahh email, my old friend. Do you know that revenge is a dish that is best served cold? And it is very cold on the Internet! -- 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: Fix git-svn for SVN 1.7
On 2012.7.30 7:18 PM, Eric Wong wrote: Michael G Schwern schw...@pobox.com wrote: On 2012.7.30 3:15 PM, Eric Wong wrote: Right now, canonicalization is a bug generator. Paths and URLs have to be in the same form when they're compared. This requires meticulous care on the part of the coder and reviewer to check every comparison. It scatters the logic for proper comparison all over the code. Redundant logic scattered around the code is a Bad Thing. It makes it more likely a coder will forget the logic, or get it wrong, and a human reviewer must be far more vigilant. snip I agree completely with canonicalization. Sorry, I'm not sure what you're agreeing with. That's it's a bug generator and we shouldn't have redundant logic. Having functions to compare objects themselves is a good thing. That doesn't make it much better than what we have now. One still has to remember to pepper those special comparisons all over the code. The only downside is when chasing down a bug related to canonicalization one might have to realize that eq is overloaded. Having to realize eq is overloaded is a huge downside to me. Presumably you'd be reviewing the change which implements the overloaded objects, so you'd know about it. And it would be documented. The change itself is easy to review. Picking up the code a few months/years down the line and having to know eq is overloaded tends to bite people. Why does a reviewer, or a reader of the code, have to know eq is overloaded? How often would string comparing an overloaded uri/path object be the wrong thing to do? Just about never. Compare that to how often it would be incorrect to string compare a non-overloaded uri/path object. Most of the time. Do you feel it would be otherwise? If they're overloaded, somebody patching the code doesn't have to know to use a special uri_eq() function. It'll just happen when they naturally string compare. The coder doesn't have to know or do anything special. The reviewer doesn't have to do any special work. If they're not overloaded, coders must know about the special URI and path requirements. Each string comparison is suspect and must be scrutinized by the reviewer. They have to think is this actually a uri or path comparison? Should it be using the special comparison functions? Which procedure offers more opportunities for mistakes? I've listed a bunch of concrete positives for using comparison overloaded URI/path objects vs how it's currently being done. How about you voice some of the downsides in concrete terms? Or an alternative that solves the current problems? Any custom comparison function would do the trick (e.g. URI::eq()). I _want_ URI/path objects. I do not want a bare eq operator to obscure the fact it's calling URI::eq() behind-the-scenes. That said, I don't mind overloads when it's obvious an overload is being used (e.g. stringify). It's things like eq which obscure the fact function calls are happening in the background. Is that a problem? If so, why? If the objects stringify, but comparing them as strings is generally the wrong thing to do (even if the object stringifies to the canonical form, you don't know the other side of the operator is an object), isn't that asking for bugs? If the objects are going to act like strings, shouldn't they act like strings completely? Object overloading fails when the encapsulation is incomplete. -- 151. The proper way to report to my Commander is Specialist Schwarz, reporting as ordered, Sir not You can't prove a thing! -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/ -- 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: Fix git-svn for SVN 1.7
It just doesn't matter. Why are we arguing over which solution will be 4% better two years from now, or if my commits are formatted perfectly, when tremendous amounts of basic work to be done improving git-svn? The code is undocumented, lacking unit tests, difficult to understand and riddled with bugs. Either solution would be a vast improvement. The most important thing is that one of them actually gets done. If both solutions offer a huge improvement, do it the way the person actually writing the code wants to do it. It'll be more enjoyable for them, they'll be more likely to complete the work, and more likely to stick around and code some more. -- 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: Fix git-svn for SVN 1.7
On 2012.7.31 1:01 PM, Eric Wong wrote: Michael G Schwern schw...@pobox.com wrote: It just doesn't matter. Why are we arguing over which solution will be 4% better two years from now, or if my commits are formatted perfectly, when tremendous amounts of basic work to be done improving git-svn? The code is undocumented, lacking unit tests, difficult to understand and riddled with bugs. Yes it does matter. git-svn has the problems it has because it traditionally had lower review standards than the rest of git. So yes, we're being more careful nowadays about the long-term ramifications of changes. Yes, review does matter. And so far we've been arguing over whether reviewing objects-with-overloading or objects-without-overloading would be better. And we can argue about that forever. That's the part that doesn't matter. People matter. I think we can all agree that either solution is a vast improvement along multiple axes, including review. So what really matters is making sure one of them gets done. Once either of them is done, we can see how it works out in practice instead of arguing theoretical futures. Once either of them is done, it's much easier to switch to the other. What I'm trying to say is I have much less interest in doing it without the overloading. It's not interesting to me. It's no fun. No fun means no patch. No patch means no improvement. No improvement is the worst of all possible options. I had a lot of enthusiasm for this project when I came in. I like refactoring Perl code. I like git. That's all but sunk at how painful and slow and nit-picking the process has been. We've barely talked about the content of the patches I've submitted, it's all process. This is no fun. We're all volunteers here and we're all getting something personal out of this. Some form of personal enjoyment. I'm not getting that, so I'm unlikely to stick around. -- Defender of Lexical Encapsulation -- 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: Fix git-svn for SVN 1.7
On 2012.7.31 4:05 PM, Junio C Hamano wrote: What I won't accept is maintainability does not matter. It does. I'm sorry, that's not what I intended to convey at all. My reply to Eric lays it out more clearly, I think. -- Reality is that which, when you stop believing in it, doesn't go away. -- Phillip K. Dick -- 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 6/7] Switch path canonicalization to use the SVN API.
On 2012.8.2 2:51 PM, Eric Wong wrote: svn_path_canonicalize() may be accessible in some versions of SVN, but it'll return undef. Yuck! Good catch! I've tested the following on an old CentOS 5.2 chroot with SVN 1.4.2: Looks good to me. -- Alligator sandwich, and make it snappy! -- 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
