git-svn failure when symlink added in svn

git-svn failure when symlink added in svn

[Seth Falcon]

Hi,

A few weeks ago I reported a symlink related error with git-svn and
I've now had a chance to track down a few more details.  The trigger
seems to be if a file is removed from svn and then later added as a
symlink.  The error I get is:

  error: git-checkout-index: unable to create symlink foo.txt (Invalid argument)

This is from the call to symlink(new, path) in entry.c and it seems
that new is ''.

Here is a recipe to reproduce:

## First create an svn repository
  svnadmin create SVN123-repos
  svn co file:///Users/seth/temp/SVN123-repos SVN123
  cd SVN123
  echo 123 > foo.txt
  svn add foo.txt 
  svn ci -m "add a file"

## Now mirror using git-svn
  cd ..
  mkdir GIT123
  cd GIT123/
  git svn init file:///Users/seth/temp/SVN123-repos
  git svn fetch

## Next remove and add a file as a symlink
  cd ..
  cd SVN123
  echo 123 > bar.txt
  svn add bar.txt 
  svn ci -m"add bar"
  svn rm foo.txt 
  svn ci -m "remove foo"
  ln -s bar.txt foo.txt
  svn add foo.txt 
  svn ci -m"add foo as symlink"

## Finally, try to rebase
  cd ../GIT123/
  git svn rebase

git version 1.5.1.53.g77e6f
svn 1.4.0


+ seth

Re: git-svn failure when symlink added in svn

[Eric Wong]

Seth Falcon <sethfalcon@gmail.com> wrote:
> A few weeks ago I reported a symlink related error with git-svn and
> I've now had a chance to track down a few more details.  The trigger
> seems to be if a file is removed from svn and then later added as a
> symlink.  The error I get is:
> 
>   error: git-checkout-index: unable to create symlink foo.txt (Invalid argument)
> 
> This is from the call to symlink(new, path) in entry.c and it seems
> that new is ''.

I can't reproduce it on Linux with ext3.  I translated your recipe into
a test script in the patch below.  Anybody familiar with OSX and/or HFS
know if there's a workaround or fix for this?

From: Eric Wong <normalperson@yhbt.net>
Date: Sat, 14 Apr 2007 13:03:24 -0700
Subject: [PATCH] git-svn: add test to handle conversion from file to symlink

This does not trigger any failures on my Linux machine with ext3,
but it may fail on OSX and/or HFS.

This test is based on a bug report by Seth Falcon:
  http://permalink.gmane.org/gmane.comp.version-control.git/44445

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 t/t9112-git-svn-file-to-symlink.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)
 create mode 100755 t/t9112-git-svn-file-to-symlink.sh

diff --git a/t/t9112-git-svn-file-to-symlink.sh b/t/t9112-git-svn-file-to-symlink.sh
new file mode 100755
index 0000000..f94310f
--- /dev/null
+++ b/t/t9112-git-svn-file-to-symlink.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+# Copyright (c) 2007 Eric Wong
+test_description='git-svn file to symlink'
+. ./lib-git-svn.sh
+
+test_expect_success 'create file in svn repository' "
+	svn co '$svnrepo' svn &&
+	cd svn &&
+		echo 123 > foo.txt &&
+		svn add foo.txt &&
+		svn commit -m 'add a file'
+		cd ..
+	"
+
+test_expect_success 'clone with git-svn' "pwd && git svn clone '$svnrepo' git"
+
+test_expect_success 'remove and add file as symlink in svn' "
+	cd svn &&
+		echo 123 > bar.txt &&
+		svn add bar.txt &&
+		svn commit -m 'add bar' &&
+		svn rm foo.txt &&
+		svn commit -m 'remove foo' &&
+		ln -s bar.txt foo.txt &&
+		svn add foo.txt &&
+		svn ci -m 'add foo as symlink'
+		cd ..
+	"
+
+test_expect_success 'rebase in git-svn' "
+	cd git &&
+		git svn rebase
+		cd ..
+	"
+
+test_done
-- 
Eric Wong

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Eric Wong <normalperson@yhbt.net> writes:
> I can't reproduce it on Linux with ext3.  I translated your recipe into
> a test script in the patch below.  Anybody familiar with OSX and/or HFS
> know if there's a workaround or fix for this?

Thanks for sending the test case.  It doesn't properly fail for me on
OSX, but if I run it with -v then I do see the error (so it is failing
on OSX and, as you found, not on Linux).

I added a silly print statement to see the symlink args:

