threads / patch / 29826

v2git-svn: Simplify calculation of GIT_DIR

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

## tl;dr

15 messages between Mar 3, 2012 and Jan 24, 2013. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Barry Wardell· Mar 3, 2012, 19:53 UTC · lore

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(-)

Show changes to git-svn.perl +11 −22

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

Eric Wong· Mar 8, 2012, 00:51 UTC · re: Barry Wardell · lore

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

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>

Show 7 quoted lines

> +	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.

Barry Wardell· Jan 21, 2013, 01:22 UTC · re: Eric Wong · lore

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

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

Barry Wardell· Jan 21, 2013, 01:22 UTC · re: Barry Wardell · lore

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

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

Show changes to t/t9100-git-svn-basic.sh +8 −0

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

Barry Wardell· Jan 21, 2013, 01:22 UTC · re: Barry Wardell · lore

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

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(-)

Show changes to git-svn.perl +13 −23

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

Junio C Hamano· Jan 21, 2013, 01:50 UTC · re: Barry Wardell · lore

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

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

Show 13 quoted lines

> 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.

Joachim Schmitz· Jan 21, 2013, 14:19 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:

Show 27 quoted lines

> 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

Philip Oakley· Jan 21, 2013, 20:29 UTC · re: Joachim Schmitz · lore

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

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:

[...]

Show 14 quoted lines

>> 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

Junio C Hamano· Jan 21, 2013, 22:08 UTC · re: Philip Oakley · lore

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

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

Show 26 quoted lines

> 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.

Eric Wong· Jan 23, 2013, 02:32 UTC · re: Barry Wardell · lore

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

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

Show 9 quoted lines

> 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:

Show changes to git-svn.perl +3 −1

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});
 }

Junio C Hamano· Jan 23, 2013, 02:53 UTC · re: Eric Wong · lore

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

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".

Show 23 quoted lines

>
> 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});
>  }

Barry Wardell· Jan 23, 2013, 12:08 UTC · re: Eric Wong · lore

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

On Wed, Jan 23, 2013 at 2:32 AM, Eric Wong <normalperson@yhbt.net> wrote:

Show 42 quoted lines

>
> 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.

Junio C Hamano· Jan 24, 2013, 05:29 UTC · re: Eric Wong · lore

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

Eric Wong <normalperson@yhbt.net> writes:

Show 19 quoted lines

> 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;

Eric Wong· Jan 24, 2013, 10:14 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote:

Show 46 quoted lines

> 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.

Show changes to git-svn.perl +2 −3

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

Eric Wong· Jan 24, 2013, 01:27 UTC · lore

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

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

Show 8 quoted lines

> 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)