git-svn dcommit fails (connection closed unexpectedly)

Hi,

I'm using git-svn, which usually works fine, but I occasionally get this:

$ git-svn dcommit           
        A       file1
        A       file2
Network connection closed unexpectedly: Connection closed unexpectedly at /path/to/git-svn line 401
$

The failure seems to depend on the commit's I'm pushing, since this is reproducible when running several times "dcommit" for the same commit, but the same command also usually works fine on the same repositories (same git repo, same svn target).

The svn repository is accessed with svn+ssh://user@machine/path/.

exporting the patch, applying it to an svn checkout, and "svn commit" works fine.

Let me know if I can provide any other usefull information for debugging.

Thanks,

Debian etch (stable)
Reproducible with both git 1.5.1.1 and the latest from git:

$ git-svn --version
git-svn version 1.5.2.rc3.3.ge347 (svn 1.4.2)

Matthieu

Is this happening if you dcommit a file in a new directory?

The reason I'm asking is that I see the same problem in that situation, but my svn repo is on a windows box, accessed over svn://url, so I just assumed it was a problem on the remote end (the service dies). But the problem always goes away if I commit the new directory using the svn client before doing git-svn dcommit again....

--
larsh

Hmm, your MUA fooled me to reply to the wrong people...

Can you please don't do this:

Mail-Followup-To: git <[email protected]>, Eric Wong <[email protected]>

--
larsh

There seem to be a correlation, yes. Usually, simple commits will work fine, and when I do something complex with the filesystem, it breaks. I don't remember all the cases where it broke, but at least, last time, it was about a new file in a new directory, yes.

Matthieu

That's it indeed.

The attached script seems to reproducibly trigger the problem on my box:

$ ../git-svn-test.sh
Checked out revision 0.
A         file
Adding         file
Transmitting file data .
Committed revision 1.
Initialized empty Git repository in .git/
moy@localhost's password: 
        A       file
r1 = 4357177f2d927931d31c8c49c0237561648001a7 (git-svn)

Checked out HEAD:
svn+ssh://localhost//home/moy/tmp/test/test.26377/svnrepo r1
Created commit aa2db5a: newdir

 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 newdir/newfile

moy@localhost's password:
A newdir/newfile
Network connection closed unexpectedly: Connection closed unexpectedly at /home/moy/bin/local/verimag/git-svn line 405 $

#! /bin/sh

mkdir test.$$
cd test.$$

svnadmin create svnrepo

svn co file://$PWD/svnrepo svnco

cd svnco
echo content > file
svn add file
svn commit -m "added file"
cd ../

# This triggers the bug.
git-svn clone svn+ssh://localhost/$PWD/svnrepo git-svn-co

# while this doesn't
# git-svn clone file://localhost/$PWD/svnrepo git-svn-co

cd git-svn-co

mkdir newdir
echo othercontent > newdir/newfile

git add newdir/newfile

git commit -m "newdir"

git-svn dcommit

-- 
Matthieu

Thanks to your script I can reproduce it.
svnserve (not git-svn) is segfaulting on me...

Thanks. I didn't realize entirely new directories (and not empty directories in svn but git thought were new) were what was causing things to break.

Eric Wong

When sorting directory names by depth (slash ("/") count) and closing the deepest directories first (as the protocol requires), we failed to put the root baton (with an empty string as its key "") after top-level directories (which did not have any slashes).

This resulted in svnserve being in a situation it couldn't handle and caused a segmentation fault on the remote server.

This bug did not affect users of DAV and filesystem repositories.

---
 git-svn.perl |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

index e38811a..6d0cdac 100755
@@ -2841,8 +2841,10 @@ sub close_edit {
 	my ($self) = @_;
 	my ($p,$bat) = ($self->{pool}, $self->{bat});
 	foreach (sort { $b =~ tr#/#/# <=> $a =~ tr#/#/# } keys %$bat) {
+		next if $_ eq '';
 		$self->close_directory($bat->{$_}, $p);
 	}
+	$self->close_directory($bat->{''}, $p);
 	$self->SUPER::close_edit($p);
 	$p->clear;
 }

Eric Wong

I have a modified version of the test I posted earlier based on your test. However, I'm not comfortable binding to any port (even without ssh) on a users machine or potentially leaving a daemon running if the test is interrupted.

From: Eric Wong <[email protected]>
Date: Sun, 13 May 2007 17:38:21 -0700
Subject: [PATCH] git-svn: test for creating new directories over svn://

As reported by Matthieu Moy, this is causing svnserve to terminate connections (because it segfaults) segfault.

---
 t/t9112-git-svn-dcommit-new-file.sh |   37 +++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)
 create mode 100755 t/t9112-git-svn-dcommit-new-file.sh

new file mode 100755
index 0000000..b186fce
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Eric Wong
+#
+
+test_description='git-svn dcommit new files over svn:// test'
+
+. ./lib-git-svn.sh
+
+# standard svnserve runs on 3690, so lets not conflict with that
+SVNSERVE_PORT=${SVNSERVE_PORT-'3691'}
+
+start_svnserve () {
+	svnserve --listen-port $SVNSERVE_PORT \
+	         --root $rawsvnrepo \
+	         --listen-once \
+	         --listen-host 127.0.0.1 &
+}
+
+test_expect_success 'start tracking an empty repo' "
+	svn mkdir -m 'empty dir' $svnrepo/empty-dir &&
+	echo anon-access = write >> $rawsvnrepo/conf/svnserve.conf &&
+	start_svnserve &&
+	git svn init svn://127.0.0.1:$SVNSERVE_PORT &&
+	git svn fetch
+	"
+
+test_expect_success 'create files in new directory with dcommit' "
+	mkdir git-new-dir &&
+	echo hello > git-new-dir/world &&
+	git update-index --add git-new-dir/world &&
+	git commit -m hello &&
+	start_svnserve &&
+	git svn dcommit
+	"
+
+test_done

