[PATCH 0/4] git-am: use trailers to add extra signatures

[PATCH 0/4] git-am: use trailers to add extra signatures

[Michael S. Tsirkin]

I'm using git am to apply patches, and I like the ability
to add arbitrary trailers instead of the standard Signed-off-by
one.

To this end, I have extended git am to call git interpret-trailers
internally. This way I can add arbitrary signatures.

For example, I have:
[trailer "t"]
        key = Tested-by
        command = "echo \"Michael S. Tsirkin <mst@redhat.com>\""
[trailer "r"]
        key = Reviewed-by
        command = "echo \"Michael S. Tsirkin <mst@redhat.com>\""
[trailer "a"]
        key = Acked-by
        command = "echo \"Michael S. Tsirkin <mst@redhat.com>\""
[trailer "s"]
        key = Signed-off-by
        command = "echo \"Michael S. Tsirkin <mst@redhat.com>\""

And now:
	git am -t t -t r -t s
adds all of:
	Tested-by: Michael S. Tsirkin <mst@redhat.com>
	Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
	Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

This was originally suggested by Junio (a long time ago).

Documentation and tests are still TBD.

Michael S. Tsirkin (4):
  builtin/interpret-trailers.c: allow -t
  builtin/interpret-trailers: suppress blank line
  builtin/am: read mailinfo from file
  builtin/am: passthrough -t and --trailer flags

 trailer.h                    |  2 +-
 builtin/am.c                 | 57 +++++++++++++++++++++++++++++++++++++++++++-
 builtin/interpret-trailers.c | 11 ++++++---
 trailer.c                    | 10 +++++---
 4 files changed, 72 insertions(+), 8 deletions(-)

-- 
MST

[PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Michael S. Tsirkin]

Allow -t as a short-cut for --trailer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index b99ae4b..18cf640 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
-		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
+		OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
 	};
-- 
MST

[PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Michael S. Tsirkin]

it's sometimes useful to be able to pass output message of
git-mailinfo through git-interpret-trailers,
but that creates problems since that does not
include the subject and an empty line after that,
making interpret-trailers add an empty line.

Add a flag to bypass adding the blank line.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 trailer.h                    |  2 +-
 builtin/interpret-trailers.c |  9 +++++++--
 trailer.c                    | 10 +++++++---
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/trailer.h b/trailer.h
index 36b40b8..afcf680 100644
--- a/trailer.h
+++ b/trailer.h
@@ -2,6 +2,6 @@
 #define TRAILER_H
 
 void process_trailers(const char *file, int in_place, int trim_empty,
-		      struct string_list *trailers);
+		      int suppress_blank_line, struct string_list *trailers);
 
 #endif /* TRAILER_H */
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 18cf640..4a92788 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
+	int suppress_blank_line = 0;
 	int in_place = 0;
 	int trim_empty = 0;
 	struct string_list trailers = STRING_LIST_INIT_DUP;
 
 	struct option options[] = {
+		OPT_BOOL(0, "suppress-blank-line", &suppress_blank_line,
+			 N_("suppress prefixing tailer(s) with a blank line ")),
 		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
 		OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
@@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], in_place, trim_empty, &trailers);
+			process_trailers(argv[i], in_place, trim_empty,
+					 suppress_blank_line, &trailers);
 	} else {
 		if (in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, in_place, trim_empty, &trailers);
+		process_trailers(NULL, in_place, trim_empty,
+				 suppress_blank_line, &trailers);
 	}
 
 	string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 8e48a5c..8e5be91 100644
