[PATCH v2] git-svn: Simplify calculation of GIT_DIR

[PATCH v2] git-svn: Simplify calculation of GIT_DIR

[Barry Wardell]

Since git-rev-parse already checks for the $GIT_DIR environment
variable and that it returns an actual git repository, there is no
need to repeat the checks again here.

This also fixes a problem where git-svn did not work in cases where
.git was a file with a gitdir: link.
---
 git-svn.perl |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4334b95..bbfd351 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -15,8 +15,6 @@ my $cmd_dir_prefix = eval {
 	command_oneline([qw/rev-parse --show-prefix/], STDERR => 0)
 } || '';
 
-my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};
-$ENV{GIT_DIR} ||= '.git';
 $Git::SVN::default_repo_id = 'svn';
 $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
 $Git::SVN::Ra::_log_window_size = 100;
@@ -292,26 +290,17 @@ for (my $i = 0; $i < @ARGV; $i++) {
 
 # make sure we're always running at the top-level working directory
 unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
-	unless (-d $ENV{GIT_DIR}) {
-		if ($git_dir_user_set) {
-			die "GIT_DIR=$ENV{GIT_DIR} explicitly set, ",
-			    "but it is not a directory\n";
-		}
-		my $git_dir = delete $ENV{GIT_DIR};
-		my $cdup = undef;
-		git_cmd_try {
-			$cdup = command_oneline(qw/rev-parse --show-cdup/);
-			$git_dir = '.' unless ($cdup);
-			chomp $cdup if ($cdup);
-			$cdup = "." unless ($cdup && length $cdup);
-		} "Already at toplevel, but $git_dir not found\n";
-		chdir $cdup or die "Unable to chdir up to '$cdup'\n";
-		unless (-d $git_dir) {
-			die "$git_dir still not found after going to ",
-			    "'$cdup'\n";
-		}
-		$ENV{GIT_DIR} = $git_dir;
-	}
+	my $toplevel = undef;
+
+	git_cmd_try {
+		$toplevel = command_oneline([qw/rev-parse --show-toplevel/]);
+	} "Unable to find toplevel directory\n";
+
+	git_cmd_try {
+		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
+	} "Unable to find .git directory\n";
+
+	chdir $toplevel or die "Unable to chdir to '$toplevel'\n";
 	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
 }
 
-- 
1.7.9.2

Re: [PATCH v2] git-svn: Simplify calculation of GIT_DIR

[Eric Wong]

Barry Wardell <barry.wardell@gmail.com> wrote:
> -my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};
> -$ENV{GIT_DIR} ||= '.git';

<snip>

>  unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {

<snip>

> +	git_cmd_try {
> +		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
> +	} "Unable to find .git directory\n";
> +
> +	chdir $toplevel or die "Unable to chdir to '$toplevel'\n";
>  	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
>  }

It looks like some places in "git svn (init|clone|multi-init)" rely on
GIT_DIR being set.  I noticed the first test of t9100-git-svn-basic.sh
failing (causing everything else in that test to fail) with your patch.

Can you fix those use cases (and ensure tests pass)?  Thanks.

[PATCH v3 0/2] Make git-svn work with gitdir links

[Barry Wardell]

These patches fix a bug which prevented git-svn from working with repositories
which use gitdir links.

Changes since v2:
 - Rebased onto latest master.
 - Added test case which verifies that the problem has been fixed.
 - Fixed problems with git svn (init|clone|multi-init).
 - All git-svn test cases now pass (except two in t9101 which also failed
   before these patches).

Barry Wardell (2):
  git-svn: Add test for git-svn repositories with a gitdir link
  git-svn: Simplify calculation of GIT_DIR

 git-svn.perl             | 36 +++++++++++++-----------------------
 t/t9100-git-svn-basic.sh |  8 ++++++++
 2 files changed, 21 insertions(+), 23 deletions(-)

-- 
1.8.0

[PATCH 1/2] git-svn: Add test for git-svn repositories with a gitdir link

[Barry Wardell]

Signed-off-by: Barry Wardell <barry.wardell@gmail.com>
---
 t/t9100-git-svn-basic.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 749b75e..4fea8d9 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -306,5 +306,13 @@ test_expect_success 'git-svn works in a bare repository' '
 	git svn fetch ) &&
 	rm -rf bare-repo
 	'
+test_expect_success 'git-svn works in in a repository with a gitdir: link' '
+	mkdir worktree gitdir &&
+	( cd worktree &&
+	git svn init "$svnrepo" &&
+	git init --separate-git-dir ../gitdir &&
+	git svn fetch ) &&
+	rm -rf worktree gitdir
+	'
 
 test_done
-- 
1.8.0

[PATCH 2/2] git-svn: Simplify calculation of GIT_DIR

[Barry Wardell]

Since git-rev-parse already checks for the $GIT_DIR environment
variable and that it returns an actual git repository, there is no
need to repeat the checks again here.

This also fixes a problem where git-svn did not work in cases where
.git was a file with a gitdir: link.

Signed-off-by: Barry Wardell <barry.wardell@gmail.com>
---
 git-svn.perl | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index bd5266c..3bcd769 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -61,8 +61,6 @@ my $cmd_dir_prefix = eval {
 	command_oneline([qw/rev-parse --show-prefix/], STDERR => 0)
 } || '';
 
-my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};
-$ENV{GIT_DIR} ||= '.git';
 $Git::SVN::Ra::_log_window_size = 100;
 
 if (! exists $ENV{SVN_SSH} && exists $ENV{GIT_SSH}) {
@@ -325,27 +323,19 @@ for (my $i = 0; $i < @ARGV; $i++) {
 };
 
 # make sure we're always running at the top-level working directory
-unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
-	unless (-d $ENV{GIT_DIR}) {
-		if ($git_dir_user_set) {
-			die "GIT_DIR=$ENV{GIT_DIR} explicitly set, ",
-			    "but it is not a directory\n";
-		}
-		my $git_dir = delete $ENV{GIT_DIR};
-		my $cdup = undef;
-		git_cmd_try {
-			$cdup = command_oneline(qw/rev-parse --show-cdup/);
-			$git_dir = '.' unless ($cdup);
-			chomp $cdup if ($cdup);
-			$cdup = "." unless ($cdup && length $cdup);
-		} "Already at toplevel, but $git_dir not found\n";
-		chdir $cdup or die "Unable to chdir up to '$cdup'\n";
-		unless (-d $git_dir) {
-			die "$git_dir still not found after going to ",
-			    "'$cdup'\n";
-		}
-		$ENV{GIT_DIR} = $git_dir;
-	}
+if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
+	$ENV{GIT_DIR} ||= ".git";
+} else {
+	git_cmd_try {
+		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
+	} "Unable to find .git directory\n";
+	my $cdup = undef;
+	git_cmd_try {
+		$cdup = command_oneline(qw/rev-parse --show-cdup/);
+		chomp $cdup if ($cdup);
+		$cdup = "." unless ($cdup && length $cdup);
+	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
+	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
 	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
 }
 
-- 
1.8.0

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Junio C Hamano]

Barry Wardell <barry.wardell@gmail.com> writes:

> These patches fix a bug which prevented git-svn from working with repositories
> which use gitdir links.
>
> Changes since v2:
>  - Rebased onto latest master.
>  - Added test case which verifies that the problem has been fixed.
>  - Fixed problems with git svn (init|clone|multi-init).
>  - All git-svn test cases now pass (except two in t9101 which also failed
>    before these patches).
>
> Barry Wardell (2):
>   git-svn: Add test for git-svn repositories with a gitdir link
>   git-svn: Simplify calculation of GIT_DIR

Thanks for your persistence ;-) As this is a pretty old topic, I'll
give two URLs for people who are interested to view the previous
threads:

    http://thread.gmane.org/gmane.comp.version-control.git/192133
    http://thread.gmane.org/gmane.comp.version-control.git/192127

You would want to mark it as test_expect_failure in the first patch
and then flip it to text_expect_success in the second patch where
you fix the breakage?  Otherwise, after applying the first patch,
the testsuite will break needlessly.

I've Cc'ed Eric Wong (git-svn maintainer) and CMN who helped in the
previous round.  If the only issue is the above success/failure one,
I think Eric can tweak the patches while applying them (I didn't
look at the changes carefully myself, by the way).

Thanks.

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Joachim Schmitz]

Junio C Hamano wrote:
> Barry Wardell <barry.wardell@gmail.com> writes:
>
>> These patches fix a bug which prevented git-svn from working with
>> repositories which use gitdir links.
>>
>> Changes since v2:
>>  - Rebased onto latest master.
>>  - Added test case which verifies that the problem has been fixed.
>>  - Fixed problems with git svn (init|clone|multi-init).
>>  - All git-svn test cases now pass (except two in t9101 which also
>>    failed before these patches).
>>
>> Barry Wardell (2):
>>   git-svn: Add test for git-svn repositories with a gitdir link
>>   git-svn: Simplify calculation of GIT_DIR
>
> Thanks for your persistence ;-) As this is a pretty old topic, I'll
> give two URLs for people who are interested to view the previous
> threads:
>
>    http://thread.gmane.org/gmane.comp.version-control.git/192133
>    http://thread.gmane.org/gmane.comp.version-control.git/192127
>
> You would want to mark it as test_expect_failure in the first patch
> and then flip it to text_expect_success in the second patch where
> you fix the breakage?  Otherwise, after applying the first patch,
> the testsuite will break needlessly.

I'd just apply them the other way round, 1st fix the problem, 2nd add a test 
for it

Bye, Jojo 

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Philip Oakley]

From: "Joachim Schmitz" <jojo@schmitz-digital.de>
Sent: Monday, January 21, 2013 2:19 PM
> Junio C Hamano wrote:
>> Barry Wardell <barry.wardell@gmail.com> writes:
[...]
>> Thanks for your persistence ;-) As this is a pretty old topic, I'll
>> give two URLs for people who are interested to view the previous
>> threads:
>>
>>    http://thread.gmane.org/gmane.comp.version-control.git/192133
>>    http://thread.gmane.org/gmane.comp.version-control.git/192127
>>
>> You would want to mark it as test_expect_failure in the first patch
>> and then flip it to text_expect_success in the second patch where
>> you fix the breakage?  Otherwise, after applying the first patch,
>> the testsuite will break needlessly.
>
> I'd just apply them the other way round, 1st fix the problem, 2nd add 
> a test for it

Isn't it a case of, 1st demonstrate the problem with a test, and then 
2nd  fix the problem.

Those less principled could could simply "fix" a non-existent problem 
merely to get themselves into the change log, or worse, even if one may 
fix-test under the hood.

>
> Bye, Jojo

Philip 

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> From: "Joachim Schmitz" <jojo@schmitz-digital.de>
> Sent: Monday, January 21, 2013 2:19 PM
>> Junio C Hamano wrote:
>>> Barry Wardell <barry.wardell@gmail.com> writes:
> [...]
>>> Thanks for your persistence ;-) As this is a pretty old topic, I'll
>>> give two URLs for people who are interested to view the previous
>>> threads:
>>>
>>>    http://thread.gmane.org/gmane.comp.version-control.git/192133
>>>    http://thread.gmane.org/gmane.comp.version-control.git/192127
>>>
>>> You would want to mark it as test_expect_failure in the first patch
>>> and then flip it to text_expect_success in the second patch where
>>> you fix the breakage?  Otherwise, after applying the first patch,
>>> the testsuite will break needlessly.
>>
>> I'd just apply them the other way round, 1st fix the problem, 2nd
>> add a test for it
>
> Isn't it a case of, 1st demonstrate the problem with a test, and then
> 2nd  fix the problem.
>
> Those less principled could could simply "fix" a non-existent problem
> merely to get themselves into the change log, or worse, even if one
> may fix-test under the hood.

For a small/trivial fix, fixing the code and protecting the fix from
future breakages by adding tests that expect success in a single
commit is the most sensible thing to do.  People who are interested,
and people who are auditing, can locally revert only the code change
to see the new tests fail fairly easily in such a case.

For a more involved series, it is easier to demonstrate a breakage
by adding tests that expect failure in the first commit, and then in
subsequent commits, to fix a class of bugs in the code and flipping
expect_failure into expect_success for the tests that the updated
code in the commit fixes.

For this particular topic, squashing the two patches into a single
commit may probably be the more appropriate between the two.

Thanks.

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Eric Wong]

Barry Wardell <barry.wardell@gmail.com> wrote:
> These patches fix a bug which prevented git-svn from working with repositories
> which use gitdir links.
> 
> Changes since v2:
>  - Rebased onto latest master.
>  - Added test case which verifies that the problem has been fixed.
>  - Fixed problems with git svn (init|clone|multi-init).
>  - All git-svn test cases now pass (except two in t9101 which also failed
>    before these patches).

t9101 did not fail for me before your patches.  However I have a
patch on top of your 2/2 which should fix things.

`git rev-parse --show-cdup` outputs nothing if GIT_DIR is set,
so I unset GIT_DIR temporarily.

I'm not sure why --show-cdup behaves like this, though..

Does squashing this on top of your changes fix all your failures?
I plan on squashing both your changes together with the below:

diff --git a/git-svn.perl b/git-svn.perl
index c232798..e5bd292 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
 		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
 	} "Unable to find .git directory\n";
 	my $cdup = undef;
+	my $git_dir = delete $ENV{GIT_DIR};
 	git_cmd_try {
 		$cdup = command_oneline(qw/rev-parse --show-cdup/);
 		chomp $cdup if ($cdup);
 		$cdup = "." unless ($cdup && length $cdup);
-	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
+	} "Already at toplevel, but $git_dir not found\n";
+	$ENV{GIT_DIR} = $git_dir;
 	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
 	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
 }

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> `git rev-parse --show-cdup` outputs nothing if GIT_DIR is set,
> so I unset GIT_DIR temporarily.
>
> I'm not sure why --show-cdup behaves like this, though..

Setting GIT_DIR is to say "That is the directory that has the
repository objects and refs; I am letting you know the location
explicitly because it does not have any relation with the location
of the working tree.  The $(cwd) is at the root of the working
tree".

If you want to say "That is the directory that has metainformation,
and that other one is the root of the working tree", you use
GIT_WORK_TREE to name the latter.

So by definition, if you only set GIT_DIR without setting
GIT_WORK_TREE, show-cdup must say "you are already at the top".

>
> Does squashing this on top of your changes fix all your failures?
> I plan on squashing both your changes together with the below:
>
> diff --git a/git-svn.perl b/git-svn.perl
> index c232798..e5bd292 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
>  		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
>  	} "Unable to find .git directory\n";
>  	my $cdup = undef;
> +	my $git_dir = delete $ENV{GIT_DIR};
>  	git_cmd_try {
>  		$cdup = command_oneline(qw/rev-parse --show-cdup/);
>  		chomp $cdup if ($cdup);
>  		$cdup = "." unless ($cdup && length $cdup);
> -	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
> +	} "Already at toplevel, but $git_dir not found\n";
> +	$ENV{GIT_DIR} = $git_dir;
>  	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
>  	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
>  }

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Barry Wardell]

On Wed, Jan 23, 2013 at 2:32 AM, Eric Wong <normalperson@yhbt.net> wrote:
>
> Barry Wardell <barry.wardell@gmail.com> wrote:
> > These patches fix a bug which prevented git-svn from working with repositories
> > which use gitdir links.
> >
> > Changes since v2:
> >  - Rebased onto latest master.
> >  - Added test case which verifies that the problem has been fixed.
> >  - Fixed problems with git svn (init|clone|multi-init).
> >  - All git-svn test cases now pass (except two in t9101 which also failed
> >    before these patches).
>
> t9101 did not fail for me before your patches.  However I have a
> patch on top of your 2/2 which should fix things.
>
> `git rev-parse --show-cdup` outputs nothing if GIT_DIR is set,
> so I unset GIT_DIR temporarily.
>
> I'm not sure why --show-cdup behaves like this, though..
>
> Does squashing this on top of your changes fix all your failures?
> I plan on squashing both your changes together with the below:
>
> diff --git a/git-svn.perl b/git-svn.perl
> index c232798..e5bd292 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
>                 $ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
>         } "Unable to find .git directory\n";
>         my $cdup = undef;
> +       my $git_dir = delete $ENV{GIT_DIR};
>         git_cmd_try {
>                 $cdup = command_oneline(qw/rev-parse --show-cdup/);
>                 chomp $cdup if ($cdup);
>                 $cdup = "." unless ($cdup && length $cdup);
> -       } "Already at toplevel, but $ENV{GIT_DIR} not found\n";
> +       } "Already at toplevel, but $git_dir not found\n";
> +       $ENV{GIT_DIR} = $git_dir;
>         chdir $cdup or die "Unable to chdir up to '$cdup'\n";
>         $_repository = Git->repository(Repository => $ENV{GIT_DIR});
>  }


Yes, I can confirm that applying this patch on top of mine makes all
git-svn tests pass again. I have also re-run the tests without my
patch applied and found that they do all indeed pass, so I apologize
for my previous incorrect comment.

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Eric Wong]

Barry Wardell <barry.wardell@gmail.com> wrote:
> On Wed, Jan 23, 2013 at 2:32 AM, Eric Wong <normalperson@yhbt.net> wrote:
> > Does squashing this on top of your changes fix all your failures?
> > I plan on squashing both your changes together with the below:
> 
> Yes, I can confirm that applying this patch on top of mine makes all
> git-svn tests pass again. I have also re-run the tests without my patch
> applied and found that they do all indeed pass, so I apologize for my
> previous incorrect comment.

