Skip to content

Commit e48fb75

Browse files
roblubEric Wong
roblub
authored and
Eric Wong
committed
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. Signed-off-by: Eric Wong <[email protected]>
1 parent 034161a commit e48fb75

File tree

2 files changed

+271
-19
lines changed

2 files changed

+271
-19
lines changed

‎git-svn.perl

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -777,6 +777,44 @@ sub populate_merge_info {
777777
return undef;
778778
}
779779

780+
sub dcommit_rebase {
781+
my ($is_last, $current, $fetched_ref, $svn_error) = @_;
782+
my @diff;
783+
784+
if ($svn_error) {
785+
print STDERR "\nERROR from SVN:\n",
786+
$svn_error->expanded_message, "\n";
787+
}
788+
unless ($_no_rebase) {
789+
# we always want to rebase against the current HEAD,
790+
# not any head that was passed to us
791+
@diff = command('diff-tree', $current,
792+
$fetched_ref, '--');
793+
my @finish;
794+
if (@diff) {
795+
@finish = rebase_cmd();
796+
print STDERR "W: $current and ", $fetched_ref,
797+
" differ, using @finish:\n",
798+
join("\n", @diff), "\n";
799+
} elsif ($is_last) {
800+
print "No changes between ", $current, " and ",
801+
$fetched_ref,
802+
"\nResetting to the latest ",
803+
$fetched_ref, "\n";
804+
@finish = qw/reset --mixed/;
805+
}
806+
command_noisy(@finish, $fetched_ref) if @finish;
807+
}
808+
if ($svn_error) {
809+
die "ERROR: Not all changes have been committed into SVN"
810+
.($_no_rebase ? ".\n" : ", however the committed\n"
811+
."ones (if any) seem to be successfully integrated "
812+
."into the working tree.\n")
813+
."Please see the above messages for details.\n";
814+
}
815+
return @diff;
816+
}
817+
780818
sub cmd_dcommit {
781819
my $head = shift;
782820
command_noisy(qw/update-index --refresh/);
@@ -904,6 +942,7 @@ sub cmd_dcommit {
904942
}
905943

906944
my $rewritten_parent;
945+
my $current_head = command_oneline(qw/rev-parse HEAD/);
907946
Git::SVN::remove_username($expect_url);
908947
if (defined($_merge_info)) {
909948
$_merge_info =~ tr{ }{\n};
@@ -943,38 +982,34 @@ sub cmd_dcommit {
943982
},
944983
mergeinfo => $_merge_info,
945984
svn_path => '');
985+
986+
my $err_handler = $SVN::Error::handler;
987+
$SVN::Error::handler = sub {
988+
my $err = shift;
989+
dcommit_rebase(1, $current_head, $gs->refname,
990+
$err);
991+
};
992+
946993
if (!Git::SVN::Editor->new(\%ed_opts)->apply_diff) {
947994
print "No changes\n$d~1 == $d\n";
948995
} elsif ($parents->{$d} && @{$parents->{$d}}) {
949996
$gs->{inject_parents_dcommit}->{$cmt_rev} =
950997
$parents->{$d};
951998
}
952999
$_fetch_all ? $gs->fetch_all : $gs->fetch;
1000+
$SVN::Error::handler = $err_handler;
9531001
$last_rev = $cmt_rev;
9541002
next if $_no_rebase;
9551003

956-
# we always want to rebase against the current HEAD,
957-
# not any head that was passed to us
958-
my @diff = command('diff-tree', $d,
959-
$gs->refname, '--');
960-
my @finish;
961-
if (@diff) {
962-
@finish = rebase_cmd();
963-
print STDERR "W: $d and ", $gs->refname,
964-
" differ, using @finish:\n",
965-
join("\n", @diff), "\n";
966-
} else {
967-
print "No changes between current HEAD and ",
968-
$gs->refname,
969-
"\nResetting to the latest ",
970-
$gs->refname, "\n";
971-
@finish = qw/reset --mixed/;
972-
}
973-
command_noisy(@finish, $gs->refname);
1004+
my @diff = dcommit_rebase(@$linear_refs == 0, $d,
1005+
$gs->refname, undef);
9741006

975-
$rewritten_parent = command_oneline(qw/rev-parse HEAD/);
1007+
$rewritten_parent = command_oneline(qw/rev-parse/,
1008+
$gs->refname);
9761009

9771010
if (@diff) {
1011+
$current_head = command_oneline(qw/rev-parse
1012+
HEAD/);
9781013
@refs = ();
9791014
my ($url_, $rev_, $uuid_, $gs_) =
9801015
working_head_info('HEAD', \@refs);
@@ -1019,6 +1054,7 @@ sub cmd_dcommit {
10191054
}
10201055
$parents = \%p;
10211056
$linear_refs = \@l;
1057+
undef $last_rev;
10221058
}
10231059
}
10241060
}