--- a/trailer.c
+++ b/trailer.c
@@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end
 
 static int process_input_file(FILE *outfile,
 			      struct strbuf **lines,
+			      int suppress_blank_line,
 			      struct trailer_item **in_tok_first,
 			      struct trailer_item **in_tok_last)
 {
@@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,
 	/* Print lines before the trailers as is */
 	print_lines(outfile, lines, 0, trailer_start);
 
-	if (!has_blank_line_before(lines, trailer_start - 1))
+	if (!suppress_blank_line &&
+	    !has_blank_line_before(lines, trailer_start - 1))
 		fprintf(outfile, "\n");
 
 	/* Parse trailer lines */
@@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
+void process_trailers(const char *file, int in_place, int trim_empty,
+		      int suppress_blank_line, struct string_list *trailers)
 {
 	struct trailer_item *in_tok_first = NULL;
 	struct trailer_item *in_tok_last = NULL;
@@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
+	trailer_end = process_input_file(outfile, lines, suppress_blank_line,
+					 &in_tok_first, &in_tok_last);
 
 	arg_tok_first = process_command_line_args(trailers);
 
-- 
MST

[PATCH 3/4] builtin/am: read mailinfo from file

[Michael S. Tsirkin]

Slightly slower, but will allow easy additional processing on it.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 builtin/am.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index d003939..4180b04 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 	FILE *fp;
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf msg = STRBUF_INIT;
+	struct strbuf log_msg = STRBUF_INIT;
 	struct strbuf author_name = STRBUF_INIT;
 	struct strbuf author_date = STRBUF_INIT;
 	struct strbuf author_email = STRBUF_INIT;
@@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)
 	}
 
 	strbuf_addstr(&msg, "\n\n");
-	strbuf_addbuf(&msg, &mi.log_message);
+
+	if (strbuf_read_file(&log_msg,  am_path(state, "msg"), 0) < 0) {
+		die_errno(_("could not read '%s'"), am_path(state, "msg"));
+	}
+
+	strbuf_addbuf(&msg, &log_msg);
 	strbuf_stripspace(&msg, 0);
 
 	if (state->signoff)
@@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 	state->msg = strbuf_detach(&msg, &state->msg_len);
 
 finish:
+	strbuf_release(&log_msg);
 	strbuf_release(&msg);
 	strbuf_release(&author_date);
 	strbuf_release(&author_email);
-- 
MST

[PATCH 4/4] builtin/am: passthrough -t and --trailer flags

[Michael S. Tsirkin]

