Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit
Robert Luberda rob...@debian.org wrote: Eric Wong wrote: Oops, I'll push the following out since Junio already merged your original: I can see that you haven't pushed the change yet. Maybe it would be a good idea to fix other style mistakes (extra spaces after redirections, lack of spaces after function names, `[' used instead of `test', etc) as well? I can prepare a patch if you think it is a good idea. Oops, I got distracted. Yes please on an updated patch. 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/RFC] git svn: handle errors and concurrent commits in dcommit
Robert Luberda rob...@debian.org wrote:
While working on my next patch, I've accidentally discovered that bash gives
the following errors in the test file introduced in my commit :
./t9164-git-svn-dcommit-concrrent.sh: line 65: $hook: ambiguous redirect
./t9164-git-svn-dcommit-concrrent.sh: line 66: $hook: ambiguous redirect
(The test succeeds, even though assignments of the PATH and svnconf
variables is missing; is seems svn treats an empty value of --config-dir
as the current dir.)
Sorry about not noticing this before, dash is used as default /bin/sh on
my system. A simple patch for this issue is included below.
There is also a typo in the test file name: `concrrent' instead of
`concurrent', but it most probably doesn't make any sense to make a
patch for it.
Oops, I'll push the following out since Junio already merged your
original:
(your original patch was also whitespace-mangled)
From 209b4ac46d70aa6f29d51c2fbefa53509513362c Mon Sep 17 00:00:00 2001
From: Robert Luberda rob...@debian.org
Date: Mon, 20 Aug 2012 00:43:19 +0200
Subject: [PATCH] t9164: Add missing quotes in test
This fixes `ambiguous redirect' error given by bash.
[ew: fix misspelled test name,
also eliminate space after to conform to guidelines]
Signed-off-by: Eric Wong normalper...@yhbt.net
---
...n-dcommit-concrrent.sh = t9164-git-svn-dcommit-concurrent.sh} | 8
1 file changed, 4 insertions(+), 4 deletions(-)
rename t/{t9164-git-svn-dcommit-concrrent.sh =
t9164-git-svn-dcommit-concurrent.sh} (97%)
diff --git a/t/t9164-git-svn-dcommit-concrrent.sh
b/t/t9164-git-svn-dcommit-concurrent.sh
similarity index 97%
rename from t/t9164-git-svn-dcommit-concrrent.sh
rename to t/t9164-git-svn-dcommit-concurrent.sh
index aac2dda..d8464d4 100755
--- a/t/t9164-git-svn-dcommit-concrrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -60,11 +60,11 @@ setup_hook()
[ $cnt = 0 ] || exit 0
EOF1
if [ $hook_type = pre-commit ]; then
- echo echo 'commit disallowed' 2; exit 1 $hook
+ echo echo 'commit disallowed' 2; exit 1 $hook
else
- echo PATH=\$PATH\; export PATH $hook
- echo svnconf=\$svnconf\ $hook
- cat $hook - 'EOF2'
+ echo PATH=\$PATH\; export PATH $hook
+ echo svnconf=\$svnconf\ $hook
+ cat $hook - 'EOF2'
cd work-auto-commits.svn
svn up --config-dir $svnconf
echo $$ auto_updated_file
--
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/RFC] git svn: handle errors and concurrent commits in dcommit
Eric Wong wrote: Hi, Junio C Hamano gits...@pobox.com wrote: I should have asked this yesterday, but do you mean you want to have your maint in the upcoming 1.7.12? This does look like a useful thing to do, but does not seem like a regression fix to me. Yeah, I wasn't sure what to name it since my master is still carrying Michael's larger SVN 1.7 changes. Perhaps I should've named my maint for-git-master in this case... While working on my next patch, I've accidentally discovered that bash gives the following errors in the test file introduced in my commit : ./t9164-git-svn-dcommit-concrrent.sh: line 65: $hook: ambiguous redirect ./t9164-git-svn-dcommit-concrrent.sh: line 66: $hook: ambiguous redirect (The test succeeds, even though assignments of the PATH and svnconf variables is missing; is seems svn treats an empty value of --config-dir as the current dir.) Sorry about not noticing this before, dash is used as default /bin/sh on my system. A simple patch for this issue is included below. There is also a typo in the test file name: `concrrent' instead of `concurrent', but it most probably doesn't make any sense to make a patch for it. 8---8---8---8---8---8---8---8---8---8---8---8---8 Subject: [PATCH] Add missing quotes in test t9164 This fixes `ambiguous redirect' error given by bash. --- t/t9164-git-svn-dcommit-concrrent.sh |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9164-git-svn-dcommit-concrrent.sh b/t/t9164-git-svn-dcommit-concrrent.sh index aac2dda..676ef00 100755 --- a/t/t9164-git-svn-dcommit-concrrent.sh +++ b/t/t9164-git-svn-dcommit-concrrent.sh @@ -62,8 +62,8 @@ EOF1 if [ $hook_type = pre-commit ]; then echo echo 'commit disallowed' 2; exit 1 $hook else - echo PATH=\$PATH\; export PATH $hook - echo svnconf=\$svnconf\ $hook + echo PATH=\$PATH\; export PATH $hook + echo svnconf=\$svnconf\ $hook cat $hook - 'EOF2' cd work-auto-commits.svn svn up --config-dir $svnconf -- 1.7.10.4 8---8---8---8---8---8---8---8---8---8---8---8---8 Regards, robert -- 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/RFC] git svn: handle errors and concurrent commits in dcommit
Junio C Hamano gits...@pobox.com wrote: I should have asked this yesterday, but do you mean you want to have your maint in the upcoming 1.7.12? This does look like a useful thing to do, but does not seem like a regression fix to me. Yeah, I wasn't sure what to name it since my master is still carrying Michael's larger SVN 1.7 changes. Perhaps I should've named my maint for-git-master in this case... -- 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/RFC] git svn: handle errors and concurrent commits in dcommit
Eric Wong normalper...@yhbt.net writes: Robert Luberda rob...@debian.org wrote: Eric Wong wrote: + echo PATH=\$PATH\; export PATH $hook + echo svnconf=\$svnconf\ $hook + cat $hook - 'EOF2' + cd work-auto-commits.svn + svn up --config-dir $svnconf That doesn't seem to interact well with users who depend on svn_cmd pointing to something non-standard. Not sure what to do about it, though I have no idea how to change it either. I've tried to source the lib-git-svn.sh file inside the hook, but it sources test-lib.sh, and the latter script doesn't work well if it is sourced by non-test script. Anyway I the part of my original patch unchanged. Ah, so svn_cmd only cares about --config-dir and you already handled that :) I misremembered it also allowed for non-standard SVN installations :x I've pushed your updated patch to my maint branch on git://bogomips.org/git-svn since master has larger pending changes. I should have asked this yesterday, but do you mean you want to have your maint in the upcoming 1.7.12? This does look like a useful thing to do, but does not seem like a regression fix to me. -- 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/RFC] git svn: handle errors and concurrent commits in dcommit
Robert Luberda rob...@debian.org wrote: Eric Wong wrote: + echo PATH=\$PATH\; export PATH $hook + echo svnconf=\$svnconf\ $hook + cat $hook - 'EOF2' + cd work-auto-commits.svn + svn up --config-dir $svnconf That doesn't seem to interact well with users who depend on svn_cmd pointing to something non-standard. Not sure what to do about it, though I have no idea how to change it either. I've tried to source the lib-git-svn.sh file inside the hook, but it sources test-lib.sh, and the latter script doesn't work well if it is sourced by non-test script. Anyway I the part of my original patch unchanged. Ah, so svn_cmd only cares about --config-dir and you already handled that :) I misremembered it also allowed for non-standard SVN installations :x I've pushed your updated patch to my maint branch on git://bogomips.org/git-svn since master has larger pending changes. 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/RFC] git svn: handle errors and concurrent commits in dcommit
Eric Wong wrote:
Hi,
A few minor comments inline...
Please ensure all error messages and code are readable in
80-column terminals.
Also, keep opening { on the same line as the if/unless.
Backticks don't nest properly, nowadays, we prefer:
N=$(expr $N + 1)
+cp auto_updated_file auto_updated_file_saved
Need to check for failure on cp
+sed -i 1d auto_updated_file git commit -am commit change
$N.3
I don't believe sed -i is portable enough for this test.
Many thanks for the comments! I've fixed all of the above and will send
updated patch in next e-mail. Please let me know if you have any further
comments.
+echo PATH=\$PATH\; export PATH $hook
+echo svnconf=\$svnconf\ $hook
+cat $hook - 'EOF2'
+cd work-auto-commits.svn
+svn up --config-dir $svnconf
That doesn't seem to interact well with users who depend on
svn_cmd pointing to something non-standard. Not sure
what to do about it, though
I have no idea how to change it either. I've tried to source the
lib-git-svn.sh file inside the hook, but it sources test-lib.sh, and the
latter script doesn't work well if it is sourced by non-test script.
Anyway I the part of my original patch unchanged.
Regards,
robert
--
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/RFC] git svn: handle errors and concurrent commits in dcommit
dcommit didn't handle errors returned by SVN and coped very
poorly with concurrent commits that appear in SVN repository
while dcommit was running. In both cases it left git repository
in inconsistent state: index (which was reset with `git reset
--mixed' after a successful commit to SVN) no longer matched the
checkouted tree, when the following commit failed or needed to be
rebased. See http://bugs.debian.org/676904 for examples.
This patch fixes the issues by:
- introducing error handler for dcommit. The handler will try
to rebase or reset working tree before returning error to the
end user. dcommit_rebase function was extracted out of cmd_dcommit
to ensure consistency between cmd_dcommit and the error handler.
- calling `git reset --mixed' only once after all patches are
successfully committed to SVN. This ensures index is not touched
for most of the time of dcommit run.
---
git-svn.perl | 74 +---
t/t9164-git-svn-dcommit-concrrent.sh | 216 ++
2 files changed, 271 insertions(+), 19 deletions(-)
create mode 100755 t/t9164-git-svn-dcommit-concrrent.sh
diff --git a/git-svn.perl b/git-svn.perl
index 5711c57..828b8f0 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -777,6 +777,44 @@ sub populate_merge_info {
return undef;
}
+sub dcommit_rebase {
+ my ($is_last, $current, $fetched_ref, $svn_error) = @_;
+ my @diff;
+
+ if ($svn_error) {
+ print STDERR \nERROR from SVN:\n,
+ $svn_error-expanded_message, \n;
+ }
+ unless ($_no_rebase) {
+ # we always want to rebase against the current HEAD,
+ # not any head that was passed to us
+ @diff = command('diff-tree', $current,
+ $fetched_ref, '--');
+ my @finish;
+ if (@diff) {
+ @finish = rebase_cmd();
+ print STDERR W: $current and , $fetched_ref,
+ differ, using @finish:\n,
+join(\n, @diff), \n;
+ } elsif ($is_last) {
+ print No changes between , $current, and ,
+ $fetched_ref,
+ \nResetting to the latest ,
+ $fetched_ref, \n;
+ @finish = qw/reset --mixed/;
+ }
+ command_noisy(@finish, $fetched_ref) if @finish;
+ }
+ if ($svn_error) {
+ die ERROR: Not all changes have been committed into SVN
+ .($_no_rebase ? .\n : , however the committed\n
+ .ones (if any) seem to be successfully integrated
+ .into the working tree.\n)
+ .Please see the above messages for details.\n;
+ }
+ return @diff;
+}
+
sub cmd_dcommit {
my $head = shift;
command_noisy(qw/update-index --refresh/);
@@ -904,6 +942,7 @@ sub cmd_dcommit {
}
my $rewritten_parent;
+ my $current_head = command_oneline(qw/rev-parse HEAD/);
Git::SVN::remove_username($expect_url);
if (defined($_merge_info)) {
$_merge_info =~ tr{ }{\n};
@@ -943,6 +982,14 @@ sub cmd_dcommit {
},
mergeinfo = $_merge_info,
svn_path = '');
+
+ my $err_handler = $SVN::Error::handler;
+ $SVN::Error::handler = sub {
+ my $err = shift;
+ dcommit_rebase(1, $current_head, $gs-refname,
+ $err);
+ };
+
if (!Git::SVN::Editor-new(\%ed_opts)-apply_diff) {
print No changes\n$d~1 == $d\n;
} elsif ($parents-{$d} @{$parents-{$d}}) {
@@ -950,31 +997,19 @@ sub cmd_dcommit {
$parents-{$d};
}
$_fetch_all ? $gs-fetch_all : $gs-fetch;
+ $SVN::Error::handler = $err_handler;
$last_rev = $cmt_rev;
next if $_no_rebase;
- # we always want to rebase against the current HEAD,
- # not any head that was passed to us
- my @diff = command('diff-tree', $d,
- $gs-refname, '--');
- my @finish;
- if (@diff) {
- @finish = rebase_cmd();
- print STDERR W: $d and , $gs-refname,
- differ, using @finish:\n,
-join(\n,
Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit
Robert Luberda rob...@debian.org wrote:
dcommit didn't handle errors returned by SVN and coped very
poorly with concurrent commits that appear in SVN repository
while dcommit was running. In both cases it left git repository
in inconsistent state: index (which was reset with `git reset
--mixed' after a successful commit to SVN) no longer matched the
checkouted tree, when the following commit failed or needed to be
rebased. See http://bugs.debian.org/676904 for examples.
This patch fixes the issues by:
- introducing error handler for dcommit. The handler will try
to rebase or reset working tree before returning error to the
end user. dcommit_rebase function was extracted out of cmd_dcommit
to ensure consistency between cmd_dcommit and the error handler.
- calling `git reset --mixed' only once after all patches are
successfully committed to SVN. This ensures index is not touched
for most of the time of dcommit run.
Thanks, this seems to make sense. I'm not sure why we didn't use
SVN::Error::handler here earlier :x
A few minor comments inline...
+ if ($svn_error)
+ {
+ die ERROR: Not all changes have been committed into SVN
+ .($_no_rebase ? . : , however the committed ones
(if any) seem to be successfully integrated into the working tree. )
+ .Please see the above messages for details.\n;
+ }
Please ensure all error messages and code are readable in
80-column terminals.
Also, keep opening { on the same line as the if/unless.
+ return @diff;
+}
+
sub cmd_dcommit {
my $head = shift;
command_noisy(qw/update-index --refresh/);
@@ -904,6 +942,7 @@ sub cmd_dcommit {
}
my $rewritten_parent;
+ my $current_head = command_oneline(qw/rev-parse HEAD/);
Git::SVN::remove_username($expect_url);
if (defined($_merge_info)) {
$_merge_info =~ tr{ }{\n};
@@ -943,6 +982,13 @@ sub cmd_dcommit {
},
mergeinfo = $_merge_info,
svn_path = '');
+
+ my $err_handler = $SVN::Error::handler;
+ $SVN::Error::handler = sub {
+ my $err = shift;
+ dcommit_rebase(1, $current_head, $gs-refname,
$err);
+ };
+
if (!Git::SVN::Editor-new(\%ed_opts)-apply_diff) {
print No changes\n$d~1 == $d\n;
} elsif ($parents-{$d} @{$parents-{$d}}) {
@@ -950,31 +996,16 @@ sub cmd_dcommit {
$parents-{$d};
}
$_fetch_all ? $gs-fetch_all : $gs-fetch;
+ $SVN::Error::handler = $err_handler;
$last_rev = $cmt_rev;
next if $_no_rebase;
- # we always want to rebase against the current HEAD,
- # not any head that was passed to us
- my @diff = command('diff-tree', $d,
-$gs-refname, '--');
- my @finish;
- if (@diff) {
- @finish = rebase_cmd();
- print STDERR W: $d and , $gs-refname,
- differ, using @finish:\n,
- join(\n, @diff), \n;
- } else {
- print No changes between current HEAD and ,
- $gs-refname,
- \nResetting to the latest ,
- $gs-refname, \n;
- @finish = qw/reset --mixed/;
- }
- command_noisy(@finish, $gs-refname);
+ my @diff = dcommit_rebase(@$linear_refs == 0, $d,
$gs-refname, undef);
- $rewritten_parent = command_oneline(qw/rev-parse HEAD/);
+ $rewritten_parent = command_oneline(qw/rev-parse/,
$gs-refname);
if (@diff) {
+ $current_head = command_oneline(qw/rev-parse
HEAD/);
@refs = ();
my ($url_, $rev_, $uuid_, $gs_) =
working_head_info('HEAD', \@refs);
@@ -1019,6 +1050,7 @@ sub cmd_dcommit {
}
$parents = \%p;
$linear_refs = \@l;
+ undef $last_rev;
}
}
}
diff --git a/t/t9164-git-svn-dcommit-concrrent.sh
b/t/t9164-git-svn-dcommit-concrrent.sh
new file mode 100755
index 000..7916a63
---
[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit
dcommit didn't handle errors returned by SVN and coped very
poorly with concurrent commits that appear in SVN repository
while dcommit was running. In both cases it left git repository
in inconsistent state: index (which was reset with `git reset
--mixed' after a successful commit to SVN) no longer matched the
checkouted tree, when the following commit failed or needed to be
rebased. See http://bugs.debian.org/676904 for examples.
This patch fixes the issues by:
- introducing error handler for dcommit. The handler will try
to rebase or reset working tree before returning error to the
end user. dcommit_rebase function was extracted out of cmd_dcommit
to ensure consistency between cmd_dcommit and the error handler.
- calling `git reset --mixed' only once after all patches are
successfully committed to SVN. This ensures index is not touched
for most of the time of dcommit run.
---
git-svn.perl | 70 ++
t/t9164-git-svn-dcommit-concrrent.sh | 173 ++
2 files changed, 224 insertions(+), 19 deletions(-)
create mode 100755 t/t9164-git-svn-dcommit-concrrent.sh
diff --git a/git-svn.perl b/git-svn.perl
index 5711c57..ca3383f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -777,6 +777,44 @@ sub populate_merge_info {
return undef;
}
+sub dcommit_rebase {
+ my ($is_last, $current, $fetched_ref, $svn_error) = @_;
+ my @diff;
+
+ if ($svn_error)
+ {
+ print STDERR \nERROR from SVN: ,
$svn_error-expanded_message, \n;
+ }
+ unless ($_no_rebase)
+ {
+ # we always want to rebase against the current HEAD,
+ # not any head that was passed to us
+ @diff = command('diff-tree', $current,
+ $fetched_ref, '--');
+ my @finish;
+ if (@diff) {
+ @finish = rebase_cmd();
+ print STDERR W: $current and , $fetched_ref,
+ differ, using @finish:\n,
+join(\n, @diff), \n;
+ } elsif ($is_last) {
+ print No changes between , $current, and ,
+ $fetched_ref,
+ \nResetting to the latest ,
+ $fetched_ref, \n;
+ @finish = qw/reset --mixed/;
+ }
+ command_noisy(@finish, $fetched_ref) if @finish;
+ }
+ if ($svn_error)
+ {
+ die ERROR: Not all changes have been committed into SVN
+ .($_no_rebase ? . : , however the committed ones
(if any) seem to be successfully integrated into the working tree. )
+ .Please see the above messages for details.\n;
+ }
+ return @diff;
+}
+
sub cmd_dcommit {
my $head = shift;
command_noisy(qw/update-index --refresh/);
@@ -904,6 +942,7 @@ sub cmd_dcommit {
}
my $rewritten_parent;
+ my $current_head = command_oneline(qw/rev-parse HEAD/);
Git::SVN::remove_username($expect_url);
if (defined($_merge_info)) {
$_merge_info =~ tr{ }{\n};
@@ -943,6 +982,13 @@ sub cmd_dcommit {
},
mergeinfo = $_merge_info,
svn_path = '');
+
+ my $err_handler = $SVN::Error::handler;
+ $SVN::Error::handler = sub {
+ my $err = shift;
+ dcommit_rebase(1, $current_head, $gs-refname,
$err);
+ };
+
if (!Git::SVN::Editor-new(\%ed_opts)-apply_diff) {
print No changes\n$d~1 == $d\n;
} elsif ($parents-{$d} @{$parents-{$d}}) {
@@ -950,31 +996,16 @@ sub cmd_dcommit {
$parents-{$d};
}
$_fetch_all ? $gs-fetch_all : $gs-fetch;
+ $SVN::Error::handler = $err_handler;
$last_rev = $cmt_rev;
next if $_no_rebase;
- # we always want to rebase against the current HEAD,
- # not any head that was passed to us
- my @diff = command('diff-tree', $d,
- $gs-refname, '--');
- my @finish;
- if (@diff) {
- @finish = rebase_cmd();
- print STDERR W: $d and , $gs-refname,
- differ, using @finish:\n,
-join(\n, @diff), \n;
- } else {
- print No changes