Eric Wong

How about keeping the pid somewhere, and using "trap" to kill the svn server process on interruption?

Matthieu

Does the patch in
http://permalink.gmane.org/gmane.comp.version-control.git/47126
help?

If so, I'm not sure why it should be failing sometimes and not all the time. If your local username the same as the username you're using for svn it shouldn't fail for the reason that patch above is needed.

Using the native svn or svn+ssh protocol is definitely flakier in my experience, and there are still known issues with connections closing during *fetch*. dcommit is actually a lot more straightforward as it only connects once, whereas fetch can reconnect to different parts of the repo for --follow-parent.

Which version of SVN is running on the server?

Eric Wong

Hi Eric,
hi Matthieu,
hi list,

I ran into that one, too ... :-(

I assume that the dcommit runs fine for *other* commits but not for that one that produced the error cited above.

In my case, it was simply a problem on my side: I had tried to add a file, that already existed in the repository - and subversion rejected the commit. Of course, it would be nice if git-svn could simply tell the user about his error, i.e., simply copy the output of libsvn or whatever it is using to connect to the subversion repo.

Another case where I stumbled upon said error message was when I added an empty (sub)directory to a project in subversion and then used git to track that project. Since git cannot track an empty directory, it did not appear in my git working copy. When I mkdir'd the directory locally (in my git wc) and then tried to add files within that repository, I got exactly the same error as Matthieu.

Not in my case, although this patch resolved the other issue, thanks again btw!

Kind regards
Martin Eisenhardt

It might be a net win to let git-svn handle empty directories by creating an empty .git-svn-empty-dir file in them, instead of pretending they don't exist.

Karl Hasselström, [email protected]
      www.treskal.com/kalle

Interesting. I don't think git-svn currently checks for that case.

No. This is *WAY* too ugly.

Eric Wong

Ok, it already *does* check for this case and works for me(TM).

From 8222388080c05157e2926faffcd853bae500c521 Mon Sep 17 00:00:00 2001
From: Eric Wong <[email protected]>
Date: Sun, 13 May 2007 17:38:21 -0700
Subject: [PATCH] git-svn: test for creating new files in empty previously directories

---
 t/t9112-git-svn-dcommit-new-file.sh |   40 +++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)
 create mode 100755 t/t9112-git-svn-dcommit-new-file.sh

new file mode 100755
index 0000000..e643c13
@@ -0,0 +1,40 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Eric Wong
+#
+
+test_description='git-svn dcommit new files over svn:// test'
+
+. ./lib-git-svn.sh
+
+# standard svnserve runs on 3690, so lets not conflict with that
+SVNSERVE_PORT=${SVNSERVE_PORT-'3691'}
+
+start_svnserve () {
+	svnserve --listen-port $SVNSERVE_PORT \
+	         --root $rawsvnrepo \
+	         --listen-once \
+	         --listen-host 127.0.0.1 &
+}
+
+test_expect_success 'start tracking an empty repo' "
+	svn mkdir -m 'empty dir' $svnrepo/empty-dir &&
+	echo anon-access = write >> $rawsvnrepo/conf/svnserve.conf &&
+	start_svnserve &&
+	git svn init svn://127.0.0.1:$SVNSERVE_PORT &&
+	git svn fetch
+	"
+
+test_expect_success 'create two files in empty-directory with dcommit' "
+	mkdir empty-dir &&
+	echo hello > empty-dir/a &&
+	echo hello > empty-dir/b &&
+	echo hello > empty-dir/c &&
+	git update-index --add empty-dir/a empty-dir/b empty-dir/c &&
+	git commit -m hello &&
+	start_svnserve &&
+	git svn dcommit
+	"
+
+test_done
+

Eric Wong

I wouldn't personally mind if somebody taught git to just track empty directories too.

There is no fundamental git database reason not to allow them: it's in fact quite easy to create an empty tree object. The problems with empty directories are in the *index*, and they shouldn't be insurmountable.

I never wanted empty directories for the kernel, but there is nothing really fundamentally wrong with them either. They do have some problems, which is why I don't particularly like tracking them, but if *not* tracking them causes more problems for people who import from SVN, I could easily see the balance moving towards "let people do it if they want".

In fact, I think the subproject support might have made it easier to track directories as empty directory entries. Pretty much every place that got impacted by subprojects would be where empty directory entries would be handled too.

In fact, one of the questions when tracking empty directories is whether you should track non-empty directories too. IOW, should we _always_ put the directory entry into the index (even when it's not empty), or should we do it only when somebody explicitly asks for it. I suspect the "always" case would is the right answer (ie a read-tree would always add a directory entry to the index before it starts adding the actual entries in the read-tree)

Anybody want to try it?

Linus

Just to add my 2 cents: I just had a case where I would have needed empty directories, and it seems reasonable as a workflow:

As a group of teachers, one created a skeleton, with stg like

./course/
./practical-works/day1
./practical-works/day2
./exercices/day1
./exercices/day2
...

He did it with svn, filled in only a few directories, I "git-svn clone"d it, and I got almost nothing!

These empty directories are not interesting by themselves, but as a placeholder to add files, I think it's a good thing to have them.

Matthieu

I don't have the failure right now, I'll tell you when I get it again.

It is the same on both machines.

$ svn --version
svn, version 1.1.4 (r13838)
   compiled Apr 12 2005, 16:01:59
$ cat /etc/redhat-release
Red Hat Enterprise Linux ES release 4 (Nahant Update 5)

Matthieu