Pass -t and --trailer flags to git-reinterpret-trailers.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 builtin/am.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index 4180b04..480c4c2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -122,6 +122,7 @@ struct am_state {
 	int message_id;
 	int scissors; /* enum scissors_type */
 	struct argv_array git_apply_opts;
+	struct argv_array git_interpret_trailers_opts;
 	const char *resolvemsg;
 	int committer_date_is_author_date;
 	int ignore_date;
@@ -157,6 +158,8 @@ static void am_state_init(struct am_state *state, const char *dir)
 
 	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
 		state->sign_commit = gpgsign ? "" : NULL;
+
+	argv_array_init(&state->git_interpret_trailers_opts);
 }
 
 /**
@@ -170,6 +173,7 @@ static void am_state_release(struct am_state *state)
 	free(state->author_date);
 	free(state->msg);
 	argv_array_clear(&state->git_apply_opts);
+	argv_array_clear(&state->git_interpret_trailers_opts);
 }
 
 /**
@@ -472,6 +476,11 @@ static void am_load(struct am_state *state)
 	if (sq_dequote_to_argv_array(sb.buf, &state->git_apply_opts) < 0)
 		die(_("could not parse %s"), am_path(state, "apply-opt"));
 
+	read_state_file(&sb, state, "interpret-trailers-opt", 1);
+	argv_array_clear(&state->git_interpret_trailers_opts);
+	if (sq_dequote_to_argv_array(sb.buf, &state->git_interpret_trailers_opts) < 0)
+		die(_("could not parse %s"), am_path(state, "interpret-trailers-opt"));
+
 	state->rebasing = !!file_exists(am_path(state, "rebasing"));
 
 	strbuf_release(&sb);
@@ -988,6 +997,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	unsigned char curr_head[GIT_SHA1_RAWSZ];
 	const char *str;
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf tsb = STRBUF_INIT;
 
 	if (!patch_format)
 		patch_format = detect_patch_format(paths);
@@ -1048,6 +1058,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	sq_quote_argv(&sb, state->git_apply_opts.argv, 0);
 	write_state_text(state, "apply-opt", sb.buf);
 
+	sq_quote_argv(&tsb, state->git_interpret_trailers_opts.argv, 0);
+	write_state_text(state, "interpret-trailers-opt", tsb.buf);
+
 	if (state->rebasing)
 		write_state_text(state, "rebasing", "");
 	else
@@ -1072,6 +1085,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	write_state_count(state, "next", state->cur);
 	write_state_count(state, "last", state->last);
 
+	strbuf_release(&tsb);
 	strbuf_release(&sb);
 }
 
@@ -1233,6 +1247,34 @@ static void am_append_signoff(struct am_state *state)
 }
 
 /**
+ * Processes the supplied message file in-place with git-interpret-trailers.
+ * Returns 0 on success, -1 otherwise.
+ */
+static int run_interpret_trailers(const struct am_state *state, const char *msg)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	if (!state->git_interpret_trailers_opts.argc)
+		return 0;
+
+	cp.git_cmd = 1;
+
+	argv_array_push(&cp.args, "interpret-trailers");
+
+	argv_array_push(&cp.args, "--in-place");
+	argv_array_push(&cp.args, "--suppress-blank-line");
+
+	argv_array_pushv(&cp.args, state->git_interpret_trailers_opts.argv);
+
+	argv_array_push(&cp.args, msg);
+
+	if (run_command(&cp))
+		return -1;
+
+	return 0;
+}
+
+/**
  * Parses `mail` using git-mailinfo, extracting its patch and authorship info.
  * state->msg will be set to the patch message. state->author_name,
  * state->author_email and state->author_date will be set to the patch author's
@@ -1301,6 +1343,9 @@ static int parse_mail(struct am_state *state, const char *mail)
 	fclose(mi.input);
 	fclose(mi.output);
 
+	if (run_interpret_trailers(state, am_path(state, "msg")) < 0)
+		die("could not interpret trailers");
+
 	/* Extract message and author information */
 	fp = xfopen(am_path(state, "info"), "r");
 	while (!strbuf_getline_lf(&sb, fp)) {
@@ -2299,6 +2344,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		OPT_PASSTHRU_ARGV('p', NULL, &state.git_apply_opts, N_("num"),
 			N_("pass it through git-apply"),
 			0),
+		OPT_PASSTHRU_ARGV('t', "trailer", &state.git_interpret_trailers_opts, N_("trailer"),
+			N_("pass it through git-interpret-trailers"),
+			0),
 		OPT_CALLBACK(0, "patch-format", &patch_format, N_("format"),
 			N_("format the patch(es) are in"),
 			parse_opt_patchformat),
-- 
MST

Re: [PATCH 4/4] builtin/am: passthrough -t and --trailer flags

[Christian Couder]

On Thu, Apr 7, 2016 at 11:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Pass -t and --trailer flags to git-reinterpret-trailers.

s/git-reinterpret-trailers/git-interpret-trailers/

Thanks,
Christian.

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Allow -t as a short-cut for --trailer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

As I do not think interpret-trailers is meant to be end-user facing,
I am not sure I should be interested in this step.

I am in principle OK with the later step that teaches a single
letter option to end-user facing "git am" that would be turned into
"--trailer" when it calls out to "interpret-trailers" (I haven't
checked if 't' is a sensible choice for that single letter option,
though).

>  builtin/interpret-trailers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index b99ae4b..18cf640 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  	struct option options[] = {
>  		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
>  		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
> -		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
> +		OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
>  				N_("trailer(s) to add")),
>  		OPT_END()
>  	};

Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> it's sometimes useful to be able to pass output message of
> git-mailinfo through git-interpret-trailers,
> but that creates problems since that does not
> include the subject and an empty line after that,
> making interpret-trailers add an empty line.
>
> Add a flag to bypass adding the blank line.

I think I understand what you are trying to do, but using output
that comes from 'mailinfo' alone as the input to anything (including
interpret-trailers) does not make much sense.

If you use the mailinfo output in the way it is expected to be used,
i.e. take the subject from the "info" that goes to its standard
output and append the "msg" with a blank between them, and feed the
result to interpret-trailers, do you still need this step in your
series?

