Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Junio C Hamano wrote: > Eric Wong 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. 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Eric Wong 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; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Barry Wardell wrote: > On Wed, Jan 23, 2013 at 2:32 AM, Eric Wong 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) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
On Wed, Jan 23, 2013 at 2:32 AM, Eric Wong wrote: > > Barry Wardell 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Eric Wong 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". > > 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}); > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Barry Wardell 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}); } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
"Philip Oakley" writes: > From: "Joachim Schmitz" > Sent: Monday, January 21, 2013 2:19 PM >> Junio C Hamano wrote: >>> Barry Wardell 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
From: "Joachim Schmitz" Sent: Monday, January 21, 2013 2:19 PM Junio C Hamano wrote: Barry Wardell 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. Bye, Jojo Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Junio C Hamano wrote: Barry Wardell 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Barry Wardell 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'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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html