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

[Junio C Hamano <gitster@pobox.com>]

Ilya Basin <basinilya@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-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