‎t/t9164-git-svn-dcommit-concrrent.sh

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
#!/bin/sh
2+
#
3+
# Copyright (c) 2012 Robert Luberda
4+
#
5+
6+
test_description='concurrent git svn dcommit'
7+
. ./lib-git-svn.sh
8+
9+
10+
11+
test_expect_success 'setup svn repository' '
12+
svn_cmd checkout "$svnrepo" work.svn &&
13+
(
14+
cd work.svn &&
15+
echo >file && echo > auto_updated_file
16+
svn_cmd add file auto_updated_file &&
17+
svn_cmd commit -m "initial commit"
18+
) &&
19+
svn_cmd checkout "$svnrepo" work-auto-commits.svn
20+
'
21+
N=0
22+
next_N()
23+
{
24+
N=$(( $N + 1 ))
25+
}
26+
27+
# Setup SVN repository hooks to emulate SVN failures or concurrent commits
28+
# The function adds
29+
# either pre-commit hook, which causes SVN commit given in second argument
30+
# to fail
31+
# or post-commit hook, which creates a new commit (a new line added to
32+
# auto_updated_file) after given SVN commit
33+
# The first argument contains a type of the hook
34+
# The second argument contains a number (not SVN revision) of commit
35+
# the hook should be applied for (each time the hook is run, the given
36+
# number is decreased by one until it gets 0, in which case the hook
37+
# will execute its real action)
38+
setup_hook()
39+
{
40+
hook_type="$1" # "pre-commit" or "post-commit"
41+
skip_revs="$2"
42+
[ "$hook_type" = "pre-commit" ] ||
43+
[ "$hook_type" = "post-commit" ] ||
44+
{ echo "ERROR: invalid argument ($hook_type)" \
45+
"passed to setup_hook" >&2 ; return 1; }
46+
echo "cnt=$skip_revs" > "$hook_type-counter"
47+
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
48+
hook="$rawsvnrepo/hooks/$hook_type"
49+
cat > "$hook" <<- 'EOF1'
50+
#!/bin/sh
51+
set -e
52+
cd "$1/.." # "$1" is repository location
53+
exec >> svn-hook.log 2>&1
54+
hook="$(basename "$0")"
55+
echo "*** Executing $hook $@"
56+
set -x
57+
. ./$hook-counter
58+
cnt="$(($cnt - 1))"
59+
echo "cnt=$cnt" > ./$hook-counter
60+
[ "$cnt" = "0" ] || exit 0
61+
EOF1
62+
if [ "$hook_type" = "pre-commit" ]; then
63+
echo "echo 'commit disallowed' >&2; exit 1" >> "$hook"
64+
else
65+
echo "PATH=\"$PATH\"; export PATH" >> $hook
66+
echo "svnconf=\"$svnconf\"" >> $hook
67+
cat >> "$hook" <<- 'EOF2'
68+
cd work-auto-commits.svn
69+
svn up --config-dir "$svnconf"
70+
echo "$$" >> auto_updated_file
71+
svn commit --config-dir "$svnconf" \
72+
-m "auto-committing concurrent change"
73+
exit 0
74+
EOF2
75+
fi
76+
chmod 755 "$hook"
77+
}
78+
79+
check_contents()
80+
{
81+
gitdir="$1"
82+
(cd ../work.svn && svn_cmd up) &&
83+
test_cmp file ../work.svn/file &&
84+
test_cmp auto_updated_file ../work.svn/auto_updated_file
85+
}
86+
87+
test_expect_success 'check if post-commit hook creates a concurrent commit' '
88+
setup_hook post-commit 1 &&
89+
(
90+
cd work.svn &&
91+
cp auto_updated_file au_file_saved &&
92+
echo 1 >> file &&
93+
svn_cmd commit -m "changing file" &&
94+
svn_cmd up &&
95+
test_must_fail test_cmp auto_updated_file au_file_saved
96+
)
97+
'
98+
99+
test_expect_success 'check if pre-commit hook fails' '
100+
setup_hook pre-commit 2 &&
101+
(
102+
cd work.svn &&
103+
echo 2 >> file &&
104+
svn_cmd commit -m "changing file once again" &&
105+
echo 3 >> file &&
106+
test_must_fail svn_cmd commit -m "this commit should fail" &&
107+
svn_cmd revert file
108+
)
109+
'
110+
111+
test_expect_success 'dcommit error handling' '
112+
setup_hook pre-commit 2 &&
113+
next_N && git svn clone "$svnrepo" work$N.git &&
114+
(
115+
cd work$N.git &&
116+
echo 1 >> file && git commit -am "commit change $N.1" &&
117+
echo 2 >> file && git commit -am "commit change $N.2" &&
118+
echo 3 >> file && git commit -am "commit change $N.3" &&
119+
# should fail to dcommit 2nd and 3rd change
120+
# but still should leave the repository in reasonable state
121+
test_must_fail git svn dcommit &&
122+
git update-index --refresh &&
123+
git show HEAD~2 | grep -q git-svn-id &&
124+
! git show HEAD~1 | grep -q git-svn-id &&
125+
! git show HEAD | grep -q git-svn-id
126+
)
127+
'
128+
129+
test_expect_success 'dcommit concurrent change in non-changed file' '
130+
setup_hook post-commit 2 &&
131+
next_N && git svn clone "$svnrepo" work$N.git &&
132+
(
133+
cd work$N.git &&
134+
echo 1 >> file && git commit -am "commit change $N.1" &&
135+
echo 2 >> file && git commit -am "commit change $N.2" &&
136+
echo 3 >> file && git commit -am "commit change $N.3" &&
137+
# should rebase and leave the repository in reasonable state
138+
git svn dcommit &&
139+
git update-index --refresh &&
140+
check_contents &&
141+
git show HEAD~3 | grep -q git-svn-id &&
142+
git show HEAD~2 | grep -q git-svn-id &&
143+
git show HEAD~1 | grep -q auto-committing &&
144+
git show HEAD | grep -q git-svn-id
145+
)
146+
'
147+
148+
# An utility function used in the following test
149+
delete_first_line()
150+
{
151+
file="$1" &&
152+
sed 1d < "$file" > "${file}.tmp" &&
153+
rm "$file" &&
154+
mv "${file}.tmp" "$file"
155+
}
156+
157+
test_expect_success 'dcommit concurrent non-conflicting change' '
158+
setup_hook post-commit 2 &&
159+
next_N && git svn clone "$svnrepo" work$N.git &&
160+
(
161+
cd work$N.git &&
162+
cat file >> auto_updated_file &&
163+
git commit -am "commit change $N.1" &&
164+
delete_first_line auto_updated_file &&
165+
git commit -am "commit change $N.2" &&
166+
delete_first_line auto_updated_file &&
167+
git commit -am "commit change $N.3" &&
168+
# should rebase and leave the repository in reasonable state
169+
git svn dcommit &&
170+
git update-index --refresh &&
171+
check_contents &&
172+
git show HEAD~3 | grep -q git-svn-id &&
173+
git show HEAD~2 | grep -q git-svn-id &&
174+
git show HEAD~1 | grep -q auto-committing &&
175+
git show HEAD | grep -q git-svn-id
176+
)
177+
'
178+
179+
test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '
180+
setup_hook post-commit 2 &&
181+
next_N && git svn clone "$svnrepo" work$N.git &&
182+
(
183+
cd work$N.git &&
184+
cat file >> auto_updated_file &&
185+
git commit -am "commit change $N.1" &&
186+
delete_first_line auto_updated_file &&
187+
git commit -am "commit change $N.2" &&
188+
delete_first_line auto_updated_file &&
189+
git commit -am "commit change $N.3" &&
190+
# should fail as rebase is needed
191+
test_must_fail git svn dcommit --no-rebase &&
192+
# but should leave HEAD unchanged
193+
git update-index --refresh &&
194+
! git show HEAD~2 | grep -q git-svn-id &&
195+
! git show HEAD~1 | grep -q git-svn-id &&
196+
! git show HEAD | grep -q git-svn-id
197+
)
198+
'
199+
200+
test_expect_success 'dcommit fails on concurrent conflicting change' '
201+
setup_hook post-commit 1 &&
202+
next_N && git svn clone "$svnrepo" work$N.git &&
203+
(
204+
cd work$N.git &&
205+
echo a >> file &&
206+
git commit -am "commit change $N.1" &&
207+
echo b >> auto_updated_file &&
208+
git commit -am "commit change $N.2" &&
209+
echo c >> auto_updated_file &&
210+
git commit -am "commit change $N.3" &&
211+
test_must_fail git svn dcommit && # rebase should fail
212+
test_must_fail git update-index --refresh
213+
)
214+
'
215+
216+
test_done

0 commit comments

Comments
 (0)