From: Junio C Hamano <gitster@pobox•com>
To: Ilya Basin <basinilya@gmail•com>
Cc: Git mailing list <git@vger•kernel.org>,
Ray Chen <rchen@cs•umd.edu>, Eric Wong <normalperson@yhbt•net>
Subject: Re: [PATCH 1/5] git-svn: fix occasional "Failed to strip path" error on fetch next commit, try #3
Date: Tue, 30 Apr 2013 12:10:37 -0700 [thread overview]
Message-ID: <7vobcvdec2.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <5180046b.6905700a.65c8.00b4@mx.google.com> (Ilya Basin's message of "Mon, 29 Apr 2013 00:10:35 +0400")
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
next prev parent reply other threads:[~2013-04-30 19:10 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-28 20:10 [PATCH 1/5] git-svn: fix occasional "Failed to strip path" error on fetch next commit, try #3 Ilya Basin
2013-04-30 19:10 ` Junio C Hamano [this message]
2013-04-30 20:10 ` Re[2]: " Ilya Basin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vobcvdec2.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=basinilya@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=normalperson@yhbt$(echo .)net \
--cc=rchen@cs$(echo .)umd.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox