{"thread":{"id":"17723","subject":"[PATCH] git-filter-branch: Add more error-handling","startedAt":"2009-02-11T14:09:25Z","lastAt":"2009-02-11T22:28:04Z","messageCount":13,"participants":["Eric Kidd","Johannes Sixt","Johannes Schindelin","Junio C Hamano","Nanako Shiraishi"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"104186","messageId":"1234361365-63711-1-git-send-email-git@randomhacks.net","threadId":"17723","inReplyTo":null,"subject":"[PATCH] git-filter-branch: Add more error-handling","fromName":"Eric Kidd","fromEmail":"git@randomhacks.net","sentAt":"2009-02-11T14:09:25Z","receivedAt":"2009-02-11T14:09:25Z","isPatch":true,"sender":{"key":"git@randomhacks.net","avatar":null},"body":"In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error\nthat had slipped by the test suites because of a missing check on 'git\nread-tree -u -m HEAD'.\n\nI mentioned to Johannes Schindelin that there were several bugs of this\ntype in git-filter-branch, and he suggested that I send a patch.\n\nI've tested this patch using t/t7003-filter-branch.sh, and it passes all\nthe existing tests.  But it's entirely possible that this patch contains\nerrors, and I would love input from people who have more experience with\nsh and who know more about git-filter-branch.\n\nIn particular, the following hunk may change the public UI to\ngit-filter-branch, although I'm not sure whether the change is for\nbetter or for worse.  As I understand it, this hunk would allow\n$filter_commit to abort the rewriting process by returning a non-0 exit\nstatus:\n\n \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n-\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n+\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n+\t\t\tdie \"could not write rewritten commit\"\n done <../revs\n\nI'd be happy to add a test case for what happens when $filter_commit\nreturns a non-0 exit status.  Is the old behavior preferable?\n---\n git-filter-branch.sh |   17 ++++++++++-------\n 1 files changed, 10 insertions(+), 7 deletions(-)\n\n I'm trying to do the constructive thing, and send patches instead of bug\n reports. :-) -Eric\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 86eef56..9d50978 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -221,7 +221,7 @@ die \"\"\n trap 'cd ../..; rm -rf \"$tempdir\"' 0\n \n # Make sure refs/original is empty\n-git for-each-ref > \"$tempdir\"/backup-refs\n+git for-each-ref > \"$tempdir\"/backup-refs || die \"Can't back up refs\"\n while read sha1 type name\n do\n \tcase \"$force,$name\" in\n@@ -242,7 +242,7 @@ export GIT_DIR GIT_WORK_TREE\n \n # The refs should be updated if their heads were rewritten\n git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD \"$@\" |\n-sed -e '/^^/d' >\"$tempdir\"/heads\n+sed -e '/^^/d' >\"$tempdir\"/heads || die \"Can't make list of original refs\"\n \n test -s \"$tempdir\"/heads ||\n \tdie \"Which ref do you want to rewrite?\"\n@@ -315,10 +315,11 @@ while read commit parents; do\n \t\t\tdie \"tree filter failed: $filter_tree\"\n \n \t\t(\n-\t\t\tgit diff-index -r --name-only $commit\n+\t\t\tgit diff-index -r --name-only $commit &&\n \t\t\tgit ls-files --others\n \t\t) |\n-\t\tgit update-index --add --replace --remove --stdin\n+\t\tgit update-index --add --replace --remove --stdin ||\n+\t\t\tdie \"unable to update index with results of tree filter\"\n \tfi\n \n \teval \"$filter_index\" < /dev/null ||\n@@ -339,7 +340,8 @@ while read commit parents; do\n \t\teval \"$filter_msg\" > ../message ||\n \t\t\tdie \"msg filter failed: $filter_msg\"\n \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n-\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n+\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n+\t\t\tdie \"could not write rewritten commit\"\n done <../revs\n \n # In case of a subdirectory filter, it is possible that a specified head\n@@ -407,7 +409,8 @@ do\n \t\t\tdie \"Could not rewrite $ref\"\n \t;;\n \tesac\n-\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1\n+\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1 ||\n+\t\t die \"Could not back up branch ref\"\n done < \"$tempdir\"/heads\n \n # TODO: This should possibly go, with the semantics that all positive given\n@@ -483,7 +486,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n }\n \n if [ \"$(is_bare_repository)\" = false ]; then\n-\tgit read-tree -u -m HEAD\n+\tgit read-tree -u -m HEAD || die \"Unable to checkout rewritten tree\"\n fi\n \n exit $ret\n-- \n1.6.0.4\n"},{"id":"104190","messageId":"4992E79D.10208@viscovery.net","threadId":"17723","inReplyTo":"1234361365-63711-1-git-send-email-git@randomhacks.net","subject":"Re: [PATCH] git-filter-branch: Add more error-handling","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-02-11T14:58:37Z","receivedAt":"2009-02-11T14:58:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Eric Kidd schrieb:\n> In particular, the following hunk may change the public UI to\n> git-filter-branch, although I'm not sure whether the change is for\n> better or for worse.  As I understand it, this hunk would allow\n> $filter_commit to abort the rewriting process by returning a non-0 exit\n> status:\n> \n>  \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n> -\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n> +\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n> +\t\t\tdie \"could not write rewritten commit\"\n>  done <../revs\n> \n> I'd be happy to add a test case for what happens when $filter_commit\n> returns a non-0 exit status.  Is the old behavior preferable?\n\nI think it's OK to die if the commit filter fails.\n\nBut generally, I think it is not necessary to use 'die with error\nmessage', a plain '|| exit' should be enough because an error will have\nbeen reported already by the tool that failed.\n\n> @@ -483,7 +486,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n>  }\n>  \n>  if [ \"$(is_bare_repository)\" = false ]; then\n> -\tgit read-tree -u -m HEAD\n> +\tgit read-tree -u -m HEAD || die \"Unable to checkout rewritten tree\"\n\nHere you shouldn't die. But unlike elsewhere, this case warrants an\nexplanation for the user:\n\n\tgit read-tree -u -m HEAD ||\n\t\techo >&2 \"WARNING: The working directory is not up-to-date!\"\n\n>  fi\n>  \n>  exit $ret\n\n-- Hannes\n"},{"id":"104193","messageId":"alpine.DEB.1.00.0902111621490.13279@intel-tinevez-2-302","threadId":"17723","inReplyTo":"1234361365-63711-1-git-send-email-git@randomhacks.net","subject":"Re: [PATCH] git-filter-branch: Add more error-handling","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-11T15:24:26Z","receivedAt":"2009-02-11T15:24:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 11 Feb 2009, Eric Kidd wrote:\n\n> In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error \n> that had slipped by the test suites because of a missing check on 'git \n> read-tree -u -m HEAD'.\n> \n> I mentioned to Johannes Schindelin that there were several bugs of this \n> type in git-filter-branch, and he suggested that I send a patch.\n> \n> I've tested this patch using t/t7003-filter-branch.sh, and it passes all\n> the existing tests.  But it's entirely possible that this patch contains\n> errors, and I would love input from people who have more experience with\n> sh and who know more about git-filter-branch.\n> \n> In particular, the following hunk may change the public UI to\n> git-filter-branch, although I'm not sure whether the change is for\n> better or for worse.  As I understand it, this hunk would allow\n> $filter_commit to abort the rewriting process by returning a non-0 exit\n> status:\n> \n>  \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n> -\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n> +\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n> +\t\t\tdie \"could not write rewritten commit\"\n>  done <../revs\n> \n> I'd be happy to add a test case for what happens when $filter_commit\n> returns a non-0 exit status.  Is the old behavior preferable?\n> ---\n\nThanks.  Although it lacks a Sign-off, and part of the commit message is \nactually a personal comment that belongs after the three dashes.\n\n> diff --git a/git-filter-branch.sh b/git-filter-branch.sh\n> index 86eef56..9d50978 100755\n> --- a/git-filter-branch.sh\n> +++ b/git-filter-branch.sh\n> @@ -242,7 +242,7 @@ export GIT_DIR GIT_WORK_TREE\n>  \n>  # The refs should be updated if their heads were rewritten\n>  git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD \"$@\" |\n> -sed -e '/^^/d' >\"$tempdir\"/heads\n> +sed -e '/^^/d' >\"$tempdir\"/heads || die \"Can't make list of original refs\"\n\nThis will catch errors in the sed invocation, but not in rev-parse.\n\n> @@ -315,10 +315,11 @@ while read commit parents; do\n>  \t\t\tdie \"tree filter failed: $filter_tree\"\n>  \n>  \t\t(\n> -\t\t\tgit diff-index -r --name-only $commit\n> +\t\t\tgit diff-index -r --name-only $commit &&\n>  \t\t\tgit ls-files --others\n>  \t\t) |\n> -\t\tgit update-index --add --replace --remove --stdin\n> +\t\tgit update-index --add --replace --remove --stdin ||\n> +\t\t\tdie \"unable to update index with results of tree filter\"\n\nThis will catch errors in the update-index call, but neither in diff-index \nnor ls-files.\n\nOtherwise, the patch looks good to me.\n\nCiao,\nDscho\n"},{"id":"104194","messageId":"4992F07F.2080005@viscovery.net","threadId":"17723","inReplyTo":"4992E79D.10208@viscovery.net","subject":"Re: [PATCH] git-filter-branch: Add more error-handling","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-02-11T15:36:31Z","receivedAt":"2009-02-11T15:36:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Sixt schrieb:\n>> @@ -483,7 +486,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n>>  }\n>>  \n>>  if [ \"$(is_bare_repository)\" = false ]; then\n>> -\tgit read-tree -u -m HEAD\n>> +\tgit read-tree -u -m HEAD || die \"Unable to checkout rewritten tree\"\n> \n> Here you shouldn't die.\n\nI take this back. I was distracted by the 'exit $ret' that is visible in\nthe context:\n\n>>  fi\n>>  \n>>  exit $ret\n\nBut this exit statement is pointless: ret is initialized to zero and never\nchanged.\n\n-- Hannes\n"},{"id":"104218","messageId":"1234372518-6924-1-git-send-email-git@randomhacks.net","threadId":"17723","inReplyTo":"1234361365-63711-1-git-send-email-git@randomhacks.net","subject":"[PATCH v2] filter-branch: Add more error-handling","fromName":"Eric Kidd","fromEmail":"git@randomhacks.net","sentAt":"2009-02-11T17:15:18Z","receivedAt":"2009-02-11T17:15:18Z","isPatch":true,"sender":{"key":"git@randomhacks.net","avatar":null},"body":"In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error\nthat had slipped by the test suites because of a missing check on 'git\nread-tree -u -m HEAD'.\n\nI mentioned to Johannes Schindelin that there were several bugs of this\ntype in git-filter-branch, and he suggested that I send a patch.  I've\ntested this patch using t/t7003-filter-branch.sh, and it passes all the\nexisting tests.\n\nIn two places, I've had to break apart pipelines in order to check the\nerror code for the first stage of the pipeline, as discussed here:\n\n  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614\n\nThank you to charon on #git for pointing me in the right direction.\n\nThis patch causes 'git filter-branch' to fail if the --commit-filter\nargument returns an error.  A test case for this behavior is included.\n\nFeedback on the original version of this patch was provided by Johannes\nSixt and Johannes Schindelin.\n\nv2:\n  Remove useless $ret variable\n  Correctly check the first command in a pipeline, not the second\n  Replace verbose 'die' messages with 'exit 1' in most cases\n\nSigned-off-by: Eric Kidd <git@randomhacks.net>\n---\n git-filter-branch.sh     |   26 ++++++++++++++------------\n t/t7003-filter-branch.sh |    4 ++++\n 2 files changed, 18 insertions(+), 12 deletions(-)\n\n Thank you for the feedback!  I've tried to incorporate everybody's\n suggestions.  Please let me know if some of those 'exit 1' statements\n should be changed back to 'die'. -Eric\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 86eef56..fff07c8 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -221,7 +221,7 @@ die \"\"\n trap 'cd ../..; rm -rf \"$tempdir\"' 0\n \n # Make sure refs/original is empty\n-git for-each-ref > \"$tempdir\"/backup-refs\n+git for-each-ref > \"$tempdir\"/backup-refs || exit 1\n while read sha1 type name\n do\n \tcase \"$force,$name\" in\n@@ -241,8 +241,9 @@ GIT_WORK_TREE=.\n export GIT_DIR GIT_WORK_TREE\n \n # The refs should be updated if their heads were rewritten\n-git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD \"$@\" |\n-sed -e '/^^/d' >\"$tempdir\"/heads\n+git rev-parse --no-flags --revs-only --symbolic-full-name \\\n+\t--default HEAD \"$@\" > \"$tempdir\"/raw-heads || exit 1\n+sed -e '/^^/d' \"$tempdir\"/raw-heads >\"$tempdir\"/heads\n \n test -s \"$tempdir\"/heads ||\n \tdie \"Which ref do you want to rewrite?\"\n@@ -251,8 +252,6 @@ GIT_INDEX_FILE=\"$(pwd)/../index\"\n export GIT_INDEX_FILE\n git read-tree || die \"Could not seed the index\"\n \n-ret=0\n-\n # map old->new commit ids for rewriting parents\n mkdir ../map || die \"Could not create map/ directory\"\n \n@@ -315,10 +314,11 @@ while read commit parents; do\n \t\t\tdie \"tree filter failed: $filter_tree\"\n \n \t\t(\n-\t\t\tgit diff-index -r --name-only $commit\n+\t\t\tgit diff-index -r --name-only $commit &&\n \t\t\tgit ls-files --others\n-\t\t) |\n-\t\tgit update-index --add --replace --remove --stdin\n+\t\t) > \"$tempdir\"/tree-state || exit 1\n+\t\tgit update-index --add --replace --remove --stdin \\\n+\t\t\t< \"$tempdir\"/tree-state || exit 1\n \tfi\n \n \teval \"$filter_index\" < /dev/null ||\n@@ -339,7 +339,8 @@ while read commit parents; do\n \t\teval \"$filter_msg\" > ../message ||\n \t\t\tdie \"msg filter failed: $filter_msg\"\n \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n-\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n+\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n+\t\t\tdie \"could not write rewritten commit\"\n done <../revs\n \n # In case of a subdirectory filter, it is possible that a specified head\n@@ -407,7 +408,8 @@ do\n \t\t\tdie \"Could not rewrite $ref\"\n \t;;\n \tesac\n-\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1\n+\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1 ||\n+\t\t exit 1\n done < \"$tempdir\"/heads\n \n # TODO: This should possibly go, with the semantics that all positive given\n@@ -483,7 +485,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n }\n \n if [ \"$(is_bare_repository)\" = false ]; then\n-\tgit read-tree -u -m HEAD\n+\tgit read-tree -u -m HEAD || exit 1\n fi\n \n-exit $ret\n+exit 0\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex cb04743..39affd9 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -48,6 +48,10 @@ test_expect_success 'result is really identical' '\n \ttest $H = $(git rev-parse HEAD)\n '\n \n+test_expect_success 'Fail if commit filter fails' '\n+\t! git filter-branch -f --commit-filter \"exit 1\" HEAD\n+'\n+\n test_expect_success 'rewrite, renaming a specific file' '\n \tgit filter-branch -f --tree-filter \"mv d doh || :\" HEAD\n '\n-- \n1.6.0.4\n"},{"id":"104272","messageId":"7vhc30eqy7.fsf@gitster.siamese.dyndns.org","threadId":"17723","inReplyTo":"1234372518-6924-1-git-send-email-git@randomhacks.net","subject":"Re: [PATCH v2] filter-branch: Add more error-handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-11T19:03:44Z","receivedAt":"2009-02-11T19:03:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Kidd <git@randomhacks.net> writes:\n\n> In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error\n> ...\n> Thank you to charon on #git for pointing me in the right direction.\n\nThe commit message is not a reception speech at Emmy Awards.  If you want\nto do the speech, do so after the three-dash lines.\n\nPlease just stick to what problem it tries to solve, how it does so, and\nwhat the outcome is.  \n\n> This patch causes 'git filter-branch' to fail if the --commit-filter\n> argument returns an error.  A test case for this behavior is included.\n\nThat's a very good start for the description of the solution, which would\nbe for the second paragraph.  The problem description is missing.\n\n> Feedback on the original version of this patch was provided by Johannes\n> Sixt and Johannes Schindelin.\n\nGiving credits to others like this with a short sentence at the end is\nfine.\n\n> v2:\n>   Remove useless $ret variable\n>   Correctly check the first command in a pipeline, not the second\n>   Replace verbose 'die' messages with 'exit 1' in most cases\n\nThis goes after three-dashes; people who read \"git log\" output wouldn't\nknow nor care what was in v1.\n\n    Subject: Fix X under condition Z\n\n    X should do Y if condition Z holds, but it does not.  This can result\n    in broken results such as W and V.\n\n    This patch fixes X by changing A, B and C.\n\n    Thanks for M, N and O for reviewing and suggesting improvements.\n\n    Signed-off-by: A U Thor <au.thor@example.xz>\n\n> diff --git a/git-filter-branch.sh b/git-filter-branch.sh\n> index 86eef56..fff07c8 100755\n> --- a/git-filter-branch.sh\n> +++ b/git-filter-branch.sh\n> @@ -221,7 +221,7 @@ die \"\"\n>  trap 'cd ../..; rm -rf \"$tempdir\"' 0\n>  \n>  # Make sure refs/original is empty\n> -git for-each-ref > \"$tempdir\"/backup-refs\n> +git for-each-ref > \"$tempdir\"/backup-refs || exit 1\n\nWhy \"exit 1\", not \"exit\"?\n\n> @@ -241,8 +241,9 @@ GIT_WORK_TREE=.\n>  export GIT_DIR GIT_WORK_TREE\n>  \n>  # The refs should be updated if their heads were rewritten\n> -git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD \"$@\" |\n> -sed -e '/^^/d' >\"$tempdir\"/heads\n> +git rev-parse --no-flags --revs-only --symbolic-full-name \\\n> +\t--default HEAD \"$@\" > \"$tempdir\"/raw-heads || exit 1\n\nLikewise.\n\n> @@ -315,10 +314,11 @@ while read commit parents; do\n>  \t\t\tdie \"tree filter failed: $filter_tree\"\n>  \n>  \t\t(\n> -\t\t\tgit diff-index -r --name-only $commit\n> +\t\t\tgit diff-index -r --name-only $commit &&\n>  \t\t\tgit ls-files --others\n> -\t\t) |\n> -\t\tgit update-index --add --replace --remove --stdin\n> +\t\t) > \"$tempdir\"/tree-state || exit 1\n> +\t\tgit update-index --add --replace --remove --stdin \\\n> +\t\t\t< \"$tempdir\"/tree-state || exit 1\n\nLikewise.\n\n> @@ -339,7 +339,8 @@ while read commit parents; do\n>  \t\teval \"$filter_msg\" > ../message ||\n>  \t\t\tdie \"msg filter failed: $filter_msg\"\n>  \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n> -\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n> +\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n> +\t\t\tdie \"could not write rewritten commit\"\n\nHmm, wouldn't commit-tree have issued its own error message already?  If\nredirect failed, then the shell would have.\n\n> @@ -407,7 +408,8 @@ do\n>  \t\t\tdie \"Could not rewrite $ref\"\n>  \t;;\n>  \tesac\n> -\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1\n> +\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1 ||\n> +\t\t exit 1\n\nWhy \"exit 1\", not \"exit\"?\n\n> @@ -483,7 +485,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n>  }\n>  \n>  if [ \"$(is_bare_repository)\" = false ]; then\n> -\tgit read-tree -u -m HEAD\n> +\tgit read-tree -u -m HEAD || exit 1\n\nLikewise\n\n> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\n> index cb04743..39affd9 100755\n> --- a/t/t7003-filter-branch.sh\n> +++ b/t/t7003-filter-branch.sh\n> @@ -48,6 +48,10 @@ test_expect_success 'result is really identical' '\n>  \ttest $H = $(git rev-parse HEAD)\n>  '\n>  \n> +test_expect_success 'Fail if commit filter fails' '\n> +\t! git filter-branch -f --commit-filter \"exit 1\" HEAD\n> +'\n> +\n\n\"test_must_fail git ...\" would be better here than \"! git ...\".\n"},{"id":"104280","messageId":"431341160902111134l7c289412r5b3f633280beb27c@mail.gmail.com","threadId":"17723","inReplyTo":"7vhc30eqy7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] filter-branch: Add more error-handling","fromName":"Eric Kidd","fromEmail":"git@randomhacks.net","sentAt":"2009-02-11T19:34:09Z","receivedAt":"2009-02-11T19:34:09Z","isPatch":true,"sender":{"key":"git@randomhacks.net","avatar":null},"body":"On Wed, Feb 11, 2009 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> @@ -339,7 +339,8 @@ while read commit parents; do\n>>               eval \"$filter_msg\" > ../message ||\n>>                       die \"msg filter failed: $filter_msg\"\n>>       @SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n>> -             $(git write-tree) $parentstr < ../message > ../map/$commit\n>> +             $(git write-tree) $parentstr < ../message > ../map/$commit ||\n>> +                     die \"could not write rewritten commit\"\n>\n> Hmm, wouldn't commit-tree have issued its own error message already?  If\n> redirect failed, then the shell would have.\n\nIf a custom $filter_commit calls 'exit', there won't be an error\nmessage from git. Of course, there may be an error message from the\ncustom $filter_commit, but I decided not to rely on that. Would you\nprefer me to remove the error message?\n\nI'll have another patch ready shortly, incorporating your suggestions.\nMy apologies for giving credit in the wrong part of the commit\nmessage, and thank you for your feedback!\n\nCheers,\nEric\n"},{"id":"104289","messageId":"1234382600-7801-1-git-send-email-git@randomhacks.net","threadId":"17723","inReplyTo":"7vhc30eqy7.fsf@gitster.siamese.dyndns.org","subject":"[PATCHv3] filter-branch: Add more error-handling","fromName":"Eric Kidd","fromEmail":"git@randomhacks.net","sentAt":"2009-02-11T20:03:20Z","receivedAt":"2009-02-11T20:03:20Z","isPatch":false,"sender":{"key":"git@randomhacks.net","avatar":null},"body":"In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error\nthat had slipped by the test suites because of a missing check on 'git\nread-tree -u -m HEAD'.\n\nThis patch attemps to add all the missing error checks to\ngit-filter-branch, and removes an existing $ret variable that did\nnothing.  I've tested this patch using t/t7003-filter-branch.sh, and it\npasses all the existing tests.\n\nThis patch also causes 'git filter-branch' to fail if the --commit-filter\nargument returns an error.  A test case for this behavior is included.\n\nIn two places, I've had to break apart pipelines in order to check the\nerror code for the first stage of the pipeline, as discussed here:\n\n  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614\n\nFeedback on this patch was provided by Johannes Sixt, Johannes Schindelin\nand Junio C Hamano.  charon on #git helped with pipeline error handling.\n\nSigned-off-by: Eric Kidd <git@randomhacks.net>\n\n---\n git-filter-branch.sh     |   26 ++++++++++++++------------\n t/t7003-filter-branch.sh |    4 ++++\n 2 files changed, 18 insertions(+), 12 deletions(-)\n\nv3:\n  Replaced 'exit 1' with 'exit' to use exit status of last command\n  Use test_must_fail for unit test expecting failure\n\nv2:\n  Remove useless $ret variable\n  Correctly check the first command in a pipeline, not the second\n  Replace verbose 'die' messages with 'exit 1' in most cases\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 86eef56..27b57b8 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -221,7 +221,7 @@ die \"\"\n trap 'cd ../..; rm -rf \"$tempdir\"' 0\n \n # Make sure refs/original is empty\n-git for-each-ref > \"$tempdir\"/backup-refs\n+git for-each-ref > \"$tempdir\"/backup-refs || exit\n while read sha1 type name\n do\n \tcase \"$force,$name\" in\n@@ -241,8 +241,9 @@ GIT_WORK_TREE=.\n export GIT_DIR GIT_WORK_TREE\n \n # The refs should be updated if their heads were rewritten\n-git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD \"$@\" |\n-sed -e '/^^/d' >\"$tempdir\"/heads\n+git rev-parse --no-flags --revs-only --symbolic-full-name \\\n+\t--default HEAD \"$@\" > \"$tempdir\"/raw-heads || exit\n+sed -e '/^^/d' \"$tempdir\"/raw-heads >\"$tempdir\"/heads\n \n test -s \"$tempdir\"/heads ||\n \tdie \"Which ref do you want to rewrite?\"\n@@ -251,8 +252,6 @@ GIT_INDEX_FILE=\"$(pwd)/../index\"\n export GIT_INDEX_FILE\n git read-tree || die \"Could not seed the index\"\n \n-ret=0\n-\n # map old->new commit ids for rewriting parents\n mkdir ../map || die \"Could not create map/ directory\"\n \n@@ -315,10 +314,11 @@ while read commit parents; do\n \t\t\tdie \"tree filter failed: $filter_tree\"\n \n \t\t(\n-\t\t\tgit diff-index -r --name-only $commit\n+\t\t\tgit diff-index -r --name-only $commit &&\n \t\t\tgit ls-files --others\n-\t\t) |\n-\t\tgit update-index --add --replace --remove --stdin\n+\t\t) > \"$tempdir\"/tree-state || exit\n+\t\tgit update-index --add --replace --remove --stdin \\\n+\t\t\t< \"$tempdir\"/tree-state || exit\n \tfi\n \n \teval \"$filter_index\" < /dev/null ||\n@@ -339,7 +339,8 @@ while read commit parents; do\n \t\teval \"$filter_msg\" > ../message ||\n \t\t\tdie \"msg filter failed: $filter_msg\"\n \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n-\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n+\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n+\t\t\tdie \"could not write rewritten commit\"\n done <../revs\n \n # In case of a subdirectory filter, it is possible that a specified head\n@@ -407,7 +408,8 @@ do\n \t\t\tdie \"Could not rewrite $ref\"\n \t;;\n \tesac\n-\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1\n+\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1 ||\n+\t\t exit\n done < \"$tempdir\"/heads\n \n # TODO: This should possibly go, with the semantics that all positive given\n@@ -483,7 +485,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n }\n \n if [ \"$(is_bare_repository)\" = false ]; then\n-\tgit read-tree -u -m HEAD\n+\tgit read-tree -u -m HEAD || exit\n fi\n \n-exit $ret\n+exit 0\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex cb04743..11ed4c4 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -48,6 +48,10 @@ test_expect_success 'result is really identical' '\n \ttest $H = $(git rev-parse HEAD)\n '\n \n+test_must_fail 'Fail if commit filter fails' '\n+\tgit filter-branch -f --commit-filter \"exit 1\" HEAD\n+'\n+\n test_expect_success 'rewrite, renaming a specific file' '\n \tgit filter-branch -f --tree-filter \"mv d doh || :\" HEAD\n '\n-- \n1.6.0.4\n"},{"id":"104292","messageId":"alpine.DEB.1.00.0902112145400.10279@pacific.mpi-cbg.de","threadId":"17723","inReplyTo":"1234382600-7801-1-git-send-email-git@randomhacks.net","subject":"Re: [PATCHv3] filter-branch: Add more error-handling","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-11T20:48:49Z","receivedAt":"2009-02-11T20:48:49Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 11 Feb 2009, Eric Kidd wrote:\n\n> charon on #git helped with pipeline error handling.\n\nJFYI charon is the nick of Thomas Rast.\n\n> diff --git a/git-filter-branch.sh b/git-filter-branch.sh\n> index 86eef56..27b57b8 100755\n> --- a/git-filter-branch.sh\n> +++ b/git-filter-branch.sh\n> @@ -221,7 +221,7 @@ die \"\"\n>  trap 'cd ../..; rm -rf \"$tempdir\"' 0\n>  \n>  # Make sure refs/original is empty\n> -git for-each-ref > \"$tempdir\"/backup-refs\n> +git for-each-ref > \"$tempdir\"/backup-refs || exit\n\nI haven't checked, but is \"$tempdir\" not the working directory?  If so, \nthis would lead to funny interaction with --tree-filter.  Rather, I'd \nwrite the file into \"$GIT_DIR\".  Likewise the other files.\n\n> +test_must_fail 'Fail if commit filter fails' '\n> +\tgit filter-branch -f --commit-filter \"exit 1\" HEAD\n> +'\n> +\n\nThat's not how it is supposed to be used.  Rather,\n\n\ttest_expect_success $LABEL '\n\t\ttest_must_fail git filter-branc $OPTIONS\n\t'\n\nThanks,\nDscho\n"},{"id":"104295","messageId":"431341160902111300r1a1c3a22n3c098a7d824a3fca@mail.gmail.com","threadId":"17723","inReplyTo":"alpine.DEB.1.00.0902112145400.10279@pacific.mpi-cbg.de","subject":"Re: [PATCHv3] filter-branch: Add more error-handling","fromName":"Eric Kidd","fromEmail":"git@randomhacks.net","sentAt":"2009-02-11T21:00:18Z","receivedAt":"2009-02-11T21:00:18Z","isPatch":false,"sender":{"key":"git@randomhacks.net","avatar":null},"body":"On Wed, Feb 11, 2009 at 3:48 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> I haven't checked, but is \"$tempdir\" not the working directory?  If so,\n> this would lead to funny interaction with --tree-filter.  Rather, I'd\n> write the file into \"$GIT_DIR\".  Likewise the other files.\n\nThe working directory actually lives one level down:\n\n tempdir=.git-rewrite\n workdir=\"$tempdir/t\"\n\nAt the end of the script, git-filter-branch cleans up all of its\ntemporary files by deleting $tempdir. There's actually a fair bit of\nstuff in there already, and none of it interferes with --tree-filter.\n\n> That's not how it is supposed to be used.  Rather,\n>\n>        test_expect_success $LABEL '\n>                test_must_fail git filter-branc $OPTIONS\n>        '\n\nWill fix. Thank you.\n\nI really appreciate all this feedback from the git team. Thank you for\ntaking the time to help me get this right!\n\nCheers,\nEric\n"},{"id":"104298","messageId":"1234386641-14683-1-git-send-email-git@randomhacks.net","threadId":"17723","inReplyTo":"alpine.DEB.1.00.0902112145400.10279@pacific.mpi-cbg.de","subject":"[PATCHv4] filter-branch: Add more error-handling","fromName":"Eric Kidd","fromEmail":"git@randomhacks.net","sentAt":"2009-02-11T21:10:41Z","receivedAt":"2009-02-11T21:10:41Z","isPatch":false,"sender":{"key":"git@randomhacks.net","avatar":null},"body":"In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error\nthat had slipped by the test suites because of a missing check on 'git\nread-tree -u -m HEAD'.\n\nThis patch attemps to add all the missing error checks to\ngit-filter-branch, and removes an existing $ret variable that did\nnothing.  I've tested this patch using t/t7003-filter-branch.sh, and it\npasses all the existing tests.\n\nThis patch also causes 'git filter-branch' to fail if the --commit-filter\nargument returns an error.  A test case for this behavior is included.\n\nIn two places, I've had to break apart pipelines in order to check the\nerror code for the first stage of the pipeline, as discussed here:\n\n  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614\n\nFeedback on this patch was provided by Johannes Sixt, Johannes Schindelin\nand Junio C Hamano.  Thomas Rast helped with pipeline error handling.\n\nSigned-off-by: Eric Kidd <git@randomhacks.net>\n\n---\n git-filter-branch.sh     |   26 ++++++++++++++------------\n t/t7003-filter-branch.sh |    4 ++++\n 2 files changed, 18 insertions(+), 12 deletions(-)\n\nv4:\n  Call test_must_fail from inside test_expect_success\n\nv3:\n  Replaced 'exit 1' with 'exit' to use exit status of last command\n  Use test_must_fail for unit test expecting failure\n\nv2:\n  Remove useless $ret variable\n  Correctly check the first command in a pipeline, not the second\n  Replace verbose 'die' messages with 'exit 1' in most cases\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 86eef56..27b57b8 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -221,7 +221,7 @@ die \"\"\n trap 'cd ../..; rm -rf \"$tempdir\"' 0\n \n # Make sure refs/original is empty\n-git for-each-ref > \"$tempdir\"/backup-refs\n+git for-each-ref > \"$tempdir\"/backup-refs || exit\n while read sha1 type name\n do\n \tcase \"$force,$name\" in\n@@ -241,8 +241,9 @@ GIT_WORK_TREE=.\n export GIT_DIR GIT_WORK_TREE\n \n # The refs should be updated if their heads were rewritten\n-git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD \"$@\" |\n-sed -e '/^^/d' >\"$tempdir\"/heads\n+git rev-parse --no-flags --revs-only --symbolic-full-name \\\n+\t--default HEAD \"$@\" > \"$tempdir\"/raw-heads || exit\n+sed -e '/^^/d' \"$tempdir\"/raw-heads >\"$tempdir\"/heads\n \n test -s \"$tempdir\"/heads ||\n \tdie \"Which ref do you want to rewrite?\"\n@@ -251,8 +252,6 @@ GIT_INDEX_FILE=\"$(pwd)/../index\"\n export GIT_INDEX_FILE\n git read-tree || die \"Could not seed the index\"\n \n-ret=0\n-\n # map old->new commit ids for rewriting parents\n mkdir ../map || die \"Could not create map/ directory\"\n \n@@ -315,10 +314,11 @@ while read commit parents; do\n \t\t\tdie \"tree filter failed: $filter_tree\"\n \n \t\t(\n-\t\t\tgit diff-index -r --name-only $commit\n+\t\t\tgit diff-index -r --name-only $commit &&\n \t\t\tgit ls-files --others\n-\t\t) |\n-\t\tgit update-index --add --replace --remove --stdin\n+\t\t) > \"$tempdir\"/tree-state || exit\n+\t\tgit update-index --add --replace --remove --stdin \\\n+\t\t\t< \"$tempdir\"/tree-state || exit\n \tfi\n \n \teval \"$filter_index\" < /dev/null ||\n@@ -339,7 +339,8 @@ while read commit parents; do\n \t\teval \"$filter_msg\" > ../message ||\n \t\t\tdie \"msg filter failed: $filter_msg\"\n \t@SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n-\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n+\t\t$(git write-tree) $parentstr < ../message > ../map/$commit ||\n+\t\t\tdie \"could not write rewritten commit\"\n done <../revs\n \n # In case of a subdirectory filter, it is possible that a specified head\n@@ -407,7 +408,8 @@ do\n \t\t\tdie \"Could not rewrite $ref\"\n \t;;\n \tesac\n-\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1\n+\tgit update-ref -m \"filter-branch: backup\" \"$orig_namespace$ref\" $sha1 ||\n+\t\t exit\n done < \"$tempdir\"/heads\n \n # TODO: This should possibly go, with the semantics that all positive given\n@@ -483,7 +485,7 @@ test -z \"$ORIG_GIT_INDEX_FILE\" || {\n }\n \n if [ \"$(is_bare_repository)\" = false ]; then\n-\tgit read-tree -u -m HEAD\n+\tgit read-tree -u -m HEAD || exit\n fi\n \n-exit $ret\n+exit 0\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex cb04743..56b5ecc 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -48,6 +48,10 @@ test_expect_success 'result is really identical' '\n \ttest $H = $(git rev-parse HEAD)\n '\n \n+test_expect_success 'Fail if commit filter fails' '\n+\ttest_must_fail git filter-branch -f --commit-filter \"exit 1\" HEAD\n+'\n+\n test_expect_success 'rewrite, renaming a specific file' '\n \tgit filter-branch -f --tree-filter \"mv d doh || :\" HEAD\n '\n-- \n1.6.0.4\n"},{"id":"104302","messageId":"20090212063038.6117@nanako3.lavabit.com","threadId":"17723","inReplyTo":"7vhc30eqy7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] filter-branch: Add more error-handling","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-02-11T21:30:38Z","receivedAt":"2009-02-11T21:30:38Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano <gitster@pobox.com>:\n\n> This goes after three-dashes; people who read \"git log\" output wouldn't\n> know nor care what was in v1.\n> \n>     Subject: Fix X under condition Z\n> \n>     X should do Y if condition Z holds, but it does not.  This can result\n>     in broken results such as W and V.\n> \n>     This patch fixes X by changing A, B and C.\n> \n>     Thanks for M, N and O for reviewing and suggesting improvements.\n> \n>     Signed-off-by: A U Thor <au.thor@example.xz>\n\nI think you meant this as a sample to follow.  Can we add it to Documentation/SubmittingPatches?\n\n-- \nNanako Shiraishi, an unofficial project secretary\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"104307","messageId":"7v8wocbocr.fsf@gitster.siamese.dyndns.org","threadId":"17723","inReplyTo":"20090212063038.6117@nanako3.lavabit.com","subject":"Re: [PATCH v2] filter-branch: Add more error-handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-11T22:28:04Z","receivedAt":"2009-02-11T22:28:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi <nanako3@lavabit.com> writes:\n\n> Quoting Junio C Hamano <gitster@pobox.com>:\n>\n>> This goes after three-dashes; people who read \"git log\" output wouldn't\n>> know nor care what was in v1.\n>> \n>>     Subject: Fix X under condition Z\n>> \n>>     X should do Y if condition Z holds, but it does not.  This can result\n>>     in broken results such as W and V.\n>> \n>>     This patch fixes X by changing A, B and C.\n>> \n>>     Thanks for M, N and O for reviewing and suggesting improvements.\n>> \n>>     Signed-off-by: A U Thor <au.thor@example.xz>\n>\n> I think you meant this as a sample to follow.  Can we add it to Documentation/SubmittingPatches?\n\nI did mean it as such, but I doubt it is good enough to be in in the\ndocument (primarily because I wrote it).\n\nJust quoting the above verbatim does not make it clear that \"Thanks for M,\nN...\" is usually not even wanted, but was merely a suggestion for this\nspecific case of Eric's commit, iow, _only if he wanted to_.  We need more\ncommentary like that, but with too much details, it would cease to be a\ngeneric recommendation.\n\nAlso, the above is not suitable for new features at all as a template.\n"}]}