[PATCH] Introduce a hook to run after formatting patches

[PATCH] Introduce a hook to run after formatting patches

[Stefan Beller]

This comes in handy if you want to post-process formatted patches.
One examplary use case would be removing ChangeIds, which are used
in Gerrit, a program sitting on top of Git, used for tracking
different versions of a patch.

Another use case would be checking if all your commits are signed off,
or have another kind of property.

So in my case a hook like the following will help a lot.

	# Run with on formatted patches. The first argument is the filename to the patch.
	sed --in-place '/^Change-Id:/d' $1

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Hi Git people,
 I haven't sent a patch for some time now, but I intend to change that
 soon, as I'll be overtaking the transactions series from Ronnie Sahlberg.

 The patch series I intend to overtake has been reviewed on this list
 as well as https://code-review.googlesource.com/#/q/project:git
 using Gerrit. Gerrit uses Change-Ids, which I want to reliably 
 remove before sending them on the list. And for reliability 
 you better trust a machine than a human like me.

 Documentation/githooks.txt |  9 +++++++++
 builtin/log.c              | 17 +++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9ef2469..b4f06a9 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -69,6 +69,15 @@ and is invoked after the patch is applied and a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of 'git am'.
 
+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.
+
+This hook can be used to alter the created patch, such as removing
+or adding Sign-Offs or similar information.
+
 pre-commit
 ~~~~~~~~~~
 
diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..863fcef 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -761,7 +761,8 @@ static const char *output_directory = NULL;
 static int outdir_offset;
 
 static int reopen_stdout(struct commit *commit, const char *subject,
-			 struct rev_info *rev, int quiet)
+			 struct rev_info *rev, int quiet,
+			 struct strbuf *choosen_filename)
 {
 	struct strbuf filename = STRBUF_INIT;
 	int suffix_len = strlen(rev->patch_suffix) + 1;
@@ -788,6 +789,11 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 	if (freopen(filename.buf, "w", stdout) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
 
+	if (choosen_filename) {
+		strbuf_reset(choosen_filename);
+		strbuf_addstr(choosen_filename, filename.buf);
+	}
+
 	strbuf_release(&filename);
 	return 0;
 }
@@ -921,7 +927,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	committer = git_committer_info(0);
 
 	if (!use_stdout &&
-	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet, NULL))
 		return;
 
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -1176,6 +1182,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf filename = STRBUF_INIT;
 	int use_patch_format = 0;
 	int quiet = 0;
 	int reroll_count = -1;
@@ -1531,7 +1538,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout &&
-		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet, &filename))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free_commit_buffer(commit);
@@ -1552,8 +1559,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			else
 				print_signature();
 		}
-		if (!use_stdout)
+		if (!use_stdout) {
 			fclose(stdout);
+			run_hook_le(NULL, "post-format-patch", filename.buf, NULL);
+		}
 	}
 	free(list);
 	free(branch_name);
-- 
2.2.0.rc1.24.g562add4

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

[Philip Oakley]

From: "Stefan Beller" <sbeller@google.com>
> This comes in handy if you want to post-process formatted patches.
> One examplary use case would be removing ChangeIds, which are used
> in Gerrit, a program sitting on top of Git, used for tracking
> different versions of a patch.
>
> Another use case would be checking if all your commits are signed off,
> or have another kind of property.
>
> So in my case a hook like the following will help a lot.
>
> # Run with on formatted patches. The first argument is the filename to 
> the patch.
> sed --in-place '/^Change-Id:/d' $1
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Hi Git people,
> I haven't sent a patch for some time now, but I intend to change that
> soon, as I'll be overtaking the transactions series from Ronnie 
> Sahlberg.
>
> The patch series I intend to overtake has been reviewed on this list
> as well as https://code-review.googlesource.com/#/q/project:git
> using Gerrit. Gerrit uses Change-Ids, which I want to reliably
> remove before sending them on the list. And for reliability
> you better trust a machine than a human like me.
>
> Documentation/githooks.txt |  9 +++++++++
> builtin/log.c              | 17 +++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 9ef2469..b4f06a9 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -69,6 +69,15 @@ and is invoked after the patch is applied and a 
> commit is made.
> This hook is meant primarily for notification, and cannot affect
> the outcome of 'git am'.
>
> +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.
> +
> +This hook can be used to alter the created patch, such as removing
> +or adding Sign-Offs or similar information.

surely   s/adding/checking/  as described in the commit message.
We wouldn't want unthinking sign-offs being applied;-)

