Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html