[PATCH 1/5] git-svn: fix occasional Failed to strip path error on fetch next commit, try #3

2013-04-30 Thread Ilya Basin
When --stdlayout and --preserve-empty-dirs flags are used and a
directory becomes empty, two things happen:

Sometimes find_empty_directories() returns empty list and no empty dir
placeholder file created. This happens, because find_empty_directories()
marks all directories as non-empty, if at least one updated directory is
non-empty.

Sometimes git-svn dies with Failed to strip path error. This happens,
because find_empty_directories() returns git paths and
add_placeholder_file() expects svn paths
---
 perl/Git/SVN/Fetcher.pm| 12 
 t/t9160-git-svn-preserve-empty-dirs.sh | 18 +-
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 046a7a2..4f96076 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -129,6 +129,7 @@ sub is_path_ignored {
 
 sub set_path_strip {
my ($self, $path) = @_;
+   $self-{pathprefix_strip} = length $path ? ($path . /) : ;
$self-{path_strip} = qr/^\Q$path\E(\/|$)/ if length $path;
 }
 
@@ -458,9 +459,12 @@ sub find_empty_directories {
my $skip_added = 0;
foreach my $t (qw/dir_prop file_prop/) {
foreach my $path (keys %{ $self-{$t} }) {
-   if (exists $self-{$t}-{dirname($path)}) {
-   $skip_added = 1;
-   last;
+   if (length $self-git_path($path)) {
+   $path = dirname($path);
+   if ($dir eq $self-git_path($path)  
exists $self-{$t}-{$path}) {
+   $skip_added = 1;
+   last;
+   }
}
}
last if $skip_added;
@@ -477,7 +481,7 @@ sub find_empty_directories {
delete $files{$_} foreach (@deleted_gpath);
 
# Report the directory if there are no filenames left.
-   push @empty_dirs, $dir unless (scalar %files);
+   push @empty_dirs, ($self-{pathprefix_strip} . $dir) unless 
(scalar %files);
}
@empty_dirs;
 }
diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh 
b/t/t9160-git-svn-preserve-empty-dirs.sh
index b4a4434..1b5a286 100755
--- a/t/t9160-git-svn-preserve-empty-dirs.sh
+++ b/t/t9160-git-svn-preserve-empty-dirs.sh
@@ -51,13 +51,21 @@ test_expect_success 'initialize source svn repo containing 
empty dirs' '
echo Conflict file  5/.placeholder 
mkdir 6/.placeholder 
svn_cmd add 5/.placeholder 6/.placeholder 
-   svn_cmd commit -m Placeholder Namespace conflict
+   svn_cmd commit -m Placeholder Namespace conflict 
+
+   echo x  fil.txt 
+   svn_cmd add fil.txt 
+   svn_cmd commit -m this commit should not kill git-svn
) 
rm -rf $SVN_TREE
 '
 
-test_expect_success 'clone svn repo with --preserve-empty-dirs' '
-   git svn clone $svnrepo/trunk --preserve-empty-dirs $GIT_REPO
+test_expect_success 'clone svn repo with --preserve-empty-dirs --stdlayout' '
+   git svn clone $svnrepo --preserve-empty-dirs --stdlayout $GIT_REPO 
|| (
+   cd $GIT_REPO
+   git svn fetch # fetch the rest can succeed even if clone failed
+   false # this test still failed
+   )
 '
 
 # $GIT_REPO/1 should only contain the placeholder file.
@@ -81,11 +89,11 @@ test_expect_success 'add entry to previously empty 
directory' '
test -f $GIT_REPO/4/a/b/c/foo
 '
 
-# The HEAD~2 commit should not have introduced .gitignore placeholder files.
+# The HEAD~3 commit should not have introduced .gitignore placeholder files.
 test_expect_success 'remove non-last entry from directory' '
(
cd $GIT_REPO 
-   git checkout HEAD~2
+   git checkout HEAD~3
) 
test_must_fail test -f $GIT_REPO/2/.gitignore 
test_must_fail test -f $GIT_REPO/3/.gitignore
-- 
1.8.1.5


--
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/5] git-svn: fix occasional Failed to strip path error on fetch next commit, try #3

2013-04-30 Thread Junio C Hamano
Ilya Basin basini...@gmail.com writes:

[PATCH 1/5] git-svn: fix occasional Failed to strip path error on fetch 
next commit, try #3

Please make it like this.

[PATCH v3 1/5] git-svn: fix occasional Failed to strip path error on 
fetch next commit

 When --stdlayout and --preserve-empty-dirs flags are used and a
 directory becomes empty, two things happen:

 Sometimes find_empty_directories() returns empty list and no empty dir
 placeholder file created. This happens, because find_empty_directories()
 marks all directories as non-empty, if at least one updated directory is
 non-empty.

 Sometimes git-svn dies with Failed to strip path error. This happens,
 because find_empty_directories() returns git paths and
 add_placeholder_file() expects svn paths

Enumeration is easier to read if you did

... two things happen:

  * Thing one.

  * Thing two.

The above is a good description of the problem and your diagnosis,
and readers may be able to guess a few approaches to fix them.

Here, after the description of the problem and before the three-dash
line, is the place to summarize the approach you took to fix it,
followed by an empty line followed by your Signed-off-by: line.

 ---

Here is a place to summarize what changed since the earlier
iterations of the patch you sent (a single liner e.g. with better
log messages, corrected an off-by-one error in function X in the
previous round, is often sufficient).

  perl/Git/SVN/Fetcher.pm| 12 
  t/t9160-git-svn-preserve-empty-dirs.sh | 18 +-
  2 files changed, 21 insertions(+), 9 deletions(-)


 diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
 index 046a7a2..4f96076 100644
 --- a/perl/Git/SVN/Fetcher.pm
 +++ b/perl/Git/SVN/Fetcher.pm
 @@ -129,6 +129,7 @@ sub is_path_ignored {
  
  sub set_path_strip {
   my ($self, $path) = @_;
 + $self-{pathprefix_strip} = length $path ? ($path . /) : ;
   $self-{path_strip} = qr/^\Q$path\E(\/|$)/ if length $path;

The name of the field (should I call it an instance variable?) feels
somewhat strange.  This is later used to be _added_ as a prefix to
the files in the directory denoted by the $path. The only thing this
is related to strip is because you have to prefix it because these
files have their prefix stripped earlier, no?

  }
  
 @@ -458,9 +459,12 @@ sub find_empty_directories {
   my $skip_added = 0;
   foreach my $t (qw/dir_prop file_prop/) {
   foreach my $path (keys %{ $self-{$t} }) {
 - if (exists $self-{$t}-{dirname($path)}) {
 - $skip_added = 1;
 - last;
 + if (length $self-git_path($path)) {
 + $path = dirname($path);
 + if ($dir eq $self-git_path($path)  
 exists $self-{$t}-{$path}) {
 + $skip_added = 1;
 + last;
 + }

I am reading that this is a solution for your second issue (use
git_path() to convert $path).  An empty $path would be a top-level
and skipping it corresponds to the next if $dir eq '.' at the
beginning of the loop, I guess.

When $dir ne $self-git_path(dirname($path)), what should happen?

   }
   }
   last if $skip_added;
 @@ -477,7 +481,7 @@ sub find_empty_directories {
   delete $files{$_} foreach (@deleted_gpath);
  
   # Report the directory if there are no filenames left.
 - push @empty_dirs, $dir unless (scalar %files);
 + push @empty_dirs, ($self-{pathprefix_strip} . $dir) unless 
 (scalar %files);

This makes me think path_prefix would be a better name.

   }
   @empty_dirs;
  }
 diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh 
 b/t/t9160-git-svn-preserve-empty-dirs.sh
 index b4a4434..1b5a286 100755
 --- a/t/t9160-git-svn-preserve-empty-dirs.sh
 +++ b/t/t9160-git-svn-preserve-empty-dirs.sh
 @@ -51,13 +51,21 @@ test_expect_success 'initialize source svn repo 
 containing empty dirs' '
   echo Conflict file  5/.placeholder 
   mkdir 6/.placeholder 
   svn_cmd add 5/.placeholder 6/.placeholder 
 - svn_cmd commit -m Placeholder Namespace conflict
 + svn_cmd commit -m Placeholder Namespace conflict 
 +
 + echo x  fil.txt 

Not a new problem but we prefer to write this as

echo x fil.txt 

That is, a SP before a redirection operator, but no SP before the
redirection target.

 + svn_cmd add fil.txt 
 + svn_cmd commit -m this commit should not kill git-svn
   ) 
   rm -rf $SVN_TREE
  '
  
 -test_expect_success 'clone svn repo with --preserve

Re[2]: [PATCH 1/5] git-svn: fix occasional Failed to strip path error on fetch next commit, try #3

2013-04-30 Thread Ilya Basin
  }
  
 @@ -458,9 +459,12 @@ sub find_empty_directories {
   my $skip_added = 0;
   foreach my $t (qw/dir_prop file_prop/) {
   foreach my $path (keys %{ $self-{$t} }) {
 - if (exists $self-{$t}-{dirname($path)}) {
 - $skip_added = 1;
 - last;
 + if (length $self-git_path($path)) {
 + $path = dirname($path);
 + if ($dir eq $self-git_path($path)  
 exists $self-{$t}-{$path}) {
 + $skip_added = 1;
 + last;
 + }

JCH I am reading that this is a solution for your second issue (use
JCH git_path() to convert $path).  An empty $path would be a top-level
JCH and skipping it corresponds to the next if $dir eq '.' at the
JCH beginning of the loop, I guess.

JCH When $dir ne $self-git_path(dirname($path)), what should happen?

'ls-tree' will be executed.
I guess, the original idea was to save processes, although I don't
know why the dir is in @deleted_gpath, if it has children.


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