{"thread":{"id":"39795","subject":"Subject: [PATCH] git am: Transform and skip patches via new hook","startedAt":"2015-07-07T07:52:10Z","lastAt":"2015-07-08T22:57:01Z","messageCount":4,"participants":["Robert Collins","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"265654","messageId":"CAJ3HoZ2YdAFVt1-4dTk04=0cLTUxQocJPNSVupr09Ee01tGCAQ@mail.gmail.com","threadId":"39795","inReplyTo":null,"subject":"Subject: [PATCH] git am: Transform and skip patches via new hook","fromName":"Robert Collins","fromEmail":"robertc@robertcollins.net","sentAt":"2015-07-07T07:52:10Z","receivedAt":"2015-07-07T07:52:10Z","isPatch":true,"sender":{"key":"robertc@robertcollins.net","avatar":null},"body":">From 0428b0a1248fb84c584a5a6c1f110770c6615d5e Mon Sep 17 00:00:00 2001\nFrom: Robert Collins <rbtcollins@hp.com>\nDate: Tue, 7 Jul 2015 15:43:24 +1200\nSubject: [PATCH] git am: Transform and skip patches via new hook\n\nA thing I need to do quite a lot of is extracting stuff from\nPython to backported libraries. This involves changing nearly\nevery patch but its automatable.\n\nUsing a new hook (applypatch-transform) was sufficient to meet all my\nneeds and should be acceptable upstream as far as I can tell.\n\nSigned-Off-By: Robert Collins <rbtcollins@hp.com>\n---\n Documentation/git-am.txt                     |  6 ++---\n Documentation/githooks.txt                   | 15 ++++++++++++\n git-am.sh                                    | 15 ++++++++++++\n templates/hooks--applypatch-transform.sample | 36 ++++++++++++++++++++++++++++\n 4 files changed, 69 insertions(+), 3 deletions(-)\n create mode 100755 templates/hooks--applypatch-transform.sample\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex dbea6e7..9ddcd87 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -215,9 +215,9 @@ errors in the \"From:\" lines).\n\n HOOKS\n -----\n-This command can run `applypatch-msg`, `pre-applypatch`,\n-and `post-applypatch` hooks.  See linkgit:githooks[5] for more\n-information.\n+This command can run `applypatch-msg`, `applypatch-transform`,\n+`pre-applypatch`, and `post-applypatch` hooks.  See\n+linkgit:githooks[5] for more information.\n\n SEE ALSO\n --------\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 7ba0ac9..251b604 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -45,6 +45,21 @@ the commit after inspecting the message file.\n The default 'applypatch-msg' hook, when enabled, runs the\n 'commit-msg' hook, if the latter is enabled.\n\n+applypatch-transform\n+~~~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by 'git am' before attempting to apply\n+patches.  It takes two parameters - the path to the patch on\n+disk, and the path to the proposed commit message (which may\n+be absent).  Like applypatch-msg, both files may be edited.\n+\n+Exiting with 1 will cause 'git am' to skip the patch. Exiting\n+with any other non-zero value will cause 'git am' to abort.\n+\n+The sample 'applypatch-transform' hook demonstrates mangling\n+a patch from one tree shape to another while discarding irrelevant\n+patches.\n+\n pre-applypatch\n ~~~~~~~~~~~~~~\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 8733071..796efea 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -869,6 +869,21 @@ To restore the original branch and stop patching\nrun \\\"\\$cmdline --abort\\\".\"\n\n  case \"$resolved\" in\n  '')\n+ # Attempt to rewrite the patch.\n+ hook=\"$(git rev-parse --git-path hooks/applypatch-transform)\"\n+ if test -x \"$hook\"\n+ then\n+ \"$hook\" \"$dotest/patch\" \"$dotest/final-commit\"\n+ status=\"$?\"\n+ if test $status -eq 1\n+ then\n+ go_next\n+ elif test $status -ne 0\n+ then\n+ stop_here $this\n+ fi\n+ fi\n+\n  # When we are allowed to fall back to 3-way later, don't give\n  # false errors during the initial attempt.\n  squelch=\ndiff --git a/templates/hooks--applypatch-transform.sample\nb/templates/hooks--applypatch-transform.sample\nnew file mode 100755\nindex 0000000..97cd789\n--- /dev/null\n+++ b/templates/hooks--applypatch-transform.sample\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+#\n+# An example hook script to transform a patch taken from an email\n+# by git am.\n+#\n+# The hook should exit with non-zero status after issuing an\n+# appropriate message if it wants to stop the commit.  The hook is\n+# allowed to edit the patch file.\n+#\n+# To enable this hook, rename this file to \"applypatch-transform\".\n+#\n+# This example changes the path of Lib/unittest/mock.py to mock.py\n+# Lib/unittest/tests/testmock to tests and Misc/NEWS to NEWS, and\n+# finally skips any patches that did not alter mock.py or its tests.\n+\n+set -eux\n+\n+patch_path=$1\n+\n+# Pull out mock.py\n+filterdiff --clean --strip 3 --addprefix=a/ -i\n'a/Lib/unittest/mock.py' $patch_path > $patch_path.mock\n+# And the tests\n+filterdiff --clean --strip 5 --addprefix=a/tests/ -i\n'a/Lib/unittest/test/testmock/' $patch_path > $patch_path.tests\n+# Lastly we want to pick up any NEWS entries.\n+filterdiff --strip 2 --addprefix=a/ -i a/Misc/NEWS $patch_path >\n$patch_path.NEWS\n+cat $patch_path.mock $patch_path.tests > $patch_path\n+filtered=$(cat $patch_path)\n+if [ -n \"${filtered}\" ]; then\n+  cat $patch_path.NEWS >> $patch_path\n+  exitcode=0\n+else\n+  exitcode=1\n+fi\n+\n+rm $patch_path.mock $patch_path.tests $patch_path.NEWS\n+exit $exitcode\n-- \n2.1.0\n\n\n-- \nRobert Collins <rbtcollins@hp.com>\nDistinguished Technologist\nHP Converged Cloud\n"},{"id":"265754","messageId":"CAPig+cQy-KHAaK_byw2nMM-S8cNosTpOiyejkHzAL6VavncaOw@mail.gmail.com","threadId":"39795","inReplyTo":"CAJ3HoZ2YdAFVt1-4dTk04=0cLTUxQocJPNSVupr09Ee01tGCAQ@mail.gmail.com","subject":"Re: Subject: [PATCH] git am: Transform and skip patches via new hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-07T16:32:52Z","receivedAt":"2015-07-07T16:32:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 7, 2015 at 3:52 AM, Robert Collins\n<robertc@robertcollins.net> wrote:\n> From 0428b0a1248fb84c584a5a6c1f110770c6615d5e Mon Sep 17 00:00:00 2001\n> From: Robert Collins <rbtcollins@hp.com>\n> Date: Tue, 7 Jul 2015 15:43:24 +1200\n> Subject: [PATCH] git am: Transform and skip patches via new hook\n\nDrop the \"From sha1\", \"Date:\", and \"Subject:\" headers. \"From sha1\" is\nmeaningful only in your repository, thus not useful here, and git-am\nwill pluck the other information directly from your email, so they are\nredundant. The \"From:\" header, however, should be kept since it\ndiffers from your sending email address.\n\n> A thing I need to do quite a lot of is extracting stuff from\n> Python to backported libraries. This involves changing nearly\n> every patch but its automatable.\n>\n> Using a new hook (applypatch-transform) was sufficient to meet all my\n> needs and should be acceptable upstream as far as I can tell.\n\nFor a commit message, you want to explain the problem you're solving,\nin what way the the current implementation is lacking, and justify why\nyour solution is desirable (possibly citing alternate approaches you\ndiscarded). Unfortunately, the above paragraphs don't really tell us\nmuch about why applypatch-tranforms is needed or how it solves a\nproblem which can't be solved with some other existing mechanism. You\ndo mention that it satisfies your \"needs\", but we don't know\nspecifically what those are.\n\nThe above paragraphs might be perfectly suitable as additional\ncommentary to supplement the commit messages, however, such commentary\nshould be placed below the \"---\" line under your sign-off and above\nthe diffstat.\n\n> Signed-Off-By: Robert Collins <rbtcollins@hp.com>\n\nThis is typically written \"Signed-off-by:\".\n\nMore below.\n\n> ---\n>  Documentation/git-am.txt                     |  6 ++---\n>  Documentation/githooks.txt                   | 15 ++++++++++++\n>  git-am.sh                                    | 15 ++++++++++++\n>  templates/hooks--applypatch-transform.sample | 36 ++++++++++++++++++++++++++++\n>  4 files changed, 69 insertions(+), 3 deletions(-)\n>  create mode 100755 templates/hooks--applypatch-transform.sample\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index 8733071..796efea 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -869,6 +869,21 @@ To restore the original branch and stop patching\n> run \\\"\\$cmdline --abort\\\".\"\n>\n>   case \"$resolved\" in\n>   '')\n> + # Attempt to rewrite the patch.\n> + hook=\"$(git rev-parse --git-path hooks/applypatch-transform)\"\n> + if test -x \"$hook\"\n> + then\n> + \"$hook\" \"$dotest/patch\" \"$dotest/final-commit\"\n> + status=\"$?\"\n> + if test $status -eq 1\n> + then\n> + go_next\n> + elif test $status -ne 0\n> + then\n> + stop_here $this\n> + fi\n> + fi\n\nThis indentation looks botched, as if the patch was pasted into your\nemail client and the client mangled the whitespace. git-send-email may\nbe of use here.\n\n> diff --git a/templates/hooks--applypatch-transform.sample\n> b/templates/hooks--applypatch-transform.sample\n> new file mode 100755\n> index 0000000..97cd789\n> --- /dev/null\n> +++ b/templates/hooks--applypatch-transform.sample\n> @@ -0,0 +1,36 @@\n> +#!/bin/sh\n> +#\n> +# An example hook script to transform a patch taken from an email\n> +# by git am.\n> +#\n> +# The hook should exit with non-zero status after issuing an\n> +# appropriate message if it wants to stop the commit.  The hook is\n> +# allowed to edit the patch file.\n> +#\n> +# To enable this hook, rename this file to \"applypatch-transform\".\n> +#\n> +# This example changes the path of Lib/unittest/mock.py to mock.py\n> +# Lib/unittest/tests/testmock to tests and Misc/NEWS to NEWS, and\n> +# finally skips any patches that did not alter mock.py or its tests.\n\nIt's not clear even from this example what applypatch-transform buys\nyou over simply running your patches through some transformation and\nfiltering script *before* feeding them to git-am. The answer to that\nquestion is the sort of thing which should be in the commit message to\njustify the patch.\n\n> +set -eux\n> +\n> +patch_path=$1\n> +\n> +# Pull out mock.py\n> +filterdiff --clean --strip 3 --addprefix=a/ -i\n> 'a/Lib/unittest/mock.py' $patch_path > $patch_path.mock\n> +# And the tests\n> +filterdiff --clean --strip 5 --addprefix=a/tests/ -i\n> 'a/Lib/unittest/test/testmock/' $patch_path > $patch_path.tests\n> +# Lastly we want to pick up any NEWS entries.\n> +filterdiff --strip 2 --addprefix=a/ -i a/Misc/NEWS $patch_path >\n> $patch_path.NEWS\n> +cat $patch_path.mock $patch_path.tests > $patch_path\n> +filtered=$(cat $patch_path)\n> +if [ -n \"${filtered}\" ]; then\n> +  cat $patch_path.NEWS >> $patch_path\n> +  exitcode=0\n> +else\n> +  exitcode=1\n> +fi\n> +\n> +rm $patch_path.mock $patch_path.tests $patch_path.NEWS\n> +exit $exitcode\n> --\n> 2.1.0\n"},{"id":"265881","messageId":"CAPig+cQx5TKwLk+MjsOJVHriqmwYbEcKUtARJVzef4HyN0thgg@mail.gmail.com","threadId":"39795","inReplyTo":"20150708194844.GA895@flurp.local","subject":"Re: Subject: [PATCH] git am: Transform and skip patches via new hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-08T21:17:10Z","receivedAt":"2015-07-08T21:17:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"(resending with 'git' list included; somehow it got dropped accidentally)\n\nOn Wed, Jul 8, 2015 at 3:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jul 08, 2015 at 11:26:33AM +1200, Robert Collins wrote:\n>> The current git am offers a pre-applypatch that actual runs after\n>> applying the patch, and so does not facilitate programmatic fixups of\n>> patches. While its possible to manually fixup and apply one patch at\n>> a time, this means sacrificing all of the automation present in git am\n>> as each patch needs to be applied before the next patch can be applied\n>> and tested. Further, being able to pipe git format-patch | git am can\n>> be extremely convenient for porting of patches between different local\n>> repositories.\n>\n> This commit message does a much better job of explaining the intent\n> of the patch. Thanks.\n>\n> There are, however, some things which are still unclear. For\n> instance, the commit message talks about \"automation present in git\n> am\" but it's not clear as to what automation you are referring or how\n> that automation helps your use-case. Likewise, I'm not sure I quite\n> understand the bit about \"each patch needs to be applied before the\n> next patch can be applied and tested\".\n>\n> The statement about \"being able to pipe git format-patch | git am\"\n> helps explain your use-case more concretely, although that use-case\n> can likely still be handled as easily (or more so) with a simple\n> filter outside of Git, so it's not clear that it's a good\n> justification for introducing yet another hook into Git. More about\n> that below.\n>\n>> Git am now calls out to a new hook 'applypatch-transform' to allow\n>> programmatic fixups of patches from git format-patch. The possible\n>> transforms include skipping a patch, or skipping the patch entirely.\n>>\n>> Signed-off-by: Robert Collins <rbtcollins@hp.com>\n>> ---\n>>  Documentation/git-am.txt                     |  6 ++---\n>>  Documentation/githooks.txt                   | 15 ++++++++++++\n>>  git-am.sh                                    | 15 ++++++++++++\n>>  templates/hooks--applypatch-transform.sample | 36 ++++++++++++++++++++++++++++\n>>  4 files changed, 69 insertions(+), 3 deletions(-)\n>\n> I forgot to mention in the previous review that this change probably\n> ought to be accompanied by tests. However, before spending more time\n> refining the patch, it might be worthwhile to wait to hear from Junio\n> whether he's even interested in this new hook. (Based upon previous\n> discussions of possible new hooks, he may not be interested in a hook\n> which adds no apparent value. Again, more on that below.)\n>\n>> --- /dev/null\n>> +++ b/templates/hooks--applypatch-transform.sample\n>> @@ -0,0 +1,36 @@\n>> +#!/bin/sh\n>> +#\n>> +# An example hook script to transform a patch taken from an email\n>> +# by git am.\n>> +#\n>> +# The hook should exit with non-zero status after issuing an\n>> +# appropriate message if it wants to stop the commit.  The hook is\n>> +# allowed to edit the patch file.\n>> +#\n>> +# To enable this hook, rename this file to \"applypatch-transform\".\n>> +#\n>> +# This example changes the path of Lib/unittest/mock.py to mock.py\n>> +# Lib/unittest/tests/testmock to tests and Misc/NEWS to NEWS, and\n>> +# finally skips any patches that did not alter mock.py or its tests.\n>> +\n>> +set -eux\n>> +\n>> +patch_path=$1\n>> +\n>> +# Pull out mock.py\n>> +filterdiff --clean --strip 3 --addprefix=a/ -i\n>> 'a/Lib/unittest/mock.py' $patch_path > $patch_path.mock\n>> +# And the tests\n>> +filterdiff --clean --strip 5 --addprefix=a/tests/ -i\n>> 'a/Lib/unittest/test/testmock/' $patch_path > $patch_path.tests\n>> +# Lastly we want to pick up any NEWS entries.\n>> +filterdiff --strip 2 --addprefix=a/ -i a/Misc/NEWS $patch_path >\n>> $patch_path.NEWS\n>> +cat $patch_path.mock $patch_path.tests > $patch_path\n>> +filtered=$(cat $patch_path)\n>> +if [ -n \"${filtered}\" ]; then\n>> +  cat $patch_path.NEWS >> $patch_path\n>> +  exitcode=0\n>> +else\n>> +  exitcode=1\n>> +fi\n>> +\n>> +rm $patch_path.mock $patch_path.tests $patch_path.NEWS\n>> +exit $exitcode\n>\n> The commit message mentions the use-case \"git format-patch | git am\",\n> with git-am invoking a hook for each patch to transform or reject it,\n> however, it's not clear why you need a hook at all when a simple\n> pipeline should work just as well. For instance:\n>\n>     git format-patch | myfilter | git-am\n>\n> where myfilter is, for instance, a Perl script such as the following:\n>\n>     #!/usr/bin/perl\n>     use strict;\n>     use warnings;\n>\n>     my ($patch, $mock, $test) = '';\n>     while (<>) {\n>         if (/^From [[:xdigit:]]{40} /) {\n>             print $patch if $patch && $mock && $test;\n>             ($patch, $mock, $test) = ('', undef, undef);\n>         }\n>         $mock = 1 if s{^([-+]{3}) ([ab])/Lib/unittest/mock.py$}{$1 $2/mock.py};\n>         $test = 1 if s{^([-+]{3}) ([ab])/Lib/unittest/test/testmock/}{$1 $2/tests/};\n>         s{^([-+]{3}) ([ab])/Misc/NEWS$}{$1 $2/NEWS};\n>         $patch .= $_;\n>     }\n>     print $patch if $patch && $mock && $test;\n>\n> This filter performs the exact transformations and rejections\n> described by the sample hook's comment block[*1*]. Aside from not\n> requiring any modifications to Git, it also is *much* faster since\n> it's only invoked once rather than once per patch (and, as a bonus,\n> it doesn't need to invoke the 'filterdiff' command three times per\n> patch, or create and delete several temporary files per patch).\n>\n> Consequently, as mentioned in the original review, it's not clear\n> that a new applypatch-transform hook is really needed. It's certainly\n> possible that your use-case is actually more involved and difficult\n> than the one presented in the sample hook, and might indeed benefit\n> from a new Git hook, in which case the commit message probably needs\n> to do a better job of justifying the change.\n>\n> Footnotes:\n>\n> [*1*]: The functionality of the sample myfilter script matches the\n> behavior described in the comment block of your sample hook, but not\n> the actual behavior. The described behavior just mentions transforms\n> and rejections of patches, however, the sample hook itself actually\n> drops changes to files not mentioned by the comment, even in\n> non-rejected patches, whereas myfilter retains them. It would be very\n> easy, however, to modify myfilter to match the hook's implemented\n> behavior, as well.\n"},{"id":"265883","messageId":"xmqq8uaqxnpe.fsf@gitster.dls.corp.google.com","threadId":"39795","inReplyTo":"CAPig+cQx5TKwLk+MjsOJVHriqmwYbEcKUtARJVzef4HyN0thgg@mail.gmail.com","subject":"Re: Subject: [PATCH] git am: Transform and skip patches via new hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-08T22:57:01Z","receivedAt":"2015-07-08T22:57:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> I forgot to mention in the previous review that this change probably\n>> ought to be accompanied by tests. However, before spending more time\n>> refining the patch, it might be worthwhile to wait to hear from Junio\n>> whether he's even interested in this new hook. (Based upon previous\n>> discussions of possible new hooks, he may not be interested in a hook\n>> which adds no apparent value. Again, more on that below.)\n>> ...\n>> The commit message mentions the use-case \"git format-patch | git am\",\n>> with git-am invoking a hook for each patch to transform or reject it,\n>> however, it's not clear why you need a hook at all when a simple\n>> pipeline should work just as well. For instance:\n>>\n>>     git format-patch | myfilter | git-am\n>>\n>> where myfilter is, for instance, a Perl script such as the following:\n>>\n>>     #!/usr/bin/perl\n>>...\n>>\n>> This filter performs the exact transformations and rejections\n>> described by the sample hook's comment block[*1*]. Aside from not\n>> requiring any modifications to Git, it also is *much* faster since\n>> it's only invoked once rather than once per patch (and, as a bonus,\n>> it doesn't need to invoke the 'filterdiff' command three times per\n>> patch, or create and delete several temporary files per patch).\n\nVery well said.  The pipeline you showed above is exactly why \"am\"\nreads from its standard input.  Incidentally, I have an \"add-by\"\nfilter (found on my 'todo' branch, which is checked out in the Meta\ndirectory in my working tree), that I use every day when I apply\npatches from my mailbox.  When I have a patch that was reviewed by\nEric, I type '|' (I happen to use Gnus; the '|' command asks for a\ncommand and pipes the message to it) and say:\n\n    Meta/add-by -r sunshine@ | git am -s\n\nand the filter adds Reviewed-by: line at an appropriate place.\n\nAnother reason why a hook is a bad match for the use case under\ndiscussion is because unlike \"myfilter\" in your example pipeline\nabove, you cannot pass options to tweak the customization to the\n'transform patches' hook even if you wanted to.\n\nPeople should learn to consider that hooks and filters are the last\nresort mechanism, not the first choice.  There are cases where you\nabsolutely need to have them (e.g. when Git generates something and\nthen uses it internally, you _might_ want to tweak and customize\nthat something), but the use case presented here is a canonical\nexample of what you shouldn't use hooks for---preprocessing the\ninput to a Git command.\n"}]}