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 0000000..7916a63
> --- /dev/null
> +++ b/t/t9164-git-svn-dcommit-concrrent.sh
> @@ -0,0 +1,173 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Robert Luberda
> +#
> +
> +test_description='concurrent git svn dcommit'
> +. ./lib-git-svn.sh
> +
> +
> +
> +test_expect_success 'setup svn repository' '
> + svn_cmd checkout "$svnrepo" work.svn &&
> + (
> + cd work.svn &&
> + echo >file && echo > auto_updated_file
> + svn_cmd add file auto_updated_file &&
> + svn_cmd commit -m "initial commit"
> + ) &&
> + svn_cmd checkout "$svnrepo" work-auto-commits.svn
> +'
> +N=0
> +next_N()
> +{
> + N=`expr $N + 1`
Backticks don't nest properly, nowadays, we prefer:
N=$(expr $N + 1)
or even
N=$(( $N + 1 ))
ref: Documentation/CodingGuidelines
> +
> +# Setup SVN repository hooks to emulate SVN failures or concurrent commits
> +# The function adds either
> +# either pre-commit hook, which causes SVN commit given in second argument
> to fail
> +# or post-commit hook, which creates a new commit (a new line added to
> +# auto_updated_file) after given SVN commit
> +# The second argument contains a number (not SVN revision) of commit the the
> hook
> +# should be applied for.
> +setup_hook()
> +{
> + hook_type="$1" # pre-commit to fail commit or post-commit to make
> concurrent commit
> + skip_revs="$2" # skip number of revisions before applying the hook
> + # the number is decremented by one each time hook is
> called until
> + # it gets 0, in which case the real part of hook is
> executed
> + [ "$hook_type" = "pre-commit" ] || [ "$hook_type" = "post-commit" ] ||
> + { echo "ERROR: invalid argument ($hook_type) passed to
> setup_hook" >&2 ; return 1; }
> + echo "cnt=$skip_revs" > "$hook_type-counter"
> + rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
> + hook="$rawsvnrepo/hooks/$hook_type"
> + cat > "$hook" <<- 'EOF1'
> + #!/bin/sh
> + set -e
> + cd "$1/.." # "$1" is repository location
> + exec >> svn-hook.log 2>&1
> + hook="`basename "$0"`"
> + echo "*** Executing $hook $@"
> + set -x
> + . ./$hook-counter
> + cnt=`expr "$cnt" - 1` || [ $? = 1 ] # expr returns error code 1
> if expression is 0
> + echo "cnt=$cnt" > ./$hook-counter
> + [ "$cnt" = "0" ] || exit 0
> +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
> + 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....
> + echo "$$" >> auto_updated_file
> + svn commit --config-dir "$svnconf" -m "auto-committing
> concurrent change from post-commit hook"
> + exit 0
> +EOF2
> + fi
> + chmod 755 "$hook"
> +}
> +
> +check_contents()
> +{
> + gitdir="$1"
> + (cd ../work.svn && svn_cmd up) &&
> + test_cmp file ../work.svn/file &&
> + test_cmp auto_updated_file ../work.svn/auto_updated_file
> +}
> +
> +test_expect_success 'check if svn post-commit hook creates a concurrent
> commit' '
> + setup_hook post-commit 1 &&
> + (cd work.svn &&
> + cp auto_updated_file auto_updated_file_saved
Need "&&" to check for failure on cp
> +test_expect_success 'git svn dcommit concurrent non-conflicting change in
> changed file' '
> + setup_hook post-commit 2 &&
> + next_N && git svn clone "$svnrepo" work$N.git &&
> + ( cd work$N.git &&
> + cat file >> auto_updated_file && git commit -am "commit change
> $N.1" &&
> + sed -i 1d auto_updated_file && git commit -am "commit change
> $N.2" &&
> + 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.
The only places we currently use "sed -i" is scripts/tests which are
hardly run (fixup-builtins and t9810-git-p4-rcs.sh).
(I didn't expect "grep -q" to be portable enough, either,
but apparently it is portable enough nowadays :)
--
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
