Subject: [PATCH] git am: Transform and skip patches via new hook

Subject: [PATCH] git am: Transform and skip patches via new hook

[Robert Collins]

>From 0428b0a1248fb84c584a5a6c1f110770c6615d5e Mon Sep 17 00:00:00 2001
From: Robert Collins <rbtcollins@hp.com>
Date: Tue, 7 Jul 2015 15:43:24 +1200
Subject: [PATCH] git am: Transform and skip patches via new hook

A thing I need to do quite a lot of is extracting stuff from
Python to backported libraries. This involves changing nearly
every patch but its automatable.

Using a new hook (applypatch-transform) was sufficient to meet all my
needs and should be acceptable upstream as far as I can tell.

Signed-Off-By: Robert Collins <rbtcollins@hp.com>
---
 Documentation/git-am.txt                     |  6 ++---
 Documentation/githooks.txt                   | 15 ++++++++++++
 git-am.sh                                    | 15 ++++++++++++
 templates/hooks--applypatch-transform.sample | 36 ++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100755 templates/hooks--applypatch-transform.sample

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index dbea6e7..9ddcd87 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -215,9 +215,9 @@ errors in the "From:" lines).

 HOOKS
 -----
-This command can run `applypatch-msg`, `pre-applypatch`,
-and `post-applypatch` hooks.  See linkgit:githooks[5] for more
-information.
+This command can run `applypatch-msg`, `applypatch-transform`,
+`pre-applypatch`, and `post-applypatch` hooks.  See
+linkgit:githooks[5] for more information.

 SEE ALSO
 --------
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 7ba0ac9..251b604 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -45,6 +45,21 @@ the commit after inspecting the message file.
 The default 'applypatch-msg' hook, when enabled, runs the
 'commit-msg' hook, if the latter is enabled.

+applypatch-transform
+~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by 'git am' before attempting to apply
+patches.  It takes two parameters - the path to the patch on
+disk, and the path to the proposed commit message (which may
+be absent).  Like applypatch-msg, both files may be edited.
+
+Exiting with 1 will cause 'git am' to skip the patch. Exiting
+with any other non-zero value will cause 'git am' to abort.
+
+The sample 'applypatch-transform' hook demonstrates mangling
+a patch from one tree shape to another while discarding irrelevant
+patches.
+
 pre-applypatch
 ~~~~~~~~~~~~~~

diff --git a/git-am.sh b/git-am.sh
index 8733071..796efea 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -869,6 +869,21 @@ To restore the original branch and stop patching
run \"\$cmdline --abort\"."

  case "$resolved" in
  '')
+ # Attempt to rewrite the patch.
+ hook="$(git rev-parse --git-path hooks/applypatch-transform)"
+ if test -x "$hook"
+ then
+ "$hook" "$dotest/patch" "$dotest/final-commit"
+ status="$?"
+ if test $status -eq 1
+ then
+ go_next
+ elif test $status -ne 0
+ then
+ stop_here $this
+ fi
+ fi
+
  # When we are allowed to fall back to 3-way later, don't give
  # false errors during the initial attempt.
  squelch=
diff --git a/templates/hooks--applypatch-transform.sample
b/templates/hooks--applypatch-transform.sample
new file mode 100755
index 0000000..97cd789
--- /dev/null
+++ b/templates/hooks--applypatch-transform.sample
@@ -0,0 +1,36 @@
+#!/bin/sh
+#
+# An example hook script to transform a patch taken from an email
+# by git am.
+#
+# The hook should exit with non-zero status after issuing an
+# appropriate message if it wants to stop the commit.  The hook is
+# allowed to edit the patch file.
+#
+# To enable this hook, rename this file to "applypatch-transform".
+#
+# This example changes the path of Lib/unittest/mock.py to mock.py
+# Lib/unittest/tests/testmock to tests and Misc/NEWS to NEWS, and
+# finally skips any patches that did not alter mock.py or its tests.
+
+set -eux
+
+patch_path=$1
+
+# Pull out mock.py
+filterdiff --clean --strip 3 --addprefix=a/ -i
'a/Lib/unittest/mock.py' $patch_path > $patch_path.mock
+# And the tests
+filterdiff --clean --strip 5 --addprefix=a/tests/ -i
'a/Lib/unittest/test/testmock/' $patch_path > $patch_path.tests
+# Lastly we want to pick up any NEWS entries.
+filterdiff --strip 2 --addprefix=a/ -i a/Misc/NEWS $patch_path >
$patch_path.NEWS
+cat $patch_path.mock $patch_path.tests > $patch_path
+filtered=$(cat $patch_path)
+if [ -n "${filtered}" ]; then
+  cat $patch_path.NEWS >> $patch_path
+  exitcode=0
+else
+  exitcode=1
+fi
+
+rm $patch_path.mock $patch_path.tests $patch_path.NEWS
+exit $exitcode
-- 
2.1.0


