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

[Eric Sunshine <sunshine@sunshineco.com>]

(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.