>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  trailer.h                    |  2 +-
>  builtin/interpret-trailers.c |  9 +++++++--
>  trailer.c                    | 10 +++++++---
>  3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/trailer.h b/trailer.h
> index 36b40b8..afcf680 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -2,6 +2,6 @@
>  #define TRAILER_H
>  
>  void process_trailers(const char *file, int in_place, int trim_empty,
> -		      struct string_list *trailers);
> +		      int suppress_blank_line, struct string_list *trailers);
>  
>  #endif /* TRAILER_H */
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 18cf640..4a92788 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {
>  
>  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  {
> +	int suppress_blank_line = 0;
>  	int in_place = 0;
>  	int trim_empty = 0;
>  	struct string_list trailers = STRING_LIST_INIT_DUP;
>  
>  	struct option options[] = {
> +		OPT_BOOL(0, "suppress-blank-line", &suppress_blank_line,
> +			 N_("suppress prefixing tailer(s) with a blank line ")),
>  		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
>  		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
>  		OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
> @@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  	if (argc) {
>  		int i;
>  		for (i = 0; i < argc; i++)
> -			process_trailers(argv[i], in_place, trim_empty, &trailers);
> +			process_trailers(argv[i], in_place, trim_empty,
> +					 suppress_blank_line, &trailers);
>  	} else {
>  		if (in_place)
>  			die(_("no input file given for in-place editing"));
> -		process_trailers(NULL, in_place, trim_empty, &trailers);
> +		process_trailers(NULL, in_place, trim_empty,
> +				 suppress_blank_line, &trailers);
>  	}
>  
>  	string_list_clear(&trailers, 0);
> diff --git a/trailer.c b/trailer.c
> index 8e48a5c..8e5be91 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end
>  
>  static int process_input_file(FILE *outfile,
>  			      struct strbuf **lines,
> +			      int suppress_blank_line,
>  			      struct trailer_item **in_tok_first,
>  			      struct trailer_item **in_tok_last)
>  {
> @@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,
>  	/* Print lines before the trailers as is */
>  	print_lines(outfile, lines, 0, trailer_start);
>  
> -	if (!has_blank_line_before(lines, trailer_start - 1))
> +	if (!suppress_blank_line &&
> +	    !has_blank_line_before(lines, trailer_start - 1))
>  		fprintf(outfile, "\n");
>  
>  	/* Parse trailer lines */
> @@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)
>  	return outfile;
>  }
>  
> -void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
> +void process_trailers(const char *file, int in_place, int trim_empty,
> +		      int suppress_blank_line, struct string_list *trailers)
>  {
>  	struct trailer_item *in_tok_first = NULL;
>  	struct trailer_item *in_tok_last = NULL;
> @@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
>  		outfile = create_in_place_tempfile(file);
>  
>  	/* Print the lines before the trailers */
> -	trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
> +	trailer_end = process_input_file(outfile, lines, suppress_blank_line,
> +					 &in_tok_first, &in_tok_last);
>  
>  	arg_tok_first = process_command_line_args(trailers);

Re: [PATCH 3/4] builtin/am: read mailinfo from file

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Slightly slower, but will allow easy additional processing on it.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

I haven't read 4/4 yet, but can guess from what this patch does that
the next step would let others futz with the contents of the message
that is on disk (i.e. what mailinfo() wrote out, which is identical
to what we have in mi.log_message at this point of the codeflow)
before you do the new strbuf_read_file().

It probably is better to do this as part of 4/4; it is easier to
understand why this is a good and necessary thing to do.  An obvious
improvement is to omit this extra "read back from the filesystem"
when we won't be making any interpret-trailer calls (i.e. no -t
option from the command line), but if we stop at this step 3/4, then
we'd end up wasting cycles without having any benefit.

>  builtin/am.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index d003939..4180b04 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	FILE *fp;
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf msg = STRBUF_INIT;
> +	struct strbuf log_msg = STRBUF_INIT;
>  	struct strbuf author_name = STRBUF_INIT;
>  	struct strbuf author_date = STRBUF_INIT;
>  	struct strbuf author_email = STRBUF_INIT;
> @@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	}
>  
>  	strbuf_addstr(&msg, "\n\n");
> -	strbuf_addbuf(&msg, &mi.log_message);
> +
> +	if (strbuf_read_file(&log_msg,  am_path(state, "msg"), 0) < 0) {
> +		die_errno(_("could not read '%s'"), am_path(state, "msg"));
> +	}

I do not think these {} serve any purpose; drop them?

> +
> +	strbuf_addbuf(&msg, &log_msg);
>  	strbuf_stripspace(&msg, 0);
>  
>  	if (state->signoff)
> @@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	state->msg = strbuf_detach(&msg, &state->msg_len);
>  
>  finish:
> +	strbuf_release(&log_msg);
>  	strbuf_release(&msg);
>  	strbuf_release(&author_date);
>  	strbuf_release(&author_email);

Re: [PATCH 3/4] builtin/am: read mailinfo from file

[Michael S. Tsirkin]

On Thu, Apr 07, 2016 at 10:08:37AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Slightly slower, but will allow easy additional processing on it.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> I haven't read 4/4 yet, but can guess from what this patch does that
> the next step would let others futz with the contents of the message
> that is on disk (i.e. what mailinfo() wrote out, which is identical
> to what we have in mi.log_message at this point of the codeflow)
> before you do the new strbuf_read_file().
> 
> It probably is better to do this as part of 4/4; it is easier to
> understand why this is a good and necessary thing to do.  An obvious
> improvement is to omit this extra "read back from the filesystem"
> when we won't be making any interpret-trailer calls (i.e. no -t
> option from the command line), but if we stop at this step 3/4, then
> we'd end up wasting cycles without having any benefit.