> +
> pre-commit
> ~~~~~~~~~~
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 734aab3..863fcef 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -761,7 +761,8 @@ static const char *output_directory = NULL;
> static int outdir_offset;
>
> static int reopen_stdout(struct commit *commit, const char *subject,
> - struct rev_info *rev, int quiet)
> + struct rev_info *rev, int quiet,
> + struct strbuf *choosen_filename)
> {
>  struct strbuf filename = STRBUF_INIT;
>  int suffix_len = strlen(rev->patch_suffix) + 1;
> @@ -788,6 +789,11 @@ static int reopen_stdout(struct commit *commit, 
> const char *subject,
>  if (freopen(filename.buf, "w", stdout) == NULL)
>  return error(_("Cannot open patch file %s"), filename.buf);
>
> + if (choosen_filename) {
> + strbuf_reset(choosen_filename);
> + strbuf_addstr(choosen_filename, filename.buf);
> + }
> +
>  strbuf_release(&filename);
>  return 0;
> }
> @@ -921,7 +927,7 @@ static void make_cover_letter(struct rev_info 
> *rev, int use_stdout,
>  committer = git_committer_info(0);
>
>  if (!use_stdout &&
> -     reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", 
> rev, quiet))
> +     reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", 
> rev, quiet, NULL))
>  return;
>
>  log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
> @@ -1176,6 +1182,7 @@ int cmd_format_patch(int argc, const char 
> **argv, const char *prefix)
>  const char *in_reply_to = NULL;
>  struct patch_ids ids;
>  struct strbuf buf = STRBUF_INIT;
> + struct strbuf filename = STRBUF_INIT;
>  int use_patch_format = 0;
>  int quiet = 0;
>  int reroll_count = -1;
> @@ -1531,7 +1538,7 @@ int cmd_format_patch(int argc, const char 
> **argv, const char *prefix)
>  }
>
>  if (!use_stdout &&
> -     reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, 
> quiet))
> +     reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, 
> quiet, &filename))
>  die(_("Failed to create output files"));
>  shown = log_tree_commit(&rev, commit);
>  free_commit_buffer(commit);
> @@ -1552,8 +1559,10 @@ int cmd_format_patch(int argc, const char 
> **argv, const char *prefix)
>  else
>  print_signature();
>  }
> - if (!use_stdout)
> + if (!use_stdout) {
>  fclose(stdout);
> + run_hook_le(NULL, "post-format-patch", filename.buf, NULL);
> + }
>  }
>  free(list);
>  free(branch_name);
> -- 
> 2.2.0.rc1.24.g562add4
>
> --
Philip 

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

[Junio C Hamano]

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.

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

[Stefan Beller]

On Sun, Nov 16, 2014 at 10:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Ok I'll try to use these commands.

>
> So I would have to say that I am fairly negative on this change in
> the presented form.

Ok, I definitely did not expect this patch to be accepted the way it is,
but rather was just proposing an idea. The post-format-patch.sample
hook file would be missing for example.

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

Git is very easy to use when importing from other VCS, because of all
the helpers like high level git-svn or low level fast-import.

Patches on mailing lists can also be easily imported as a commit,
so I think having an exporting system in place with various options
like custom post processing would help for some use cases.

Anyway I'll just drop this patch and go with your suggestion.

Thanks,
Stefan

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

[Junio C Hamano]

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> (I am not saying that there should be an easy way to drop cruft left
> by third-party systems such as "Change-id:" line) ...

Heh, that was "should not be", but I guess it was probably obvious.

Sorry for the noise.

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

[Stefan Beller]

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:
> 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

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

[Christian Couder]

On Mon, Nov 17, 2014 at 8:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> (I am not saying that there should be an easy way to drop cruft left
>> by third-party systems such as "Change-id:" line) ...
>
> Heh, that was "should not be", but I guess it was probably obvious.
>
> Sorry for the noise.

I am not sure it is very easy yet but as Change-id: ... line are
trailers, you can do that with git interpret-trailers.

For example:

$ echo -e "\nChange-id: stuff\nOther: thing"  | git -c
trailer.Change-id.ifexists=replace interpret-trailers --trim-empty
--trailer Change-id=
>
> Other: thing

The idea is that the above command replaces an existing "Change-id:
stuff" trailer with an empty "Change-id:" trailer and then removes all
the empty trailers.

Best,
Christian.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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 clean/smudge filters interacts with the payload data and the end
user configuration in a similar way, I would say.

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

That depends on how you define "what a hook is", I think.

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

In the message you are responding, I said "detect a request to use",
exactly because I wanted to leave it up to you to design what
form(s) that "request detection" takes.  One of the forms could be
"a script with this $filename exists in $GIT_DIR/", and the $filname
may be "hooks/format-patch-redaction-filter".

