{"thread":{"id":"33683","subject":"[PATCH 1/5] git-svn: fix occasional \"Failed to strip path\" error on fetch next commit, try #3","startedAt":"2013-04-28T20:10:35Z","lastAt":"2013-04-30T20:10:28Z","messageCount":3,"participants":["Ilya Basin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"216022","messageId":"5180046b.6905700a.65c8.00b4@mx.google.com","threadId":"33683","inReplyTo":null,"subject":"[PATCH 1/5] git-svn: fix occasional \"Failed to strip path\" error on fetch next commit, try #3","fromName":"Ilya Basin","fromEmail":"basinilya@gmail.com","sentAt":"2013-04-28T20:10:35Z","receivedAt":"2013-04-28T20:10:35Z","isPatch":true,"sender":{"key":"basinilya@gmail.com","avatar":null},"body":"When --stdlayout and --preserve-empty-dirs flags are used and a\ndirectory becomes empty, two things happen:\n\nSometimes find_empty_directories() returns empty list and no empty dir\nplaceholder file created. This happens, because find_empty_directories()\nmarks all directories as non-empty, if at least one updated directory is\nnon-empty.\n\nSometimes git-svn dies with \"Failed to strip path\" error. This happens,\nbecause find_empty_directories() returns git paths and\nadd_placeholder_file() expects svn paths\n---\n perl/Git/SVN/Fetcher.pm                | 12 ++++++++----\n t/t9160-git-svn-preserve-empty-dirs.sh | 18 +++++++++++++-----\n 2 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 046a7a2..4f96076 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -129,6 +129,7 @@ sub is_path_ignored {\n \n sub set_path_strip {\n \tmy ($self, $path) = @_;\n+\t$self->{pathprefix_strip} = length $path ? ($path . \"/\") : \"\";\n \t$self->{path_strip} = qr/^\\Q$path\\E(\\/|$)/ if length $path;\n }\n \n@@ -458,9 +459,12 @@ sub find_empty_directories {\n \t\tmy $skip_added = 0;\n \t\tforeach my $t (qw/dir_prop file_prop/) {\n \t\t\tforeach my $path (keys %{ $self->{$t} }) {\n-\t\t\t\tif (exists $self->{$t}->{dirname($path)}) {\n-\t\t\t\t\t$skip_added = 1;\n-\t\t\t\t\tlast;\n+\t\t\t\tif (length $self->git_path($path)) {\n+\t\t\t\t\t$path = dirname($path);\n+\t\t\t\t\tif ($dir eq $self->git_path($path) && exists $self->{$t}->{$path}) {\n+\t\t\t\t\t\t$skip_added = 1;\n+\t\t\t\t\t\tlast;\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t}\n \t\t\tlast if $skip_added;\n@@ -477,7 +481,7 @@ sub find_empty_directories {\n \t\tdelete $files{$_} foreach (@deleted_gpath);\n \n \t\t# Report the directory if there are no filenames left.\n-\t\tpush @empty_dirs, $dir unless (scalar %files);\n+\t\tpush @empty_dirs, ($self->{pathprefix_strip} . $dir) unless (scalar %files);\n \t}\n \t@empty_dirs;\n }\ndiff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh\nindex b4a4434..1b5a286 100755\n--- a/t/t9160-git-svn-preserve-empty-dirs.sh\n+++ b/t/t9160-git-svn-preserve-empty-dirs.sh\n@@ -51,13 +51,21 @@ test_expect_success 'initialize source svn repo containing empty dirs' '\n \t\techo \"Conflict file\" > 5/.placeholder &&\n \t\tmkdir 6/.placeholder &&\n \t\tsvn_cmd add 5/.placeholder 6/.placeholder &&\n-\t\tsvn_cmd commit -m \"Placeholder Namespace conflict\"\n+\t\tsvn_cmd commit -m \"Placeholder Namespace conflict\" &&\n+\n+\t\techo x > fil.txt &&\n+\t\tsvn_cmd add fil.txt &&\n+\t\tsvn_cmd commit -m \"this commit should not kill git-svn\"\n \t) &&\n \trm -rf \"$SVN_TREE\"\n '\n \n-test_expect_success 'clone svn repo with --preserve-empty-dirs' '\n-\tgit svn clone \"$svnrepo\"/trunk --preserve-empty-dirs \"$GIT_REPO\"\n+test_expect_success 'clone svn repo with --preserve-empty-dirs --stdlayout' '\n+\tgit svn clone \"$svnrepo\" --preserve-empty-dirs --stdlayout \"$GIT_REPO\" || (\n+\t\tcd \"$GIT_REPO\"\n+\t\tgit svn fetch # fetch the rest can succeed even if clone failed\n+\t\tfalse # this test still failed\n+\t)\n '\n \n # \"$GIT_REPO\"/1 should only contain the placeholder file.\n@@ -81,11 +89,11 @@ test_expect_success 'add entry to previously empty directory' '\n \ttest -f \"$GIT_REPO\"/4/a/b/c/foo\n '\n \n-# The HEAD~2 commit should not have introduced .gitignore placeholder files.\n+# The HEAD~3 commit should not have introduced .gitignore placeholder files.\n test_expect_success 'remove non-last entry from directory' '\n \t(\n \t\tcd \"$GIT_REPO\" &&\n-\t\tgit checkout HEAD~2\n+\t\tgit checkout HEAD~3\n \t) &&\n \ttest_must_fail test -f \"$GIT_REPO\"/2/.gitignore &&\n \ttest_must_fail test -f \"$GIT_REPO\"/3/.gitignore\n-- \n1.8.1.5\n"},{"id":"216045","messageId":"7vobcvdec2.fsf@alter.siamese.dyndns.org","threadId":"33683","inReplyTo":"5180046b.6905700a.65c8.00b4@mx.google.com","subject":"Re: [PATCH 1/5] git-svn: fix occasional \"Failed to strip path\" error on fetch next commit, try #3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-30T19:10:37Z","receivedAt":"2013-04-30T19:10:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilya Basin <basinilya@gmail.com> writes:\n\n    [PATCH 1/5] git-svn: fix occasional \"Failed to strip path\" error on fetch next commit, try #3\n\nPlease make it like this.\n\n    [PATCH v3 1/5] git-svn: fix occasional \"Failed to strip path\" error on fetch next commit\n\n    > When --stdlayout and --preserve-empty-dirs flags are used and a\n    > directory becomes empty, two things happen:\n    >\n    > Sometimes find_empty_directories() returns empty list and no empty dir\n    > placeholder file created. This happens, because find_empty_directories()\n    > marks all directories as non-empty, if at least one updated directory is\n    > non-empty.\n    >\n    > Sometimes git-svn dies with \"Failed to strip path\" error. This happens,\n    > because find_empty_directories() returns git paths and\n    > add_placeholder_file() expects svn paths\n\nEnumeration is easier to read if you did\n\n    ... two things happen:\n\n      * Thing one.\n\n      * Thing two.\n\nThe above is a good description of the problem and your diagnosis,\nand readers may be able to guess a few approaches to fix them.\n\nHere, after the description of the problem and before the three-dash\nline, is the place to summarize the approach you took to fix it,\nfollowed by an empty line followed by your Signed-off-by: line.\n\n    > ---\n\nHere is a place to summarize what changed since the earlier\niterations of the patch you sent (a single liner e.g. \"with better\nlog messages\", \"corrected an off-by-one error in function X in the\nprevious round\", is often sufficient).\n\n    >  perl/Git/SVN/Fetcher.pm                | 12 ++++++++----\n    >  t/t9160-git-svn-preserve-empty-dirs.sh | 18 +++++++++++++-----\n    >  2 files changed, 21 insertions(+), 9 deletions(-)\n\n>\n> diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\n> index 046a7a2..4f96076 100644\n> --- a/perl/Git/SVN/Fetcher.pm\n> +++ b/perl/Git/SVN/Fetcher.pm\n> @@ -129,6 +129,7 @@ sub is_path_ignored {\n>  \n>  sub set_path_strip {\n>  \tmy ($self, $path) = @_;\n> +\t$self->{pathprefix_strip} = length $path ? ($path . \"/\") : \"\";\n>  \t$self->{path_strip} = qr/^\\Q$path\\E(\\/|$)/ if length $path;\n\nThe name of the field (should I call it an instance variable?) feels\nsomewhat strange.  This is later used to be _added_ as a prefix to\nthe files in the directory denoted by the $path. The only thing this\nis related to \"strip\" is because you have to prefix it because these\nfiles have their prefix stripped earlier, no?\n\n>  }\n>  \n> @@ -458,9 +459,12 @@ sub find_empty_directories {\n>  \t\tmy $skip_added = 0;\n>  \t\tforeach my $t (qw/dir_prop file_prop/) {\n>  \t\t\tforeach my $path (keys %{ $self->{$t} }) {\n> -\t\t\t\tif (exists $self->{$t}->{dirname($path)}) {\n> -\t\t\t\t\t$skip_added = 1;\n> -\t\t\t\t\tlast;\n> +\t\t\t\tif (length $self->git_path($path)) {\n> +\t\t\t\t\t$path = dirname($path);\n> +\t\t\t\t\tif ($dir eq $self->git_path($path) && exists $self->{$t}->{$path}) {\n> +\t\t\t\t\t\t$skip_added = 1;\n> +\t\t\t\t\t\tlast;\n> +\t\t\t\t\t}\n\nI am reading that this is a solution for your second issue (use\ngit_path() to convert $path).  An empty $path would be a top-level\nand skipping it corresponds to the \"next if $dir eq '.'\" at the\nbeginning of the loop, I guess.\n\nWhen \"$dir ne $self->git_path(dirname($path))\", what should happen?\n\n>  \t\t\t\t}\n>  \t\t\t}\n>  \t\t\tlast if $skip_added;\n> @@ -477,7 +481,7 @@ sub find_empty_directories {\n>  \t\tdelete $files{$_} foreach (@deleted_gpath);\n>  \n>  \t\t# Report the directory if there are no filenames left.\n> -\t\tpush @empty_dirs, $dir unless (scalar %files);\n> +\t\tpush @empty_dirs, ($self->{pathprefix_strip} . $dir) unless (scalar %files);\n\nThis makes me think \"path_prefix\" would be a better name.\n\n>  \t}\n>  \t@empty_dirs;\n>  }\n> diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh\n> index b4a4434..1b5a286 100755\n> --- a/t/t9160-git-svn-preserve-empty-dirs.sh\n> +++ b/t/t9160-git-svn-preserve-empty-dirs.sh\n> @@ -51,13 +51,21 @@ test_expect_success 'initialize source svn repo containing empty dirs' '\n>  \t\techo \"Conflict file\" > 5/.placeholder &&\n>  \t\tmkdir 6/.placeholder &&\n>  \t\tsvn_cmd add 5/.placeholder 6/.placeholder &&\n> -\t\tsvn_cmd commit -m \"Placeholder Namespace conflict\"\n> +\t\tsvn_cmd commit -m \"Placeholder Namespace conflict\" &&\n> +\n> +\t\techo x > fil.txt &&\n\nNot a new problem but we prefer to write this as\n\n\t\techo x >fil.txt &&\n\nThat is, a SP before a redirection operator, but no SP before the\nredirection target.\n\n> +\t\tsvn_cmd add fil.txt &&\n> +\t\tsvn_cmd commit -m \"this commit should not kill git-svn\"\n>  \t) &&\n>  \trm -rf \"$SVN_TREE\"\n>  '\n>  \n> -test_expect_success 'clone svn repo with --preserve-empty-dirs' '\n> -\tgit svn clone \"$svnrepo\"/trunk --preserve-empty-dirs \"$GIT_REPO\"\n> +test_expect_success 'clone svn repo with --preserve-empty-dirs --stdlayout' '\n> +\tgit svn clone \"$svnrepo\" --preserve-empty-dirs --stdlayout \"$GIT_REPO\" || (\n> +\t\tcd \"$GIT_REPO\"\n> +\t\tgit svn fetch # fetch the rest can succeed even if clone failed\n> +\t\tfalse # this test still failed\n> +\t)\n>  '\n>  \n>  # \"$GIT_REPO\"/1 should only contain the placeholder file.\n> @@ -81,11 +89,11 @@ test_expect_success 'add entry to previously empty directory' '\n>  \ttest -f \"$GIT_REPO\"/4/a/b/c/foo\n>  '\n>  \n> -# The HEAD~2 commit should not have introduced .gitignore placeholder files.\n> +# The HEAD~3 commit should not have introduced .gitignore placeholder files.\n>  test_expect_success 'remove non-last entry from directory' '\n>  \t(\n>  \t\tcd \"$GIT_REPO\" &&\n> -\t\tgit checkout HEAD~2\n> +\t\tgit checkout HEAD~3\n>  \t) &&\n>  \ttest_must_fail test -f \"$GIT_REPO\"/2/.gitignore &&\n>  \ttest_must_fail test -f \"$GIT_REPO\"/3/.gitignore\n"},{"id":"216055","messageId":"1285665433.20130501001028@gmail.com","threadId":"33683","inReplyTo":"7vobcvdec2.fsf@alter.siamese.dyndns.org","subject":"Re[2]: [PATCH 1/5] git-svn: fix occasional \"Failed to strip path\" error on fetch next commit, try #3","fromName":"Ilya Basin","fromEmail":"basinilya@gmail.com","sentAt":"2013-04-30T20:10:28Z","receivedAt":"2013-04-30T20:10:28Z","isPatch":true,"sender":{"key":"basinilya@gmail.com","avatar":null},"body":">>  }\n>>  \n>> @@ -458,9 +459,12 @@ sub find_empty_directories {\n>>               my $skip_added = 0;\n>>               foreach my $t (qw/dir_prop file_prop/) {\n>>                       foreach my $path (keys %{ $self->{$t} }) {\n>> -                             if (exists $self->{$t}->{dirname($path)}) {\n>> -                                     $skip_added = 1;\n>> -                                     last;\n>> +                             if (length $self->git_path($path)) {\n>> +                                     $path = dirname($path);\n>> +                                     if ($dir eq $self->git_path($path) && exists $self->{$t}->{$path}) {\n>> +                                             $skip_added = 1;\n>> +                                             last;\n>> +                                     }\n\nJCH> I am reading that this is a solution for your second issue (use\nJCH> git_path() to convert $path).  An empty $path would be a top-level\nJCH> and skipping it corresponds to the \"next if $dir eq '.'\" at the\nJCH> beginning of the loop, I guess.\n\nJCH> When \"$dir ne $self->git_path(dirname($path))\", what should happen?\n\n'ls-tree' will be executed.\nI guess, the original idea was to save processes, although I don't\nknow why the dir is in @deleted_gpath, if it has children.\n"}]}