Hmm - splitting it out was easy for development since I could verify all
tests pass.  But if you do want the optimization, then sure, I'll have
to squash it in.

> >  builtin/am.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/am.c b/builtin/am.c
> > index d003939..4180b04 100644
> > --- a/builtin/am.c
> > +++ b/builtin/am.c
> > @@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)
> >  	FILE *fp;
> >  	struct strbuf sb = STRBUF_INIT;
> >  	struct strbuf msg = STRBUF_INIT;
> > +	struct strbuf log_msg = STRBUF_INIT;
> >  	struct strbuf author_name = STRBUF_INIT;
> >  	struct strbuf author_date = STRBUF_INIT;
> >  	struct strbuf author_email = STRBUF_INIT;
> > @@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)
> >  	}
> >  
> >  	strbuf_addstr(&msg, "\n\n");
> > -	strbuf_addbuf(&msg, &mi.log_message);
> > +
> > +	if (strbuf_read_file(&log_msg,  am_path(state, "msg"), 0) < 0) {
> > +		die_errno(_("could not read '%s'"), am_path(state, "msg"));
> > +	}
> 
> I do not think these {} serve any purpose; drop them?
> 
> > +
> > +	strbuf_addbuf(&msg, &log_msg);
> >  	strbuf_stripspace(&msg, 0);
> >  
> >  	if (state->signoff)
> > @@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)
> >  	state->msg = strbuf_detach(&msg, &state->msg_len);
> >  
> >  finish:
> > +	strbuf_release(&log_msg);
> >  	strbuf_release(&msg);
> >  	strbuf_release(&author_date);
> >  	strbuf_release(&author_email);

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Matthieu Moy]

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

> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> Allow -t as a short-cut for --trailer.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>
> As I do not think interpret-trailers is meant to be end-user facing,
> I am not sure I should be interested in this step.
>
> I am in principle OK with the later step that teaches a single
> letter option to end-user facing "git am" that would be turned into
> "--trailer" when it calls out to "interpret-trailers" (I haven't
> checked if 't' is a sensible choice for that single letter option,
> though).

If 'am' has -t == --trailer, I think it makes sense to have the same
shortcut in interpret-trailers for consistency.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Junio C Hamano]

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

> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> it's sometimes useful to be able to pass output message of
>> git-mailinfo through git-interpret-trailers,
>> but that creates problems since that does not
>> include the subject and an empty line after that,
>> making interpret-trailers add an empty line.
>>
>> Add a flag to bypass adding the blank line.
>
> I think I understand what you are trying to do, but using output
> that comes from 'mailinfo' alone as the input to anything (including
> interpret-trailers) does not make much sense.
>
> If you use the mailinfo output in the way it is expected to be used,
> i.e. take the subject from the "info" that goes to its standard
> output and append the "msg" with a blank between them, and feed the
> result to interpret-trailers, do you still need this step in your
> series?

OK, after reading 3/4 and guessing that you hand "msg" to
interpret-trailers to let it munge its contents, it makes sense to
allow us to tell interpret-trailers that you are feeding only the
body of the message without the title and the blank line before it.

"suppress blank line" is a terrible title for that new feature,
though.  Perhaps "--body-only"?

The difference in behaviour in two modes (i.e. with title and
without title) is that when the input does not have any blank line
in it, the normal mode considers that there is no body (i.e. only
title exists in the input) hence there is no existing trailer lines,
and new trailer lines need to be added after adding blank, while the
new "body only" mode considers that there is only one paragraph in
the body, hence it may be the existing trailer block without any
message (in which case that is the block new trailer lines are to be
added to or existing ones to be removed from), or there is no
trailer block but one paragraph of the message (in which case you
would do the "add blank and append new trailer lines" thing).