Of course, any "configured into repository" solution must have a
command line override, so the order you would develop this would be:

 (0) make the code that interracts with the filter if given by the
     user work, without worrying about how the user specifies the
     filter.

 (1) add a --command-line-option=<filter-name> to trigger the code
     you wrote in (0) above.

 (2) add a --no-command-line-option to defeat the configured filter,
     without worrying about how the user configures.  For your new
     command line option --frotz, "git cmd --frotz --no-frotz"
     should make your cmd refrain from doing frotz.

 (3) add configuration variable to point at a filter script,
     e.g. "format.redactionFilter"; you must make sure that this is
     disabled with "--no-*" you added in (2) above;

 (4) perhaps add support for "hooks/format-patch-redaction-filter",
     if you strongly feel like it.  The user must be able to disable
     this with the same "--no-*" added in (2).

I'd say (4) is optional; by the time you reach (3), you already have
the same "write once and forget" capability.

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

[Stefan Beller]

On Tue, Nov 18, 2014 at 07:40:07AM +0100, Christian Couder wrote:
> On Mon, Nov 17, 2014 at 8:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> (I am not saying that there should be an easy way to drop cruft left
> >> by third-party systems such as "Change-id:" line) ...
> >
> > Heh, that was "should not be", but I guess it was probably obvious.
> >
> > Sorry for the noise.
> 
> I am not sure it is very easy yet but as Change-id: ... line are
> trailers, you can do that with git interpret-trailers.
> 
> For example:
> 
> $ echo -e "\nChange-id: stuff\nOther: thing"  | git -c
> trailer.Change-id.ifexists=replace interpret-trailers --trim-empty
> --trailer Change-id=
> >
> > Other: thing
> 
> The idea is that the above command replaces an existing "Change-id:
> stuff" trailer with an empty "Change-id:" trailer and then removes all
> the empty trailers.
> 

So I have read the man page on the trailers and it seems like the solution
to my problem in removing parts from the commit message.
However I did not find out, if it can be run automatically, whenever
calling format-patch

Maybe all that is missing here is an option

	git config format.enable_trailers 
?

Best,
Stefan

> Best,

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So I have read the man page on the trailers and it seems like the solution
> to my problem in removing parts from the commit message.
> However I did not find out, if it can be run automatically, whenever
> calling format-patch
>
> Maybe all that is missing here is an option
>
> 	git config format.enable_trailers 
> ?

The idea has been to first give a standalone text transmonger as a
filter for let people to try out, so that we know what kind of
changes are useful (e.g. "insert s-o-b at the very end") and make
sure the configuration language to specify the changes is easy and
expressive enough, which is more or less what we have in 'master'.

Once we gain experience (and that may result in updates to what is
in 'master'), in the second phase, we would figure out what code
paths can make use of this text transmonger (e.g. your configuration
variable "format.trailers" to affect the format-patch code path) and
integrate it more tightly to the codebase.

We are not there yet.

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

[Christian Couder]

On Fri, Nov 21, 2014 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So I have read the man page on the trailers and it seems like the solution
>> to my problem in removing parts from the commit message.
>> However I did not find out, if it can be run automatically, whenever
>> calling format-patch
>>
>> Maybe all that is missing here is an option
>>
>>       git config format.enable_trailers
>> ?

Yeah, we could use config variables or command options or both to make
it easier to enable it and pass it arguments.

For example we could add:

1) an "--interpret-trailers" option to "git commit", "git
format-patch", "git am" and maybe others too, so that "git
interpret-trailers" is called (without arguments),
2) an "--trailer <trailer>" option (that can be repeated) to the same
commands, so that "git interpret-trailers" is called with the
"--trailer <trailer>" arguments,
3) a "trailer.enable_commands =  'format-patch, commit'" config
variable, so that "git interpret-trailers" is called (without
arguments) every time one of the specified command is used,
4) your suggested "format.enable_trailers" and maybe
"commit.enable_trailers" and others like that,
5) a "trailer" command for rebase -i todo lists.

My preference would be for 1), 2), 3) and 5).

> The idea has been to first give a standalone text transmonger as a
> filter for let people to try out, so that we know what kind of
> changes are useful (e.g. "insert s-o-b at the very end") and make
> sure the configuration language to specify the changes is easy and
> expressive enough, which is more or less what we have in 'master'.
>
> Once we gain experience (and that may result in updates to what is
> in 'master'), in the second phase, we would figure out what code
> paths can make use of this text transmonger (e.g. your configuration
> variable "format.trailers" to affect the format-patch code path) and
> integrate it more tightly to the codebase.
>
> We are not there yet.

Yeah, "interpret-trailers" will be in Git 2.2 without the above
suggested features, but maybe, if we can get  some feedback from users
about which features they would use, we can aim to have some in Git
2.3.

Thanks,
Christian.