{"thread":{"id":"8863","subject":"[PATCH 2/2] filter-branch: fail gracefully when a filter fails","startedAt":"2007-07-04T14:36:01Z","lastAt":"2007-07-06T09:31:44Z","messageCount":10,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano","Josh Triplett","Johannes Sixt","David Kastrup"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"46489","messageId":"Pine.LNX.4.64.0707041535420.4071@racer.site","threadId":"8863","inReplyTo":null,"subject":"[PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-04T14:36:01Z","receivedAt":"2007-07-04T14:36:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nA common mistake is to provide a filter which fails unwantedly. For\nexample, this will stop in the middle:\n\n\tgit filter-branch --env-filter '\n\t\ttest $GIT_COMMITTER_EMAIL = xyz &&\n\t\texport GIT_COMMITTER_EMAIL = abc' rewritten\n\nWhen $GIT_COMMITTER_EMAIL is not \"xyz\", the test fails, and consequently\nthe whole filter has a non-zero exit status. However, as demonstrated\nin this example, filter-branch would just stop, and the user would be\nnone the wiser.\n\nAlso, a failing msg-filter would not have been caught, as was the\ncase with one of the tests.\n\nThis patch fixes both issues, by paying attention to the exit status\nof msg-filter, and by saying what failed before exiting.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tIt is slightly ugly that the output of msg-filter is written\n\tto a temporary file. But I do not know a better method to\n\tcatch a failing msg-filter. Help?\n\n git-filter-branch.sh | 39 +++++++++++++++++++++++++++++----------\n t/t7003-filter-branch.sh | 8 +++++++-\n 2 files changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 3bf5d88..f0a5070 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -20,6 +20,16 @@ map()\n \tcat \"$workdir/../map/$1\"\n }\n \n+# override die(): this version puts in an extra line break, so that\n+# the progress is still visible\n+\n+die()\n+{\n+\techo >&2\n+\techo \"$*\" >&2\n+\texit 1\n+}\n+\n # When piped a commit, output a script to set the ident of either\n # \"author\" or \"committer\n \n@@ -173,23 +183,29 @@ while read commit parents; do\n \texport GIT_COMMIT=$commit\n \tgit cat-file commit \"$commit\" >../commit\n \n-\teval \"$(set_ident AUTHOR <../commit)\"\n-\teval \"$(set_ident COMMITTER <../commit)\"\n-\teval \"$filter_env\" < /dev/null\n+\teval \"$(set_ident AUTHOR <../commit)\" ||\n+\t\tdie \"setting author failed for commit $commit\"\n+\teval \"$(set_ident COMMITTER <../commit)\" ||\n+\t\tdie \"setting committer failed for commit $commit\"\n+\teval \"$filter_env\" < /dev/null ||\n+\t\tdie \"env filter failed: $filter_env\"\n \n \tif [ \"$filter_tree\" ]; then\n \t\tgit checkout-index -f -u -a\n \t\t# files that $commit removed are now still in the working tree;\n \t\t# remove them, else they would be added again\n \t\tgit ls-files -z --others | xargs -0 rm -f\n-\t\teval \"$filter_tree\" < /dev/null\n+\t\teval \"$filter_tree\" < /dev/null ||\n+\t\t\tdie \"tree filter failed: $filter_tree\"\n+\n \t\tgit diff-index -r $commit | cut -f 2- | tr '\\n' '\\0' | \\\n \t\t\txargs -0 git update-index --add --replace --remove\n \t\tgit ls-files -z --others | \\\n \t\t\txargs -0 git update-index --add --replace --remove\n \tfi\n \n-\teval \"$filter_index\" < /dev/null\n+\teval \"$filter_index\" < /dev/null ||\n+\t\tdie \"index filter failed: $filter_index\"\n \n \tparentstr=\n \tfor parent in $parents; do\n@@ -198,13 +214,15 @@ while read commit parents; do\n \t\tdone\n \tdone\n \tif [ \"$filter_parent\" ]; then\n-\t\tparentstr=\"$(echo \"$parentstr\" | eval \"$filter_parent\")\"\n+\t\tparentstr=\"$(echo \"$parentstr\" | eval \"$filter_parent\")\" ||\n+\t\t\t\tdie \"parent filter failed: $filter_parent\"\n \tfi\n \n \tsed -e '1,/^$/d' <../commit | \\\n-\t\teval \"$filter_msg\" | \\\n-\t\tsh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n-\t\t\t$parentstr > ../map/$commit\n+\t\teval \"$filter_msg\" > ../message ||\n+\t\t\tdie \"msg filter failed: $filter_msg\"\n+\tsh -c \"$filter_commit\" \"git commit-tree\" \\\n+\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\n done <../revs\n \n src_head=$(tail -n 1 ../revs | sed -e 's/ .*//')\n@@ -241,7 +259,8 @@ if [ \"$filter_tag_name\" ]; then\n \t\t[ -f \"../map/$sha1\" ] || continue\n \t\tnew_sha1=\"$(cat \"../map/$sha1\")\"\n \t\texport GIT_COMMIT=\"$sha1\"\n-\t\tnew_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\"\n+\t\tnew_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\" ||\n+\t\t\tdie \"tag name filter failed: $filter_tag_name\"\n \n \t\techo \"$ref -> $new_ref ($sha1 -> $new_sha1)\"\n \ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 451ac86..4ddd656 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -107,13 +107,19 @@ test_expect_success 'use index-filter to move into a subdirectory' '\n \t\t mv \\$GIT_INDEX_FILE.new \\$GIT_INDEX_FILE\" directorymoved &&\n \ttest -z \"$(git diff HEAD directorymoved:newsubdir)\"'\n \n+test_expect_success 'stops when msg filter fails' '\n+\t! git-filter-branch --msg-filter false nonono &&\n+\trm -rf .git-rewrite &&\n+\t! git rev-parse nonono\n+'\n+\n test_expect_success 'author information is preserved' '\n \t: > i &&\n \tgit add i &&\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=\"B V Uips\" git commit -m bvuips &&\n \tgit-filter-branch --msg-filter \"cat; \\\n-\t\t\ttest \\$GIT_COMMIT = $(git rev-parse master) && \\\n+\t\t\ttest \\$GIT_COMMIT != $(git rev-parse master) || \\\n \t\t\techo Hallo\" \\\n \t\tpreserved-author &&\n \ttest 1 = $(git rev-list --author=\"B V Uips\" preserved-author | wc -l)\n-- \n1.5.3.rc0.2646.g88600-dirty\n"},{"id":"46549","messageId":"20070705135824.GB5493@sigill.intra.peff.net","threadId":"8863","inReplyTo":"Pine.LNX.4.64.0707041535420.4071@racer.site","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-05T13:58:24Z","receivedAt":"2007-07-05T13:58:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 04, 2007 at 03:36:01PM +0100, Johannes Schindelin wrote:\n\n> \tIt is slightly ugly that the output of msg-filter is written\n> \tto a temporary file. But I do not know a better method to\n> \tcatch a failing msg-filter. Help?\n\nIf you mean, in general, to catch the exit code of the first part of a\npipe, you have to do something like this:\n\nstatus=`((cmd1; echo $? >&3) | cmd2) 3>&1`\n\nwhich is pretty ugly in itself, and if you want the stdout of cmd2, then\nyou have to add even more redirection. I'm not sure it's worth it.\n\n-Peff\n"},{"id":"46551","messageId":"Pine.LNX.4.64.0707051636240.9789@racer.site","threadId":"8863","inReplyTo":"20070705135824.GB5493@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-05T15:38:06Z","receivedAt":"2007-07-05T15:38:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 5 Jul 2007, Jeff King wrote:\n\n> status=`((cmd1; echo $? >&3) | cmd2) 3>&1`\n\nCute.\n\nThis is the replacement patch, then (I guess there are still some nits to \nbe had, so I did not redo the proper patch yet):\n\n git-filter-branch.sh | 43 ++++++++++++++++++++++++++++++++-----------\n 1 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nold mode 100644\nnew mode 100755\nindex 6cf67df..f08288d\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -28,6 +28,16 @@ map()\n \tfi\n }\n \n+# override die(): this version puts in an extra line break, so that\n+# the progress is still visible\n+\n+die()\n+{\n+\techo \\ >&2\n+\techo \"$*\" >&2\n+\texit 1\n+}\n+\n # When piped a commit, output a script to set the ident of either\n # \"author\" or \"committer\n \n@@ -181,23 +191,29 @@ while read commit parents; do\n \texport GIT_COMMIT=$commit\n \tgit cat-file commit \"$commit\" >../commit\n \n-\teval \"$(set_ident AUTHOR <../commit)\"\n-\teval \"$(set_ident COMMITTER <../commit)\"\n-\teval \"$filter_env\" < /dev/null\n+\teval \"$(set_ident AUTHOR <../commit)\" ||\n+\t\tdie \"setting author failed for commit $commit\"\n+\teval \"$(set_ident COMMITTER <../commit)\" ||\n+\t\tdie \"setting committer failed for commit $commit\"\n+\teval \"$filter_env\" < /dev/null ||\n+\t\tdie \"env filter failed: $filter_env\"\n \n \tif [ \"$filter_tree\" ]; then\n \t\tgit checkout-index -f -u -a\n \t\t# files that $commit removed are now still in the working tree;\n \t\t# remove them, else they would be added again\n \t\tgit ls-files -z --others | xargs -0 rm -f\n-\t\teval \"$filter_tree\" < /dev/null\n+\t\teval \"$filter_tree\" < /dev/null ||\n+\t\t\tdie \"tree filter failed: $filter_tree\"\n+\n \t\tgit diff-index -r $commit | cut -f 2- | tr '\\n' '\\0' | \\\n \t\t\txargs -0 git update-index --add --replace --remove\n \t\tgit ls-files -z --others | \\\n \t\t\txargs -0 git update-index --add --replace --remove\n \tfi\n \n-\teval \"$filter_index\" < /dev/null\n+\teval \"$filter_index\" < /dev/null ||\n+\t\tdie \"index filter failed: $filter_index\"\n \n \tparentstr=\n \tfor parent in $parents; do\n@@ -206,13 +222,17 @@ while read commit parents; do\n \t\tdone\n \tdone\n \tif [ \"$filter_parent\" ]; then\n-\t\tparentstr=\"$(echo \"$parentstr\" | eval \"$filter_parent\")\"\n+\t\tparentstr=\"$(echo \"$parentstr\" | eval \"$filter_parent\")\" ||\n+\t\t\t\tdie \"parent filter failed: $filter_parent\"\n \tfi\n \n-\tsed -e '1,/^$/d' <../commit | \\\n-\t\teval \"$filter_msg\" | \\\n-\t\tsh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n-\t\t\t$parentstr > ../map/$commit\n+\t(sed -e '1,/^$/d' <../commit |\n+\t\t(eval \"$filter_msg\" ||\n+\t\t die \"msg filter failed: $filter_msg\" 2>&3) |\n+\t\t(sh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n+\t\t\t$parentstr > ../map/$commit ||\n+\t\t die \"commit filter failed: $filter_commit\" 2>&3)) 3>&1 |\n+\t grep . && die\n done <../revs\n \n src_head=$(tail -n 1 ../revs | sed -e 's/ .*//')\n@@ -249,7 +269,8 @@ if [ \"$filter_tag_name\" ]; then\n \t\t[ -f \"../map/$sha1\" ] || continue\n \t\tnew_sha1=\"$(cat \"../map/$sha1\")\"\n \t\texport GIT_COMMIT=\"$sha1\"\n-\t\tnew_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\"\n+\t\tnew_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\" ||\n+\t\t\tdie \"tag name filter failed: $filter_tag_name\"\n \n \t\techo \"$ref -> $new_ref ($sha1 -> $new_sha1)\"\n \n"},{"id":"46558","messageId":"7vk5tedc8c.fsf@assigned-by-dhcp.cox.net","threadId":"8863","inReplyTo":"Pine.LNX.4.64.0707051636240.9789@racer.site","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-05T16:52:35Z","receivedAt":"2007-07-05T16:52:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 5 Jul 2007, Jeff King wrote:\n>\n>> status=`((cmd1; echo $? >&3) | cmd2) 3>&1`\n>\n> Cute.\n>\n> This is the replacement patch, then (I guess there are still some nits to \n> be had, so I did not redo the proper patch yet):\n> ...\n> +\t(sed -e '1,/^$/d' <../commit |\n> +\t\t(eval \"$filter_msg\" ||\n> +\t\t die \"msg filter failed: $filter_msg\" 2>&3) |\n> +\t\t(sh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n> +\t\t\t$parentstr > ../map/$commit ||\n> +\t\t die \"commit filter failed: $filter_commit\" 2>&3)) 3>&1 |\n> +\t grep . && die\n\nYou certainly meant \"grep >&2 .\" at the end of that pipeline;\nwhile I welcome people to be aware that it is possible with\nesoteric shell redirection games, I personally feel this is not\nworth it.\n"},{"id":"46590","messageId":"Pine.LNX.4.64.0707060416460.4093@racer.site","threadId":"8863","inReplyTo":"468DB570.1090900@freedesktop.org","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-06T03:18:58Z","receivedAt":"2007-07-06T03:18:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 5 Jul 2007, Josh Triplett wrote:\n\n> bash has \"set -o pipefail\", but that would require bash. However, you \n> could try setting pipefail, and ignoring any failure to set it; that \n> would give the more friendly behavior with bash, while still allowing \n> any /bin/sh in general.\n\nI was aware of pipefail when I wrote that patch. However, I have zero \ninterest in a \"solution\" which works on bash, but fails on other shells. \n\nThat is like allowing a precious few to overstep some serious line (and \ncommuting them), but severely punish all others. And that's wrong. And \nto allow it to happen is wrong, too.\n\nCiao,\nDscho\n"},{"id":"46588","messageId":"468DB570.1090900@freedesktop.org","threadId":"8863","inReplyTo":"20070705135824.GB5493@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-07-06T03:22:24Z","receivedAt":"2007-07-06T03:22:24Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Jeff King wrote:\n> On Wed, Jul 04, 2007 at 03:36:01PM +0100, Johannes Schindelin wrote:\n> \n>> \tIt is slightly ugly that the output of msg-filter is written\n>> \tto a temporary file. But I do not know a better method to\n>> \tcatch a failing msg-filter. Help?\n> \n> If you mean, in general, to catch the exit code of the first part of a\n> pipe, you have to do something like this:\n> \n> status=`((cmd1; echo $? >&3) | cmd2) 3>&1`\n> \n> which is pretty ugly in itself, and if you want the stdout of cmd2, then\n> you have to add even more redirection. I'm not sure it's worth it.\n\nbash has \"set -o pipefail\", but that would require bash. However, you could\ntry setting pipefail, and ignoring any failure to set it; that would give the\nmore friendly behavior with bash, while still allowing any /bin/sh in general.\n\n- Josh Triplett\n\n\n"},{"id":"46599","messageId":"468DE9F3.B4489CDE@eudaptics.com","threadId":"8863","inReplyTo":"Pine.LNX.4.64.0707051636240.9789@racer.site","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-07-06T07:06:27Z","receivedAt":"2007-07-06T07:06:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin wrote:\n> - sed -e '1,/^$/d' <../commit | \\\n> - eval \"$filter_msg\" | \\\n> - sh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n> - $parentstr > ../map/$commit\n> + (sed -e '1,/^$/d' <../commit |\n> + (eval \"$filter_msg\" ||\n> + die \"msg filter failed: $filter_msg\" 2>&3) |\n> + (sh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n> + $parentstr > ../map/$commit ||\n> + die \"commit filter failed: $filter_commit\" 2>&3)) 3>&1 |\n> + grep . && die\n\nYou introduce a handful of new forks and an exec. Isn't an intermediate\nfile much cheaper?\n\n-- Hannes\n"},{"id":"46601","messageId":"863b02j6qs.fsf@lola.quinscape.zz","threadId":"8863","inReplyTo":"468DE9F3.B4489CDE@eudaptics.com","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-07-06T08:06:51Z","receivedAt":"2007-07-06T08:06:51Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Sixt writes:\n\n> Johannes Schindelin wrote:\n>> - sed -e '1,/^$/d' <../commit | \\\n>> - eval \"$filter_msg\" | \\\n>> - sh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n>> - $parentstr > ../map/$commit\n>> + (sed -e '1,/^$/d' <../commit |\n>> + (eval \"$filter_msg\" ||\n>> + die \"msg filter failed: $filter_msg\" 2>&3) |\n>> + (sh -c \"$filter_commit\" \"git commit-tree\" $(git write-tree) \\\n>> + $parentstr > ../map/$commit ||\n>> + die \"commit filter failed: $filter_commit\" 2>&3)) 3>&1 |\n>> + grep . && die\n>\n> You introduce a handful of new forks and an exec. Isn't an intermediate\n> file much cheaper?\n\nThe number of forks can be reduced by using { ...; } instead of (\n... ) here (though it is possible the shell optimizes them away).\ngrep . should likely redirect its output with >&2 so that it ends up\non stderr. I'd probably prefer grep ^ or grep '' since that matches\nempty lines as well. When done that way, I don't see a \"handful of\nnew forks\".\n\nInstead of \"grep .\" one could also do something like\n\nif read line then\n while echo \"$line\" && read line; do :; done\n die\nfi\n\nwhich is fork-less.\n\n-- \nDavid Kastrup\n"},{"id":"46604","messageId":"7vir8yextt.fsf@assigned-by-dhcp.cox.net","threadId":"8863","inReplyTo":"863b02j6qs.fsf@lola.quinscape.zz","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-06T08:33:02Z","receivedAt":"2007-07-06T08:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n> Instead of \"grep .\" one could also do something like\n>\n> if read line then\n> while echo \"$line\" && read line; do :; done\n> die\n> fi\n>\n> which is fork-less.\n\nI'd agree with you that \"grep ^\" would be preferable _if_ we\nwere to do this. But in your fork-less example, you are\nassuming that (1) \"read\" does not molest what is read, (2)\n\"echo\" is built-in, and (3) \"echo\" does not munge the\nparameter.\n\nSince I am one of old fashioned shell people (I readily admit\nthat I used to have _fun_ with autoconf generated shell\nscripts), I'd love to continue shell skill show-offs, but for\nthe purpose of updating this script, I would say a temporary\nfile is much better than any of the alternatives around 1>&3.\nFor one thing, it would make debugging the script while\ndeveloping and tweaking it, _and_ while using it, much more\npleasant.\n"},{"id":"46609","messageId":"20070706093138.GA12737@sigill.intra.peff.net","threadId":"8863","inReplyTo":"7vir8yextt.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-06T09:31:44Z","receivedAt":"2007-07-06T09:31:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 06, 2007 at 01:33:02AM -0700, Junio C Hamano wrote:\n\n> the purpose of updating this script, I would say a temporary\n> file is much better than any of the alternatives around 1>&3.\n\nAs the one who originally mentioned the redirection trick, let me say\nthat I agree. I brought it up more as \"yuck, this is the way you have\nto do it\" than a real suggestion. I think the tempfile, while ugly, is\nless ugly.\n\n-Peff\n"}]}