-- 
Robert Collins <rbtcollins@hp.com>
Distinguished Technologist
HP Converged Cloud

Re: Subject: [PATCH] git am: Transform and skip patches via new hook

[Eric Sunshine]

On Tue, Jul 7, 2015 at 3:52 AM, Robert Collins
<robertc@robertcollins.net> wrote:
> From 0428b0a1248fb84c584a5a6c1f110770c6615d5e Mon Sep 17 00:00:00 2001
> From: Robert Collins <rbtcollins@hp.com>
> Date: Tue, 7 Jul 2015 15:43:24 +1200
> Subject: [PATCH] git am: Transform and skip patches via new hook

Drop the "From sha1", "Date:", and "Subject:" headers. "From sha1" is
meaningful only in your repository, thus not useful here, and git-am
will pluck the other information directly from your email, so they are
redundant. The "From:" header, however, should be kept since it
differs from your sending email address.

> A thing I need to do quite a lot of is extracting stuff from
> Python to backported libraries. This involves changing nearly
> every patch but its automatable.
>
> Using a new hook (applypatch-transform) was sufficient to meet all my
> needs and should be acceptable upstream as far as I can tell.

For a commit message, you want to explain the problem you're solving,
in what way the the current implementation is lacking, and justify why
your solution is desirable (possibly citing alternate approaches you
discarded). Unfortunately, the above paragraphs don't really tell us
much about why applypatch-tranforms is needed or how it solves a
problem which can't be solved with some other existing mechanism. You
do mention that it satisfies your "needs", but we don't know
specifically what those are.

The above paragraphs might be perfectly suitable as additional
commentary to supplement the commit messages, however, such commentary
should be placed below the "---" line under your sign-off and above
the diffstat.

> Signed-Off-By: Robert Collins <rbtcollins@hp.com>

This is typically written "Signed-off-by:".

More below.

> ---
>  Documentation/git-am.txt                     |  6 ++---
>  Documentation/githooks.txt                   | 15 ++++++++++++
>  git-am.sh                                    | 15 ++++++++++++
>  templates/hooks--applypatch-transform.sample | 36 ++++++++++++++++++++++++++++
>  4 files changed, 69 insertions(+), 3 deletions(-)
>  create mode 100755 templates/hooks--applypatch-transform.sample
>
> diff --git a/git-am.sh b/git-am.sh
> index 8733071..796efea 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -869,6 +869,21 @@ To restore the original branch and stop patching
> run \"\$cmdline --abort\"."
>
>   case "$resolved" in
>   '')
> + # Attempt to rewrite the patch.
> + hook="$(git rev-parse --git-path hooks/applypatch-transform)"
> + if test -x "$hook"
> + then
> + "$hook" "$dotest/patch" "$dotest/final-commit"
> + status="$?"
> + if test $status -eq 1
> + then
> + go_next
> + elif test $status -ne 0
> + then
> + stop_here $this
> + fi
> + fi

This indentation looks botched, as if the patch was pasted into your
email client and the client mangled the whitespace. git-send-email may
be of use here.

> diff --git a/templates/hooks--applypatch-transform.sample
> b/templates/hooks--applypatch-transform.sample
> new file mode 100755
> index 0000000..97cd789
> --- /dev/null
> +++ b/templates/hooks--applypatch-transform.sample
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +#
> +# An example hook script to transform a patch taken from an email
> +# by git am.
> +#
> +# The hook should exit with non-zero status after issuing an
> +# appropriate message if it wants to stop the commit.  The hook is
> +# allowed to edit the patch file.
> +#
> +# To enable this hook, rename this file to "applypatch-transform".
> +#
> +# This example changes the path of Lib/unittest/mock.py to mock.py
> +# Lib/unittest/tests/testmock to tests and Misc/NEWS to NEWS, and
> +# finally skips any patches that did not alter mock.py or its tests.

It's not clear even from this example what applypatch-transform buys
you over simply running your patches through some transformation and
filtering script *before* feeding them to git-am. The answer to that
question is the sort of thing which should be in the commit message to
justify the patch.

