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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2013, 05:29 UTC
Message-ID: <7vehhbdu8y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130123023235.GA24135@dcvr.yhbt.net>

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;