Re: [PATCH] Introduce a hook to run after formatting patches

From: Stefan Beller <sbeller@google.com>
Date: Nov 18, 2014, 02:30 UTC
Message-ID: <CAGZ79kZ=1qekoKfY5cwdmDp68oU-poTcc6A4Fa7u5LhwCB5HyQ@mail.gmail.com>
In-Reply-To: <xmqqlhn9y7dn.fsf@gitster.dls.corp.google.com>

Junio,

thanks for pointing out, why my patch doesn't make sense here.

Do we have similar filters somewhere in place already, so I could have a look at the code architecture, the api, and how the user would operate that?

The way you're proposing, doesn't sound as if a hook would be the right thing for such filtering.

The one big thing I liked over the first patch in this thread was the 'maintainability', i.e. if it were a hook, I could set that up and forget about it. No need to change my behavior in using git, but still I have the desired postprocessed results. A command line option for such filtering is inconvenient because of having it type all the time and not just having it setup and forgetting about it. Sure we can have a command line option to specify such a filter, but I'd be more interested in having an option in maybe the git config file like

[format-patch]
  external-commit-message-stream-processor = $(GIT_DIR)/../foo/bar.sh

The command line option would rather only be used to override that config in non-default cases.

Thanks for your thoughts, Stefan

On Mon, Nov 17, 2014 at 11:06 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 60 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> +post-format-patch
>>> +~~~~~~~~~~~~
>>> +
>>> +This hook is called after format-patch created a patch and it is
>>> +invoked with the filename of the patch as the first parameter.
>>
>> Such an interface would not work well with --stdout mode, would it?
>>
>> And if this only works with output generated into the files, then
>>
>>     $ git format-patch $range | xargs -n1 $your_post_processing_script
>>
>> would do the same without any change to Git, I would imagine.
>>
>> So I would have to say that I am fairly negative on this change in
>> the presented form.
>>
>> An alternative design to implement this as a post-processing filter
>> to work for both "to individual files" and "to standard output
>> stream" output filter may be possible, but even in that case I am
>> not sure if it is worth the churn.
>>
>> In general I'd look at post-anything hook that works locally with a
>> great suspicion, so that may partly be where my comment above is
>> coming from.  I dunno.
>
> Another reason, in addition to that this only works on the already
> created output files, why I find this particular design distasteful
> (I am not saying that there should be an easy way to drop cruft left
> by third-party systems such as "Change-id:" line) is because the
> mechanism the patch adds does not attempt to take advantage of being
> inside Git, so the "xargs -n1" above is strictly an equivalent.  You
> have a chance to make the life better for users, but not you are not
> doing so.
>
> The design of this feature could be made to allow the user to
> specify a filter to munge _ONLY_ the log message part.  For example,
> just after logmsg_reencode() returns the proposed commit log message
> to msg in pretty.c::pretty_print_commit(), you can detect a request
> to use some external filter program and let the program munge the
> message.  With such a design:
>
>  * The external filter your users would write does not have to worry
>    about limiting its damage only to the log message part, as it
>    will never see the patch text part; and
>
>  * The same mechanism would work just as well for --stdout mode.
>
> The former is what I mean by "to take advantage of being inside".
> Incidentally, it falls into #2 of "5 valid reasons to admit a new
> hook" [*1*].
>
>
> [Reference]
>
> *1* http://thread.gmane.org/gmane.comp.version-control.git/232809/focus=71069