I wrote the above down, hoping that it would give you a hint to
better explain what this patch aims to do, so please feel free to
further rephrase (or steal outright from) it when you reroll the
series.

Thanks.

Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Michael S. Tsirkin]

On Thu, Apr 07, 2016 at 10:00:37AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > it's sometimes useful to be able to pass output message of
> > git-mailinfo through git-interpret-trailers,
> > but that creates problems since that does not
> > include the subject and an empty line after that,
> > making interpret-trailers add an empty line.
> >
> > Add a flag to bypass adding the blank line.
> 
> I think I understand what you are trying to do, but using output
> that comes from 'mailinfo' alone as the input to anything (including
> interpret-trailers) does not make much sense.
> 
> If you use the mailinfo output in the way it is expected to be used,
> i.e. take the subject from the "info" that goes to its standard
> output and append the "msg" with a blank between them, and feed the
> result to interpret-trailers, do you still need this step in your
> series?

No - but then I will need to re-run mailinfo to parse the result,
will I not?

And unfortunately it appears that interpret-trailers can't
handle arbitrary mail - it wants data from commit.

> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  trailer.h                    |  2 +-
> >  builtin/interpret-trailers.c |  9 +++++++--
> >  trailer.c                    | 10 +++++++---
> >  3 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/trailer.h b/trailer.h
> > index 36b40b8..afcf680 100644
> > --- a/trailer.h
> > +++ b/trailer.h
> > @@ -2,6 +2,6 @@
> >  #define TRAILER_H
> >  
> >  void process_trailers(const char *file, int in_place, int trim_empty,
> > -		      struct string_list *trailers);
> > +		      int suppress_blank_line, struct string_list *trailers);
> >  
> >  #endif /* TRAILER_H */
> > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> > index 18cf640..4a92788 100644
> > --- a/builtin/interpret-trailers.c
> > +++ b/builtin/interpret-trailers.c
> > @@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {
> >  
> >  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> >  {
> > +	int suppress_blank_line = 0;
> >  	int in_place = 0;
> >  	int trim_empty = 0;
> >  	struct string_list trailers = STRING_LIST_INIT_DUP;
> >  
> >  	struct option options[] = {
> > +		OPT_BOOL(0, "suppress-blank-line", &suppress_blank_line,
> > +			 N_("suppress prefixing tailer(s) with a blank line ")),
> >  		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
> >  		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
> >  		OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
> > @@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> >  	if (argc) {
> >  		int i;
> >  		for (i = 0; i < argc; i++)
> > -			process_trailers(argv[i], in_place, trim_empty, &trailers);
> > +			process_trailers(argv[i], in_place, trim_empty,
> > +					 suppress_blank_line, &trailers);
> >  	} else {
> >  		if (in_place)
> >  			die(_("no input file given for in-place editing"));
> > -		process_trailers(NULL, in_place, trim_empty, &trailers);
> > +		process_trailers(NULL, in_place, trim_empty,
> > +				 suppress_blank_line, &trailers);
> >  	}
> >  
> >  	string_list_clear(&trailers, 0);
> > diff --git a/trailer.c b/trailer.c
> > index 8e48a5c..8e5be91 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end
> >  
> >  static int process_input_file(FILE *outfile,
> >  			      struct strbuf **lines,
> > +			      int suppress_blank_line,
> >  			      struct trailer_item **in_tok_first,
> >  			      struct trailer_item **in_tok_last)
> >  {
> > @@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,
> >  	/* Print lines before the trailers as is */
> >  	print_lines(outfile, lines, 0, trailer_start);
> >  
> > -	if (!has_blank_line_before(lines, trailer_start - 1))
> > +	if (!suppress_blank_line &&
> > +	    !has_blank_line_before(lines, trailer_start - 1))
> >  		fprintf(outfile, "\n");
> >  
> >  	/* Parse trailer lines */
> > @@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)
> >  	return outfile;
> >  }
> >  
> > -void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
> > +void process_trailers(const char *file, int in_place, int trim_empty,
> > +		      int suppress_blank_line, struct string_list *trailers)
> >  {
> >  	struct trailer_item *in_tok_first = NULL;
> >  	struct trailer_item *in_tok_last = NULL;
> > @@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
> >  		outfile = create_in_place_tempfile(file);
> >  
> >  	/* Print the lines before the trailers */
> > -	trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
> > +	trailer_end = process_input_file(outfile, lines, suppress_blank_line,
> > +					 &in_tok_first, &in_tok_last);
> >  
> >  	arg_tok_first = process_command_line_args(trailers);

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> I am in principle OK with the later step that teaches a single
>> letter option to end-user facing "git am" that would be turned into
>> "--trailer" when it calls out to "interpret-trailers" (I haven't
>> checked if 't' is a sensible choice for that single letter option,
>> though).
>
> If 'am' has -t == --trailer, I think it makes sense to have the same
> shortcut in interpret-trailers for consistency.

It is the other way around.  "git am" may be OK with "-t" (or it may
not--I do not know yet), but other commands that are currently
unaware of "interpret-trailers" (cherry-pick, revert, etc.) may have
better uses for a short-and-sweet 't'.

In the ideal future, "interpret-trailers" should not have to exist
in the end-users' vocabulary, as all the front-line end-user facing
programs would be aware of it.  But we are not there.

Letting it reserve a short-and-sweet 't' that allows it to dictate
that its callers must have the same 't' is tail wagging the dog that
I want to avoid.

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Michael S. Tsirkin]

On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Allow -t as a short-cut for --trailer.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> As I do not think interpret-trailers is meant to be end-user facing,
> I am not sure I should be interested in this step.
> 
> I am in principle OK with the later step that teaches a single
> letter option to end-user facing "git am" that would be turned into
> "--trailer" when it calls out to "interpret-trailers" (I haven't
> checked if 't' is a sensible choice for that single letter option,
> though).

Does OPT_PASSTHRU_ARGV handle this transformation for me?

> >  builtin/interpret-trailers.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> > index b99ae4b..18cf640 100644
> > --- a/builtin/interpret-trailers.c
> > +++ b/builtin/interpret-trailers.c
> > @@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> >  	struct option options[] = {
> >  		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
> >  		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
> > -		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
> > +		OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
> >  				N_("trailer(s) to add")),
> >  		OPT_END()
> >  	};

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > Allow -t as a short-cut for --trailer.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> 
>> As I do not think interpret-trailers is meant to be end-user facing,
>> I am not sure I should be interested in this step.
>> 
>> I am in principle OK with the later step that teaches a single
>> letter option to end-user facing "git am" that would be turned into
>> "--trailer" when it calls out to "interpret-trailers" (I haven't
>> checked if 't' is a sensible choice for that single letter option,
>> though).
>
> Does OPT_PASSTHRU_ARGV handle this transformation for me?

As I wrote in my response to Matthieu, PASSTHRU_ARGV is one thing I
specifically do not want to see used in this codepath.

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Michael S. Tsirkin]

