Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Junio C Hamano gits...@pobox.com wrote:
Eric Wong normalper...@yhbt.net 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
On Wed, Jan 23, 2013 at 2:32 AM, Eric Wong normalper...@yhbt.net wrote:
Barry Wardell barry.ward...@gmail.com 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
Barry Wardell barry.ward...@gmail.com 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
Eric Wong normalper...@yhbt.net 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
Junio C Hamano wrote: Barry Wardell barry.ward...@gmail.com 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
From: Joachim Schmitz j...@schmitz-digital.de Sent: Monday, January 21, 2013 2:19 PM Junio C Hamano wrote: Barry Wardell barry.ward...@gmail.com 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
Philip Oakley philipoak...@iee.org writes: From: Joachim Schmitz j...@schmitz-digital.de Sent: Monday, January 21, 2013 2:19 PM Junio C Hamano wrote: Barry Wardell barry.ward...@gmail.com 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
[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
Re: [PATCH v3 0/2] Make git-svn work with gitdir links
Barry Wardell barry.ward...@gmail.com 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
