Deskin Miller <deskinm@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