On Thu, Apr 07, 2016 at 10:26:33AM -0700, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
> >> I am in principle OK with the later step that teaches a single
> >> letter option to end-user facing "git am" that would be turned into
> >> "--trailer" when it calls out to "interpret-trailers" (I haven't
> >> checked if 't' is a sensible choice for that single letter option,
> >> though).
> >
> > If 'am' has -t == --trailer, I think it makes sense to have the same
> > shortcut in interpret-trailers for consistency.
> 
> It is the other way around.  "git am" may be OK with "-t" (or it may
> not--I do not know yet), but other commands that are currently
> unaware of "interpret-trailers" (cherry-pick, revert, etc.) may have
> better uses for a short-and-sweet 't'.
> 
> In the ideal future, "interpret-trailers" should not have to exist
> in the end-users' vocabulary, as all the front-line end-user facing
> programs would be aware of it.  But we are not there.
> 
> Letting it reserve a short-and-sweet 't' that allows it to dictate
> that its callers must have the same 't' is tail wagging the dog that
> I want to avoid.

It's mostly a short-cut I took by copying calls to applypatch.
Are there examples of other commands doing such transformations
on the fly?

-- 
MST

Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> No - but then I will need to re-run mailinfo to parse the result,
> will I not?

By the way, I suspect (if Christian did his implementation right
when he did interpret-trailers) all these points may become moot.