> +set -eux
> +
> +patch_path=$1
> +
> +# Pull out mock.py
> +filterdiff --clean --strip 3 --addprefix=a/ -i
> 'a/Lib/unittest/mock.py' $patch_path > $patch_path.mock
> +# And the tests
> +filterdiff --clean --strip 5 --addprefix=a/tests/ -i
> 'a/Lib/unittest/test/testmock/' $patch_path > $patch_path.tests
> +# Lastly we want to pick up any NEWS entries.
> +filterdiff --strip 2 --addprefix=a/ -i a/Misc/NEWS $patch_path >
> $patch_path.NEWS
> +cat $patch_path.mock $patch_path.tests > $patch_path
> +filtered=$(cat $patch_path)
> +if [ -n "${filtered}" ]; then
> +  cat $patch_path.NEWS >> $patch_path
> +  exitcode=0
> +else
> +  exitcode=1
> +fi
> +
> +rm $patch_path.mock $patch_path.tests $patch_path.NEWS
> +exit $exitcode
> --
> 2.1.0

Re: Subject: [PATCH] git am: Transform and skip patches via new hook

[Eric Sunshine]

(resending with 'git' list included; somehow it got dropped accidentally)

On Wed, Jul 8, 2015 at 3:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jul 08, 2015 at 11:26:33AM +1200, Robert Collins wrote:
>> The current git am offers a pre-applypatch that actual runs after
>> applying the patch, and so does not facilitate programmatic fixups of
>> patches. While its possible to manually fixup and apply one patch at
>> a time, this means sacrificing all of the automation present in git am
>> as each patch needs to be applied before the next patch can be applied
>> and tested. Further, being able to pipe git format-patch | git am can
>> be extremely convenient for porting of patches between different local
>> repositories.
>
> This commit message does a much better job of explaining the intent
> of the patch. Thanks.
>
> There are, however, some things which are still unclear. For
> instance, the commit message talks about "automation present in git
> am" but it's not clear as to what automation you are referring or how
> that automation helps your use-case. Likewise, I'm not sure I quite
> understand the bit about "each patch needs to be applied before the
> next patch can be applied and tested".
>
> The statement about "being able to pipe git format-patch | git am"
> helps explain your use-case more concretely, although that use-case
> can likely still be handled as easily (or more so) with a simple
> filter outside of Git, so it's not clear that it's a good
> justification for introducing yet another hook into Git. More about
> that below.
>
>> Git am now calls out to a new hook 'applypatch-transform' to allow
>> programmatic fixups of patches from git format-patch. The possible
>> transforms include skipping a patch, or skipping the patch entirely.
>>
>> Signed-off-by: Robert Collins <rbtcollins@hp.com>
>> ---
>>  Documentation/git-am.txt                     |  6 ++---
>>  Documentation/githooks.txt                   | 15 ++++++++++++
>>  git-am.sh                                    | 15 ++++++++++++
>>  templates/hooks--applypatch-transform.sample | 36 ++++++++++++++++++++++++++++
>>  4 files changed, 69 insertions(+), 3 deletions(-)
>
> I forgot to mention in the previous review that this change probably
> ought to be accompanied by tests. However, before spending more time
> refining the patch, it might be worthwhile to wait to hear from Junio
> whether he's even interested in this new hook. (Based upon previous
> discussions of possible new hooks, he may not be interested in a hook
> which adds no apparent value. Again, more on that below.)
>
>> --- /dev/null
>> +++ b/templates/hooks--applypatch-transform.sample
>> @@ -0,0 +1,36 @@
>> +#!/bin/sh
>> +#
>> +# An example hook script to transform a patch taken from an email
>> +# by git am.
>> +#
>> +# The hook should exit with non-zero status after issuing an
>> +# appropriate message if it wants to stop the commit.  The hook is
>> +# allowed to edit the patch file.
>> +#
>> +# To enable this hook, rename this file to "applypatch-transform".
>> +#
>> +# This example changes the path of Lib/unittest/mock.py to mock.py
>> +# Lib/unittest/tests/testmock to tests and Misc/NEWS to NEWS, and
>> +# finally skips any patches that did not alter mock.py or its tests.
>> +
>> +set -eux
>> +
>> +patch_path=$1
>> +
>> +# Pull out mock.py
>> +filterdiff --clean --strip 3 --addprefix=a/ -i
>> 'a/Lib/unittest/mock.py' $patch_path > $patch_path.mock
>> +# And the tests
>> +filterdiff --clean --strip 5 --addprefix=a/tests/ -i
>> 'a/Lib/unittest/test/testmock/' $patch_path > $patch_path.tests
>> +# Lastly we want to pick up any NEWS entries.
>> +filterdiff --strip 2 --addprefix=a/ -i a/Misc/NEWS $patch_path >
>> $patch_path.NEWS
>> +cat $patch_path.mock $patch_path.tests > $patch_path
>> +filtered=$(cat $patch_path)
>> +if [ -n "${filtered}" ]; then
>> +  cat $patch_path.NEWS >> $patch_path
>> +  exitcode=0
>> +else
>> +  exitcode=1
>> +fi
>> +
>> +rm $patch_path.mock $patch_path.tests $patch_path.NEWS
>> +exit $exitcode
>
> The commit message mentions the use-case "git format-patch | git am",
> with git-am invoking a hook for each patch to transform or reject it,
> however, it's not clear why you need a hook at all when a simple
> pipeline should work just as well. For instance:
>
>     git format-patch | myfilter | git-am
>
> where myfilter is, for instance, a Perl script such as the following:
>
>     #!/usr/bin/perl
>     use strict;
>     use warnings;
>
>     my ($patch, $mock, $test) = '';
>     while (<>) {
>         if (/^From [[:xdigit:]]{40} /) {
>             print $patch if $patch && $mock && $test;
>             ($patch, $mock, $test) = ('', undef, undef);
>         }
>         $mock = 1 if s{^([-+]{3}) ([ab])/Lib/unittest/mock.py$}{$1 $2/mock.py};
>         $test = 1 if s{^([-+]{3}) ([ab])/Lib/unittest/test/testmock/}{$1 $2/tests/};
>         s{^([-+]{3}) ([ab])/Misc/NEWS$}{$1 $2/NEWS};
>         $patch .= $_;
>     }
>     print $patch if $patch && $mock && $test;
>
> This filter performs the exact transformations and rejections
> described by the sample hook's comment block[*1*]. Aside from not
> requiring any modifications to Git, it also is *much* faster since
> it's only invoked once rather than once per patch (and, as a bonus,
> it doesn't need to invoke the 'filterdiff' command three times per
> patch, or create and delete several temporary files per patch).
>
> Consequently, as mentioned in the original review, it's not clear
> that a new applypatch-transform hook is really needed. It's certainly
> possible that your use-case is actually more involved and difficult
> than the one presented in the sample hook, and might indeed benefit
> from a new Git hook, in which case the commit message probably needs
> to do a better job of justifying the change.
>
> Footnotes:
>
> [*1*]: The functionality of the sample myfilter script matches the
> behavior described in the comment block of your sample hook, but not
> the actual behavior. The described behavior just mentions transforms
> and rejections of patches, however, the sample hook itself actually
> drops changes to files not mentioned by the comment, even in
> non-rejected patches, whereas myfilter retains them. It would be very
> easy, however, to modify myfilter to match the hook's implemented
> behavior, as well.