diff --git a/entry.c b/entry.c
index d72f811..70f6402 100644
--- a/entry.c
+++ b/entry.c
@@ -129,6 +129,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct checkout *stat
                                return error("git-checkout-index: unable to write file %s",
                                        path);
                } else {
+                        fprintf(stderr, "symlink: '%s' => '%s'\n", path, new);
                        wrote = symlink(new, path);
                        free(new);
                        if (wrote)

And so then on Linux with -v I get (after snipping most of the
output):

   * expecting success: 
           cd git &&
                   git svn rebase
                   cd ..
   
           A       bar.txt
   r2 = 31e734669e3fe4dbbd375e5a9f5af828a5b7ba92 (git-svn)
           D       foo.txt
   r3 = bd3b318730e8efc77235976abb18d04bc927bf9e (git-svn)
           A       foo.txt
   r4 = 2376eedcfec1de7cbe69b2bbad1c5de231a0ed0d (git-svn)
   First, rewinding head to replay your work on top of it...
   symlink: 'foo.txt' => 'bar.txt'
   HEAD is now at 2376eed... add foo as symlink
   Fast-forwarded master to refs/remotes/git-svn.
   *   ok 4: rebase in git-svn
   
   * passed all 4 test(s)

On my OSX laptop I get:

   * expecting success: 
           cd git &&
                   git svn rebase
                   cd ..
   
           A       bar.txt
   r2 = 4964f302b94ede0301b33faf5f4242c4bab3108b (git-svn)
           D       foo.txt
   r3 = 178a9ff3c7013d4ad8ec7defa93b91a1080c1e53 (git-svn)
           A       foo.txt
   r4 = 9f0bc38df8113fe1e11e47b708589d82bfa035a0 (git-svn)
   First, rewinding head to replay your work on top of it...
   symlink: 'foo.txt' => ''
   error: git-checkout-index: unable to create symlink foo.txt (Invalid argument)
   HEAD is now at 9f0bc38... add foo as symlink
   Fast-forwarded master to refs/remotes/git-svn.
   *   ok 4: rebase in git-svn
   
   * passed all 4 test(s)

If you're still with me, the curious part is what the symlink call is
trying to do.

  Linux:    symlink: 'foo.txt' => 'bar.txt'
    OSX:    symlink: 'foo.txt' => ''

So it looks like the problem is some sort of off-by-one that happens
well before the symlink call.  Perhaps this is enough for someone more
knowledgable than me to have a clue where to look next?

+ seth

Re: git-svn failure when symlink added in svn

[Alexander Klink]

Hi,

Seth Falcon <sethfalcon <at> gmail.com> writes:
> Eric Wong <normalperson <at> yhbt.net> writes:
> > I can't reproduce it on Linux with ext3.  I translated your recipe into
> > a test script in the patch below.  Anybody familiar with OSX and/or HFS
> > know if there's a workaround or fix for this?

I've been investigating this problem too, as it keeps biting me when importing
our (OpenXPKIs) subversion tree using git-svn. I'd love to work with git and
am happy to help with debugging this further. Still, I am a pretty puzzled on why
this happens ...

> And so then on Linux with -v I get (after snipping most of the
> output):
>    First, rewinding head to replay your work on top of it...
>    symlink: 'foo.txt' => 'bar.txt'

> On my OSX laptop I get:
>    First, rewinding head to replay your work on top of it...
>    symlink: 'foo.txt' => ''

Same here (this is a MacBook Pro, for what it's worth, BTW). As said, I've
investigated this a bit further. The empty filename in new seems to come from
trying to read the wrong SHA1 file. If one outputs ce->sha1 before
        void *new = read_sha1_file(ce->sha1, &type, size);
is called, one gets different output on Linux and Mac OS X.
For Seth's example, I get 5f34b0af07646aa529b5b005cde3a9559e606210 on Linux
and e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 on Mac OS X ...

I've tried tracking down where this comes from. Here is what I've learned:
- read_blob_entry() is called from write_entry().
  SHA1 is already incorrect at that point in time.
- write_entry() is called from checkout_entry().
  SHA1 is already incorrect at that point in time.
- checkout_entry() is called from check_updates().
  SHA1 is already incorrect at that point in time.

Unluckily I could not figure out, where it is computed in the first place.
One idea was that maybe it was cached from the old file in the Mac OS X case
and recomputed on Linux or so? Or maybe it's not git's fault but git-svn
messes up (although I doubt it)?

I'll happy try out anything that has a slight chance of solving this issue
(workarounds greatly appreciated, too).

Best regards,
  Alex

Re: git-svn failure when symlink added in svn

[Linus Torvalds]

On Thu, 26 Apr 2007, Alexander Klink wrote:
> 
> Same here (this is a MacBook Pro, for what it's worth, BTW). As said, I've
> investigated this a bit further. The empty filename in new seems to come from
> trying to read the wrong SHA1 file. If one outputs ce->sha1 before
>         void *new = read_sha1_file(ce->sha1, &type, size);
> is called, one gets different output on Linux and Mac OS X.
> For Seth's example, I get 5f34b0af07646aa529b5b005cde3a9559e606210 on Linux
> and e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 on Mac OS X ...

Well, 5f34b0af0 is the "bar.txt" blob, while e69de29b is the empty blob

(Eg do

	[torvalds@woody git]$ echo -en "blob 7\0bar.txt" | sha1sum
	5f34b0af07646aa529b5b005cde3a9559e606210  -

	[torvalds@woody git]$ echo -en "blob 0\0" | sha1sum
	e69de29bb2d1d6434b8b29ae775ad8c2e48c5391  -

to verify: git objects not only contain the data, but embed the object 
type and size too).

So yeah, the printout matches the SHA1's, and the SHA1's are clearly not 
corrupted: they are just a sign of the fact that the data that was fed to 
whoever generated the SHA1's was simply different.

But why git-svn would act differently under OS X than under Linux I have 
no idea.

		Linus

Re: git-svn failure when symlink added in svn

[Alexander Klink]

Linus Torvalds <torvalds <at> linux-foundation.org> writes:

> > trying to read the wrong SHA1 file. If one outputs ce->sha1 before
> >         void *new = read_sha1_file(ce->sha1, &type, size);
> > is called, one gets different output on Linux and Mac OS X.
> > For Seth's example, I get 5f34b0af07646aa529b5b005cde3a9559e606210 on Linux
> > and e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 on Mac OS X ...
> 
> Well, 5f34b0af0 is the "bar.txt" blob, while e69de29b is the empty blob
> 
> So yeah, the printout matches the SHA1's, and the SHA1's are clearly not 
> corrupted: they are just a sign of the fact that the data that was fed to 
> whoever generated the SHA1's was simply different.

The SHA1 hashes are generated in the close_file() function in
git-svn.perl by forking git-hash-object -w --stdin and redirecting
STDIN to the passed filehandle. This is what goes wrong (for some
perl-internal reason?) on Mac OS X. Here is a patch that works
around that by putting the data to be hashed in a temporary file
and calling git-hash-object with a filename.

Best regards,
    Alex

>From 504892b882d05bdf1fbf2325e7544f52115555d1 Mon Sep 17 00:00:00 2001
From: Alexander Klink <ak-git@cynops.de>
Date: Sat, 28 Apr 2007 14:46:27 +0200
Subject: [PATCH] Workaround for git-svn symlink problem on Mac OS X

git-svn had a problem with creating a symlink for a file which existed
as a "real" file beforehand. See the report from Seth Falcon:
http://permalink.gmane.org/gmane.comp.version-control.git/44445
and the test patch by Eric Wong:
http://permalink.gmane.org/gmane.comp.version-control.git/44469

Apparently, the reason for this is that in this case, perl on Mac OS X
does not like the STDIN redirect in close_file() (which forks to
git-hash-object -w --stdin to create the SHA1 hash).
The workaround now creates a temporary file for the git-hash-object input
using File::Temp, calls git-hash-object -w with the filename and safely
unlinks the file afterwards.
---
 git-svn.perl |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 077d6b3..7af45aa 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2266,6 +2266,7 @@ use warnings;
 use Carp qw/croak/;
 use IO::File qw//;
 use Digest::MD5;
+use File::Temp;
 
 # file baton members: path, mode_a, mode_b, pool, fh, blob, base
 sub new {
@@ -2448,13 +2449,16 @@ sub close_file {
 			$buf eq 'link ' or die "$path has mode 120000",
 			                       "but is not a link\n";
 		}
-		defined(my $pid = open my $out,'-|') or die "Can't fork: $!\n";
-		if (!$pid) {
-			open STDIN, '<&', $fh or croak $!;
-			exec qw/git-hash-object -w --stdin/ or croak $!;
-		}
-		chomp($hash = do { local $/; <$out> });
-		close $out or croak $!;
+
+        # put the data for git-hash-object in a temporary file,
+        # as redirecting STDIN does not always work for some reason on
+        # Mac OS X
+        my ($temp_fh, $temp_filename) = mkstemp("git-hash-input-XXXXXX");
+        print $temp_fh do { local $/; <$fh> };
+
+        chomp($hash = qx(git-hash-object -w $temp_filename));
+        File::Temp::unlink0($temp_fh, $temp_filename)
+            or die "Error unlinking temporary file $temp_filename";
 		close $fh or croak $!;
 		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
 		close $fb->{base} or croak $!;
-- 
1.5.2.rc0.34.gda94-dirty

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Hi Alex,

Alexander Klink <ak-git@cynops.de> writes:
> The SHA1 hashes are generated in the close_file() function in
> git-svn.perl by forking git-hash-object -w --stdin and redirecting
> STDIN to the passed filehandle. This is what goes wrong (for some
> perl-internal reason?) on Mac OS X. Here is a patch that works
> around that by putting the data to be hashed in a temporary file
> and calling git-hash-object with a filename.

I tested your patch and it works for me as well.  Eric's test case now
passes and I was able to create a fresh clone of the problem svn
repository that contains the removal + symlink revisions [*1*].  I'm
not an OS X or Perl expert so it isn't obvious to me why the pipe
approach isn't working.

Thanks much,

+ seth

[*1*] I can't seem to fix the git repository where this problem first
appeared for me.  I tried creating a branch starting before the
removal and symlink creation and then running git-svn rebase, but that
didn't work -- maybe because git-svn already stored the commits in
garbled form.  Hence re-cloning was required.

Also: when I cloned the repository, I got a bus error when running git
svn fetch.  Rerunning git svn fetch completed.  Perhaps this is
related to the other bug report that is in progress?  Sorry that I
don't have more details...

Re: git-svn failure when symlink added in svn

[Junio C Hamano]

Is it really the redirection that is the problem?

The process seeks $fh back to the beginning, reads 5 bytes from
it (to ensure that is 'link '), and then forks to feed $fh to
git-hash-object.

Now what do you really want to hash here?  I do not know what
this "file that begins with 'link '" magic is about, but I
suspect that the child may or may not start reading from byte
offset 5 of that file, depending on how the low-level I/O is
tied to Perl.

Here is a little test script to imitate what the part in
close_file sub is doing.  What does it output on MacOS (or
whatever systems that are having the same problem)?

On a Linux box, it appears that it reads the remainder of the
file and the test script says "child says: >>12345", so I am
assuming that is what close_file sub wants to do.  If my
suspicion is correct, you would get "child says: >>link 12345",
in which case sysseek() commented out below would help,
perhaps.

-- >8 --
#!/usr/bin/perl -w

open F, ">footest";
print F "link 12345\n";
close F;

my ($fh, $buf, $n, $pid, $out);

open $fh, "<footest";
seek($fh, 0, 0);
$n = read($fh, $buf, 5);
print "read[$n]: $buf\n";

$pid = open $out , '-|';
if (!$pid) {
	my $at = tell $fh;
	print "child: at $at\n";

	# We may want to do
        #
	#	 sysseek($fh, 5, 0);
        #
        # here.

	open STDIN, '<&', $fh;
	exec qw(sed -e s/^/>>/);
}
while (my $read = <$out>) {
	print "child says: $read";
}
close ($out);
close ($fh);

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Junio C Hamano <junkio@cox.net> writes:

> Is it really the redirection that is the problem?
>
> The process seeks $fh back to the beginning, reads 5 bytes from
> it (to ensure that is 'link '), and then forks to feed $fh to
> git-hash-object.
>
> Now what do you really want to hash here?  I do not know what
> this "file that begins with 'link '" magic is about, but I
> suspect that the child may or may not start reading from byte
> offset 5 of that file, depending on how the low-level I/O is
> tied to Perl.
>
> Here is a little test script to imitate what the part in
> close_file sub is doing.  What does it output on MacOS (or
> whatever systems that are having the same problem)?
>
> On a Linux box, it appears that it reads the remainder of the
> file and the test script says "child says: >>12345", so I am
> assuming that is what close_file sub wants to do.  If my
> suspicion is correct, you would get "child says: >>link 12345",
> in which case sysseek() commented out below would help,
> perhaps.

On OS X, I get:

    ziti:~/temp seth$ ./perltest1.pl 
    read[5]: link 
    child says: child: at 5

And uncommenting the sysseek call, I get:

    ziti:~/temp seth$ ./perltest1.pl 
    read[5]: link 
    child says: child: at 5
    child says: >>12345

+ seth

Re: git-svn failure when symlink added in svn

[Junio C Hamano]

Seth Falcon <sethfalcon@gmail.com> writes:

> On OS X, I get:
>
>     ziti:~/temp seth$ ./perltest1.pl 
>     read[5]: link 
>     child says: child: at 5

Ah, so the previous read($fh, $buf, 5) lets stdio absorb the
whole (short) input, and the underlying seek pointer is not
adjusted back across fork, and the child does not have anything
to read.

> And uncommenting the sysseek call, I get:
>
>     ziti:~/temp seth$ ./perltest1.pl 
>     read[5]: link 
>     child says: child: at 5
>     child says: >>12345

Then I suspect the following could be less invasive and more
efficient fix for the problem.  I do not have an access to MacOS
box, and I do not have a working sync with any SVN repository,
so I cannot test it myself, though...

diff --git a/git-svn.perl b/git-svn.perl
index 7b5f8ab..e487da6 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2454,6 +2454,7 @@ sub close_file {
 		}
 		defined(my $pid = open my $out,'-|') or die "Can't fork: $!\n";
 		if (!$pid) {
+			sysseek($fh, 5, 0);
 			open STDIN, '<&', $fh or croak $!;
 			exec qw/git-hash-object -w --stdin/ or croak $!;
 		}

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Junio C Hamano <junkio@cox.net> writes:

> Seth Falcon <sethfalcon@gmail.com> writes:
>
>> On OS X, I get:
>>
>>     ziti:~/temp seth$ ./perltest1.pl 
>>     read[5]: link 
>>     child says: child: at 5
>
> Ah, so the previous read($fh, $buf, 5) lets stdio absorb the
> whole (short) input, and the underlying seek pointer is not
> adjusted back across fork, and the child does not have anything
> to read.
>
>> And uncommenting the sysseek call, I get:
>>
>>     ziti:~/temp seth$ ./perltest1.pl 
>>     read[5]: link 
>>     child says: child: at 5
>>     child says: >>12345
>
> Then I suspect the following could be less invasive and more
> efficient fix for the problem.  I do not have an access to MacOS
> box, and I do not have a working sync with any SVN repository,
> so I cannot test it myself, though...

This also works as a fix for me on OS X and obviously is nicer than
resorting to temp files.  Again, with this patch against git master
the test case that Eric posted passes as does one of my own examples.

+ seth

Re: git-svn failure when symlink added in svn

[Junio C Hamano]

Seth Falcon <sethfalcon@gmail.com> writes:

> Junio C Hamano <junkio@cox.net> writes:
> ...
>> Then I suspect the following could be less invasive and more
>> efficient fix for the problem.  I do not have an access to MacOS
>> box, and I do not have a working sync with any SVN repository,
>> so I cannot test it myself, though...
>
> This also works as a fix for me on OS X and obviously is nicer than
> resorting to temp files.  Again, with this patch against git master
> the test case that Eric posted passes as does one of my own examples.

Well, I think the sysseek should be done only when we did read
'link ' from the beginning and not in other cases, so in that
sense my patch is very broken.  Probably the sysseek() needs to
be done inside the "if ($fb->mode_b} == 120000)" part, after it
checks for 'link '.

By the way.

I admit I have never given a serious look at the code of
git-svn.perl until now.

It has comparison with 120000 and 100644 all over with ==/!=.
Even though these originally come from parse result of textual
output from ls-tree and diff-tree, and the code never treats
$mode strings as octal integer, I would feel better if the
literals were quoted and comparison done with eq/ne.

Re: git-svn failure when symlink added in svn

[Eric Wong]

Seth Falcon <sethfalcon@gmail.com> wrote:
> Junio C Hamano <junkio@cox.net> writes:
> > Seth Falcon <sethfalcon@gmail.com> writes:
> >> Junio C Hamano <junkio@cox.net> writes:
> >> ...
> >>> Then I suspect the following could be less invasive and more
> >>> efficient fix for the problem.  I do not have an access to MacOS
> >>> box, and I do not have a working sync with any SVN repository,
> >>> so I cannot test it myself, though...
> >>
> >> This also works as a fix for me on OS X and obviously is nicer than
> >> resorting to temp files.  Again, with this patch against git master
> >> the test case that Eric posted passes as does one of my own examples.
> >
> > Well, I think the sysseek should be done only when we did read
> > 'link ' from the beginning and not in other cases, so in that
> > sense my patch is very broken.  Probably the sysseek() needs to
> > be done inside the "if ($fb->mode_b} == 120000)" part, after it
> > checks for 'link '.
> 
> So I can confirm, unfortunately, that the suggested patch is indeed
> very broken and have a couple of corrupt git-svn based repositories to
> show for it -- in other words, I had the opportunity to learn a lesson
> the hard way :-\
> 
> Eric: is there any way to undo some of the svn revs that have been
> retrieved using git-svn fetch and then refetch them?  I naively tried
> out Junio's fix and ran fetch on a few repositories.  The data
> retrieved is bogus in a fun way that things work, but patches have
> been repatched to remove 5 chars, e.g.:
> 
>     -factor <- function (x = character(), levels = sort(unique.default(x),
>     +r <- function (x = character(), levels = sort(unique.default(x),

Assuming you're not using something crazy like noMetadata, you can just
use update-ref on the remote heads to the last known good revisions and
remove the associated .rev_db files.

Otherwise you'll have to delete entries from the .rev_db files, the
format is one line per-revision, the revision is the line number of the
file.

-- 
Eric Wong

Re: git-svn failure when symlink added in svn

[Eric Wong]

Alexander: please don't drop me from the Cc next time, thanks.

Junio C Hamano <junkio@cox.net> wrote:
> Seth Falcon <sethfalcon@gmail.com> writes:
> > Junio C Hamano <junkio@cox.net> writes:
> > ...
> >> Then I suspect the following could be less invasive and more
> >> efficient fix for the problem.  I do not have an access to MacOS
> >> box, and I do not have a working sync with any SVN repository,
> >> so I cannot test it myself, though...
> >
> > This also works as a fix for me on OS X and obviously is nicer than
> > resorting to temp files.  Again, with this patch against git master
> > the test case that Eric posted passes as does one of my own examples.
> 
> Well, I think the sysseek should be done only when we did read
> 'link ' from the beginning and not in other cases, so in that
> sense my patch is very broken.  Probably the sysseek() needs to
> be done inside the "if ($fb->mode_b} == 120000)" part, after it
> checks for 'link '.

Yes, don't add the new sysseek there.  All the reads and seeks in that
block of code should probably be sysreads and sysseeks instead.  Feel
free to patch and test this as I don't have time at the moment.

> By the way.
> 
> I admit I have never given a serious look at the code of
> git-svn.perl until now.
> 
> It has comparison with 120000 and 100644 all over with ==/!=.
> Even though these originally come from parse result of textual
> output from ls-tree and diff-tree, and the code never treats
> $mode strings as octal integer, I would feel better if the
> literals were quoted and comparison done with eq/ne.

It works either way as stringifying those would be unambiguous,
although I understand strings can be considered better style...
Feel free to change this.

-- 
Eric Wong

Re: git-svn failure when symlink added in svn

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Alexander: please don't drop me from the Cc next time, thanks.
>
> Junio C Hamano <junkio@cox.net> wrote:
>> Seth Falcon <sethfalcon@gmail.com> writes:
>> > Junio C Hamano <junkio@cox.net> writes:
>> > ...
>> >> Then I suspect the following could be less invasive and more
>> >> efficient fix for the problem.  I do not have an access to MacOS
>> >> box, and I do not have a working sync with any SVN repository,
>> >> so I cannot test it myself, though...
>> ...
>> 
>> Well, I think the sysseek should be done only when we did read
>> 'link ' from the beginning and not in other cases, so in that
>> sense my patch is very broken.  Probably the sysseek() needs to
>> be done inside the "if ($fb->mode_b} == 120000)" part, after it
>> checks for 'link '.
>
> Yes, don't add the new sysseek there.  All the reads and seeks in that
> block of code should probably be sysreads and sysseeks instead.  Feel
> free to patch and test this as I don't have time at the moment.

Ok.  As I do not have an access to a working sync with an SVN
repository nor a Mac OS box, I cannot test this, but something
like this should be applied to 'maint' before v1.5.1.3.  I've
run testsuite we have including t9XXX series, but that is the
only test I did.

Testing, acks and feedback are very much appreciated.

-- >8 --
Fix symlink handling in git-svn, related to PerlIO

After reading the leading contents from a symlink data obtained
from subversion, which we expect to begin with 'link ', the code
forked to hash the remainder (which should match readlink()
result) using git-hash-objects, by redirecting its STDIN from
the filehandle we read that 'link ' from.  This was Ok with Perl
on modern Linux, but on Mac OS, the read in the parent process
slurped more than we asked for in stdio buffer, and the child
did not correctly see the "remainder".

This attempts to fix the issue by using lower level sysseek and
sysread instead of seek and read to bypass the stdio buffer.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 git-svn.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4be8576..cef6697 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2464,15 +2464,15 @@ sub close_file {
 	my $hash;
 	my $path = $self->git_path($fb->{path});
 	if (my $fh = $fb->{fh}) {
-		seek($fh, 0, 0) or croak $!;
+		sysseek($fh, 0, 0) or croak $!;
 		my $md5 = Digest::MD5->new;
 		$md5->addfile($fh);
 		my $got = $md5->hexdigest;
 		die "Checksum mismatch: $path\n",
 		    "expected: $exp\n    got: $got\n" if ($got ne $exp);
-		seek($fh, 0, 0) or croak $!;
+		sysseek($fh, 0, 0) or croak $!;
 		if ($fb->{mode_b} == 120000) {
-			read($fh, my $buf, 5) == 5 or croak $!;
+			sysread($fh, my $buf, 5) == 5 or croak $!;
 			$buf eq 'link ' or die "$path has mode 120000",
 			                       "but is not a link\n";
 		}

Re: git-svn failure when symlink added in svn

[Eric Wong]

Junio C Hamano <junkio@cox.net> wrote:
> diff --git a/git-svn.perl b/git-svn.perl
> index 4be8576..cef6697 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -2464,15 +2464,15 @@ sub close_file {
>  	my $hash;
>  	my $path = $self->git_path($fb->{path});
>  	if (my $fh = $fb->{fh}) {
> -		seek($fh, 0, 0) or croak $!;
> +		sysseek($fh, 0, 0) or croak $!;
>  		my $md5 = Digest::MD5->new;
>  		$md5->addfile($fh);

We may want to keep the plain seek() here and do both seek and sysseek,
I'm not sure if $md5->addfile() uses read or sysread internally.

-- 
Eric Wong

Re: git-svn failure when symlink added in svn

[Alexander Klink]

Hi,

On Sun, Apr 29, 2007 at 03:21:36PM -0700, Eric Wong wrote:
> >  	my $path = $self->git_path($fb->{path});
> >  	if (my $fh = $fb->{fh}) {
> > -		seek($fh, 0, 0) or croak $!;
> > +		sysseek($fh, 0, 0) or croak $!;
> >  		my $md5 = Digest::MD5->new;
> >  		$md5->addfile($fh);
> 
> We may want to keep the plain seek() here and do both seek and sysseek,
> I'm not sure if $md5->addfile() uses read or sysread internally.
I've just had a quick look: it uses read.
Junio: I'll test the patch tomorrow or the day after tommorow and
let you know whether it works for me. Thanks for the quick fix(es) ...

Best regards,
    Alex
-- 
Dipl.-Math. Alexander Klink | IT-Security Engineer |    a.klink@cynops.de
 mobile: +49 (0)178 2121703 |          Cynops GmbH | http://www.cynops.de
----------------------------+----------------------+---------------------
      HRB 7833, Amtsgericht | USt-Id: DE 213094986 |     Geschäftsführer:
     Bad Homburg v. d. Höhe |                      |      Martin Bartosch

Re: git-svn failure when symlink added in svn

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Junio C Hamano <junkio@cox.net> wrote:
>> diff --git a/git-svn.perl b/git-svn.perl
>> index 4be8576..cef6697 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -2464,15 +2464,15 @@ sub close_file {
>>  	my $hash;
>>  	my $path = $self->git_path($fb->{path});
>>  	if (my $fh = $fb->{fh}) {
>> -		seek($fh, 0, 0) or croak $!;
>> +		sysseek($fh, 0, 0) or croak $!;
>>  		my $md5 = Digest::MD5->new;
>>  		$md5->addfile($fh);
>
> We may want to keep the plain seek() here and do both seek and sysseek,
> I'm not sure if $md5->addfile() uses read or sysread internally.

Ok.  The seek before Digest::MD5 can stay as it has been that
way for a long time without causing problems.  How about this as
an replacement then?

-- >8 --
[PATCH] Fix symlink handling in git-svn, related to PerlIO

After reading the leading contents from a symlink data obtained
from subversion, which we expect to begin with 'link ', the code
forked to hash the remainder (which should match readlink()
result) using git-hash-objects, by redirecting its STDIN from
the filehandle we read that 'link ' from.  This was Ok with Perl
on modern Linux, but on Mac OS, the read in the parent process
slurped more than we asked for in stdio buffer, and the child
did not correctly see the "remainder".

This attempts to fix the issue by using lower level sysseek and
sysread instead of seek and read to bypass the stdio buffer.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 git-svn.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4be8576..6f509f8 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2470,9 +2470,9 @@ sub close_file {
 		my $got = $md5->hexdigest;
 		die "Checksum mismatch: $path\n",
 		    "expected: $exp\n    got: $got\n" if ($got ne $exp);
-		seek($fh, 0, 0) or croak $!;
+		sysseek($fh, 0, 0) or croak $!;
 		if ($fb->{mode_b} == 120000) {
-			read($fh, my $buf, 5) == 5 or croak $!;
+			sysread($fh, my $buf, 5) == 5 or croak $!;
 			$buf eq 'link ' or die "$path has mode 120000",
 			                       "but is not a link\n";
 		}
-- 
1.5.2.rc0.781.g5868

Re: git-svn failure when symlink added in svn

[Eric Wong]

Junio C Hamano <junkio@cox.net> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > Junio C Hamano <junkio@cox.net> wrote:
> >> diff --git a/git-svn.perl b/git-svn.perl
> >> index 4be8576..cef6697 100755
> >> --- a/git-svn.perl
> >> +++ b/git-svn.perl
> >> @@ -2464,15 +2464,15 @@ sub close_file {
> >>  	my $hash;
> >>  	my $path = $self->git_path($fb->{path});
> >>  	if (my $fh = $fb->{fh}) {
> >> -		seek($fh, 0, 0) or croak $!;
> >> +		sysseek($fh, 0, 0) or croak $!;
> >>  		my $md5 = Digest::MD5->new;
> >>  		$md5->addfile($fh);
> >
> > We may want to keep the plain seek() here and do both seek and sysseek,
> > I'm not sure if $md5->addfile() uses read or sysread internally.
> 
> Ok.  The seek before Digest::MD5 can stay as it has been that
> way for a long time without causing problems.  How about this as
> an replacement then?

Looks good to me.  Seth?

If Seth is okay with it, then:
Acked-by: Eric Wong <normalperson@yhbt.net>

> -- >8 --
> [PATCH] Fix symlink handling in git-svn, related to PerlIO
> 
> After reading the leading contents from a symlink data obtained
> from subversion, which we expect to begin with 'link ', the code
> forked to hash the remainder (which should match readlink()
> result) using git-hash-objects, by redirecting its STDIN from
> the filehandle we read that 'link ' from.  This was Ok with Perl
> on modern Linux, but on Mac OS, the read in the parent process
> slurped more than we asked for in stdio buffer, and the child
> did not correctly see the "remainder".
> 
> This attempts to fix the issue by using lower level sysseek and
> sysread instead of seek and read to bypass the stdio buffer.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---
>  git-svn.perl |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 4be8576..6f509f8 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -2470,9 +2470,9 @@ sub close_file {
>  		my $got = $md5->hexdigest;
>  		die "Checksum mismatch: $path\n",
>  		    "expected: $exp\n    got: $got\n" if ($got ne $exp);
> -		seek($fh, 0, 0) or croak $!;
> +		sysseek($fh, 0, 0) or croak $!;
>  		if ($fb->{mode_b} == 120000) {
> -			read($fh, my $buf, 5) == 5 or croak $!;
> +			sysread($fh, my $buf, 5) == 5 or croak $!;
>  			$buf eq 'link ' or die "$path has mode 120000",
>  			                       "but is not a link\n";
>  		}
> -- 
> 1.5.2.rc0.781.g5868

-- 
Eric Wong

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Eric Wong <normalperson@yhbt.net> writes:

> Junio C Hamano <junkio@cox.net> wrote:
>> Eric Wong <normalperson@yhbt.net> writes:
>> 
>> > Junio C Hamano <junkio@cox.net> wrote:
>> >> diff --git a/git-svn.perl b/git-svn.perl
>> >> index 4be8576..cef6697 100755
>> >> --- a/git-svn.perl
>> >> +++ b/git-svn.perl
>> >> @@ -2464,15 +2464,15 @@ sub close_file {
>> >>  	my $hash;
>> >>  	my $path = $self->git_path($fb->{path});
>> >>  	if (my $fh = $fb->{fh}) {
>> >> -		seek($fh, 0, 0) or croak $!;
>> >> +		sysseek($fh, 0, 0) or croak $!;
>> >>  		my $md5 = Digest::MD5->new;
>> >>  		$md5->addfile($fh);
>> >
>> > We may want to keep the plain seek() here and do both seek and sysseek,
>> > I'm not sure if $md5->addfile() uses read or sysread internally.
>> 
>> Ok.  The seek before Digest::MD5 can stay as it has been that
>> way for a long time without causing problems.  How about this as
>> an replacement then?
>
> Looks good to me.  Seth?

The test cases passes as does the small example I had come up with.  I
also tried doing a git svn clone on a small repos and checking that
the resulting HEAD was the same as a previously created one (it was).

> If Seth is okay with it, then:
> Acked-by: Eric Wong <normalperson@yhbt.net>
Acked-by: Seth Falcon <sethfalcon@gmail.com>


+ seth

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Eric Wong <normalperson@yhbt.net> writes:

> Seth Falcon <sethfalcon@gmail.com> wrote:
>> Eric: is there any way to undo some of the svn revs that have been
>> retrieved using git-svn fetch and then refetch them? 

> Assuming you're not using something crazy like noMetadata, you can just
> use update-ref on the remote heads to the last known good revisions and
> remove the associated .rev_db files.
>
> Otherwise you'll have to delete entries from the .rev_db files, the
> format is one line per-revision, the revision is the line number of the
> file.

Hmm, not sure I understood.  Here's what I tried:

I'm tracking two branches via git-svn.  For each, I used git log
remotes/<branch> to find a revision that I expect to be ok and noted
the sha1.  Then I did: 

    git-update-ref remotes/git-svn a27b11c1

and similar, but with different sha1 for the other branch.  Next I
removed the .rev_db* files (there was one for each svn branch) and
tried doing git-svn fetch.  This seemed to rebuild the .rev_db, but
eventually I ended up with:

Done rebuilding .git/svn/git-svn/.rev_db.00db46b3-68df-0310-9c12-caf00c1e9a41
        M       src/<somepath>
        M       src/<another>
Incomplete data: Delta source ended unexpectedly at /Users/seth/scm/bin/git-svn line 2982

And if I rerun git svn fetch, I get:

Index mismatch: 9c07a6009029e4a1d834ff126f705c4db3c4bce7 != 67dc53678f759c52a93a281f13fadb08799f86b2
rereading 0f12c8c092600c8a3337ec35d153d3a76ce2329d
        M       src/<somepath>
        M       src/<another>
Incomplete data: Delta source ended unexpectedly at /Users/seth/scm/bin/git-svn line 2982


[where <somepath> and <another> are the same in both cases]

Did I miss a step or misunderstand how to undo?  What's strange is
that if I do git show 0f12c8c, I see a patch that is looks like it came
from a fetch using the my broken version of git-svn -- do I need to
clear out objects before refetching?

Thanks,

+ seth

Re: git-svn failure when symlink added in svn

[Eric Wong]

Seth Falcon <sethfalcon@gmail.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > Seth Falcon <sethfalcon@gmail.com> wrote:
> >> Eric: is there any way to undo some of the svn revs that have been
> >> retrieved using git-svn fetch and then refetch them? 
> 
> > Assuming you're not using something crazy like noMetadata, you can just
> > use update-ref on the remote heads to the last known good revisions and
> > remove the associated .rev_db files.
> >
> > Otherwise you'll have to delete entries from the .rev_db files, the
> > format is one line per-revision, the revision is the line number of the
> > file.
> 
> Hmm, not sure I understood.  Here's what I tried:
> 
> I'm tracking two branches via git-svn.  For each, I used git log
> remotes/<branch> to find a revision that I expect to be ok and noted
> the sha1.  Then I did: 
> 
>     git-update-ref remotes/git-svn a27b11c1

You may need to specify "refs/": "refs/remotes/git-svn".
Is there a .git/remotes/git-svn ref file now?

> and similar, but with different sha1 for the other branch.  Next I
> removed the .rev_db* files (there was one for each svn branch) and
> tried doing git-svn fetch.  This seemed to rebuild the .rev_db, but
> eventually I ended up with:
> 
> Done rebuilding .git/svn/git-svn/.rev_db.00db46b3-68df-0310-9c12-caf00c1e9a41
>         M       src/<somepath>
>         M       src/<another>
> Incomplete data: Delta source ended unexpectedly at /Users/seth/scm/bin/git-svn line 2982
> 
> And if I rerun git svn fetch, I get:
> 
> Index mismatch: 9c07a6009029e4a1d834ff126f705c4db3c4bce7 != 67dc53678f759c52a93a281f13fadb08799f86b2
> rereading 0f12c8c092600c8a3337ec35d153d3a76ce2329d
>         M       src/<somepath>
>         M       src/<another>
> Incomplete data: Delta source ended unexpectedly at /Users/seth/scm/bin/git-svn line 2982
> 
> 
> [where <somepath> and <another> are the same in both cases]
> 
> Did I miss a step or misunderstand how to undo?  What's strange is
> that if I do git show 0f12c8c, I see a patch that is looks like it came
> from a fetch using the my broken version of git-svn -- do I need to
> clear out objects before refetching?

I might have left some steps (I've been all over the place lately :/).
You probably need to do all that and also need to edit
.git/svn/.metadata and set the {branches,tags}-maxRev fields to the last
known good revisions if you use globs.

Also you can try removing the index files inside .git/svn (but they
*should* be auto-checked and rebuilt (as they were in the second
"git-svn fetch" run you did...

-- 
Eric Wong

Re: git-svn failure when symlink added in svn

[Seth Falcon]

Eric Wong <normalperson@yhbt.net> writes:

> Seth Falcon <sethfalcon@gmail.com> wrote:
>> Eric Wong <normalperson@yhbt.net> writes:
>> 
>> > Seth Falcon <sethfalcon@gmail.com> wrote:
>> >> Eric: is there any way to undo some of the svn revs that have been
>> >> retrieved using git-svn fetch and then refetch them? 
>> 
>> > Assuming you're not using something crazy like noMetadata, you can just
>> > use update-ref on the remote heads to the last known good revisions and
>> > remove the associated .rev_db files.
>> >
>> > Otherwise you'll have to delete entries from the .rev_db files, the
>> > format is one line per-revision, the revision is the line number of the
>> > file.
>> 
>> Hmm, not sure I understood.  Here's what I tried:
>> 
>> I'm tracking two branches via git-svn.  For each, I used git log
>> remotes/<branch> to find a revision that I expect to be ok and noted
>> the sha1.  Then I did: 
>> 
>>     git-update-ref remotes/git-svn a27b11c1
>
> You may need to specify "refs/": "refs/remotes/git-svn".
> Is there a .git/remotes/git-svn ref file now?

Yes.  I removed those and redid the git-update-ref specifying
refs/remotes/git-svn and the branch I have.

>> Did I miss a step or misunderstand how to undo?  What's strange is
>> that if I do git show 0f12c8c, I see a patch that is looks like it came
>> from a fetch using the my broken version of git-svn -- do I need to
>> clear out objects before refetching?
>
> I might have left some steps (I've been all over the place lately :/).
> You probably need to do all that and also need to edit
> .git/svn/.metadata and set the {branches,tags}-maxRev fields to the last
> known good revisions if you use globs.

I ran git-gc --prune.  I also took a look at .git/svn/.metadata, but
all I have there is:

    ; This file is used internally by git-svn
    ; You should not have to edit it
    [svn-remote "svn"]
            uuid = 00db46b3-68df-0310-9c12-caf00c1e9a41

So I left that alone.  I tried refetching and end up with the
following error after the rev dbs were rebuilt:

    error: invalid object 67e31e0ada47e8e9d15547ff1a48298869b3907b
    fatal: git-write-tree: error building trees
    write-tree: command returned error: 128

I found a backup of this repository and will use that.  Since I have a
backup, it isn't worth the effort.  I think the obvious lesson is to
use a copy of a repos when testing git-svn so you can throw it away if
things go awry.

+ seth

Re: git-svn failure when symlink added in svn

[Alexander Klink]

On Sun, Apr 29, 2007 at 10:08:26PM -0700, Junio C Hamano wrote:
> -- >8 --
> [PATCH] Fix symlink handling in git-svn, related to PerlIO
> 
[...]
> This attempts to fix the issue by using lower level sysseek and
> sysread instead of seek and read to bypass the stdio buffer.
Works fine here, too. Thanks again for the quick response ...

Regards,
    Alex