Re: [RFC PATCH] git-svn: proper detection of bare repositories

[Eric Wong <normalperson@yhbt.net>]

Deskin Miller <deskinm@umich.edu> wrote:
> I keep coming across commands like this, which don't work properly in bare
> repositories, and thinking that they need to be patched (see e.g. ddff8563, or
> Duy's comments on the thread starting at
> http://thread.gmane.org/gmane.comp.version-control.git/98849), but now I'm not
> so sure.  For one, despite this patch working, it turns out that 'git --bare
> svn <cmd>' also works (and presumably has) for some time.

Interesting.  I've never even looked at --bare myself.  It always
should've worked if GIT_DIR= was explicitly set and I guess back in the
old days when I wrote git-svn, --bare wasn't even a flag :)

> Is git --bare the correct way to deal with this situation?  That is to say, do
> we intend commands to 'just work' regardless of whether the repo is bare or
> not, or should the user be thinking about the difference and including --bare
> in the command invocation when necessary?  I'm a vote for the 'just work' camp,
> but it seems a lot of things aren't necessarily that way.  On the other hand,
> the majority of commands do just work.
>
> I guess I'm asking for a sanity check before I write any more such patches;
> certainly I find them useful, as the issues come up during my normal use of
> Git, but I don't want to be pursuing things of no use to anyone else, or
> (worse) things that are fundamentally wrong for some reason I don't understand
> yet.

I don't think there's anything fundamentally wrong with things 'just
working' on bare repos.  It's just another case of something that not
many people end up using (especially for git-svn) and hence got fewer
testers and bug reports.

> -- 8< --
> 
> When in a bare repository (or .git, for that matter), git-svn would fail
> to initialise properly, since git rev-parse --show-cdup would not output
> anything.  However, git rev-parse --show-cdup actually returns an error
> code if it's really not in a git directory.
> 
> Fix the issue by checking for an explicit error from git rev-parse, and
> setting $git_dir appropriately if instead it just does not output.
> 
> Signed-off-by: Deskin Miller <deskinm@umich.edu>
> ---
>  git-svn.perl             |   14 ++++++++++----
>  t/t9100-git-svn-basic.sh |    9 +++++++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 56238da..d25e9be 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -42,6 +42,7 @@ use File::Path qw/mkpath/;
>  use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
>  use IPC::Open3;
>  use Git;
> +use Error qw/:try/;
>  
>  BEGIN {
>  	# import functions from Git into our packages, en masse
> @@ -214,11 +215,16 @@ unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
>  			    "but it is not a directory\n";
>  		}
>  		my $git_dir = delete $ENV{GIT_DIR};
> -		chomp(my $cdup = command_oneline(qw/rev-parse --show-cdup/));
> -		unless (length $cdup) {
> -			die "Already at toplevel, but $git_dir ",
> -			    "not found '$cdup'\n";
> -		}
> +		my $cdup = undef;
> +		try {
> +			$cdup = command_oneline(qw/rev-parse --show-cdup/);
> +			$git_dir = '.' unless ($cdup);
> +			chomp $cdup if ($cdup);
> +			$cdup = "." unless ($cdup && length $cdup);
> +		}
> +		catch Git::Error::Command with {
> +			die "Already at toplevel, but $git_dir not found\n";
> +		};

How about using git_cmd_try instead?

The Error.pm try/catch stuff makes me a bit uncomfortable.  I realize
it's (unfortunately) in Git.pm; but I'd rather keep it confined there so
we can more easily remove it later if someone were inclined.

Otherwise I like the idea of this patch.

Thanks,

-- 
Eric Wong