Re: Subject: [PATCH] git am: Transform and skip patches via new hook

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> I forgot to mention in the previous review that this change probably
>> ought to be accompanied by tests. However, before spending more time
>> refining the patch, it might be worthwhile to wait to hear from Junio
>> whether he's even interested in this new hook. (Based upon previous
>> discussions of possible new hooks, he may not be interested in a hook
>> which adds no apparent value. Again, more on that below.)
>> ...
>> The commit message mentions the use-case "git format-patch | git am",
>> with git-am invoking a hook for each patch to transform or reject it,
>> however, it's not clear why you need a hook at all when a simple
>> pipeline should work just as well. For instance:
>>
>>     git format-patch | myfilter | git-am
>>
>> where myfilter is, for instance, a Perl script such as the following:
>>
>>     #!/usr/bin/perl
>>...
>>
>> This filter performs the exact transformations and rejections
>> described by the sample hook's comment block[*1*]. Aside from not
>> requiring any modifications to Git, it also is *much* faster since
>> it's only invoked once rather than once per patch (and, as a bonus,
>> it doesn't need to invoke the 'filterdiff' command three times per
>> patch, or create and delete several temporary files per patch).

Very well said.  The pipeline you showed above is exactly why "am"
reads from its standard input.  Incidentally, I have an "add-by"
filter (found on my 'todo' branch, which is checked out in the Meta
directory in my working tree), that I use every day when I apply
patches from my mailbox.  When I have a patch that was reviewed by
Eric, I type '|' (I happen to use Gnus; the '|' command asks for a
command and pipes the message to it) and say:

    Meta/add-by -r sunshine@ | git am -s

and the filter adds Reviewed-by: line at an appropriate place.

Another reason why a hook is a bad match for the use case under
discussion is because unlike "myfilter" in your example pipeline
above, you cannot pass options to tweak the customization to the
'transform patches' hook even if you wanted to.

People should learn to consider that hooks and filters are the last
resort mechanism, not the first choice.  There are cases where you
absolutely need to have them (e.g. when Git generates something and
then uses it internally, you _might_ want to tweak and customize
that something), but the use case presented here is a canonical
example of what you shouldn't use hooks for---preprocessing the
input to a Git command.