{"thread":{"id":"31165","subject":"[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","startedAt":"2012-08-01T21:26:37Z","lastAt":"2012-08-31T18:01:09Z","messageCount":14,"participants":["Robert Luberda","Eric Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"196307","messageId":"1343856397-6536-1-git-send-email-robert@debian.org","threadId":"31165","inReplyTo":null,"subject":"[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Robert Luberda","fromEmail":"robert@debian.org","sentAt":"2012-08-01T21:26:37Z","receivedAt":"2012-08-01T21:26:37Z","isPatch":true,"sender":{"key":"robert@debian.org","avatar":"https://avatars.githubusercontent.com/u/18017362?v=4"},"body":"dcommit didn't handle errors returned by SVN and coped very\npoorly with concurrent commits that appear in SVN repository\nwhile dcommit was running. In both cases it left git repository\nin inconsistent state: index (which was reset with `git reset\n--mixed' after a successful commit to SVN) no longer matched the\ncheckouted tree, when the following commit failed or needed to be\nrebased. See http://bugs.debian.org/676904 for examples.\n\nThis patch fixes the issues by:\n- introducing error handler for dcommit. The handler will try\n  to rebase or reset working tree before returning error to the\n  end user. dcommit_rebase function was extracted out of cmd_dcommit\n  to ensure consistency between cmd_dcommit and the error handler.\n- calling `git reset --mixed' only once after all patches are\n  successfully committed to SVN. This ensures index is not touched\n  for most of the time of dcommit run.\n---\n git-svn.perl                         |   70 ++++++++++----\n t/t9164-git-svn-dcommit-concrrent.sh |  173 ++++++++++++++++++++++++++++++++++\n 2 files changed, 224 insertions(+), 19 deletions(-)\n create mode 100755 t/t9164-git-svn-dcommit-concrrent.sh\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 5711c57..ca3383f 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -777,6 +777,44 @@ sub populate_merge_info {\n \treturn undef;\n }\n \n+sub dcommit_rebase {\n+\tmy ($is_last, $current, $fetched_ref, $svn_error) = @_;\n+\tmy @diff;\n+\n+\tif ($svn_error)\n+\t{\n+\t\tprint STDERR \"\\nERROR from SVN: \", $svn_error->expanded_message, \"\\n\";\n+\t}\n+\tunless ($_no_rebase)\n+\t{\n+\t\t# we always want to rebase against the current HEAD,\n+\t\t# not any head that was passed to us\n+\t\t@diff = command('diff-tree', $current,\n+\t                   $fetched_ref, '--');\n+\t\tmy @finish;\n+\t\tif (@diff) {\n+\t\t\t@finish = rebase_cmd();\n+\t\t\tprint STDERR \"W: $current and \", $fetched_ref,\n+\t\t\t             \" differ, using @finish:\\n\",\n+\t\t\t             join(\"\\n\", @diff), \"\\n\";\n+\t\t} elsif ($is_last) {\n+\t\t\tprint \"No changes between \", $current, \" and \",\n+\t\t\t      $fetched_ref,\n+\t\t\t      \"\\nResetting to the latest \",\n+\t\t\t      $fetched_ref, \"\\n\";\n+\t\t\t@finish = qw/reset --mixed/;\n+\t\t}\n+\t\tcommand_noisy(@finish, $fetched_ref) if @finish;\n+\t}\n+\tif ($svn_error)\n+\t{\n+\t\tdie \"ERROR: Not all changes have been committed into SVN\"\n+\t\t\t.($_no_rebase ? \". \" : \", however the committed ones (if any) seem to be successfully integrated into the working tree. \")\n+\t\t\t.\"Please see the above messages for details.\\n\";\n+\t}\n+\treturn @diff;\n+}\n+\n sub cmd_dcommit {\n \tmy $head = shift;\n \tcommand_noisy(qw/update-index --refresh/);\n@@ -904,6 +942,7 @@ sub cmd_dcommit {\n \t}\n \n \tmy $rewritten_parent;\n+\tmy $current_head =  command_oneline(qw/rev-parse HEAD/);\n \tGit::SVN::remove_username($expect_url);\n \tif (defined($_merge_info)) {\n \t\t$_merge_info =~ tr{ }{\\n};\n@@ -943,6 +982,13 @@ sub cmd_dcommit {\n \t\t\t                },\n \t\t\t\t\tmergeinfo => $_merge_info,\n \t\t\t                svn_path => '');\n+\n+\t\t\tmy $err_handler = $SVN::Error::handler;\n+\t\t\t$SVN::Error::handler = sub {\n+\t\t\t\tmy $err = shift;\n+\t\t\t\tdcommit_rebase(1, $current_head, $gs->refname, $err);\n+\t\t\t};\n+\n \t\t\tif (!Git::SVN::Editor->new(\\%ed_opts)->apply_diff) {\n \t\t\t\tprint \"No changes\\n$d~1 == $d\\n\";\n \t\t\t} elsif ($parents->{$d} && @{$parents->{$d}}) {\n@@ -950,31 +996,16 @@ sub cmd_dcommit {\n \t\t\t\t                               $parents->{$d};\n \t\t\t}\n \t\t\t$_fetch_all ? $gs->fetch_all : $gs->fetch;\n+\t\t\t$SVN::Error::handler = $err_handler;\n \t\t\t$last_rev = $cmt_rev;\n \t\t\tnext if $_no_rebase;\n \n-\t\t\t# we always want to rebase against the current HEAD,\n-\t\t\t# not any head that was passed to us\n-\t\t\tmy @diff = command('diff-tree', $d,\n-\t\t\t                   $gs->refname, '--');\n-\t\t\tmy @finish;\n-\t\t\tif (@diff) {\n-\t\t\t\t@finish = rebase_cmd();\n-\t\t\t\tprint STDERR \"W: $d and \", $gs->refname,\n-\t\t\t\t             \" differ, using @finish:\\n\",\n-\t\t\t\t             join(\"\\n\", @diff), \"\\n\";\n-\t\t\t} else {\n-\t\t\t\tprint \"No changes between current HEAD and \",\n-\t\t\t\t      $gs->refname,\n-\t\t\t\t      \"\\nResetting to the latest \",\n-\t\t\t\t      $gs->refname, \"\\n\";\n-\t\t\t\t@finish = qw/reset --mixed/;\n-\t\t\t}\n-\t\t\tcommand_noisy(@finish, $gs->refname);\n+\t\t\tmy @diff = dcommit_rebase(@$linear_refs == 0, $d, $gs->refname, undef);\n \n-\t\t\t$rewritten_parent = command_oneline(qw/rev-parse HEAD/);\n+\t\t\t$rewritten_parent = command_oneline(qw/rev-parse/, $gs->refname);\n \n \t\t\tif (@diff) {\n+\t\t\t\t$current_head = command_oneline(qw/rev-parse HEAD/);\n \t\t\t\t@refs = ();\n \t\t\t\tmy ($url_, $rev_, $uuid_, $gs_) =\n \t\t\t\t              working_head_info('HEAD', \\@refs);\n@@ -1019,6 +1050,7 @@ sub cmd_dcommit {\n \t\t\t\t}\n \t\t\t\t$parents = \\%p;\n \t\t\t\t$linear_refs = \\@l;\n+\t\t\t\tundef $last_rev;\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t9164-git-svn-dcommit-concrrent.sh b/t/t9164-git-svn-dcommit-concrrent.sh\nnew file mode 100755\nindex 0000000..7916a63\n--- /dev/null\n+++ b/t/t9164-git-svn-dcommit-concrrent.sh\n@@ -0,0 +1,173 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Robert Luberda\n+#\n+\n+test_description='concurrent git svn dcommit'\n+. ./lib-git-svn.sh\n+\n+\n+\n+test_expect_success 'setup svn repository' '\n+\tsvn_cmd checkout \"$svnrepo\" work.svn &&\n+\t(\n+\t\tcd work.svn &&\n+\t\techo >file && echo > auto_updated_file\n+\t\tsvn_cmd add file auto_updated_file &&\n+\t\tsvn_cmd commit -m \"initial commit\"\n+       ) &&\n+\tsvn_cmd checkout \"$svnrepo\" work-auto-commits.svn\n+'\n+N=0\n+next_N()\n+{\n+\tN=`expr $N + 1`\n+}\n+\n+# Setup SVN repository hooks to emulate SVN failures or concurrent commits\n+# The function adds either\n+# either pre-commit  hook, which causes SVN commit given in second argument to fail\n+# or     post-commit hook, which creates a new commit (a new line added to\n+#                    auto_updated_file) after given SVN commit\n+# The second argument contains a number (not SVN revision) of commit the the hook\n+# should be applied for.\n+setup_hook()\n+{\n+\thook_type=\"$1\"  # pre-commit to fail commit or post-commit to make concurrent commit\n+\tskip_revs=\"$2\"  # skip number of revisions before applying the hook\n+\t\t\t# the number is decremented by one each time hook is called until\n+\t\t\t# it gets 0, in which case the real part of hook is executed\n+\t[ \"$hook_type\" = \"pre-commit\" ] || [ \"$hook_type\" = \"post-commit\" ] ||\n+\t\t{ echo \"ERROR: invalid argument ($hook_type) passed to setup_hook\" >&2 ; return 1; }\n+\techo \"cnt=$skip_revs\" > \"$hook_type-counter\"\n+\trm -f \"$rawsvnrepo/hooks/\"*-commit # drop previous hooks\n+\thook=\"$rawsvnrepo/hooks/$hook_type\"\n+\tcat > \"$hook\" <<- 'EOF1'\n+\t\t#!/bin/sh\n+\t\tset -e\n+\t\tcd \"$1/..\"  # \"$1\" is repository location\n+\t\texec >> svn-hook.log 2>&1\n+\t\thook=\"`basename \"$0\"`\"\n+\t\techo \"*** Executing $hook $@\"\n+\t\tset -x\n+\t\t. ./$hook-counter\n+\t\tcnt=`expr \"$cnt\" - 1` || [ $? = 1 ] # expr returns error code 1 if expression is 0\n+\t\techo \"cnt=$cnt\" > ./$hook-counter\n+\t\t[ \"$cnt\" = \"0\" ] || exit 0\n+EOF1\n+\tif [ \"$hook_type\" = \"pre-commit\" ]; then\n+\t\techo \"echo 'commit disallowed' >&2; exit 1\" >> \"$hook\"\n+\telse\n+\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n+\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n+\t\tcat >> \"$hook\" <<- 'EOF2'\n+\t\t\tcd work-auto-commits.svn\n+\t\t\tsvn up --config-dir \"$svnconf\"\n+\t\t\techo \"$$\" >> auto_updated_file\n+\t\t\tsvn commit --config-dir \"$svnconf\" -m \"auto-committing concurrent change from post-commit hook\"\n+\t\t\texit 0\n+EOF2\n+\tfi\n+\tchmod 755 \"$hook\"\n+}\n+\n+check_contents()\n+{\n+\tgitdir=\"$1\"\n+\t(cd ../work.svn && svn_cmd up) &&\n+\ttest_cmp file ../work.svn/file &&\n+\ttest_cmp auto_updated_file ../work.svn/auto_updated_file\n+}\n+\n+test_expect_success 'check if svn post-commit hook creates a concurrent commit' '\n+\tsetup_hook post-commit 1 &&\n+\t(cd work.svn &&\n+\t\tcp auto_updated_file auto_updated_file_saved\n+\t\techo 1 >> file &&\n+\t\tsvn_cmd commit -m \"changing file\" &&\n+\t\tsvn_cmd up &&\n+\t\ttest_must_fail test_cmp auto_updated_file auto_updated_file_saved)\n+'\n+\n+test_expect_success 'check if svn pre-commit hook fails' '\n+\tsetup_hook pre-commit 2 &&\n+\t(cd work.svn &&\n+\t\techo 2 >> file &&\n+\t\tsvn_cmd commit -m \"changing file once again\" &&\n+\t\techo 3 >> file &&\n+\t\ttest_must_fail svn_cmd commit -m \"this commit should fail\" &&\n+\t\tsvn_cmd revert file)\n+'\n+\n+test_expect_success 'git svn dcommit error handling' '\n+\tsetup_hook pre-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t( cd work$N.git &&\n+\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n+\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n+\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n+\t\ttest_must_fail git svn dcommit && # should fail to dcommit 2nd and 3rd changes\n+\t\tgit update-index --refresh && # but still should leave repository in reasonable state\n+\t\tgit show HEAD~2   | grep -q git-svn-id &&\n+\t\t! git show HEAD~1 | grep -q git-svn-id &&\n+\t\t! git show HEAD   | grep -q git-svn-id )\n+'\n+\n+test_expect_success 'git svn dcommit concurrent change in non-changed file' '\n+\tsetup_hook post-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t( cd work$N.git &&\n+\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n+\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n+\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n+\t\tgit svn dcommit # should rebase\n+\t\tgit update-index --refresh && # and leave repository in reasonable state\n+\t\tcheck_contents &&\n+\t\tgit show HEAD~3 | grep -q git-svn-id &&\n+\t\tgit show HEAD~2 | grep -q git-svn-id &&\n+\t\tgit show HEAD~1 | grep -q auto-committing &&\n+\t\tgit show HEAD   | grep -q git-svn-id )\n+'\n+\n+test_expect_success 'git svn dcommit concurrent non-conflicting change in changed file' '\n+\tsetup_hook post-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t( cd work$N.git &&\n+\t\tcat file >> auto_updated_file && git commit -am \"commit change $N.1\" &&\n+\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.2\" &&\n+\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.3\" &&\n+\t\tgit svn dcommit && # should rebase\n+\t\tgit update-index --refresh && # and leave repository in reasonable state\n+\t\tcheck_contents &&\n+\t\tgit show HEAD~3 | grep -q git-svn-id &&\n+\t\tgit show HEAD~2 | grep -q git-svn-id &&\n+\t\tgit show HEAD~1 | grep -q auto-committing &&\n+\t\tgit show HEAD   | grep -q git-svn-id )\n+'\n+\n+test_expect_success 'git svn dcommit --no-rebase on concurrent non-conflicting change in changed file' '\n+\tsetup_hook post-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t( cd work$N.git &&\n+\t\tcat file >> auto_updated_file && git commit -am \"commit change $N.1\" &&\n+\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.2\" &&\n+\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.3\" &&\n+\t\ttest_must_fail git svn dcommit --no-rebase && # should fail as rebase is needed\n+\t\tgit update-index --refresh && # but still should leave repository in reasonable state\n+\t\t! git show HEAD~2 | grep -q git-svn-id &&\n+\t\t! git show HEAD~1 | grep -q git-svn-id &&\n+\t\t! git show HEAD   | grep -q git-svn-id)\n+'\n+\n+test_expect_success 'git svn dcommit fails on concurrent conflicting change' '\n+\tsetup_hook post-commit 1 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t( cd work$N.git &&\n+\t\techo a >> file && git commit -am \"commit change $N.1\" &&\n+\t\techo b >> auto_updated_file && git commit -am \"commit change $N.2\" &&\n+\t\techo c >> auto_updated_file && git commit -am \"commit change $N.3\" &&\n+\t\ttest_must_fail git svn dcommit && # rebase should fail\n+\t\ttest_must_fail git update-index --refresh )\n+'\n+\n+test_done\n-- \n1.7.10.4\n"},{"id":"196329","messageId":"20120802104421.GA13271@dcvr.yhbt.net","threadId":"31165","inReplyTo":"1343856397-6536-1-git-send-email-robert@debian.org","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-08-02T10:44:21Z","receivedAt":"2012-08-02T10:44:21Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Robert Luberda <robert@debian.org> wrote:\n> dcommit didn't handle errors returned by SVN and coped very\n> poorly with concurrent commits that appear in SVN repository\n> while dcommit was running. In both cases it left git repository\n> in inconsistent state: index (which was reset with `git reset\n> --mixed' after a successful commit to SVN) no longer matched the\n> checkouted tree, when the following commit failed or needed to be\n> rebased. See http://bugs.debian.org/676904 for examples.\n> \n> This patch fixes the issues by:\n> - introducing error handler for dcommit. The handler will try\n>   to rebase or reset working tree before returning error to the\n>   end user. dcommit_rebase function was extracted out of cmd_dcommit\n>   to ensure consistency between cmd_dcommit and the error handler.\n> - calling `git reset --mixed' only once after all patches are\n>   successfully committed to SVN. This ensures index is not touched\n>   for most of the time of dcommit run.\n\nThanks, this seems to make sense.  I'm not sure why we didn't use\nSVN::Error::handler here earlier :x\n\nA few minor comments inline...\n\n> +\tif ($svn_error)\n> +\t{\n> +\t\tdie \"ERROR: Not all changes have been committed into SVN\"\n> +\t\t\t.($_no_rebase ? \". \" : \", however the committed ones (if any) seem to be successfully integrated into the working tree. \")\n> +\t\t\t.\"Please see the above messages for details.\\n\";\n> +\t}\n\nPlease ensure all error messages and code are readable in\n80-column terminals.\n\nAlso, keep opening \"{\" on the same line as the if/unless.\n\n> +\treturn @diff;\n> +}\n> +\n>  sub cmd_dcommit {\n>  \tmy $head = shift;\n>  \tcommand_noisy(qw/update-index --refresh/);\n> @@ -904,6 +942,7 @@ sub cmd_dcommit {\n>  \t}\n>  \n>  \tmy $rewritten_parent;\n> +\tmy $current_head =  command_oneline(qw/rev-parse HEAD/);\n>  \tGit::SVN::remove_username($expect_url);\n>  \tif (defined($_merge_info)) {\n>  \t\t$_merge_info =~ tr{ }{\\n};\n> @@ -943,6 +982,13 @@ sub cmd_dcommit {\n>  \t\t\t                },\n>  \t\t\t\t\tmergeinfo => $_merge_info,\n>  \t\t\t                svn_path => '');\n> +\n> +\t\t\tmy $err_handler = $SVN::Error::handler;\n> +\t\t\t$SVN::Error::handler = sub {\n> +\t\t\t\tmy $err = shift;\n> +\t\t\t\tdcommit_rebase(1, $current_head, $gs->refname, $err);\n> +\t\t\t};\n> +\n>  \t\t\tif (!Git::SVN::Editor->new(\\%ed_opts)->apply_diff) {\n>  \t\t\t\tprint \"No changes\\n$d~1 == $d\\n\";\n>  \t\t\t} elsif ($parents->{$d} && @{$parents->{$d}}) {\n> @@ -950,31 +996,16 @@ sub cmd_dcommit {\n>  \t\t\t\t                               $parents->{$d};\n>  \t\t\t}\n>  \t\t\t$_fetch_all ? $gs->fetch_all : $gs->fetch;\n> +\t\t\t$SVN::Error::handler = $err_handler;\n>  \t\t\t$last_rev = $cmt_rev;\n>  \t\t\tnext if $_no_rebase;\n>  \n> -\t\t\t# we always want to rebase against the current HEAD,\n> -\t\t\t# not any head that was passed to us\n> -\t\t\tmy @diff = command('diff-tree', $d,\n> -\t\t\t                   $gs->refname, '--');\n> -\t\t\tmy @finish;\n> -\t\t\tif (@diff) {\n> -\t\t\t\t@finish = rebase_cmd();\n> -\t\t\t\tprint STDERR \"W: $d and \", $gs->refname,\n> -\t\t\t\t             \" differ, using @finish:\\n\",\n> -\t\t\t\t             join(\"\\n\", @diff), \"\\n\";\n> -\t\t\t} else {\n> -\t\t\t\tprint \"No changes between current HEAD and \",\n> -\t\t\t\t      $gs->refname,\n> -\t\t\t\t      \"\\nResetting to the latest \",\n> -\t\t\t\t      $gs->refname, \"\\n\";\n> -\t\t\t\t@finish = qw/reset --mixed/;\n> -\t\t\t}\n> -\t\t\tcommand_noisy(@finish, $gs->refname);\n> +\t\t\tmy @diff = dcommit_rebase(@$linear_refs == 0, $d, $gs->refname, undef);\n>  \n> -\t\t\t$rewritten_parent = command_oneline(qw/rev-parse HEAD/);\n> +\t\t\t$rewritten_parent = command_oneline(qw/rev-parse/, $gs->refname);\n>  \n>  \t\t\tif (@diff) {\n> +\t\t\t\t$current_head = command_oneline(qw/rev-parse HEAD/);\n>  \t\t\t\t@refs = ();\n>  \t\t\t\tmy ($url_, $rev_, $uuid_, $gs_) =\n>  \t\t\t\t              working_head_info('HEAD', \\@refs);\n> @@ -1019,6 +1050,7 @@ sub cmd_dcommit {\n>  \t\t\t\t}\n>  \t\t\t\t$parents = \\%p;\n>  \t\t\t\t$linear_refs = \\@l;\n> +\t\t\t\tundef $last_rev;\n>  \t\t\t}\n>  \t\t}\n>  \t}\n> diff --git a/t/t9164-git-svn-dcommit-concrrent.sh b/t/t9164-git-svn-dcommit-concrrent.sh\n> new file mode 100755\n> index 0000000..7916a63\n> --- /dev/null\n> +++ b/t/t9164-git-svn-dcommit-concrrent.sh\n> @@ -0,0 +1,173 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2012 Robert Luberda\n> +#\n> +\n> +test_description='concurrent git svn dcommit'\n> +. ./lib-git-svn.sh\n> +\n> +\n> +\n> +test_expect_success 'setup svn repository' '\n> +\tsvn_cmd checkout \"$svnrepo\" work.svn &&\n> +\t(\n> +\t\tcd work.svn &&\n> +\t\techo >file && echo > auto_updated_file\n> +\t\tsvn_cmd add file auto_updated_file &&\n> +\t\tsvn_cmd commit -m \"initial commit\"\n> +       ) &&\n> +\tsvn_cmd checkout \"$svnrepo\" work-auto-commits.svn\n> +'\n> +N=0\n> +next_N()\n> +{\n> +\tN=`expr $N + 1`\n\nBackticks don't nest properly, nowadays, we prefer:\n\n\tN=$(expr $N + 1)\n\nor even\n\n\tN=$(( $N + 1 ))\n\nref: Documentation/CodingGuidelines\n\n> +\n> +# Setup SVN repository hooks to emulate SVN failures or concurrent commits\n> +# The function adds either\n> +# either pre-commit  hook, which causes SVN commit given in second argument to fail\n> +# or     post-commit hook, which creates a new commit (a new line added to\n> +#                    auto_updated_file) after given SVN commit\n> +# The second argument contains a number (not SVN revision) of commit the the hook\n> +# should be applied for.\n> +setup_hook()\n> +{\n> +\thook_type=\"$1\"  # pre-commit to fail commit or post-commit to make concurrent commit\n> +\tskip_revs=\"$2\"  # skip number of revisions before applying the hook\n> +\t\t\t# the number is decremented by one each time hook is called until\n> +\t\t\t# it gets 0, in which case the real part of hook is executed\n> +\t[ \"$hook_type\" = \"pre-commit\" ] || [ \"$hook_type\" = \"post-commit\" ] ||\n\n\n\n> +\t\t{ echo \"ERROR: invalid argument ($hook_type) passed to setup_hook\" >&2 ; return 1; }\n> +\techo \"cnt=$skip_revs\" > \"$hook_type-counter\"\n> +\trm -f \"$rawsvnrepo/hooks/\"*-commit # drop previous hooks\n> +\thook=\"$rawsvnrepo/hooks/$hook_type\"\n> +\tcat > \"$hook\" <<- 'EOF1'\n> +\t\t#!/bin/sh\n> +\t\tset -e\n> +\t\tcd \"$1/..\"  # \"$1\" is repository location\n> +\t\texec >> svn-hook.log 2>&1\n> +\t\thook=\"`basename \"$0\"`\"\n> +\t\techo \"*** Executing $hook $@\"\n> +\t\tset -x\n> +\t\t. ./$hook-counter\n> +\t\tcnt=`expr \"$cnt\" - 1` || [ $? = 1 ] # expr returns error code 1 if expression is 0\n> +\t\techo \"cnt=$cnt\" > ./$hook-counter\n> +\t\t[ \"$cnt\" = \"0\" ] || exit 0\n> +EOF1\n> +\tif [ \"$hook_type\" = \"pre-commit\" ]; then\n> +\t\techo \"echo 'commit disallowed' >&2; exit 1\" >> \"$hook\"\n> +\telse\n> +\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n> +\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n> +\t\tcat >> \"$hook\" <<- 'EOF2'\n> +\t\t\tcd work-auto-commits.svn\n> +\t\t\tsvn up --config-dir \"$svnconf\"\n\nThat doesn't seem to interact well with users who depend on\nsvn_cmd pointing to something non-standard.  Not sure\nwhat to do about it, though....\n\n> +\t\t\techo \"$$\" >> auto_updated_file\n> +\t\t\tsvn commit --config-dir \"$svnconf\" -m \"auto-committing concurrent change from post-commit hook\"\n> +\t\t\texit 0\n> +EOF2\n> +\tfi\n> +\tchmod 755 \"$hook\"\n> +}\n> +\n> +check_contents()\n> +{\n> +\tgitdir=\"$1\"\n> +\t(cd ../work.svn && svn_cmd up) &&\n> +\ttest_cmp file ../work.svn/file &&\n> +\ttest_cmp auto_updated_file ../work.svn/auto_updated_file\n> +}\n> +\n> +test_expect_success 'check if svn post-commit hook creates a concurrent commit' '\n> +\tsetup_hook post-commit 1 &&\n> +\t(cd work.svn &&\n> +\t\tcp auto_updated_file auto_updated_file_saved\n\nNeed \"&&\" to check for failure on cp\n\n> +test_expect_success 'git svn dcommit concurrent non-conflicting change in changed file' '\n> +\tsetup_hook post-commit 2 &&\n> +\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n> +\t( cd work$N.git &&\n> +\t\tcat file >> auto_updated_file && git commit -am \"commit change $N.1\" &&\n> +\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.2\" &&\n> +\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.3\" &&\n\nI don't believe \"sed -i\" is portable enough for this test.\n\nThe only places we currently use \"sed -i\" is scripts/tests which are\nhardly run (fixup-builtins and t9810-git-p4-rcs.sh).\n\n(I didn't expect \"grep -q\" to be portable enough, either,\n but apparently it is portable enough nowadays :)\n"},{"id":"196638","messageId":"5021F9D4.1010700@debian.org","threadId":"31165","inReplyTo":"20120802104421.GA13271@dcvr.yhbt.net","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Robert Luberda","fromEmail":"robert@debian.org","sentAt":"2012-08-08T05:32:04Z","receivedAt":"2012-08-08T05:32:04Z","isPatch":true,"sender":{"key":"robert@debian.org","avatar":"https://avatars.githubusercontent.com/u/18017362?v=4"},"body":"Eric Wong wrote:\n\nHi,\n\n> \n> A few minor comments inline...\n> Please ensure all error messages and code are readable in\n> 80-column terminals.\n> Also, keep opening \"{\" on the same line as the if/unless.\n> Backticks don't nest properly, nowadays, we prefer:\n> \tN=$(expr $N + 1)\n>> +\t\tcp auto_updated_file auto_updated_file_saved\n> Need \"&&\" to check for failure on cp\n>> +\t\tsed -i 1d auto_updated_file && git commit -am \"commit change $N.3\" &&\n> I don't believe \"sed -i\" is portable enough for this test.\n\n\nMany thanks for the comments! I've fixed all of the above and will send\nupdated patch in next e-mail. Please let me know if you have any further\ncomments.\n\n\n\n>> +\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n>> +\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n>> +\t\tcat >> \"$hook\" <<- 'EOF2'\n>> +\t\t\tcd work-auto-commits.svn\n>> +\t\t\tsvn up --config-dir \"$svnconf\"\n> \n> That doesn't seem to interact well with users who depend on\n> svn_cmd pointing to something non-standard.  Not sure\n> what to do about it, though....\n\nI have no idea how to change it either. I've tried to source the\nlib-git-svn.sh file inside the hook, but it sources test-lib.sh, and the\nlatter script doesn't work well if it is sourced by non-test script.\nAnyway I the part of my original patch unchanged.\n\nRegards,\nrobert\n"},{"id":"196639","messageId":"1344404100-9383-1-git-send-email-robert@debian.org","threadId":"31165","inReplyTo":"5021F9D4.1010700@debian.org","subject":"[PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Robert Luberda","fromEmail":"robert@debian.org","sentAt":"2012-08-08T05:35:00Z","receivedAt":"2012-08-08T05:35:00Z","isPatch":true,"sender":{"key":"robert@debian.org","avatar":"https://avatars.githubusercontent.com/u/18017362?v=4"},"body":"dcommit didn't handle errors returned by SVN and coped very\npoorly with concurrent commits that appear in SVN repository\nwhile dcommit was running. In both cases it left git repository\nin inconsistent state: index (which was reset with `git reset\n--mixed' after a successful commit to SVN) no longer matched the\ncheckouted tree, when the following commit failed or needed to be\nrebased. See http://bugs.debian.org/676904 for examples.\n\nThis patch fixes the issues by:\n- introducing error handler for dcommit. The handler will try\n  to rebase or reset working tree before returning error to the\n  end user. dcommit_rebase function was extracted out of cmd_dcommit\n  to ensure consistency between cmd_dcommit and the error handler.\n- calling `git reset --mixed' only once after all patches are\n  successfully committed to SVN. This ensures index is not touched\n  for most of the time of dcommit run.\n---\n git-svn.perl                         |   74 +++++++++---\n t/t9164-git-svn-dcommit-concrrent.sh |  216 ++++++++++++++++++++++++++++++++++\n 2 files changed, 271 insertions(+), 19 deletions(-)\n create mode 100755 t/t9164-git-svn-dcommit-concrrent.sh\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 5711c57..828b8f0 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -777,6 +777,44 @@ sub populate_merge_info {\n \treturn undef;\n }\n \n+sub dcommit_rebase {\n+\tmy ($is_last, $current, $fetched_ref, $svn_error) = @_;\n+\tmy @diff;\n+\n+\tif ($svn_error) {\n+\t\tprint STDERR \"\\nERROR from SVN:\\n\",\n+\t\t\t\t$svn_error->expanded_message, \"\\n\";\n+\t}\n+\tunless ($_no_rebase) {\n+\t\t# we always want to rebase against the current HEAD,\n+\t\t# not any head that was passed to us\n+\t\t@diff = command('diff-tree', $current,\n+\t                   $fetched_ref, '--');\n+\t\tmy @finish;\n+\t\tif (@diff) {\n+\t\t\t@finish = rebase_cmd();\n+\t\t\tprint STDERR \"W: $current and \", $fetched_ref,\n+\t\t\t             \" differ, using @finish:\\n\",\n+\t\t\t             join(\"\\n\", @diff), \"\\n\";\n+\t\t} elsif ($is_last) {\n+\t\t\tprint \"No changes between \", $current, \" and \",\n+\t\t\t      $fetched_ref,\n+\t\t\t      \"\\nResetting to the latest \",\n+\t\t\t      $fetched_ref, \"\\n\";\n+\t\t\t@finish = qw/reset --mixed/;\n+\t\t}\n+\t\tcommand_noisy(@finish, $fetched_ref) if @finish;\n+\t}\n+\tif ($svn_error) {\n+\t\tdie \"ERROR: Not all changes have been committed into SVN\"\n+\t\t\t.($_no_rebase ? \".\\n\" : \", however the committed\\n\"\n+\t\t\t.\"ones (if any) seem to be successfully integrated \"\n+\t\t\t.\"into the working tree.\\n\")\n+\t\t\t.\"Please see the above messages for details.\\n\";\n+\t}\n+\treturn @diff;\n+}\n+\n sub cmd_dcommit {\n \tmy $head = shift;\n \tcommand_noisy(qw/update-index --refresh/);\n@@ -904,6 +942,7 @@ sub cmd_dcommit {\n \t}\n \n \tmy $rewritten_parent;\n+\tmy $current_head = command_oneline(qw/rev-parse HEAD/);\n \tGit::SVN::remove_username($expect_url);\n \tif (defined($_merge_info)) {\n \t\t$_merge_info =~ tr{ }{\\n};\n@@ -943,6 +982,14 @@ sub cmd_dcommit {\n \t\t\t                },\n \t\t\t\t\tmergeinfo => $_merge_info,\n \t\t\t                svn_path => '');\n+\n+\t\t\tmy $err_handler = $SVN::Error::handler;\n+\t\t\t$SVN::Error::handler = sub {\n+\t\t\t\tmy $err = shift;\n+\t\t\t\tdcommit_rebase(1, $current_head, $gs->refname,\n+\t\t\t\t\t$err);\n+\t\t\t};\n+\n \t\t\tif (!Git::SVN::Editor->new(\\%ed_opts)->apply_diff) {\n \t\t\t\tprint \"No changes\\n$d~1 == $d\\n\";\n \t\t\t} elsif ($parents->{$d} && @{$parents->{$d}}) {\n@@ -950,31 +997,19 @@ sub cmd_dcommit {\n \t\t\t\t                               $parents->{$d};\n \t\t\t}\n \t\t\t$_fetch_all ? $gs->fetch_all : $gs->fetch;\n+\t\t\t$SVN::Error::handler = $err_handler;\n \t\t\t$last_rev = $cmt_rev;\n \t\t\tnext if $_no_rebase;\n \n-\t\t\t# we always want to rebase against the current HEAD,\n-\t\t\t# not any head that was passed to us\n-\t\t\tmy @diff = command('diff-tree', $d,\n-\t\t\t                   $gs->refname, '--');\n-\t\t\tmy @finish;\n-\t\t\tif (@diff) {\n-\t\t\t\t@finish = rebase_cmd();\n-\t\t\t\tprint STDERR \"W: $d and \", $gs->refname,\n-\t\t\t\t             \" differ, using @finish:\\n\",\n-\t\t\t\t             join(\"\\n\", @diff), \"\\n\";\n-\t\t\t} else {\n-\t\t\t\tprint \"No changes between current HEAD and \",\n-\t\t\t\t      $gs->refname,\n-\t\t\t\t      \"\\nResetting to the latest \",\n-\t\t\t\t      $gs->refname, \"\\n\";\n-\t\t\t\t@finish = qw/reset --mixed/;\n-\t\t\t}\n-\t\t\tcommand_noisy(@finish, $gs->refname);\n+\t\t\tmy @diff = dcommit_rebase(@$linear_refs == 0, $d,\n+\t\t\t\t\t\t$gs->refname, undef);\n \n-\t\t\t$rewritten_parent = command_oneline(qw/rev-parse HEAD/);\n+\t\t\t$rewritten_parent = command_oneline(qw/rev-parse/,\n+\t\t\t\t\t\t\t$gs->refname);\n \n \t\t\tif (@diff) {\n+\t\t\t\t$current_head = command_oneline(qw/rev-parse\n+\t\t\t\t\t\t\t\tHEAD/);\n \t\t\t\t@refs = ();\n \t\t\t\tmy ($url_, $rev_, $uuid_, $gs_) =\n \t\t\t\t              working_head_info('HEAD', \\@refs);\n@@ -1019,6 +1054,7 @@ sub cmd_dcommit {\n \t\t\t\t}\n \t\t\t\t$parents = \\%p;\n \t\t\t\t$linear_refs = \\@l;\n+\t\t\t\tundef $last_rev;\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t9164-git-svn-dcommit-concrrent.sh b/t/t9164-git-svn-dcommit-concrrent.sh\nnew file mode 100755\nindex 0000000..aac2dda\n--- /dev/null\n+++ b/t/t9164-git-svn-dcommit-concrrent.sh\n@@ -0,0 +1,216 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Robert Luberda\n+#\n+\n+test_description='concurrent git svn dcommit'\n+. ./lib-git-svn.sh\n+\n+\n+\n+test_expect_success 'setup svn repository' '\n+\tsvn_cmd checkout \"$svnrepo\" work.svn &&\n+\t(\n+\t\tcd work.svn &&\n+\t\techo >file && echo > auto_updated_file\n+\t\tsvn_cmd add file auto_updated_file &&\n+\t\tsvn_cmd commit -m \"initial commit\"\n+\t) &&\n+\tsvn_cmd checkout \"$svnrepo\" work-auto-commits.svn\n+'\n+N=0\n+next_N()\n+{\n+\tN=$(( $N + 1 ))\n+}\n+\n+# Setup SVN repository hooks to emulate SVN failures or concurrent commits\n+# The function adds\n+# either pre-commit  hook, which causes SVN commit given in second argument\n+#                    to fail\n+# or     post-commit hook, which creates a new commit (a new line added to\n+#                    auto_updated_file) after given SVN commit\n+# The first argument contains a type of the hook\n+# The second argument contains a number (not SVN revision) of commit\n+# the hook should be applied for (each time the hook is run, the given\n+# number is decreased by one until it gets 0, in which case the hook\n+# will execute its real action)\n+setup_hook()\n+{\n+\thook_type=\"$1\"  # \"pre-commit\" or \"post-commit\"\n+\tskip_revs=\"$2\"\n+\t[ \"$hook_type\" = \"pre-commit\" ] ||\n+\t\t[ \"$hook_type\" = \"post-commit\" ] ||\n+\t\t{ echo \"ERROR: invalid argument ($hook_type)\" \\\n+\t\t\t\"passed to setup_hook\" >&2 ; return 1; }\n+\techo \"cnt=$skip_revs\" > \"$hook_type-counter\"\n+\trm -f \"$rawsvnrepo/hooks/\"*-commit # drop previous hooks\n+\thook=\"$rawsvnrepo/hooks/$hook_type\"\n+\tcat > \"$hook\" <<- 'EOF1'\n+\t\t#!/bin/sh\n+\t\tset -e\n+\t\tcd \"$1/..\"  # \"$1\" is repository location\n+\t\texec >> svn-hook.log 2>&1\n+\t\thook=\"$(basename \"$0\")\"\n+\t\techo \"*** Executing $hook $@\"\n+\t\tset -x\n+\t\t. ./$hook-counter\n+\t\tcnt=\"$(($cnt - 1))\"\n+\t\techo \"cnt=$cnt\" > ./$hook-counter\n+\t\t[ \"$cnt\" = \"0\" ] || exit 0\n+EOF1\n+\tif [ \"$hook_type\" = \"pre-commit\" ]; then\n+\t\techo \"echo 'commit disallowed' >&2; exit 1\" >> \"$hook\"\n+\telse\n+\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n+\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n+\t\tcat >> \"$hook\" <<- 'EOF2'\n+\t\t\tcd work-auto-commits.svn\n+\t\t\tsvn up --config-dir \"$svnconf\"\n+\t\t\techo \"$$\" >> auto_updated_file\n+\t\t\tsvn commit --config-dir \"$svnconf\" \\\n+\t\t\t\t-m \"auto-committing concurrent change\"\n+\t\t\texit 0\n+EOF2\n+\tfi\n+\tchmod 755 \"$hook\"\n+}\n+\n+check_contents()\n+{\n+\tgitdir=\"$1\"\n+\t(cd ../work.svn && svn_cmd up) &&\n+\ttest_cmp file ../work.svn/file &&\n+\ttest_cmp auto_updated_file ../work.svn/auto_updated_file\n+}\n+\n+test_expect_success 'check if post-commit hook creates a concurrent commit' '\n+\tsetup_hook post-commit 1 &&\n+\t(\n+\t\tcd work.svn &&\n+\t\tcp auto_updated_file au_file_saved &&\n+\t\techo 1 >> file &&\n+\t\tsvn_cmd commit -m \"changing file\" &&\n+\t\tsvn_cmd up &&\n+\t\ttest_must_fail test_cmp auto_updated_file au_file_saved\n+\t)\n+'\n+\n+test_expect_success 'check if pre-commit hook fails' '\n+\tsetup_hook pre-commit 2 &&\n+\t(\n+\t\tcd work.svn &&\n+\t\techo 2 >> file &&\n+\t\tsvn_cmd commit -m \"changing file once again\" &&\n+\t\techo 3 >> file &&\n+\t\ttest_must_fail svn_cmd commit -m \"this commit should fail\" &&\n+\t\tsvn_cmd revert file\n+\t)\n+'\n+\n+test_expect_success 'dcommit error handling' '\n+\tsetup_hook pre-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t(\n+\t\tcd work$N.git &&\n+\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n+\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n+\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n+\t\t# should fail to dcommit 2nd and 3rd change\n+\t\t# but still should leave the repository in reasonable state\n+\t\ttest_must_fail git svn dcommit &&\n+\t\tgit update-index --refresh &&\n+\t\tgit show HEAD~2   | grep -q git-svn-id &&\n+\t\t! git show HEAD~1 | grep -q git-svn-id &&\n+\t\t! git show HEAD   | grep -q git-svn-id\n+\t)\n+'\n+\n+test_expect_success 'dcommit concurrent change in non-changed file' '\n+\tsetup_hook post-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t(\n+\t\tcd work$N.git &&\n+\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n+\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n+\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n+\t\t# should rebase and leave the repository in reasonable state\n+\t\tgit svn dcommit &&\n+\t\tgit update-index --refresh &&\n+\t\tcheck_contents &&\n+\t\tgit show HEAD~3 | grep -q git-svn-id &&\n+\t\tgit show HEAD~2 | grep -q git-svn-id &&\n+\t\tgit show HEAD~1 | grep -q auto-committing &&\n+\t\tgit show HEAD   | grep -q git-svn-id\n+\t)\n+'\n+\n+# An utility function used in the following test\n+delete_first_line()\n+{\n+\tfile=\"$1\" &&\n+\tsed 1d < \"$file\" > \"${file}.tmp\" &&\n+\trm \"$file\" &&\n+\tmv \"${file}.tmp\" \"$file\"\n+}\n+\n+test_expect_success 'dcommit concurrent non-conflicting change' '\n+\tsetup_hook post-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t(\n+\t\tcd work$N.git &&\n+\t\tcat file >> auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.1\" &&\n+\t\tdelete_first_line auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.2\" &&\n+\t\tdelete_first_line auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.3\" &&\n+\t\t# should rebase and leave the repository in reasonable state\n+\t\tgit svn dcommit &&\n+\t\tgit update-index --refresh &&\n+\t\tcheck_contents &&\n+\t\tgit show HEAD~3 | grep -q git-svn-id &&\n+\t\tgit show HEAD~2 | grep -q git-svn-id &&\n+\t\tgit show HEAD~1 | grep -q auto-committing &&\n+\t\tgit show HEAD   | grep -q git-svn-id\n+\t)\n+'\n+\n+test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '\n+\tsetup_hook post-commit 2 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t(\n+\t\tcd work$N.git &&\n+\t\tcat file >> auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.1\" &&\n+\t\tdelete_first_line auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.2\" &&\n+\t\tdelete_first_line auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.3\" &&\n+\t\t# should fail as rebase is needed\n+\t\ttest_must_fail git svn dcommit --no-rebase &&\n+\t\t# but should leave HEAD unchanged\n+\t\tgit update-index --refresh &&\n+\t\t! git show HEAD~2 | grep -q git-svn-id &&\n+\t\t! git show HEAD~1 | grep -q git-svn-id &&\n+\t\t! git show HEAD   | grep -q git-svn-id\n+\t)\n+'\n+\n+test_expect_success 'dcommit fails on concurrent conflicting change' '\n+\tsetup_hook post-commit 1 &&\n+\tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n+\t(\n+\t\tcd work$N.git &&\n+\t\techo a >> file &&\n+\t\t\tgit commit -am \"commit change $N.1\" &&\n+\t\techo b >> auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.2\" &&\n+\t\techo c >> auto_updated_file &&\n+\t\t\tgit commit -am \"commit change $N.3\" &&\n+\t\ttest_must_fail git svn dcommit && # rebase should fail\n+\t\ttest_must_fail git update-index --refresh\n+\t)\n+'\n+\n+test_done\n-- \n1.7.10.4\n"},{"id":"196718","messageId":"20120808230754.GB24956@dcvr.yhbt.net","threadId":"31165","inReplyTo":"5021F9D4.1010700@debian.org","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-08-08T23:07:54Z","receivedAt":"2012-08-08T23:07:54Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Robert Luberda <robert@debian.org> wrote:\n> Eric Wong wrote:\n> >> +\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n> >> +\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n> >> +\t\tcat >> \"$hook\" <<- 'EOF2'\n> >> +\t\t\tcd work-auto-commits.svn\n> >> +\t\t\tsvn up --config-dir \"$svnconf\"\n> > \n> > That doesn't seem to interact well with users who depend on\n> > svn_cmd pointing to something non-standard.  Not sure\n> > what to do about it, though....\n\n> I have no idea how to change it either. I've tried to source the\n> lib-git-svn.sh file inside the hook, but it sources test-lib.sh, and the\n> latter script doesn't work well if it is sourced by non-test script.\n> Anyway I the part of my original patch unchanged.\n\nAh, so svn_cmd only cares about --config-dir and you already handled\nthat :)   I misremembered it also allowed for non-standard SVN\ninstallations :x\n\nI've pushed your updated patch to my \"maint\" branch on\ngit://bogomips.org/git-svn since \"master\" has larger pending changes.\n\nThanks!\n"},{"id":"196746","messageId":"7v1ujgot8h.fsf@alter.siamese.dyndns.org","threadId":"31165","inReplyTo":"20120808230754.GB24956@dcvr.yhbt.net","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-09T17:44:30Z","receivedAt":"2012-08-09T17:44:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> Robert Luberda <robert@debian.org> wrote:\n>> Eric Wong wrote:\n>> >> +\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n>> >> +\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n>> >> +\t\tcat >> \"$hook\" <<- 'EOF2'\n>> >> +\t\t\tcd work-auto-commits.svn\n>> >> +\t\t\tsvn up --config-dir \"$svnconf\"\n>> > \n>> > That doesn't seem to interact well with users who depend on\n>> > svn_cmd pointing to something non-standard.  Not sure\n>> > what to do about it, though....\n>\n>> I have no idea how to change it either. I've tried to source the\n>> lib-git-svn.sh file inside the hook, but it sources test-lib.sh, and the\n>> latter script doesn't work well if it is sourced by non-test script.\n>> Anyway I the part of my original patch unchanged.\n>\n> Ah, so svn_cmd only cares about --config-dir and you already handled\n> that :)   I misremembered it also allowed for non-standard SVN\n> installations :x\n>\n> I've pushed your updated patch to my \"maint\" branch on\n> git://bogomips.org/git-svn since \"master\" has larger pending changes.\n\nI should have asked this yesterday, but do you mean you want to have\nyour \"maint\" in the upcoming 1.7.12?  This does look like a useful\nthing to do, but does not seem like a regression fix to me.\n"},{"id":"196827","messageId":"20120810195133.GA16423@dcvr.yhbt.net","threadId":"31165","inReplyTo":"7v1ujgot8h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-08-10T19:51:33Z","receivedAt":"2012-08-10T19:51:33Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> I should have asked this yesterday, but do you mean you want to have\n> your \"maint\" in the upcoming 1.7.12?  This does look like a useful\n> thing to do, but does not seem like a regression fix to me.\n\nYeah, I wasn't sure what to name it since my master is still carrying\nMichael's larger SVN 1.7 changes.   Perhaps I should've named my \"maint\"\n\"for-git-master\" in this case...\n"},{"id":"197306","messageId":"50316C07.30907@debian.org","threadId":"31165","inReplyTo":"20120810195133.GA16423@dcvr.yhbt.net","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Robert Luberda","fromEmail":"robert@debian.org","sentAt":"2012-08-19T22:43:19Z","receivedAt":"2012-08-19T22:43:19Z","isPatch":true,"sender":{"key":"robert@debian.org","avatar":"https://avatars.githubusercontent.com/u/18017362?v=4"},"body":"Eric Wong wrote:\n\nHi,\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>> I should have asked this yesterday, but do you mean you want to have\n>> your \"maint\" in the upcoming 1.7.12?  This does look like a useful\n>> thing to do, but does not seem like a regression fix to me.\n> \n> Yeah, I wasn't sure what to name it since my master is still carrying\n> Michael's larger SVN 1.7 changes.   Perhaps I should've named my \"maint\"\n> \"for-git-master\" in this case...\n\n\nWhile working on my next patch, I've accidentally discovered that bash gives\nthe following errors in the test file introduced in my commit :\n\n./t9164-git-svn-dcommit-concrrent.sh: line 65: $hook: ambiguous redirect\n./t9164-git-svn-dcommit-concrrent.sh: line 66: $hook: ambiguous redirect\n\n(The test succeeds, even though assignments of the PATH and svnconf\nvariables is missing; is seems svn treats an empty value of --config-dir\nas the current dir.)\n\n\nSorry about not noticing this before, dash is used as default /bin/sh on\nmy system. A simple patch for this issue is included below.\n\nThere is also a typo in the test file name: `concrrent' instead of\n`concurrent', but it most probably doesn't make any sense to make a\npatch for it.\n\n\n>8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8\nSubject: [PATCH] Add missing quotes in test t9164\n\nThis fixes `ambiguous redirect' error given by bash.\n---\n t/t9164-git-svn-dcommit-concrrent.sh |    4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9164-git-svn-dcommit-concrrent.sh\nb/t/t9164-git-svn-dcommit-concrrent.sh\nindex aac2dda..676ef00 100755\n--- a/t/t9164-git-svn-dcommit-concrrent.sh\n+++ b/t/t9164-git-svn-dcommit-concrrent.sh\n@@ -62,8 +62,8 @@ EOF1\n        if [ \"$hook_type\" = \"pre-commit\" ]; then\n                echo \"echo 'commit disallowed' >&2; exit 1\" >> \"$hook\"\n        else\n-               echo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n-               echo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n+               echo \"PATH=\\\"$PATH\\\"; export PATH\" >> \"$hook\"\n+               echo \"svnconf=\\\"$svnconf\\\"\" >> \"$hook\"\n                cat >> \"$hook\" <<- 'EOF2'\n                        cd work-auto-commits.svn\n                        svn up --config-dir \"$svnconf\"\n--\n1.7.10.4\n\n\n>8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8\n\n\n\n\nRegards,\nrobert\n"},{"id":"197314","messageId":"7v393inyug.fsf@alter.siamese.dyndns.org","threadId":"31165","inReplyTo":"50316C07.30907@debian.org","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T01:20:23Z","receivedAt":"2012-08-20T01:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Luberda <robert@debian.org> writes:\n\n> Eric Wong wrote:\n>\n> Hi,\n>\n>> Junio C Hamano <gitster@pobox.com> wrote:\n>>> I should have asked this yesterday, but do you mean you want to have\n>>> your \"maint\" in the upcoming 1.7.12?  This does look like a useful\n>>> thing to do, but does not seem like a regression fix to me.\n>> \n>> Yeah, I wasn't sure what to name it since my master is still carrying\n>> Michael's larger SVN 1.7 changes.   Perhaps I should've named my \"maint\"\n>> \"for-git-master\" in this case...\n>\n>\n> While working on my next patch, I've accidentally discovered that bash gives\n> the following errors in the test file introduced in my commit :\n>\n> ./t9164-git-svn-dcommit-concrrent.sh: line 65: $hook: ambiguous redirect\n> ./t9164-git-svn-dcommit-concrrent.sh: line 66: $hook: ambiguous redirect\n\nThanks.  It is this one (especially the latter half \"Note that\")\nin the Documentation/CodingGuidelines.\n\n - Redirection operators should be written with space before, but no\n   space after them.  In other words, write 'echo test >\"$file\"'\n   instead of 'echo test> $file' or 'echo test > $file'.  Note that\n   even though it is not required by POSIX to double-quote the\n   redirection target in a variable (as shown above), our code does so\n   because some versions of bash issue a warning without the quotes.\n"},{"id":"197561","messageId":"20120821220125.GA2040@dcvr.yhbt.net","threadId":"31165","inReplyTo":"50316C07.30907@debian.org","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-08-21T22:01:25Z","receivedAt":"2012-08-21T22:01:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Robert Luberda <robert@debian.org> wrote:\n> While working on my next patch, I've accidentally discovered that bash gives\n> the following errors in the test file introduced in my commit :\n> \n> ./t9164-git-svn-dcommit-concrrent.sh: line 65: $hook: ambiguous redirect\n> ./t9164-git-svn-dcommit-concrrent.sh: line 66: $hook: ambiguous redirect\n> \n> (The test succeeds, even though assignments of the PATH and svnconf\n> variables is missing; is seems svn treats an empty value of --config-dir\n> as the current dir.)\n> \n> \n> Sorry about not noticing this before, dash is used as default /bin/sh on\n> my system. A simple patch for this issue is included below.\n> \n> There is also a typo in the test file name: `concrrent' instead of\n> `concurrent', but it most probably doesn't make any sense to make a\n> patch for it.\n\nOops, I'll push the following out since Junio already merged your\noriginal:\n\n(your original patch was also whitespace-mangled)\n\n>From 209b4ac46d70aa6f29d51c2fbefa53509513362c Mon Sep 17 00:00:00 2001\nFrom: Robert Luberda <robert@debian.org>\nDate: Mon, 20 Aug 2012 00:43:19 +0200\nSubject: [PATCH] t9164: Add missing quotes in test\n\nThis fixes `ambiguous redirect' error given by bash.\n\n[ew: fix misspelled test name,\n     also eliminate space after \">>\" to conform to guidelines]\n\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n ...n-dcommit-concrrent.sh => t9164-git-svn-dcommit-concurrent.sh} | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n rename t/{t9164-git-svn-dcommit-concrrent.sh => t9164-git-svn-dcommit-concurrent.sh} (97%)\n\ndiff --git a/t/t9164-git-svn-dcommit-concrrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\nsimilarity index 97%\nrename from t/t9164-git-svn-dcommit-concrrent.sh\nrename to t/t9164-git-svn-dcommit-concurrent.sh\nindex aac2dda..d8464d4 100755\n--- a/t/t9164-git-svn-dcommit-concrrent.sh\n+++ b/t/t9164-git-svn-dcommit-concurrent.sh\n@@ -60,11 +60,11 @@ setup_hook()\n \t\t[ \"$cnt\" = \"0\" ] || exit 0\n EOF1\n \tif [ \"$hook_type\" = \"pre-commit\" ]; then\n-\t\techo \"echo 'commit disallowed' >&2; exit 1\" >> \"$hook\"\n+\t\techo \"echo 'commit disallowed' >&2; exit 1\" >>\"$hook\"\n \telse\n-\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >> $hook\n-\t\techo \"svnconf=\\\"$svnconf\\\"\" >> $hook\n-\t\tcat >> \"$hook\" <<- 'EOF2'\n+\t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >>\"$hook\"\n+\t\techo \"svnconf=\\\"$svnconf\\\"\" >>\"$hook\"\n+\t\tcat >>\"$hook\" <<- 'EOF2'\n \t\t\tcd work-auto-commits.svn\n \t\t\tsvn up --config-dir \"$svnconf\"\n \t\t\techo \"$$\" >> auto_updated_file\n-- \nEric Wong\n"},{"id":"197804","messageId":"50381287.4090605@debian.org","threadId":"31165","inReplyTo":"20120821220125.GA2040@dcvr.yhbt.net","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Robert Luberda","fromEmail":"robert@debian.org","sentAt":"2012-08-24T23:47:19Z","receivedAt":"2012-08-24T23:47:19Z","isPatch":true,"sender":{"key":"robert@debian.org","avatar":"https://avatars.githubusercontent.com/u/18017362?v=4"},"body":"Eric Wong wrote:\n\nHi,\n\n> \n> Oops, I'll push the following out since Junio already merged your\n> original:\n\nI can see that you haven't pushed the change yet. Maybe it would be a\ngood idea to fix other style mistakes  (extra spaces after redirections,\nlack of spaces after function names, `[' used instead of `test', etc) as\nwell? I can prepare a patch if you think it is a good idea.\n\nRegards,\nrobert\n"},{"id":"197852","messageId":"20120826003430.GA32346@dcvr.yhbt.net","threadId":"31165","inReplyTo":"50381287.4090605@debian.org","subject":"Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-08-26T00:34:30Z","receivedAt":"2012-08-26T00:34:30Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Robert Luberda <robert@debian.org> wrote:\n> Eric Wong wrote:\n> > Oops, I'll push the following out since Junio already merged your\n> > original:\n> \n> I can see that you haven't pushed the change yet. Maybe it would be a\n> good idea to fix other style mistakes  (extra spaces after redirections,\n> lack of spaces after function names, `[' used instead of `test', etc) as\n> well? I can prepare a patch if you think it is a good idea.\n\nOops, I got distracted.  Yes please on an updated patch.  Thanks.\n"},{"id":"198139","messageId":"1346394580-12083-1-git-send-email-robert@debian.org","threadId":"31165","inReplyTo":"20120826003430.GA32346@dcvr.yhbt.net","subject":"[PATCH] t9164: More style fixes","fromName":"Robert Luberda","fromEmail":"robert@debian.org","sentAt":"2012-08-31T06:29:39Z","receivedAt":"2012-08-31T06:29:39Z","isPatch":true,"sender":{"key":"robert@debian.org","avatar":"https://avatars.githubusercontent.com/u/18017362?v=4"},"body":"Make sure t9164 conforms with the coding guidelines:\n - remove spaces after redirection operators;\n - insert spaces between function names and parentheses;\n - split `if ...; then' lines;\n - use `test' instead of `['.\n\nSigned-off-by: Robert Luberda <robert@debian.org>\n---\n t/t9164-git-svn-dcommit-concurrent.sh |   69 ++++++++++++++++-----------------\n 1 file changed, 33 insertions(+), 36 deletions(-)\n\ndiff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\nindex d8464d4..d75ebdc 100755\n--- a/t/t9164-git-svn-dcommit-concurrent.sh\n+++ b/t/t9164-git-svn-dcommit-concurrent.sh\n@@ -12,16 +12,15 @@ test_expect_success 'setup svn repository' '\n \tsvn_cmd checkout \"$svnrepo\" work.svn &&\n \t(\n \t\tcd work.svn &&\n-\t\techo >file && echo > auto_updated_file\n+\t\techo >file && echo >auto_updated_file\n \t\tsvn_cmd add file auto_updated_file &&\n \t\tsvn_cmd commit -m \"initial commit\"\n \t) &&\n \tsvn_cmd checkout \"$svnrepo\" work-auto-commits.svn\n '\n N=0\n-next_N()\n-{\n-\tN=$(( $N + 1 ))\n+next_N () {\n+\tN=$(($N + 1))\n }\n \n # Setup SVN repository hooks to emulate SVN failures or concurrent commits\n@@ -35,39 +34,39 @@ next_N()\n # the hook should be applied for (each time the hook is run, the given\n # number is decreased by one until it gets 0, in which case the hook\n # will execute its real action)\n-setup_hook()\n-{\n+setup_hook () {\n \thook_type=\"$1\"  # \"pre-commit\" or \"post-commit\"\n \tskip_revs=\"$2\"\n-\t[ \"$hook_type\" = \"pre-commit\" ] ||\n-\t\t[ \"$hook_type\" = \"post-commit\" ] ||\n+\ttest \"$hook_type\" = \"pre-commit\"  ||\n+\t\ttest \"$hook_type\" = \"post-commit\"  ||\n \t\t{ echo \"ERROR: invalid argument ($hook_type)\" \\\n \t\t\t\"passed to setup_hook\" >&2 ; return 1; }\n-\techo \"cnt=$skip_revs\" > \"$hook_type-counter\"\n+\techo \"cnt=$skip_revs\" >\"$hook_type-counter\"\n \trm -f \"$rawsvnrepo/hooks/\"*-commit # drop previous hooks\n \thook=\"$rawsvnrepo/hooks/$hook_type\"\n-\tcat > \"$hook\" <<- 'EOF1'\n+\tcat >\"$hook\" <<-'EOF1'\n \t\t#!/bin/sh\n \t\tset -e\n \t\tcd \"$1/..\"  # \"$1\" is repository location\n-\t\texec >> svn-hook.log 2>&1\n+\t\texec >>svn-hook.log 2>&1\n \t\thook=\"$(basename \"$0\")\"\n \t\techo \"*** Executing $hook $@\"\n \t\tset -x\n-\t\t. ./$hook-counter\n+\t\t. \"./$hook-counter\"\n \t\tcnt=\"$(($cnt - 1))\"\n-\t\techo \"cnt=$cnt\" > ./$hook-counter\n-\t\t[ \"$cnt\" = \"0\" ] || exit 0\n+\t\techo \"cnt=$cnt\" >\"./$hook-counter\"\n+\t\ttest \"$cnt\" = \"0\" || exit 0\n EOF1\n-\tif [ \"$hook_type\" = \"pre-commit\" ]; then\n+\tif test \"$hook_type\" = \"pre-commit\"\n+\tthen\n \t\techo \"echo 'commit disallowed' >&2; exit 1\" >>\"$hook\"\n \telse\n \t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >>\"$hook\"\n \t\techo \"svnconf=\\\"$svnconf\\\"\" >>\"$hook\"\n-\t\tcat >>\"$hook\" <<- 'EOF2'\n+\t\tcat >>\"$hook\" <<-'EOF2'\n \t\t\tcd work-auto-commits.svn\n \t\t\tsvn up --config-dir \"$svnconf\"\n-\t\t\techo \"$$\" >> auto_updated_file\n+\t\t\techo \"$$\" >>auto_updated_file\n \t\t\tsvn commit --config-dir \"$svnconf\" \\\n \t\t\t\t-m \"auto-committing concurrent change\"\n \t\t\texit 0\n@@ -76,8 +75,7 @@ EOF2\n \tchmod 755 \"$hook\"\n }\n \n-check_contents()\n-{\n+check_contents () {\n \tgitdir=\"$1\"\n \t(cd ../work.svn && svn_cmd up) &&\n \ttest_cmp file ../work.svn/file &&\n@@ -89,7 +87,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n \t(\n \t\tcd work.svn &&\n \t\tcp auto_updated_file au_file_saved &&\n-\t\techo 1 >> file &&\n+\t\techo 1 >>file &&\n \t\tsvn_cmd commit -m \"changing file\" &&\n \t\tsvn_cmd up &&\n \t\ttest_must_fail test_cmp auto_updated_file au_file_saved\n@@ -100,9 +98,9 @@ test_expect_success 'check if pre-commit hook fails' '\n \tsetup_hook pre-commit 2 &&\n \t(\n \t\tcd work.svn &&\n-\t\techo 2 >> file &&\n+\t\techo 2 >>file &&\n \t\tsvn_cmd commit -m \"changing file once again\" &&\n-\t\techo 3 >> file &&\n+\t\techo 3 >>file &&\n \t\ttest_must_fail svn_cmd commit -m \"this commit should fail\" &&\n \t\tsvn_cmd revert file\n \t)\n@@ -113,9 +111,9 @@ test_expect_success 'dcommit error handling' '\n \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n \t(\n \t\tcd work$N.git &&\n-\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n-\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n-\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n+\t\techo 1 >>file && git commit -am \"commit change $N.1\" &&\n+\t\techo 2 >>file && git commit -am \"commit change $N.2\" &&\n+\t\techo 3 >>file && git commit -am \"commit change $N.3\" &&\n \t\t# should fail to dcommit 2nd and 3rd change\n \t\t# but still should leave the repository in reasonable state\n \t\ttest_must_fail git svn dcommit &&\n@@ -131,9 +129,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '\n \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n \t(\n \t\tcd work$N.git &&\n-\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n-\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n-\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n+\t\techo 1 >>file && git commit -am \"commit change $N.1\" &&\n+\t\techo 2 >>file && git commit -am \"commit change $N.2\" &&\n+\t\techo 3 >>file && git commit -am \"commit change $N.3\" &&\n \t\t# should rebase and leave the repository in reasonable state\n \t\tgit svn dcommit &&\n \t\tgit update-index --refresh &&\n@@ -146,10 +144,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '\n '\n \n # An utility function used in the following test\n-delete_first_line()\n-{\n+delete_first_line () {\n \tfile=\"$1\" &&\n-\tsed 1d < \"$file\" > \"${file}.tmp\" &&\n+\tsed 1d <\"$file\" >\"${file}.tmp\" &&\n \trm \"$file\" &&\n \tmv \"${file}.tmp\" \"$file\"\n }\n@@ -159,7 +156,7 @@ test_expect_success 'dcommit concurrent non-conflicting change' '\n \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n \t(\n \t\tcd work$N.git &&\n-\t\tcat file >> auto_updated_file &&\n+\t\tcat file >>auto_updated_file &&\n \t\t\tgit commit -am \"commit change $N.1\" &&\n \t\tdelete_first_line auto_updated_file &&\n \t\t\tgit commit -am \"commit change $N.2\" &&\n@@ -181,7 +178,7 @@ test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '\n \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n \t(\n \t\tcd work$N.git &&\n-\t\tcat file >> auto_updated_file &&\n+\t\tcat file >>auto_updated_file &&\n \t\t\tgit commit -am \"commit change $N.1\" &&\n \t\tdelete_first_line auto_updated_file &&\n \t\t\tgit commit -am \"commit change $N.2\" &&\n@@ -202,11 +199,11 @@ test_expect_success 'dcommit fails on concurrent conflicting change' '\n \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n \t(\n \t\tcd work$N.git &&\n-\t\techo a >> file &&\n+\t\techo a >>file &&\n \t\t\tgit commit -am \"commit change $N.1\" &&\n-\t\techo b >> auto_updated_file &&\n+\t\techo b >>auto_updated_file &&\n \t\t\tgit commit -am \"commit change $N.2\" &&\n-\t\techo c >> auto_updated_file &&\n+\t\techo c >>auto_updated_file &&\n \t\t\tgit commit -am \"commit change $N.3\" &&\n \t\ttest_must_fail git svn dcommit && # rebase should fail\n \t\ttest_must_fail git update-index --refresh\n-- \n1.7.10.4\n"},{"id":"198159","messageId":"7vipbzndp6.fsf@alter.siamese.dyndns.org","threadId":"31165","inReplyTo":"1346394580-12083-1-git-send-email-robert@debian.org","subject":"Re: [PATCH] t9164: More style fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-31T18:01:09Z","receivedAt":"2012-08-31T18:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Luberda <robert@debian.org> writes:\n\n> Make sure t9164 conforms with the coding guidelines:\n>  - remove spaces after redirection operators;\n>  - insert spaces between function names and parentheses;\n>  - split `if ...; then' lines;\n>  - use `test' instead of `['.\n>\n> Signed-off-by: Robert Luberda <robert@debian.org>\n> ---\n>  t/t9164-git-svn-dcommit-concurrent.sh |   69 ++++++++++++++++-----------------\n>  1 file changed, 33 insertions(+), 36 deletions(-)\n\nInteresting.\n\n$ git log --oneline --stat -1 -- 't/t9164-*'\ne48fb75 git svn: handle errors and concurrent commits in dcommit\n t/t9164-git-svn-dcommit-concrrent.sh | 216 +++++++++++++++++++++++++++++++++++\n 1 file changed, 216 insertions(+)\n\nI wonder where the typo in filename came from (notice the missing \"u\")?\n\nPerhaps the \"More\" in your subject implies there were some earlier\npatches I haven't seen?  The patch does not seem to apply to my tree\neven with the target file renamed.\n\n> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh\n> index d8464d4..d75ebdc 100755\n> --- a/t/t9164-git-svn-dcommit-concurrent.sh\n> +++ b/t/t9164-git-svn-dcommit-concurrent.sh\n> @@ -12,16 +12,15 @@ test_expect_success 'setup svn repository' '\n>  \tsvn_cmd checkout \"$svnrepo\" work.svn &&\n>  \t(\n>  \t\tcd work.svn &&\n> -\t\techo >file && echo > auto_updated_file\n> +\t\techo >file && echo >auto_updated_file\n>  \t\tsvn_cmd add file auto_updated_file &&\n>  \t\tsvn_cmd commit -m \"initial commit\"\n>  \t) &&\n>  \tsvn_cmd checkout \"$svnrepo\" work-auto-commits.svn\n>  '\n>  N=0\n> -next_N()\n> -{\n> -\tN=$(( $N + 1 ))\n> +next_N () {\n> +\tN=$(($N + 1))\n>  }\n>  \n>  # Setup SVN repository hooks to emulate SVN failures or concurrent commits\n> @@ -35,39 +34,39 @@ next_N()\n>  # the hook should be applied for (each time the hook is run, the given\n>  # number is decreased by one until it gets 0, in which case the hook\n>  # will execute its real action)\n> -setup_hook()\n> -{\n> +setup_hook () {\n>  \thook_type=\"$1\"  # \"pre-commit\" or \"post-commit\"\n>  \tskip_revs=\"$2\"\n> -\t[ \"$hook_type\" = \"pre-commit\" ] ||\n> -\t\t[ \"$hook_type\" = \"post-commit\" ] ||\n> +\ttest \"$hook_type\" = \"pre-commit\"  ||\n> +\t\ttest \"$hook_type\" = \"post-commit\"  ||\n>  \t\t{ echo \"ERROR: invalid argument ($hook_type)\" \\\n>  \t\t\t\"passed to setup_hook\" >&2 ; return 1; }\n> -\techo \"cnt=$skip_revs\" > \"$hook_type-counter\"\n> +\techo \"cnt=$skip_revs\" >\"$hook_type-counter\"\n>  \trm -f \"$rawsvnrepo/hooks/\"*-commit # drop previous hooks\n>  \thook=\"$rawsvnrepo/hooks/$hook_type\"\n> -\tcat > \"$hook\" <<- 'EOF1'\n> +\tcat >\"$hook\" <<-'EOF1'\n>  \t\t#!/bin/sh\n>  \t\tset -e\n>  \t\tcd \"$1/..\"  # \"$1\" is repository location\n> -\t\texec >> svn-hook.log 2>&1\n> +\t\texec >>svn-hook.log 2>&1\n>  \t\thook=\"$(basename \"$0\")\"\n>  \t\techo \"*** Executing $hook $@\"\n>  \t\tset -x\n> -\t\t. ./$hook-counter\n> +\t\t. \"./$hook-counter\"\n>  \t\tcnt=\"$(($cnt - 1))\"\n> -\t\techo \"cnt=$cnt\" > ./$hook-counter\n> -\t\t[ \"$cnt\" = \"0\" ] || exit 0\n> +\t\techo \"cnt=$cnt\" >\"./$hook-counter\"\n> +\t\ttest \"$cnt\" = \"0\" || exit 0\n>  EOF1\n> -\tif [ \"$hook_type\" = \"pre-commit\" ]; then\n> +\tif test \"$hook_type\" = \"pre-commit\"\n> +\tthen\n>  \t\techo \"echo 'commit disallowed' >&2; exit 1\" >>\"$hook\"\n>  \telse\n>  \t\techo \"PATH=\\\"$PATH\\\"; export PATH\" >>\"$hook\"\n>  \t\techo \"svnconf=\\\"$svnconf\\\"\" >>\"$hook\"\n> -\t\tcat >>\"$hook\" <<- 'EOF2'\n> +\t\tcat >>\"$hook\" <<-'EOF2'\n>  \t\t\tcd work-auto-commits.svn\n>  \t\t\tsvn up --config-dir \"$svnconf\"\n> -\t\t\techo \"$$\" >> auto_updated_file\n> +\t\t\techo \"$$\" >>auto_updated_file\n>  \t\t\tsvn commit --config-dir \"$svnconf\" \\\n>  \t\t\t\t-m \"auto-committing concurrent change\"\n>  \t\t\texit 0\n> @@ -76,8 +75,7 @@ EOF2\n>  \tchmod 755 \"$hook\"\n>  }\n>  \n> -check_contents()\n> -{\n> +check_contents () {\n>  \tgitdir=\"$1\"\n>  \t(cd ../work.svn && svn_cmd up) &&\n>  \ttest_cmp file ../work.svn/file &&\n> @@ -89,7 +87,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' '\n>  \t(\n>  \t\tcd work.svn &&\n>  \t\tcp auto_updated_file au_file_saved &&\n> -\t\techo 1 >> file &&\n> +\t\techo 1 >>file &&\n>  \t\tsvn_cmd commit -m \"changing file\" &&\n>  \t\tsvn_cmd up &&\n>  \t\ttest_must_fail test_cmp auto_updated_file au_file_saved\n> @@ -100,9 +98,9 @@ test_expect_success 'check if pre-commit hook fails' '\n>  \tsetup_hook pre-commit 2 &&\n>  \t(\n>  \t\tcd work.svn &&\n> -\t\techo 2 >> file &&\n> +\t\techo 2 >>file &&\n>  \t\tsvn_cmd commit -m \"changing file once again\" &&\n> -\t\techo 3 >> file &&\n> +\t\techo 3 >>file &&\n>  \t\ttest_must_fail svn_cmd commit -m \"this commit should fail\" &&\n>  \t\tsvn_cmd revert file\n>  \t)\n> @@ -113,9 +111,9 @@ test_expect_success 'dcommit error handling' '\n>  \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n>  \t(\n>  \t\tcd work$N.git &&\n> -\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n> -\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n> -\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n> +\t\techo 1 >>file && git commit -am \"commit change $N.1\" &&\n> +\t\techo 2 >>file && git commit -am \"commit change $N.2\" &&\n> +\t\techo 3 >>file && git commit -am \"commit change $N.3\" &&\n>  \t\t# should fail to dcommit 2nd and 3rd change\n>  \t\t# but still should leave the repository in reasonable state\n>  \t\ttest_must_fail git svn dcommit &&\n> @@ -131,9 +129,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '\n>  \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n>  \t(\n>  \t\tcd work$N.git &&\n> -\t\techo 1 >> file && git commit -am \"commit change $N.1\" &&\n> -\t\techo 2 >> file && git commit -am \"commit change $N.2\" &&\n> -\t\techo 3 >> file && git commit -am \"commit change $N.3\" &&\n> +\t\techo 1 >>file && git commit -am \"commit change $N.1\" &&\n> +\t\techo 2 >>file && git commit -am \"commit change $N.2\" &&\n> +\t\techo 3 >>file && git commit -am \"commit change $N.3\" &&\n>  \t\t# should rebase and leave the repository in reasonable state\n>  \t\tgit svn dcommit &&\n>  \t\tgit update-index --refresh &&\n> @@ -146,10 +144,9 @@ test_expect_success 'dcommit concurrent change in non-changed file' '\n>  '\n>  \n>  # An utility function used in the following test\n> -delete_first_line()\n> -{\n> +delete_first_line () {\n>  \tfile=\"$1\" &&\n> -\tsed 1d < \"$file\" > \"${file}.tmp\" &&\n> +\tsed 1d <\"$file\" >\"${file}.tmp\" &&\n>  \trm \"$file\" &&\n>  \tmv \"${file}.tmp\" \"$file\"\n>  }\n> @@ -159,7 +156,7 @@ test_expect_success 'dcommit concurrent non-conflicting change' '\n>  \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n>  \t(\n>  \t\tcd work$N.git &&\n> -\t\tcat file >> auto_updated_file &&\n> +\t\tcat file >>auto_updated_file &&\n>  \t\t\tgit commit -am \"commit change $N.1\" &&\n>  \t\tdelete_first_line auto_updated_file &&\n>  \t\t\tgit commit -am \"commit change $N.2\" &&\n> @@ -181,7 +178,7 @@ test_expect_success 'dcommit --no-rebase concurrent non-conflicting change' '\n>  \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n>  \t(\n>  \t\tcd work$N.git &&\n> -\t\tcat file >> auto_updated_file &&\n> +\t\tcat file >>auto_updated_file &&\n>  \t\t\tgit commit -am \"commit change $N.1\" &&\n>  \t\tdelete_first_line auto_updated_file &&\n>  \t\t\tgit commit -am \"commit change $N.2\" &&\n> @@ -202,11 +199,11 @@ test_expect_success 'dcommit fails on concurrent conflicting change' '\n>  \tnext_N && git svn clone \"$svnrepo\" work$N.git &&\n>  \t(\n>  \t\tcd work$N.git &&\n> -\t\techo a >> file &&\n> +\t\techo a >>file &&\n>  \t\t\tgit commit -am \"commit change $N.1\" &&\n> -\t\techo b >> auto_updated_file &&\n> +\t\techo b >>auto_updated_file &&\n>  \t\t\tgit commit -am \"commit change $N.2\" &&\n> -\t\techo c >> auto_updated_file &&\n> +\t\techo c >>auto_updated_file &&\n>  \t\t\tgit commit -am \"commit change $N.3\" &&\n>  \t\ttest_must_fail git svn dcommit && # rebase should fail\n>  \t\ttest_must_fail git update-index --refresh\n"}]}