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

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 [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Stefan Beller [off-list ref] writes:

quoted

+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