{"thread":{"id":"29825","subject":"[PATCH] git-svn: Simplify calculation of GIT_DIR","startedAt":"2012-03-03T18:08:27Z","lastAt":"2012-03-03T19:45:47Z","messageCount":3,"participants":["Barry Wardell","Carlos Martín Nieto"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186005","messageId":"1330798107-33561-1-git-send-email-barry.wardell@gmail.com","threadId":"29825","inReplyTo":null,"subject":"[PATCH] git-svn: Simplify calculation of GIT_DIR","fromName":"Barry Wardell","fromEmail":"barry.wardell@gmail.com","sentAt":"2012-03-03T18:08:27Z","receivedAt":"2012-03-03T18:08:27Z","isPatch":true,"sender":{"key":"barry.wardell@gmail.com","avatar":"https://avatars.githubusercontent.com/u/354095?v=4"},"body":"Since git-rev-parse already checks for the $GIT_DIR environment\nvariable and that it returns an actual git repository, there is no\nneed to repeat the checks again here.\n\nThis also fixes a problem where git-svn did not work in cases where\n.git was a file with a gitdir: link.\n---\n git-svn.perl |   25 ++-----------------------\n 1 file changed, 2 insertions(+), 23 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 4334b95..cf2cef8 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -15,8 +15,6 @@ my $cmd_dir_prefix = eval {\n \tcommand_oneline([qw/rev-parse --show-prefix/], STDERR => 0)\n } || '';\n \n-my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};\n-$ENV{GIT_DIR} ||= '.git';\n $Git::SVN::default_repo_id = 'svn';\n $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';\n $Git::SVN::Ra::_log_window_size = 100;\n@@ -290,28 +288,9 @@ for (my $i = 0; $i < @ARGV; $i++) {\n \t}\n };\n \n-# make sure we're always running at the top-level working directory\n+# Access an existing repository\n unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {\n-\tunless (-d $ENV{GIT_DIR}) {\n-\t\tif ($git_dir_user_set) {\n-\t\t\tdie \"GIT_DIR=$ENV{GIT_DIR} explicitly set, \",\n-\t\t\t    \"but it is not a directory\\n\";\n-\t\t}\n-\t\tmy $git_dir = delete $ENV{GIT_DIR};\n-\t\tmy $cdup = undef;\n-\t\tgit_cmd_try {\n-\t\t\t$cdup = command_oneline(qw/rev-parse --show-cdup/);\n-\t\t\t$git_dir = '.' unless ($cdup);\n-\t\t\tchomp $cdup if ($cdup);\n-\t\t\t$cdup = \".\" unless ($cdup && length $cdup);\n-\t\t} \"Already at toplevel, but $git_dir not found\\n\";\n-\t\tchdir $cdup or die \"Unable to chdir up to '$cdup'\\n\";\n-\t\tunless (-d $git_dir) {\n-\t\t\tdie \"$git_dir still not found after going to \",\n-\t\t\t    \"'$cdup'\\n\";\n-\t\t}\n-\t\t$ENV{GIT_DIR} = $git_dir;\n-\t}\n+\t$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);\n \t$_repository = Git->repository(Repository => $ENV{GIT_DIR});\n }\n \n-- \n1.7.9.2\n"},{"id":"186006","messageId":"1330799233.691.40.camel@centaur.lab.cmartin.tk","threadId":"29825","inReplyTo":"1330798107-33561-1-git-send-email-barry.wardell@gmail.com","subject":"Re: [PATCH] git-svn: Simplify calculation of GIT_DIR","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-03-03T18:27:13Z","receivedAt":"2012-03-03T18:27:13Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Sat, 2012-03-03 at 18:08 +0000, Barry Wardell wrote:\n> Since git-rev-parse already checks for the $GIT_DIR environment\n> variable and that it returns an actual git repository, there is no\n> need to repeat the checks again here.\n> \n> This also fixes a problem where git-svn did not work in cases where\n> .git was a file with a gitdir: link.\n> ---\n>  git-svn.perl |   25 ++-----------------------\n>  1 file changed, 2 insertions(+), 23 deletions(-)\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index 4334b95..cf2cef8 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -15,8 +15,6 @@ my $cmd_dir_prefix = eval {\n>  \tcommand_oneline([qw/rev-parse --show-prefix/], STDERR => 0)\n>  } || '';\n>  \n> -my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};\n> -$ENV{GIT_DIR} ||= '.git';\n>  $Git::SVN::default_repo_id = 'svn';\n>  $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';\n>  $Git::SVN::Ra::_log_window_size = 100;\n> @@ -290,28 +288,9 @@ for (my $i = 0; $i < @ARGV; $i++) {\n>  \t}\n>  };\n>  \n> -# make sure we're always running at the top-level working directory\n> +# Access an existing repository\n\nIs there a reason making sure we're at the top-level dir isn't necessary\nanymore?\n\n>  unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {\n> -\tunless (-d $ENV{GIT_DIR}) {\n> -\t\tif ($git_dir_user_set) {\n> -\t\t\tdie \"GIT_DIR=$ENV{GIT_DIR} explicitly set, \",\n> -\t\t\t    \"but it is not a directory\\n\";\n> -\t\t}\n> -\t\tmy $git_dir = delete $ENV{GIT_DIR};\n> -\t\tmy $cdup = undef;\n> -\t\tgit_cmd_try {\n> -\t\t\t$cdup = command_oneline(qw/rev-parse --show-cdup/);\n> -\t\t\t$git_dir = '.' unless ($cdup);\n> -\t\t\tchomp $cdup if ($cdup);\n> -\t\t\t$cdup = \".\" unless ($cdup && length $cdup);\n> -\t\t} \"Already at toplevel, but $git_dir not found\\n\";\n> -\t\tchdir $cdup or die \"Unable to chdir up to '$cdup'\\n\";\n\nHere you delete a chdir to the top-level directory, just as you deleted\nthe comment above, yet in the commit message you don't explain why this\nisn't necessary anymore. Doesn't the rest of the code still assume that\nit's running at the top-level dir?\n\n> -\t\tunless (-d $git_dir) {\n> -\t\t\tdie \"$git_dir still not found after going to \",\n> -\t\t\t    \"'$cdup'\\n\";\n> -\t\t}\n> -\t\t$ENV{GIT_DIR} = $git_dir;\n> -\t}\n> +\t$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);\n>  \t$_repository = Git->repository(Repository => $ENV{GIT_DIR});\n>  }\n>  \n\n\n"},{"id":"186010","messageId":"CAHrK+Z9fRQqLSY=3kc+8WkifTy1mJoA5ZFyGjXbdS8SXg9v=9w@mail.gmail.com","threadId":"29825","inReplyTo":"1330799233.691.40.camel@centaur.lab.cmartin.tk","subject":"Re: [PATCH] git-svn: Simplify calculation of GIT_DIR","fromName":"Barry Wardell","fromEmail":"barry.wardell@gmail.com","sentAt":"2012-03-03T19:45:47Z","receivedAt":"2012-03-03T19:45:47Z","isPatch":true,"sender":{"key":"barry.wardell@gmail.com","avatar":"https://avatars.githubusercontent.com/u/354095?v=4"},"body":"On Sat, Mar 3, 2012 at 6:27 PM, Carlos Martín Nieto <cmn@elego.de> wrote:\n>\n> > -# make sure we're always running at the top-level working directory\n> > +# Access an existing repository\n>\n> Is there a reason making sure we're at the top-level dir isn't necessary\n> anymore?\n\nNo, in fact it is still necessary.\n\n> >  unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {\n> > -     unless (-d $ENV{GIT_DIR}) {\n> > -             if ($git_dir_user_set) {\n> > -                     die \"GIT_DIR=$ENV{GIT_DIR} explicitly set, \",\n> > -                         \"but it is not a directory\\n\";\n> > -             }\n> > -             my $git_dir = delete $ENV{GIT_DIR};\n> > -             my $cdup = undef;\n> > -             git_cmd_try {\n> > -                     $cdup = command_oneline(qw/rev-parse --show-cdup/);\n> > -                     $git_dir = '.' unless ($cdup);\n> > -                     chomp $cdup if ($cdup);\n> > -                     $cdup = \".\" unless ($cdup && length $cdup);\n> > -             } \"Already at toplevel, but $git_dir not found\\n\";\n> > -             chdir $cdup or die \"Unable to chdir up to '$cdup'\\n\";\n>\n> Here you delete a chdir to the top-level directory, just as you deleted\n> the comment above, yet in the commit message you don't explain why this\n> isn't necessary anymore. Doesn't the rest of the code still assume that\n> it's running at the top-level dir?\n\n\nYes, you're right. I will restore the chdir and submit a new patch.\n\nBarry\n"}]}