I haven't re-reviewed what is in interpret-trailers, but the vision
has been that its internal workings should be callable directly into
instead of running it via run_commands() interface passing the data
via on-disk file.  In the codepath you touch in 3/4 and 4/4, you
already have not just mi.log_message but msg that has the whole
payload to create a commit object out of already, so shouldn't it be
just the matter of passing <msg.buf, msg.len> to some API function
that was prepared to implement interpret-trailers?

Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Matthieu Moy]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> it's sometimes useful to be able to pass output message of
> git-mailinfo through git-interpret-trailers,
> but that creates problems since that does not
> include the subject and an empty line after that,
> making interpret-trailers add an empty line.

Nit: we usually wrap our text around 72 columns in the commit message.
Yours is wrapped weirdly.

> Add a flag to bypass adding the blank line.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  trailer.h                    |  2 +-
>  builtin/interpret-trailers.c |  9 +++++++--
>  trailer.c                    | 10 +++++++---
>  3 files changed, 15 insertions(+), 6 deletions(-)

You'd definitely need some tests and documentation if you introduce a
new option.

No time for a real review, sorry.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/4] builtin/am: read mailinfo from file

[Matthieu Moy]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> -	strbuf_addbuf(&msg, &mi.log_message);
> +
> +	if (strbuf_read_file(&log_msg,  am_path(state, "msg"), 0) < 0) {
> +		die_errno(_("could not read '%s'"), am_path(state, "msg"));
> +	}

Style: we omit the braces where not needed.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Michael S. Tsirkin]

On Thu, Apr 07, 2016 at 10:30:02AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > Allow -t as a short-cut for --trailer.
> >> >
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > ---
> >> 
> >> As I do not think interpret-trailers is meant to be end-user facing,
> >> I am not sure I should be interested in this step.
> >> 
> >> I am in principle OK with the later step that teaches a single
> >> letter option to end-user facing "git am" that would be turned into
> >> "--trailer" when it calls out to "interpret-trailers" (I haven't
> >> checked if 't' is a sensible choice for that single letter option,
> >> though).
> >
> > Does OPT_PASSTHRU_ARGV handle this transformation for me?
> 
> As I wrote in my response to Matthieu, PASSTHRU_ARGV is one thing I
> specifically do not want to see used in this codepath.

It sounds like a general kind of thing, does it not?
Aren't there other cases where a short option needs to be
converted to a long one?

-- 
MST

Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Aren't there other cases where a short option needs to be
> converted to a long one?

As I already said, making internal call to libified part (perhaps in
trailer.c) would make this part of conversation a moot point, but in
general you can find

	argv_push(&child.args, "cmd2");
	if (... some condition that involves variables parsed out ...
            ... by parse_options() of implementation of cmd1 ...)
		argv_push(&child.args, "--option-for-cmd2");
	...
        run_command(&child);

in implementation of cmd1 that calls out to cmd2.

Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line

[Michael S. Tsirkin]

On Thu, Apr 07, 2016 at 10:34:51AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > No - but then I will need to re-run mailinfo to parse the result,
> > will I not?
> 
> By the way, I suspect (if Christian did his implementation right
> when he did interpret-trailers) all these points may become moot.
> 
> I haven't re-reviewed what is in interpret-trailers, but the vision
> has been that its internal workings should be callable directly into
> instead of running it via run_commands() interface passing the data
> via on-disk file.  In the codepath you touch in 3/4 and 4/4, you
> already have not just mi.log_message but msg that has the whole
> payload to create a commit object out of already, so shouldn't it be
> just the matter of passing <msg.buf, msg.len> to some API function
> that was prepared to implement interpret-trailers?

That's certainly possible, though it will need a rework
of the internal API: we currently have:

void process_trailers(const char *file, int in_place, int trim_empty,
                      int suppress_blank_line, struct string_list *trailers)
{
        struct trailer_item *in_tok_first = NULL;
        struct trailer_item *in_tok_last = NULL;
        struct trailer_item *arg_tok_first;
        struct strbuf **lines;
        int trailer_end;
        FILE *outfile = stdout;

        /* Default config must be setup first */
        git_config(git_trailer_default_config, NULL);
        git_config(git_trailer_config, NULL);

        lines = read_input_file(file);

So process_trailers can be changed to get struct strbuf ** instead.

But it seems that the output would have to go into a temporary file
anyway, unless trailer.c is completely rewritten, since it
currently does all output by writing it into a file.
Is that an issue?

-- 
MST