Thanks, squashed, tested and pushed (have another unrelated patch coming)

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> diff --git a/git-svn.perl b/git-svn.perl
> index c232798..e5bd292 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
>  		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
>  	} "Unable to find .git directory\n";
>  	my $cdup = undef;
> +	my $git_dir = delete $ENV{GIT_DIR};
>  	git_cmd_try {
>  		$cdup = command_oneline(qw/rev-parse --show-cdup/);
>  		chomp $cdup if ($cdup);
>  		$cdup = "." unless ($cdup && length $cdup);
> -	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
> +	} "Already at toplevel, but $git_dir not found\n";
> +	$ENV{GIT_DIR} = $git_dir;
>  	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
>  	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
>  }

This does not look quite right, though.

Can't the user have his own $GIT_DIR when this command is invoked?
The first command_oneline() runs rev-parse with that environment and
get the user specified value of GIT_DIR in $ENV{GIT_DIR}, but by
doing a "delete" before running --show-cdup, you are not honoring
that GIT_DIR (and GIT_WORK_TREE if exists) the user gave you.  You
already used that GIT_DIR when you asked rev-parse --git-dir to find
what the GIT_DIR value should be, so you would be operating with
values of $git_dir and $cdup that you discovered in an inconsistent
way, no?

Shouldn't it be more like this instead?

	my ($git_dir, $cdup) = undef;
        try {
		$git_dir = command_oneline(qw(rev-parse --git-dir));
	} "Unable to ...";
        try {
		$cdup = command_oneline(qw(rev-parse --show-cdup));
		... tweak $cdup ...
	} "Unable to ...";
	if (defined $git_dir) { $ENV{GIT_DIR} = $git_dir; }
	chdir $cdup;

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > diff --git a/git-svn.perl b/git-svn.perl
> > index c232798..e5bd292 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
> >  		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
> >  	} "Unable to find .git directory\n";
> >  	my $cdup = undef;
> > +	my $git_dir = delete $ENV{GIT_DIR};
> >  	git_cmd_try {
> >  		$cdup = command_oneline(qw/rev-parse --show-cdup/);
> >  		chomp $cdup if ($cdup);
> >  		$cdup = "." unless ($cdup && length $cdup);
> > -	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
> > +	} "Already at toplevel, but $git_dir not found\n";
> > +	$ENV{GIT_DIR} = $git_dir;
> >  	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
> >  	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
> >  }
> 
> This does not look quite right, though.
> 
> Can't the user have his own $GIT_DIR when this command is invoked?
> The first command_oneline() runs rev-parse with that environment and
> get the user specified value of GIT_DIR in $ENV{GIT_DIR}, but by
> doing a "delete" before running --show-cdup, you are not honoring
> that GIT_DIR (and GIT_WORK_TREE if exists) the user gave you.  You
> already used that GIT_DIR when you asked rev-parse --git-dir to find
> what the GIT_DIR value should be, so you would be operating with
> values of $git_dir and $cdup that you discovered in an inconsistent
> way, no?
> 
> Shouldn't it be more like this instead?
> 
> 	my ($git_dir, $cdup) = undef;
>         try {
> 		$git_dir = command_oneline(qw(rev-parse --git-dir));
> 	} "Unable to ...";
>         try {
> 		$cdup = command_oneline(qw(rev-parse --show-cdup));
> 		... tweak $cdup ...
> 	} "Unable to ...";
> 	if (defined $git_dir) { $ENV{GIT_DIR} = $git_dir; }
> 	chdir $cdup;

Thanks, I'll squash the following and push a new branch.  I don't
believe the (defined $git_dir) check is necessary since we already
checked for errors with git_cmd_try.

diff --git a/git-svn.perl b/git-svn.perl
index e5bd292..b46795f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -324,19 +324,18 @@ for (my $i = 0; $i < @ARGV; $i++) {
 	}
 };
 
 # make sure we're always running at the top-level working directory
 if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
 	$ENV{GIT_DIR} ||= ".git";
 } else {
+	my ($git_dir, $cdup);
 	git_cmd_try {
-		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
+		$git_dir = command_oneline([qw/rev-parse --git-dir/]);
 	} "Unable to find .git directory\n";
-	my $cdup = undef;
-	my $git_dir = delete $ENV{GIT_DIR};
 	git_cmd_try {
 		$cdup = command_oneline(qw/rev-parse --show-cdup/);
 		chomp $cdup if ($cdup);
 		$cdup = "." unless ($cdup && length $cdup);
 	} "Already at toplevel, but $git_dir not found\n";
 	$ENV{GIT_DIR} = $git_dir;
 	chdir $cdup or die "Unable to chdir up to '$cdup'\n";

-- 
Eric Wong