[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Robert Luberda]

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 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`
+}
+
+# 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"
+			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
+		echo 1 >> file &&
+		svn_cmd commit -m "changing file" &&
+		svn_cmd up &&
+		test_must_fail test_cmp auto_updated_file auto_updated_file_saved)
+'
+
+test_expect_success 'check if svn pre-commit hook fails' '
+	setup_hook pre-commit 2 &&
+	(cd work.svn &&
+		echo 2 >> file &&
+		svn_cmd commit -m "changing file once again" &&
+		echo 3 >> file &&
+		test_must_fail svn_cmd commit -m "this commit should fail" &&
+		svn_cmd revert file)
+'
+
+test_expect_success 'git svn dcommit error handling' '
+	setup_hook pre-commit 2 &&
+	next_N && git svn clone "$svnrepo" work$N.git &&
+	( cd work$N.git &&
+		echo 1 >> file && git commit -am "commit change $N.1" &&
+		echo 2 >> file && git commit -am "commit change $N.2" &&
+		echo 3 >> file && git commit -am "commit change $N.3" &&
+		test_must_fail git svn dcommit && # should fail to dcommit 2nd and 3rd changes
+		git update-index --refresh && # but still should leave repository in reasonable state
+		git show HEAD~2   | grep -q git-svn-id &&
+		! git show HEAD~1 | grep -q git-svn-id &&
+		! git show HEAD   | grep -q git-svn-id )
+'
+
+test_expect_success 'git svn dcommit concurrent change in non-changed file' '
+	setup_hook post-commit 2 &&
+	next_N && git svn clone "$svnrepo" work$N.git &&
+	( cd work$N.git &&
+		echo 1 >> file && git commit -am "commit change $N.1" &&
+		echo 2 >> file && git commit -am "commit change $N.2" &&
+		echo 3 >> file && git commit -am "commit change $N.3" &&
+		git svn dcommit # should rebase
+		git update-index --refresh && # and leave repository in reasonable state
+		check_contents &&
+		git show HEAD~3 | grep -q git-svn-id &&
+		git show HEAD~2 | grep -q git-svn-id &&
+		git show HEAD~1 | grep -q auto-committing &&
+		git show HEAD   | grep -q git-svn-id )
+'
+
+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" &&
+		git svn dcommit && # should rebase
+		git update-index --refresh && # and leave repository in reasonable state
+		check_contents &&
+		git show HEAD~3 | grep -q git-svn-id &&
+		git show HEAD~2 | grep -q git-svn-id &&
+		git show HEAD~1 | grep -q auto-committing &&
+		git show HEAD   | grep -q git-svn-id )
+'
+
+test_expect_success 'git svn dcommit --no-rebase on 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" &&
+		test_must_fail git svn dcommit --no-rebase && # should fail as rebase is needed
+		git update-index --refresh && # but still should leave repository in reasonable state
+		! git show HEAD~2 | grep -q git-svn-id &&
+		! git show HEAD~1 | grep -q git-svn-id &&
+		! git show HEAD   | grep -q git-svn-id)
+'
+
+test_expect_success 'git svn dcommit fails on concurrent conflicting change' '
+	setup_hook post-commit 1 &&
+	next_N && git svn clone "$svnrepo" work$N.git &&
+	( cd work$N.git &&
+		echo a >> file && git commit -am "commit change $N.1" &&
+		echo b >> auto_updated_file && git commit -am "commit change $N.2" &&
+		echo c >> auto_updated_file && git commit -am "commit change $N.3" &&
+		test_must_fail git svn dcommit && # rebase should fail
+		test_must_fail git update-index --refresh )
+'
+
+test_done
-- 
1.7.10.4

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Eric Wong]

Robert Luberda <robert@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 :)

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Robert Luberda]

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

[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Robert Luberda]

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", @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 +1054,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..aac2dda
--- /dev/null
+++ b/t/t9164-git-svn-dcommit-concrrent.sh
@@ -0,0 +1,216 @@
+#!/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=$(( $N + 1 ))
+}
+
+# Setup SVN repository hooks to emulate SVN failures or concurrent commits
+# The function adds
+# 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 first argument contains a type of the hook
+# The second argument contains a number (not SVN revision) of commit
+# the hook should be applied for (each time the hook is run, the given
+# number is decreased by one until it gets 0, in which case the hook
+# will execute its real action)
+setup_hook()
+{
+	hook_type="$1"  # "pre-commit" or "post-commit"
+	skip_revs="$2"
+	[ "$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="$(($cnt - 1))"
+		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"
+			echo "$$" >> auto_updated_file
+			svn commit --config-dir "$svnconf" \
+				-m "auto-committing concurrent change"
+			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 post-commit hook creates a concurrent commit' '
+	setup_hook post-commit 1 &&
+	(
+		cd work.svn &&
+		cp auto_updated_file au_file_saved &&
+		echo 1 >> file &&
+		svn_cmd commit -m "changing file" &&
+		svn_cmd up &&
+		test_must_fail test_cmp auto_updated_file au_file_saved
+	)
+'
+
+test_expect_success 'check if pre-commit hook fails' '
+	setup_hook pre-commit 2 &&
+	(
+		cd work.svn &&
+		echo 2 >> file &&
+		svn_cmd commit -m "changing file once again" &&
+		echo 3 >> file &&
+		test_must_fail svn_cmd commit -m "this commit should fail" &&
+		svn_cmd revert file
+	)
+'
+
+test_expect_success 'dcommit error handling' '
+	setup_hook pre-commit 2 &&
+	next_N && git svn clone "$svnrepo" work$N.git &&
+	(
+		cd work$N.git &&
+		echo 1 >> file && git commit -am "commit change $N.1" &&
+		echo 2 >> file && git commit -am "commit change $N.2" &&
+		echo 3 >> file && git commit -am "commit change $N.3" &&
+		# should fail to dcommit 2nd and 3rd change
+		# but still should leave the repository in reasonable state
+		test_must_fail git svn dcommit &&
+		git update-index --refresh &&
+		git show HEAD~2   | grep -q git-svn-id &&
+		! git show HEAD~1 | grep -q git-svn-id &&
+		! git show HEAD   | grep -q git-svn-id
+	)
+'
+
+test_expect_success 'dcommit concurrent change in non-changed file' '
+	setup_hook post-commit 2 &&
+	next_N && git svn clone "$svnrepo" work$N.git &&
+	(
+		cd work$N.git &&
+		echo 1 >> file && git commit -am "commit change $N.1" &&
+		echo 2 >> file && git commit -am "commit change $N.2" &&
+		echo 3 >> file && git commit -am "commit change $N.3" &&
+		# should rebase and leave the repository in reasonable state
+		git svn dcommit &&
+		git update-index --refresh &&
+		check_contents &&
+		git show HEAD~3 | grep -q git-svn-id &&
+		git show HEAD~2 | grep -q git-svn-id &&
+		git show HEAD~1 | grep -q auto-committing &&
+		git show HEAD   | grep -q git-svn-id
+	)
+'
+
+# An utility function used in the following test
+delete_first_line()
+{
+	file="$1" &&
+	sed 1d < "$file" > "${file}.tmp" &&
+	rm "$file" &&
+	mv "${file}.tmp" "$file"
+}
+
+test_expect_success 'dcommit concurrent non-conflicting change' '
+	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" &&
+		delete_first_line auto_updated_file &&
+			git commit -am "commit change $N.2" &&
+		delete_first_line auto_updated_file &&
+			git commit -am "commit change $N.3" &&
+		# should rebase and leave the repository in reasonable state
+		git svn dcommit &&
+		git update-index --refresh &&
+		check_contents &&
+		git show HEAD~3 | grep -q git-svn-id &&
+		git show HEAD~2 | grep -q git-svn-id &&
+		git show HEAD~1 | grep -q auto-committing &&
+		git show HEAD   | grep -q git-svn-id
+	)
+'
+
+test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '
+	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" &&
+		delete_first_line auto_updated_file &&
+			git commit -am "commit change $N.2" &&
+		delete_first_line auto_updated_file &&
+			git commit -am "commit change $N.3" &&
+		# should fail as rebase is needed
+		test_must_fail git svn dcommit --no-rebase &&
+		# but should leave HEAD unchanged
+		git update-index --refresh &&
+		! git show HEAD~2 | grep -q git-svn-id &&
+		! git show HEAD~1 | grep -q git-svn-id &&
+		! git show HEAD   | grep -q git-svn-id
+	)
+'
+
+test_expect_success 'dcommit fails on concurrent conflicting change' '
+	setup_hook post-commit 1 &&
+	next_N && git svn clone "$svnrepo" work$N.git &&
+	(
+		cd work$N.git &&
+		echo a >> file &&
+			git commit -am "commit change $N.1" &&
+		echo b >> auto_updated_file &&
+			git commit -am "commit change $N.2" &&
+		echo c >> auto_updated_file &&
+			git commit -am "commit change $N.3" &&
+		test_must_fail git svn dcommit && # rebase should fail
+		test_must_fail git update-index --refresh
+	)
+'
+
+test_done
-- 
1.7.10.4

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Eric Wong]

Robert Luberda <robert@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!

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Robert Luberda <robert@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.

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Eric Wong]

Junio C Hamano <gitster@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...

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Robert Luberda]

Eric Wong wrote:

Hi,

> Junio C Hamano <gitster@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

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Junio C Hamano]

Robert Luberda <robert@debian.org> writes:

> Eric Wong wrote:
>
> Hi,
>
>> Junio C Hamano <gitster@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

Thanks.  It is this one (especially the latter half "Note that")
in the Documentation/CodingGuidelines.

 - Redirection operators should be written with space before, but no
   space after them.  In other words, write 'echo test >"$file"'
   instead of 'echo test> $file' or 'echo test > $file'.  Note that
   even though it is not required by POSIX to double-quote the
   redirection target in a variable (as shown above), our code does so
   because some versions of bash issue a warning without the quotes.

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Eric Wong]

Robert Luberda <robert@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 <robert@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 <normalperson@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

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Robert Luberda]

Eric Wong wrote:

Hi,

> 
> 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.

Regards,
robert

Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit

[Eric Wong]

Robert Luberda <robert@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.

[PATCH] t9164: More style fixes

[Robert Luberda]

Make sure t9164 conforms with the coding guidelines:
 - remove spaces after redirection operators;
 - insert spaces between function names and parentheses;
 - split `if ...; then' lines;
 - use `test' instead of `['.

Signed-off-by: Robert Luberda <robert@debian.org>
---
 t/t9164-git-svn-dcommit-concurrent.sh |   69 ++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index d8464d4..d75ebdc 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -12,16 +12,15 @@ test_expect_success 'setup svn repository' '
 	svn_cmd checkout "$svnrepo" work.svn &&
 	(
 		cd work.svn &&
-		echo >file && echo > auto_updated_file
+		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=$(( $N + 1 ))
+next_N () {
+	N=$(($N + 1))
 }
 
 # Setup SVN repository hooks to emulate SVN failures or concurrent commits
@@ -35,39 +34,39 @@ next_N()
 # the hook should be applied for (each time the hook is run, the given
 # number is decreased by one until it gets 0, in which case the hook
 # will execute its real action)
-setup_hook()
-{
+setup_hook () {
 	hook_type="$1"  # "pre-commit" or "post-commit"
 	skip_revs="$2"
-	[ "$hook_type" = "pre-commit" ] ||
-		[ "$hook_type" = "post-commit" ] ||
+	test "$hook_type" = "pre-commit"  ||
+		test "$hook_type" = "post-commit"  ||
 		{ echo "ERROR: invalid argument ($hook_type)" \
 			"passed to setup_hook" >&2 ; return 1; }
-	echo "cnt=$skip_revs" > "$hook_type-counter"
+	echo "cnt=$skip_revs" >"$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
 	hook="$rawsvnrepo/hooks/$hook_type"
-	cat > "$hook" <<- 'EOF1'
+	cat >"$hook" <<-'EOF1'
 		#!/bin/sh
 		set -e
 		cd "$1/.."  # "$1" is repository location
-		exec >> svn-hook.log 2>&1
+		exec >>svn-hook.log 2>&1
 		hook="$(basename "$0")"
 		echo "*** Executing $hook $@"
 		set -x
-		. ./$hook-counter
+		. "./$hook-counter"
 		cnt="$(($cnt - 1))"
-		echo "cnt=$cnt" > ./$hook-counter
-		[ "$cnt" = "0" ] || exit 0
+		echo "cnt=$cnt" >"./$hook-counter"
+		test "$cnt" = "0" || exit 0
 EOF1
-	if [ "$hook_type" = "pre-commit" ]; then
+	if test "$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'
+		cat >>"$hook" <<-'EOF2'
 			cd work-auto-commits.svn
 			svn up --config-dir "$svnconf"
-			echo "$$" >> auto_updated_file
+			echo "$$" >>auto_updated_file
 			svn commit --config-dir "$svnconf" \
 				-m "auto-committing concurrent change"
 			exit 0
@@ -76,8 +75,7 @@ EOF2
 	chmod 755 "$hook"
 }
 
-check_contents()
-{
+check_contents () {
 	gitdir="$1"
 	(cd ../work.svn && svn_cmd up) &&
 	test_cmp file ../work.svn/file &&
@@ -89,7 +87,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
 	(
 		cd work.svn &&
 		cp auto_updated_file au_file_saved &&
-		echo 1 >> file &&
+		echo 1 >>file &&
 		svn_cmd commit -m "changing file" &&
 		svn_cmd up &&
 		test_must_fail test_cmp auto_updated_file au_file_saved
@@ -100,9 +98,9 @@ test_expect_success 'check if pre-commit hook fails' '
 	setup_hook pre-commit 2 &&
 	(
 		cd work.svn &&
-		echo 2 >> file &&
+		echo 2 >>file &&
 		svn_cmd commit -m "changing file once again" &&
-		echo 3 >> file &&
+		echo 3 >>file &&
 		test_must_fail svn_cmd commit -m "this commit should fail" &&
 		svn_cmd revert file
 	)
@@ -113,9 +111,9 @@ test_expect_success 'dcommit error handling' '
 	next_N && git svn clone "$svnrepo" work$N.git &&
 	(
 		cd work$N.git &&
-		echo 1 >> file && git commit -am "commit change $N.1" &&
-		echo 2 >> file && git commit -am "commit change $N.2" &&
-		echo 3 >> file && git commit -am "commit change $N.3" &&
+		echo 1 >>file && git commit -am "commit change $N.1" &&
+		echo 2 >>file && git commit -am "commit change $N.2" &&
+		echo 3 >>file && git commit -am "commit change $N.3" &&
 		# should fail to dcommit 2nd and 3rd change
 		# but still should leave the repository in reasonable state
 		test_must_fail git svn dcommit &&
@@ -131,9 +129,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '
 	next_N && git svn clone "$svnrepo" work$N.git &&
 	(
 		cd work$N.git &&
-		echo 1 >> file && git commit -am "commit change $N.1" &&
-		echo 2 >> file && git commit -am "commit change $N.2" &&
-		echo 3 >> file && git commit -am "commit change $N.3" &&
+		echo 1 >>file && git commit -am "commit change $N.1" &&
+		echo 2 >>file && git commit -am "commit change $N.2" &&
+		echo 3 >>file && git commit -am "commit change $N.3" &&
 		# should rebase and leave the repository in reasonable state
 		git svn dcommit &&
 		git update-index --refresh &&
@@ -146,10 +144,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '
 '
 
 # An utility function used in the following test
-delete_first_line()
-{
+delete_first_line () {
 	file="$1" &&
-	sed 1d < "$file" > "${file}.tmp" &&
+	sed 1d <"$file" >"${file}.tmp" &&
 	rm "$file" &&
 	mv "${file}.tmp" "$file"
 }
@@ -159,7 +156,7 @@ test_expect_success 'dcommit concurrent non-conflicting change' '
 	next_N && git svn clone "$svnrepo" work$N.git &&
 	(
 		cd work$N.git &&
-		cat file >> auto_updated_file &&
+		cat file >>auto_updated_file &&
 			git commit -am "commit change $N.1" &&
 		delete_first_line auto_updated_file &&
 			git commit -am "commit change $N.2" &&
@@ -181,7 +178,7 @@ test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '
 	next_N && git svn clone "$svnrepo" work$N.git &&
 	(
 		cd work$N.git &&
-		cat file >> auto_updated_file &&
+		cat file >>auto_updated_file &&
 			git commit -am "commit change $N.1" &&
 		delete_first_line auto_updated_file &&
 			git commit -am "commit change $N.2" &&
@@ -202,11 +199,11 @@ test_expect_success 'dcommit fails on concurrent conflicting change' '
 	next_N && git svn clone "$svnrepo" work$N.git &&
 	(
 		cd work$N.git &&
-		echo a >> file &&
+		echo a >>file &&
 			git commit -am "commit change $N.1" &&
-		echo b >> auto_updated_file &&
+		echo b >>auto_updated_file &&
 			git commit -am "commit change $N.2" &&
-		echo c >> auto_updated_file &&
+		echo c >>auto_updated_file &&
 			git commit -am "commit change $N.3" &&
 		test_must_fail git svn dcommit && # rebase should fail
 		test_must_fail git update-index --refresh
-- 
1.7.10.4

Re: [PATCH] t9164: More style fixes

[Junio C Hamano]

Robert Luberda <robert@debian.org> writes:

> Make sure t9164 conforms with the coding guidelines:
>  - remove spaces after redirection operators;
>  - insert spaces between function names and parentheses;
>  - split `if ...; then' lines;
>  - use `test' instead of `['.
>
> Signed-off-by: Robert Luberda <robert@debian.org>
> ---
>  t/t9164-git-svn-dcommit-concurrent.sh |   69 ++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 36 deletions(-)

Interesting.

$ git log --oneline --stat -1 -- 't/t9164-*'
e48fb75 git svn: handle errors and concurrent commits in dcommit
 t/t9164-git-svn-dcommit-concrrent.sh | 216 +++++++++++++++++++++++++++++++++++
 1 file changed, 216 insertions(+)

I wonder where the typo in filename came from (notice the missing "u")?

Perhaps the "More" in your subject implies there were some earlier
patches I haven't seen?  The patch does not seem to apply to my tree
even with the target file renamed.

> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index d8464d4..d75ebdc 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -12,16 +12,15 @@ test_expect_success 'setup svn repository' '
>  	svn_cmd checkout "$svnrepo" work.svn &&
>  	(
>  		cd work.svn &&
> -		echo >file && echo > auto_updated_file
> +		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=$(( $N + 1 ))
> +next_N () {
> +	N=$(($N + 1))
>  }
>  
>  # Setup SVN repository hooks to emulate SVN failures or concurrent commits
> @@ -35,39 +34,39 @@ next_N()
>  # the hook should be applied for (each time the hook is run, the given
>  # number is decreased by one until it gets 0, in which case the hook
>  # will execute its real action)
> -setup_hook()
> -{
> +setup_hook () {
>  	hook_type="$1"  # "pre-commit" or "post-commit"
>  	skip_revs="$2"
> -	[ "$hook_type" = "pre-commit" ] ||
> -		[ "$hook_type" = "post-commit" ] ||
> +	test "$hook_type" = "pre-commit"  ||
> +		test "$hook_type" = "post-commit"  ||
>  		{ echo "ERROR: invalid argument ($hook_type)" \
>  			"passed to setup_hook" >&2 ; return 1; }
> -	echo "cnt=$skip_revs" > "$hook_type-counter"
> +	echo "cnt=$skip_revs" >"$hook_type-counter"
>  	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
>  	hook="$rawsvnrepo/hooks/$hook_type"
> -	cat > "$hook" <<- 'EOF1'
> +	cat >"$hook" <<-'EOF1'
>  		#!/bin/sh
>  		set -e
>  		cd "$1/.."  # "$1" is repository location
> -		exec >> svn-hook.log 2>&1
> +		exec >>svn-hook.log 2>&1
>  		hook="$(basename "$0")"
>  		echo "*** Executing $hook $@"
>  		set -x
> -		. ./$hook-counter
> +		. "./$hook-counter"
>  		cnt="$(($cnt - 1))"
> -		echo "cnt=$cnt" > ./$hook-counter
> -		[ "$cnt" = "0" ] || exit 0
> +		echo "cnt=$cnt" >"./$hook-counter"
> +		test "$cnt" = "0" || exit 0
>  EOF1
> -	if [ "$hook_type" = "pre-commit" ]; then
> +	if test "$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'
> +		cat >>"$hook" <<-'EOF2'
>  			cd work-auto-commits.svn
>  			svn up --config-dir "$svnconf"
> -			echo "$$" >> auto_updated_file
> +			echo "$$" >>auto_updated_file
>  			svn commit --config-dir "$svnconf" \
>  				-m "auto-committing concurrent change"
>  			exit 0
> @@ -76,8 +75,7 @@ EOF2
>  	chmod 755 "$hook"
>  }
>  
> -check_contents()
> -{
> +check_contents () {
>  	gitdir="$1"
>  	(cd ../work.svn && svn_cmd up) &&
>  	test_cmp file ../work.svn/file &&
> @@ -89,7 +87,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '
>  	(
>  		cd work.svn &&
>  		cp auto_updated_file au_file_saved &&
> -		echo 1 >> file &&
> +		echo 1 >>file &&
>  		svn_cmd commit -m "changing file" &&
>  		svn_cmd up &&
>  		test_must_fail test_cmp auto_updated_file au_file_saved
> @@ -100,9 +98,9 @@ test_expect_success 'check if pre-commit hook fails' '
>  	setup_hook pre-commit 2 &&
>  	(
>  		cd work.svn &&
> -		echo 2 >> file &&
> +		echo 2 >>file &&
>  		svn_cmd commit -m "changing file once again" &&
> -		echo 3 >> file &&
> +		echo 3 >>file &&
>  		test_must_fail svn_cmd commit -m "this commit should fail" &&
>  		svn_cmd revert file
>  	)
> @@ -113,9 +111,9 @@ test_expect_success 'dcommit error handling' '
>  	next_N && git svn clone "$svnrepo" work$N.git &&
>  	(
>  		cd work$N.git &&
> -		echo 1 >> file && git commit -am "commit change $N.1" &&
> -		echo 2 >> file && git commit -am "commit change $N.2" &&
> -		echo 3 >> file && git commit -am "commit change $N.3" &&
> +		echo 1 >>file && git commit -am "commit change $N.1" &&
> +		echo 2 >>file && git commit -am "commit change $N.2" &&
> +		echo 3 >>file && git commit -am "commit change $N.3" &&
>  		# should fail to dcommit 2nd and 3rd change
>  		# but still should leave the repository in reasonable state
>  		test_must_fail git svn dcommit &&
> @@ -131,9 +129,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '
>  	next_N && git svn clone "$svnrepo" work$N.git &&
>  	(
>  		cd work$N.git &&
> -		echo 1 >> file && git commit -am "commit change $N.1" &&
> -		echo 2 >> file && git commit -am "commit change $N.2" &&
> -		echo 3 >> file && git commit -am "commit change $N.3" &&
> +		echo 1 >>file && git commit -am "commit change $N.1" &&
> +		echo 2 >>file && git commit -am "commit change $N.2" &&
> +		echo 3 >>file && git commit -am "commit change $N.3" &&
>  		# should rebase and leave the repository in reasonable state
>  		git svn dcommit &&
>  		git update-index --refresh &&
> @@ -146,10 +144,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '
>  '
>  
>  # An utility function used in the following test
> -delete_first_line()
> -{
> +delete_first_line () {
>  	file="$1" &&
> -	sed 1d < "$file" > "${file}.tmp" &&
> +	sed 1d <"$file" >"${file}.tmp" &&
>  	rm "$file" &&
>  	mv "${file}.tmp" "$file"
>  }
> @@ -159,7 +156,7 @@ test_expect_success 'dcommit concurrent non-conflicting change' '
>  	next_N && git svn clone "$svnrepo" work$N.git &&
>  	(
>  		cd work$N.git &&
> -		cat file >> auto_updated_file &&
> +		cat file >>auto_updated_file &&
>  			git commit -am "commit change $N.1" &&
>  		delete_first_line auto_updated_file &&
>  			git commit -am "commit change $N.2" &&
> @@ -181,7 +178,7 @@ test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '
>  	next_N && git svn clone "$svnrepo" work$N.git &&
>  	(
>  		cd work$N.git &&
> -		cat file >> auto_updated_file &&
> +		cat file >>auto_updated_file &&
>  			git commit -am "commit change $N.1" &&
>  		delete_first_line auto_updated_file &&
>  			git commit -am "commit change $N.2" &&
> @@ -202,11 +199,11 @@ test_expect_success 'dcommit fails on concurrent conflicting change' '
>  	next_N && git svn clone "$svnrepo" work$N.git &&
>  	(
>  		cd work$N.git &&
> -		echo a >> file &&
> +		echo a >>file &&
>  			git commit -am "commit change $N.1" &&
> -		echo b >> auto_updated_file &&
> +		echo b >>auto_updated_file &&
>  			git commit -am "commit change $N.2" &&
> -		echo c >> auto_updated_file &&
> +		echo c >>auto_updated_file &&
>  			git commit -am "commit change $N.3" &&
>  		test_must_fail git svn dcommit && # rebase should fail
>  		test_must_fail git update-index --refresh