[RFC] cover-at-tip

[RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Hi,

I'm starting to look into the cover-at-tip topic that I found in the leftover bits (http://www.spinics.net/lists/git/msg259573.html)

Here's a first draft of a patch that adds support for format-patch --cover-at-tip. It compiles and works in my nice and user firnedly test case.
Just wanted to make sure I was going roughly in the right direction here.


I was wondering where is the right place to put a commit_is_cover_at_tip() as the test will be needed in other place as the feature is extended to git am/merge/pull.

Feel free to comment. I know the help is not clear at this point and there's still some work to do on option handling (add a config option, probably have --cover-at-tip imply --cover-letter, etc) and
some testing :)


---
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 38 +++++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6cbe462a7..0ac9d4b71 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -228,6 +228,10 @@ feeding the result to `git send-email`.
 	containing the branch description, shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--[no-]cover-letter-at-tip::
+	Use the tip of the series as a cover letter if it is an empty commit.
+    If no cover-letter is to be sent, the tip is ignored.
+
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896a..a0e9e61a3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
-			      int quiet)
+			      int quiet,
+			      struct commit *cover_at_tip_commit)
 {
 	const char *committer;
 	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
-	const char *msg;
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
 	int i;
@@ -1021,14 +1021,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (!branch_name)
 		branch_name = find_branch_name(rev);
 
-	msg = body;
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;
 	pp.print_email_subject = 1;
-	pp_user_info(&pp, NULL, &sb, committer, encoding);
-	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
-	pp_remainder(&pp, &msg, &sb, 0);
+
+	if (!cover_at_tip_commit) {
+		pp_user_info(&pp, NULL, &sb, committer, encoding);
+		pp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);
+		pp_remainder(&pp, &body, &sb, 0);
+	} else {
+		pretty_print_commit(&pp, cover_at_tip_commit, &sb);
+	}
 	add_branch_description(&sb, branch_name);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
@@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int just_numbers = 0;
 	int ignore_if_in_upstream = 0;
 	int cover_letter = -1;
+	int cover_at_tip = -1;
+	struct commit *cover_at_tip_commit = NULL;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
 	int zero_commit = 0;
@@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("print patches to standard out")),
 		OPT_BOOL(0, "cover-letter", &cover_letter,
 			    N_("generate a cover letter")),
+		OPT_BOOL(0, "cover-at-tip", &cover_at_tip,
+			    N_("fill the cover letter with the tip of the branch")),
 		OPT_BOOL(0, "numbered-files", &just_numbers,
 			    N_("use simple number sequence for output file names")),
 		OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
@@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))
 			continue;
 
+		if (!nr && cover_at_tip == 1 && !cover_at_tip_commit) {
+			/* Check that it is a candidate to be a cover at tip
+			 * Meaning:
+			 * - a single parent (merge commits are not eligible)
+			 * - tree oid == parent->tree->oid (no diff to the tree)
+			 */
+			if (commit->parents && !commit->parents->next &&
+			    !oidcmp(&commit->tree->object.oid,
+				    &commit->parents->item->tree->object.oid)) {
+				cover_at_tip_commit = commit;
+				continue;
+			} else {
+				cover_at_tip = 0;
+			}
+		}
 		nr++;
 		REALLOC_ARRAY(list, nr);
 		list[nr - 1] = commit;
@@ -1748,7 +1771,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (thread)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
-				  origin, nr, list, branch_name, quiet);
+				  origin, nr, list, branch_name, quiet,
+				  cover_at_tip_commit);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
 		total++;
-- 
2.15.0.rc0.1.g69b4f6344.dirty

Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 10/11/2017 à 11:24, Nicolas Morey-Chaisemartin a écrit :
> Hi,
>
> I'm starting to look into the cover-at-tip topic that I found in the leftover bits (http://www.spinics.net/lists/git/msg259573.html)
>
> Here's a first draft of a patch that adds support for format-patch --cover-at-tip. It compiles and works in my nice and user firnedly test case.
> Just wanted to make sure I was going roughly in the right direction here.
>
>
> I was wondering where is the right place to put a commit_is_cover_at_tip() as the test will be needed in other place as the feature is extended to git am/merge/pull.
>
> Feel free to comment. I know the help is not clear at this point and there's still some work to do on option handling (add a config option, probably have --cover-at-tip imply --cover-letter, etc) and
> some testing :)
>
>
> ---

Leaving some more updates and questions before the week end:

I started on git am --cover-at-tip.

The proposed patch for format-patch does not output any "---" to signal the end of the commit log and the begining of the patch in the cover letter.
This means that the log summary, the diffstat and the git footer ( --\n<git version>) is seen as part of the commit log. Which is just wrong.

Removing them would solve the issue but I feel they bring some useful info (or they would not be here).
Adding a "---" between the commit log and those added infos poses another problem: git am does not see an empty patch anymore.
I would need to add "some" level of parsing to am.c to make sure the patch content is just garbage and that there are no actual hunks for that.

I did not find any public API that would allow me to do that, although apply_path/parse_chunk would fit the bill.
Is that the right way to approach this ?

My branch is here if anyone want to give a look: https://github.com/nmorey/git/tree/dev/cover-at-tip

Nicolas

Re: [RFC] cover-at-tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

> I would need to add "some" level of parsing to am.c to make sure
> the patch content is just garbage and that there are no actual
> hunks for that.
>
> I did not find any public API that would allow me to do that,
> although apply_path/parse_chunk would fit the bill.  Is that the
> right way to approach this ?

I do not think you would want this non-patch cruft seen at the apply
layer at all.  Reading a mailbox, with the help of mailsplit and
mailinfo, and being the driver to create a series of commits is what
"am" is about, and it would have to notice that the non-patch cruft
at the beginning is not a patch at all and defer creation of an
empty commit with that cover material at the end.  For each of the
other messages in the series that has patches, it will need to call
apply to update the index and the working tree so that it can make a
commit, but there is NO reason whatsoever to ask help from apply, whose
sole purpose is to read a patch and make modifications to the index
and the working tree, to handle the cover material.

Re: [RFC] cover-at-tip

[Jonathan Tan]

On Fri, 10 Nov 2017 16:37:49 +0100
Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> wrote:

> > Hi,
> >
> > I'm starting to look into the cover-at-tip topic that I found in the leftover bits (http://www.spinics.net/lists/git/msg259573.html)

Thanks - I personally would find this very useful.

> > Here's a first draft of a patch that adds support for format-patch --cover-at-tip. It compiles and works in my nice and user firnedly test case.
> > Just wanted to make sure I was going roughly in the right direction here.
> >
> >
> > I was wondering where is the right place to put a commit_is_cover_at_tip() as the test will be needed in other place as the feature is extended to git am/merge/pull.

I think you can put this in (root)/commit.c, especially since that test
operates on a "struct commit *".

> > Feel free to comment. I know the help is not clear at this point and there's still some work to do on option handling (add a config option, probably have --cover-at-tip imply --cover-letter, etc) and
> > some testing :)

Both are good ideas. You should probably use a
--cover-letter={no,auto,yes} instead of the current boolean, so that the
config can use the same options and configuring it to "auto" (to use a
cover letter if the tip is empty and singly-parented, and not to use a
cover letter otherwise) is meaningful.

> The proposed patch for format-patch does not output any "---" to signal the end of the commit log and the begining of the patch in the cover letter.
> This means that the log summary, the diffstat and the git footer ( --\n<git version>) is seen as part of the commit log. Which is just wrong.
> 
> Removing them would solve the issue but I feel they bring some useful info (or they would not be here).
> Adding a "---" between the commit log and those added infos poses another problem: git am does not see an empty patch anymore.
> I would need to add "some" level of parsing to am.c to make sure the patch content is just garbage and that there are no actual hunks for that.

Could you just take the message from the commit and put that in the
cover letter? The summary and diffstat do normally have useful info, but
if the commit is specifically made to be used only for the cover letter,
I think that is no longer true.

Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 10/11/2017 à 19:22, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>
>> I would need to add "some" level of parsing to am.c to make sure
>> the patch content is just garbage and that there are no actual
>> hunks for that.
>>
>> I did not find any public API that would allow me to do that,
>> although apply_path/parse_chunk would fit the bill.  Is that the
>> right way to approach this ?
> I do not think you would want this non-patch cruft seen at the apply
> layer at all.  Reading a mailbox, with the help of mailsplit and
> mailinfo, and being the driver to create a series of commits is what
> "am" is about, and it would have to notice that the non-patch cruft
> at the beginning is not a patch at all and defer creation of an
> empty commit with that cover material at the end.  For each of the
> other messages in the series that has patches, it will need to call
> apply to update the index and the working tree so that it can make a
> commit, but there is NO reason whatsoever to ask help from apply, whose
> sole purpose is to read a patch and make modifications to the index
> and the working tree, to handle the cover material.
>
>

I agree this is a "am" job. Was just wondering if reusing some of the code from apply (and move it so it makes more sense) wouldnd't make more sense than rewriting a patch detection function.

Nicolas

Re: [RFC] cover-at-tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

> I agree this is a "am" job. Was just wondering if reusing some of
> the code from apply (and move it so it makes more sense) wouldnd't
> make more sense than rewriting a patch detection function.

Yes, I understood that and have already given an answer, no?

Re: [RFC] cover-at-tip

[Junio C Hamano]

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

> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>
>> I agree this is a "am" job. Was just wondering if reusing some of
>> the code from apply (and move it so it makes more sense) wouldnd't
>> make more sense than rewriting a patch detection function.
>
> Yes, I understood that and have already given an answer, no?

This was a bit too terse to be useful, so let me try again.

I think the ideal endgame would be to allow people to come up with a
topic branch of this shape (illustrated is a three-patch series on
top of 'origin'):

    ---o---o (origin)
            \
             1---2---3

and then add an empty commit C whose log message is used to store
"cover letter material", i.e.

    ---o---o (origin)
            \
             1---2---3---C (topic)

And then you should be able to 

 (1) merge such branch yourself, coming up with a history like this,
     where merge M uses material from C in the merge log message

    ---o---o---x---x---M
            \         /
             1---2---3

 (2) "git format-patch origin..topic" that would create the cover
     letter using material found in C in addition to the usual
     stuff (like shortlog) generated by "format-patch --cover",
     followed by these three patches.

 (3) "git format-patch M" should be able to (a) realize that M
     merges a side branch that is a three-commit series (i.e.
     M^1..M^2), and (b) notice that log message of M has
     human-readable description.  Then it grabs the merge log
     message of M and do the same as (2).

 (4) "git am" the result from (2) or (3) should recreate the
     original history i.e. what we started with with C.

    ---o---o (origin)
            \
             1---2---3---C (topic)

Now, I _think_ what the machinery needs a lot more is to be able to
detect C is an empty commit (when doing (2)), and then you have
quite a lattitude in designing what exactly such an automated cover
letter looks like, so that the receiving end (4) can recognize it
more easily and (more importantly) more robustly than "the message
does not have any patch in it".  Not all random messages that do
not have a patch in it are cover letters, and that is why I do not
think touching any code in the apply layer in an attempt to "reuse"
anything is a bad idea.  It will risk butchering the code without
any real gain, because what we really need to know is *not* absence
of patch, but presence of cover letter material.

The simplest would probably be to notice that the subject of one has
0/N on it, while other messages were labeled with 1/N..(N-1)/N; that
would be a lot stronger clue that 0/N has a cover than "it does not
have any patch in it".

It may be that we would not just want to identify which message is
cover and which message is not, but which part of the cover letter
message should go back to the log message of the capping empty
commit (and moved to the merge log message).  Just like we invented
the conventions like scissors, three-dashes, etc., you might want to
come up with a way to do so in your format-patch enhancement used to
do the (2) and (3) above.  Then it will be the matter of teaching
that convention to "am" used in (4).

Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 13/11/2017 à 11:30, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>>
>>> I agree this is a "am" job. Was just wondering if reusing some of
>>> the code from apply (and move it so it makes more sense) wouldnd't
>>> make more sense than rewriting a patch detection function.
>> Yes, I understood that and have already given an answer, no?
> This was a bit too terse to be useful, so let me try again.

Thanks ;)

>
> I think the ideal endgame would be to allow people to come up with a
> topic branch of this shape (illustrated is a three-patch series on
> top of 'origin'):
>
>     ---o---o (origin)
>             \
>              1---2---3
>
> and then add an empty commit C whose log message is used to store
> "cover letter material", i.e.
>
>     ---o---o (origin)
>             \
>              1---2---3---C (topic)
>
> And then you should be able to 
>
>  (1) merge such branch yourself, coming up with a history like this,
>      where merge M uses material from C in the merge log message
>
>     ---o---o---x---x---M
>             \         /
>              1---2---3
>
>  (2) "git format-patch origin..topic" that would create the cover
>      letter using material found in C in addition to the usual
>      stuff (like shortlog) generated by "format-patch --cover",
>      followed by these three patches.
>
>  (3) "git format-patch M" should be able to (a) realize that M
>      merges a side branch that is a three-commit series (i.e.
>      M^1..M^2), and (b) notice that log message of M has
>      human-readable description.  Then it grabs the merge log
>      message of M and do the same as (2).
>
>  (4) "git am" the result from (2) or (3) should recreate the
>      original history i.e. what we started with with C.
>
>     ---o---o (origin)
>             \
>              1---2---3---C (topic)

That what I got from the archive referenced in the leftover bits.
I'm currently focusing on (2) and (4).
(3) might come reasonably "easy" after (2) but I don't know enough about the internal API yet so I focused on the simplest ;)

>
> Now, I _think_ what the machinery needs a lot more is to be able to
> detect C is an empty commit (when doing (2)),

Unless I'm mistaken, this should be covered by the RFC for format-patch:

+			if (commit->parents && !commit->parents->next &&
+			    !oidcmp(&commit->tree->object.oid,
+				    &commit->parents->item->tree->object.oid)) {
+				cover_at_tip_commit = commit;

As I said, I'm focusing only for (2) now, so we check there is only one parent and that the commit did not change the tree hash. (meaning an empty commit right ?)

>  and then you have
> quite a lattitude in designing what exactly such an automated cover
> letter looks like, so that the receiving end (4) can recognize it
> more easily and (more importantly) more robustly than "the message
> does not have any patch in it".  Not all random messages that do
> not have a patch in it are cover letters, and that is why I do not
> think touching any code in the apply layer in an attempt to "reuse"
> anything is a bad idea.  It will risk butchering the code without
> any real gain, because what we really need to know is *not* absence
> of patch, but presence of cover letter material.

Agreed.

> The simplest would probably be to notice that the subject of one has
> 0/N on it, while other messages were labeled with 1/N..(N-1)/N; that
> would be a lot stronger clue that 0/N has a cover than "it does not
> have any patch in it".

Good idea, and should be easy enough to put in place.

>
> It may be that we would not just want to identify which message is
> cover and which message is not, but which part of the cover letter
> message should go back to the log message of the capping empty
> commit (and moved to the merge log message).  Just like we invented
> the conventions like scissors, three-dashes, etc., you might want to
> come up with a way to do so in your format-patch enhancement used to
> do the (2) and (3) above.  Then it will be the matter of teaching
> that convention to "am" used in (4).
>

I like that too. It could also allow using (4) on series with a manual cover letter without pulling the shortlog/diffstat stuff into the new topic commit.

Nicolas

[RFC 0/3] Add support for --cover-at-tip

[Nicolas Morey-Chaisemartin]

v2:
- Enhance mailinfo to parse patch series id from subject
- Detect cover using mailinfo parsed ids in git am
- Support multiple patch series in a single run

TODO:
- Add doc/comments
- Add tests
- Add a new "seperator" at the end of a cover letter.
  Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat
  This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but
  breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that.

Note: Cover letter automatically generated with --cover-at-tip ;)

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
Nicolas Morey-Chaisemartin (3):
  mailinfo: extract patch series id
  am: semi working --cover-at-tip
  log: add an option to generate cover letter from a branch tip

 Documentation/git-format-patch.txt |   4 ++
 builtin/am.c                       | 143 ++++++++++++++++++++++++++++++++-----
 builtin/log.c                      |  44 +++++++++---
 mailinfo.c                         |  35 +++++++++
 mailinfo.h                         |   2 +
 5 files changed, 201 insertions(+), 27 deletions(-)

-- 
2.15.0.169.g3d3eebb67.dirty

[RFC 1/3] mailinfo: extract patch series id

[Nicolas Morey-Chaisemartin]

Extract the patch ID and series length from the [PATCH N/M]
 prefix in the mail header

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 mailinfo.c | 35 +++++++++++++++++++++++++++++++++++
 mailinfo.h |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/mailinfo.c b/mailinfo.c
index a89db22ab..2ab9d446d 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -308,6 +308,39 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)
 			if (!pos)
 				break;
 			remove = pos - subject->buf + at + 1;
+			if (7 <= remove &&
+			    memmem(subject->buf + at, remove, "PATCH", 5)){
+				/*
+				 * Look for a N/M series identifier at
+				 * the end of the brackets
+				 */
+				int is_series = 1;
+				int ret, num, total;
+				int pt = at + remove - 2;
+
+				if (!isdigit(subject->buf[pt]))
+					is_series = 0;
+				else {
+					while(isdigit(subject->buf[--pt]));
+				}
+
+				if(is_series && subject->buf[pt--] != '/')
+					is_series = 0;
+
+				if (!isdigit(subject->buf[pt]))
+					is_series = 0;
+				if (is_series)
+					while(isdigit(subject->buf[--pt]));
+
+				pt++;
+
+				ret = sscanf(subject->buf + pt, "%d/%d]", &num, &total);
+				if (ret == 2){
+					mi->series_id = num;
+					mi->series_len = total;
+				}
+			}
+
 			if (!mi->keep_non_patch_brackets_in_subject ||
 			    (7 <= remove &&
 			     memmem(subject->buf + at, remove, "PATCH", 5)))
@@ -1154,6 +1187,8 @@ void setup_mailinfo(struct mailinfo *mi)
 	mi->header_stage = 1;
 	mi->use_inbody_headers = 1;
 	mi->content_top = mi->content;
+	mi->series_id = -1;
+	mi->series_len = -1;
 	git_config(git_mailinfo_config, mi);
 }
 
diff --git a/mailinfo.h b/mailinfo.h
index 04a25351d..bd4f7c9e0 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -21,6 +21,8 @@ struct mailinfo {
 	struct strbuf **content_top;
 	struct strbuf charset;
 	char *message_id;
+	int series_id;    /* Id of the patch within a patch series. -1 if not a patch series */
+	int series_len;   /* Length of the patch series. -1 if not a patch series */
 	enum  {
 		TE_DONTCARE, TE_QP, TE_BASE64
 	} transfer_encoding;
-- 
2.15.0.169.g3d3eebb67.dirty

[RFC 2/3] am: semi working --cover-at-tip

[Nicolas Morey-Chaisemartin]

Issue with empty patch detection

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 builtin/am.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 126 insertions(+), 17 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 92c485350..702cbf8e0 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -111,6 +111,11 @@ struct am_state {
 	char *msg;
 	size_t msg_len;
 
+	/* Series metadata */
+	int series_id;
+	int series_len;
+	int cover_id;
+
 	/* when --rebasing, records the original commit the patch came from */
 	struct object_id orig_commit;
 
@@ -131,6 +136,8 @@ struct am_state {
 	int committer_date_is_author_date;
 	int ignore_date;
 	int allow_rerere_autoupdate;
+	int cover_at_tip;
+	int applying_cover;
 	const char *sign_commit;
 	int rebasing;
 };
@@ -160,6 +167,7 @@ static void am_state_init(struct am_state *state)
 
 	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
 		state->sign_commit = gpgsign ? "" : NULL;
+
 }
 
 /**
@@ -432,6 +440,20 @@ static void am_load(struct am_state *state)
 	read_state_file(&sb, state, "utf8", 1);
 	state->utf8 = !strcmp(sb.buf, "t");
 
+	read_state_file(&sb, state, "cover-at-tip", 1);
+	state->cover_at_tip = !strcmp(sb.buf, "t");
+
+	if (state->cover_at_tip) {
+		read_state_file(&sb, state, "series_id", 1);
+		state->series_id = strtol(sb.buf, NULL, 10);
+
+		read_state_file(&sb, state, "series_len", 1);
+		state->series_len = strtol(sb.buf, NULL, 10);
+
+		read_state_file(&sb, state, "cover_id", 1);
+		state->cover_id = strtol(sb.buf, NULL, 10);
+	}
+
 	if (file_exists(am_path(state, "rerere-autoupdate"))) {
 		read_state_file(&sb, state, "rerere-autoupdate", 1);
 		state->allow_rerere_autoupdate = strcmp(sb.buf, "t") ?
@@ -1020,6 +1042,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	write_state_bool(state, "quiet", state->quiet);
 	write_state_bool(state, "sign", state->signoff);
 	write_state_bool(state, "utf8", state->utf8);
+	write_state_bool(state, "cover-at-tip", state->cover_at_tip);
 
 	if (state->allow_rerere_autoupdate)
 		write_state_bool(state, "rerere-autoupdate",
@@ -1076,6 +1099,12 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 			delete_ref(NULL, "ORIG_HEAD", NULL, 0);
 	}
 
+	if (state->cover_at_tip) {
+		write_state_count(state, "series_id", state->series_id);
+		write_state_count(state, "series_len", state->series_len);
+		write_state_count(state, "cover_id", state->cover_id);
+	}
+
 	/*
 	 * NOTE: Since the "next" and "last" files determine if an am_state
 	 * session is in progress, they should be written last.
@@ -1088,13 +1117,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 }
 
 /**
- * Increments the patch pointer, and cleans am_state for the application of the
- * next patch.
- */
-static void am_next(struct am_state *state)
+ * Cleans am_state.
+ */static void am_clean(struct am_state *state)
 {
-	struct object_id head;
-
 	FREE_AND_NULL(state->author_name);
 	FREE_AND_NULL(state->author_email);
 	FREE_AND_NULL(state->author_date);
@@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)
 
 	oidclr(&state->orig_commit);
 	unlink(am_path(state, "original-commit"));
-
-	if (!get_oid("HEAD", &head))
-		write_state_text(state, "abort-safety", oid_to_hex(&head));
-	else
-		write_state_text(state, "abort-safety", "");
-
-	state->cur++;
-	write_state_count(state, "next", state->cur);
 }
 
 /**
@@ -1274,6 +1291,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 	fclose(mi.input);
 	fclose(mi.output);
 
+
 	/* Extract message and author information */
 	fp = xfopen(am_path(state, "info"), "r");
 	while (!strbuf_getline_lf(&sb, fp)) {
@@ -1298,9 +1316,30 @@ static int parse_mail(struct am_state *state, const char *mail)
 		goto finish;
 	}
 
-	if (is_empty_file(am_path(state, "patch"))) {
-		printf_ln(_("Patch is empty."));
-		die_user_resolve(state);
+	if (!state->applying_cover) {
+
+		state->series_id = mi.series_id;
+		state->series_len = mi.series_len;
+
+		if (state->cover_at_tip) {
+			write_state_count(state, "series_id", state->series_id);
+			write_state_count(state, "series_len", state->series_len);
+			write_state_count(state, "cover_id", state->cover_id);
+		}
+
+		if (mi.series_id == 0){
+			state->cover_id = state->cur;
+			ret = 1;
+			goto finish;
+		}
+
+		if (is_empty_file(am_path(state, "patch"))) {
+				printf_ln(_("Patch is empty."));
+				die_user_resolve(state);
+		} else if (state->cur == 1) {
+			/* First mail is not empty. cover-at-tip cannot apply */
+			state->cover_at_tip = 0;
+		}
 	}
 
 	strbuf_addstr(&msg, "\n\n");
@@ -1776,6 +1815,74 @@ static int do_interactive(struct am_state *state)
 	}
 }
 
+
+/**
+ * Apply the cover letter of a patch series
+ */
+static void do_apply_cover(struct am_state *state)
+{
+	int previous_cur = state->cur;
+	const char *mail;
+
+	am_clean(state);
+
+	state->cur = state->cover_id;
+	state->applying_cover = 1;
+	mail = am_path(state, msgnum(state));
+	if (!file_exists(mail))
+		die("BUG: cover has disapeared");
+
+	if(parse_mail(state, mail))
+		die("BUG: first patch is not a cover-letter");
+
+	if (state->signoff)
+		am_append_signoff(state);
+
+	write_author_script(state);
+	write_commit_msg(state);
+
+	if (state->interactive && do_interactive(state))
+		goto cancel_cover;
+
+	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
+
+	do_commit(state);
+ cancel_cover:
+	state->cur = previous_cur;
+	state->applying_cover = 0;
+
+	/* Reset series metadata */
+	state->series_len = 0;
+	state->series_id = 0;
+	state->cover_id = 0;
+}
+
+/**
+ * Increments the patch pointer, and cleans am_state for the application of the
+ * next patch.
+ */
+static void am_next(struct am_state *state)
+{
+	struct object_id head;
+
+	/* Flush the cover letter if needed */
+	if (state->cover_at_tip == 1 &&
+	    state->series_len > 0 &&
+	    state->series_id == state->series_len &&
+	    state->cover_id > 0)
+		do_apply_cover(state);
+
+	am_clean(state);
+
+	if (!get_oid("HEAD", &head))
+		write_state_text(state, "abort-safety", oid_to_hex(&head));
+	else
+		write_state_text(state, "abort-safety", "");
+
+	state->cur++;
+	write_state_count(state, "next", state->cur);
+}
+
 /**
  * Applies all queued mail.
  *
@@ -2287,6 +2394,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			N_("lie about committer date")),
 		OPT_BOOL(0, "ignore-date", &state.ignore_date,
 			N_("use current timestamp for author date")),
+		OPT_BOOL(0, "cover-at-tip", &state.cover_at_tip,
+			N_("apply cover letter to the tip of the branch")),
 		OPT_RERERE_AUTOUPDATE(&state.allow_rerere_autoupdate),
 		{ OPTION_STRING, 'S', "gpg-sign", &state.sign_commit, N_("key-id"),
 		  N_("GPG-sign commits"),
-- 
2.15.0.169.g3d3eebb67.dirty

[RFC 3/3] log: add an option to generate cover letter from a branch tip

[Nicolas Morey-Chaisemartin]

TODO: figure out defaults, add a config option, move tip detection to specific function

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 44 +++++++++++++++++++++++++++++---------
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6cbe462a7..0ac9d4b71 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -228,6 +228,10 @@ feeding the result to `git send-email`.
 	containing the branch description, shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--[no-]cover-letter-at-tip::
+	Use the tip of the series as a cover letter if it is an empty commit.
+    If no cover-letter is to be sent, the tip is ignored.
+
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896a..292626482 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
-			      int quiet)
+			      int quiet,
+			      struct commit *cover_at_tip_commit)
 {
 	const char *committer;
-	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
-	const char *msg;
+	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n";
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
 	int i;
@@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (!branch_name)
 		branch_name = find_branch_name(rev);
 
-	msg = body;
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;
 	pp.print_email_subject = 1;
-	pp_user_info(&pp, NULL, &sb, committer, encoding);
-	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
-	pp_remainder(&pp, &msg, &sb, 0);
-	add_branch_description(&sb, branch_name);
-	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
+	if (!cover_at_tip_commit) {
+		pp_user_info(&pp, NULL, &sb, committer, encoding);
+		pp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);
+		pp_remainder(&pp, &body, &sb, 0);
+	} else {
+		pretty_print_commit(&pp, cover_at_tip_commit, &sb);
+	}
+	add_branch_description(&sb, branch_name);
+	fprintf(rev->diffopt.file, "%s", sb.buf);
+	fprintf(rev->diffopt.file, "---\n", sb.buf);
 	strbuf_release(&sb);
 
 	shortlog_init(&log);
@@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int just_numbers = 0;
 	int ignore_if_in_upstream = 0;
 	int cover_letter = -1;
+	int cover_at_tip = -1;
+	struct commit *cover_at_tip_commit = NULL;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
 	int zero_commit = 0;
@@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("print patches to standard out")),
 		OPT_BOOL(0, "cover-letter", &cover_letter,
 			    N_("generate a cover letter")),
+		OPT_BOOL(0, "cover-at-tip", &cover_at_tip,
+			    N_("fill the cover letter with the tip of the branch")),
 		OPT_BOOL(0, "numbered-files", &just_numbers,
 			    N_("use simple number sequence for output file names")),
 		OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
@@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))
 			continue;
 
+		if (!nr && cover_at_tip == 1 && !cover_at_tip_commit) {
+			/* Check that it is a candidate to be a cover at tip
+			 * Meaning:
+			 * - a single parent (merge commits are not eligible)
+			 * - tree oid == parent->tree->oid (no diff to the tree)
+			 */
+			if (commit->parents && !commit->parents->next &&
+			    !oidcmp(&commit->tree->object.oid,
+				    &commit->parents->item->tree->object.oid)) {
+				cover_at_tip_commit = commit;
+				continue;
+			} else {
+				cover_at_tip = 0;
+			}
+		}
 		nr++;
 		REALLOC_ARRAY(list, nr);
 		list[nr - 1] = commit;
@@ -1748,7 +1771,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (thread)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
-				  origin, nr, list, branch_name, quiet);
+				  origin, nr, list, branch_name, quiet,
+				  cover_at_tip_commit);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
 		total++;
-- 
2.15.0.169.g3d3eebb67.dirty

Re: [RFC 0/3] Add support for --cover-at-tip

[Jonathan Tan]

On Mon, 13 Nov 2017 18:13:27 +0100
Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> wrote:

> v2:
> - Enhance mailinfo to parse patch series id from subject
> - Detect cover using mailinfo parsed ids in git am

I noticed that this was done in the patch set by searching for "PATCH" -
that is probably quite error-prone as not all patches will have a
subject line of that form. It may be better to search for "0/" and
ensure that it is immediately followed by an integer.

Also, it might be worth checking the message IDs to ensure that the
PATCH M/Ns all indeed are replies to PATCH 0/N.

> - Support multiple patch series in a single run

Is this done? I would have expected that some buffering of messages
would be necessary, since you're writing a series of messages of the
form <cover><patch 1>...<patch N> to the commits <patch 1>...<patch
N><cover>.

> TODO:
> - Add doc/comments
> - Add tests
> - Add a new "seperator" at the end of a cover letter.
>   Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat
>   This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but
>   breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that.

I think the triple dash works. I tried "git am" with a cover letter with
no triple dash, and it complains that the commit is empty anyway, so
compatibility with older git am might not be such a big issue. (With the
triple dash, it indeed complains about a malformed patch, as you
describe.)

Re: [RFC 0/3] Add support for --cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 13/11/2017 à 20:40, Jonathan Tan a écrit :
> On Mon, 13 Nov 2017 18:13:27 +0100
> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> wrote:
>
>> v2:
>> - Enhance mailinfo to parse patch series id from subject
>> - Detect cover using mailinfo parsed ids in git am
> I noticed that this was done in the patch set by searching for "PATCH" -
> that is probably quite error-prone as not all patches will have a
> subject line of that form. It may be better to search for "0/" and
> ensure that it is immediately followed by an integer.

Yes this could be moved. I wasn't sure about the risk of colliding with something else.


> Also, it might be worth checking the message IDs to ensure that the
> PATCH M/Ns all indeed are replies to PATCH 0/N.

Doesn't that only work if mails [1..N] are reply to the cover ? Depending on the mailer and your option, this might not always be the case (I think).

>
>> - Support multiple patch series in a single run
> Is this done? I would have expected that some buffering of messages
> would be necessary, since you're writing a series of messages of the
> form <cover><patch 1>...<patch N> to the commits <patch 1>...<patch
> N><cover>.
>

This is for git am. It handles the fact that an input may contain multiple series (expect them to be in order).
When reaching patch N/N, it flushed the cover.


>> TODO:
>> - Add doc/comments
>> - Add tests
>> - Add a new "seperator" at the end of a cover letter.
>>   Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat
>>   This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but
>>   breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that.
> I think the triple dash works. I tried "git am" with a cover letter with
> no triple dash, and it complains that the commit is empty anyway, so
> compatibility with older git am might not be such a big issue. (With the
> triple dash, it indeed complains about a malformed patch, as you
> describe.)

It kinda work but it makes the message difficult to understand for the user.
And change the behaviour from the previous releases.

Thanks for the feedback.

Nicolas

Re: [RFC 1/3] mailinfo: extract patch series id

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

> Extract the patch ID and series length from the [PATCH N/M]
>  prefix in the mail header
>
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  mailinfo.c | 35 +++++++++++++++++++++++++++++++++++
>  mailinfo.h |  2 ++
>  2 files changed, 37 insertions(+)

As JTan already mentioned, relying on a substring "PATCH" may not be
very reliable, and trying to locate "%d/%d]" feels like a better
approach.

cleanup_subject() is called only when keep_subject is false, so this
code will not trigger in that case at all.  Is this intended?

I would have expected that a new helper function would be written,
without changing existing helpers like cleanup_subject(), and that
new helper gets called by handle_info() after output_header_lines()
helper is called for the "Subject".

Whenever mailinfo learns to glean a new useful piece of information,
it should be made available to scripts that run "git mailinfo", too.
Perhaps show something like

	PatchNumber: 1
	TotalPatches: 3

at the end of handle_info() to mi->output?  I do not think existing
tools mind too much, even if we added a for-debug output e.g.

	RawSubject: [RFC 1/3] mailinfo: extract patch series id

to the output.

Re: [RFC 2/3] am: semi working --cover-at-tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

>  	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
>  		state->sign_commit = gpgsign ? "" : NULL;
> +
>  }

Please give at least a cursory proof-reading before sending things
out.

> @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)
>  
>  	oidclr(&state->orig_commit);
>  	unlink(am_path(state, "original-commit"));
> -
> -	if (!get_oid("HEAD", &head))
> -		write_state_text(state, "abort-safety", oid_to_hex(&head));
> -	else
> -		write_state_text(state, "abort-safety", "");
> -
> -	state->cur++;
> -	write_state_count(state, "next", state->cur);

Moving these lines to a later part of the source file is fine, but
can you do so as a separate preparatory patch that does not change
anything else?  That would unclutter the main patch that adds the
feature, allowing better reviews from reviewers.

The hunk below...

> +/**
> + * Increments the patch pointer, and cleans am_state for the application of the
> + * next patch.
> + */
> +static void am_next(struct am_state *state)
> +{
> +	struct object_id head;
> +
> +	/* Flush the cover letter if needed */
> +	if (state->cover_at_tip == 1 &&
> +	    state->series_len > 0 &&
> +	    state->series_id == state->series_len &&
> +	    state->cover_id > 0)
> +		do_apply_cover(state);
> +
> +	am_clean(state);
> +
> +	if (!get_oid("HEAD", &head))
> +		write_state_text(state, "abort-safety", oid_to_hex(&head));
> +	else
> +		write_state_text(state, "abort-safety", "");
> +
> +	state->cur++;
> +	write_state_count(state, "next", state->cur);
> +}

... if you followed that "separate preparatory step" approach, would
show clearly that you added the logic to call do_apply_cover() when
we transition after applying the Nth patch of a series with N patches,
as all the existing lines will show only as unchanged context lines.

By the way, don't we want to sanity check state->last (which we
learn by running "git mailsplit" that splits the incoming mbox into
pieces and counts the number of messages) against state->series_len?
Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover
letter, and then follow-up with [PATCH 7/6].  For somebody like me,
it would be more convenient if the above code (more-or-less) ignored
series_len and called do_apply_cover() after applying the last patch
(which would be [PATCH 7/6]) based on what state->last says.

Thanks.

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

> -	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
> -	const char *msg;
> +	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n";

Hmmmm.

> @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	if (!branch_name)
>  		branch_name = find_branch_name(rev);
>  
> -	msg = body;
>  	pp.fmt = CMIT_FMT_EMAIL;
>  	pp.date_mode.type = DATE_RFC2822;
>  	pp.rev = rev;
>  	pp.print_email_subject = 1;
> -	pp_user_info(&pp, NULL, &sb, committer, encoding);
> -	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
> -	pp_remainder(&pp, &msg, &sb, 0);
> -	add_branch_description(&sb, branch_name);
> -	fprintf(rev->diffopt.file, "%s\n", sb.buf);
>  
> +	if (!cover_at_tip_commit) {
> +		pp_user_info(&pp, NULL, &sb, committer, encoding);
> +		pp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);
> +		pp_remainder(&pp, &body, &sb, 0);
> +	} else {
> +		pretty_print_commit(&pp, cover_at_tip_commit, &sb);
> +	}
> +	add_branch_description(&sb, branch_name);
> +	fprintf(rev->diffopt.file, "%s", sb.buf);
> +	fprintf(rev->diffopt.file, "---\n", sb.buf);
>  	strbuf_release(&sb);

I would have expected that this feature would not change anything
other than replacing the constant string *body we unconditionally
print with the log message of the empty commit at the tip, so from
that expectation, I was hoping that a patch looked nothing more than
this:

 builtin/log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896ad..0af19d5b36 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -986,6 +986,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
+			      struct commit *cover,
 			      int quiet)
 {
 	const char *committer;
@@ -1021,7 +1022,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (!branch_name)
 		branch_name = find_branch_name(rev);
 
-	msg = body;
+	if (cover)
+		msg = get_cover_from_commit(cover);
+	else
+		msg = body;
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;


plus a newly written function get_cover_from_commit().  Why does
this patch need to change a lot more than that, I have to wonder.

This is totally unrelated, but I wonder if it makes sense to do
something similar for branch.description, too.  If the user has a
meaningful description prepared with "git branch --edit-desc", it is
somewhat insulting to the user to still add "*** BLURB HERE ***".

Re: [RFC 1/3] mailinfo: extract patch series id

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 06:47, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>
>> Extract the patch ID and series length from the [PATCH N/M]
>>  prefix in the mail header
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>> ---
>>  mailinfo.c | 35 +++++++++++++++++++++++++++++++++++
>>  mailinfo.h |  2 ++
>>  2 files changed, 37 insertions(+)
> As JTan already mentioned, relying on a substring "PATCH" may not be
> very reliable, and trying to locate "%d/%d]" feels like a better
> approach.
>
> cleanup_subject() is called only when keep_subject is false, so this
> code will not trigger in that case at all.  Is this intended?
>
> I would have expected that a new helper function would be written,
> without changing existing helpers like cleanup_subject(), and that
> new helper gets called by handle_info() after output_header_lines()
> helper is called for the "Subject".
Isn't that too late ?
If keep subject is not set, will cleanup_subject not drop all those info from the strbuf  ?
But yes it is not in the right function now.
But I would call the function before
            if (!mi->keep_subject) {

>
> Whenever mailinfo learns to glean a new useful piece of information,
> it should be made available to scripts that run "git mailinfo", too.
> Perhaps show something like
>
> 	PatchNumber: 1
> 	TotalPatches: 3
>
> at the end of handle_info() to mi->output?  I do not think existing
> tools mind too much, even if we added a for-debug output e.g.
>
> 	RawSubject: [RFC 1/3] mailinfo: extract patch series id
>
> to the output.
Will do.

Re: [RFC 2/3] am: semi working --cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 07:00, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>
>>  	if (!git_config_get_bool("commit.gpgsign", &gpgsign))
>>  		state->sign_commit = gpgsign ? "" : NULL;
>> +
>>  }
> Please give at least a cursory proof-reading before sending things
> out.
>
>> @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)
>>  
>>  	oidclr(&state->orig_commit);
>>  	unlink(am_path(state, "original-commit"));
>> -
>> -	if (!get_oid("HEAD", &head))
>> -		write_state_text(state, "abort-safety", oid_to_hex(&head));
>> -	else
>> -		write_state_text(state, "abort-safety", "");
>> -
>> -	state->cur++;
>> -	write_state_count(state, "next", state->cur);
> Moving these lines to a later part of the source file is fine, but
> can you do so as a separate preparatory patch that does not change
> anything else?  That would unclutter the main patch that adds the
> feature, allowing better reviews from reviewers.
>
> The hunk below...

Sure. I usually do all this later in the process.
>> +/**
>> + * Increments the patch pointer, and cleans am_state for the application of the
>> + * next patch.
>> + */
>> +static void am_next(struct am_state *state)
>> +{
>> +	struct object_id head;
>> +
>> +	/* Flush the cover letter if needed */
>> +	if (state->cover_at_tip == 1 &&
>> +	    state->series_len > 0 &&
>> +	    state->series_id == state->series_len &&
>> +	    state->cover_id > 0)
>> +		do_apply_cover(state);
>> +
>> +	am_clean(state);
>> +
>> +	if (!get_oid("HEAD", &head))
>> +		write_state_text(state, "abort-safety", oid_to_hex(&head));
>> +	else
>> +		write_state_text(state, "abort-safety", "");
>> +
>> +	state->cur++;
>> +	write_state_count(state, "next", state->cur);
>> +}
> ... if you followed that "separate preparatory step" approach, would
> show clearly that you added the logic to call do_apply_cover() when
> we transition after applying the Nth patch of a series with N patches,
> as all the existing lines will show only as unchanged context lines.

Agreed. The split of am_clean should probably have its own commit too.

>
> By the way, don't we want to sanity check state->last (which we
> learn by running "git mailsplit" that splits the incoming mbox into
> pieces and counts the number of messages) against state->series_len?
> Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover
> letter, and then follow-up with [PATCH 7/6].  For somebody like me,
> it would be more convenient if the above code (more-or-less) ignored
> series_len and called do_apply_cover() after applying the last patch
> (which would be [PATCH 7/6]) based on what state->last says.

I thought about that.
Is there a use case for cover after the last patch works and removes the need to touch am_next (can be done out of the loop in am_run).

If that multiple series in a mbox is something people do, your concern could be solved by flushing the cover when state->series_id goes back to a lower value.

Nicolas

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 07:14, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>
>> -	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
>> -	const char *msg;
>> +	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n";
> Hmmmm.

The \n from fprintf(rev->diffopt.file, "%s\n", sb.buf); added an extra line after the cover which is fine for the default one but changed the commit value (at least at some point while I was playing with scissors)
so I moved it up there to avoid adding a if() around the fprintf

>
>> @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>  	if (!branch_name)
>>  		branch_name = find_branch_name(rev);
>>  
>> -	msg = body;
>>  	pp.fmt = CMIT_FMT_EMAIL;
>>  	pp.date_mode.type = DATE_RFC2822;
>>  	pp.rev = rev;
>>  	pp.print_email_subject = 1;
>> -	pp_user_info(&pp, NULL, &sb, committer, encoding);
>> -	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
>> -	pp_remainder(&pp, &msg, &sb, 0);
>> -	add_branch_description(&sb, branch_name);
>> -	fprintf(rev->diffopt.file, "%s\n", sb.buf);
>>  
>> +	if (!cover_at_tip_commit) {
>> +		pp_user_info(&pp, NULL, &sb, committer, encoding);
>> +		pp_title_line(&pp, &body, &sb, encoding, need_8bit_cte);
>> +		pp_remainder(&pp, &body, &sb, 0);
>> +	} else {
>> +		pretty_print_commit(&pp, cover_at_tip_commit, &sb);
>> +	}
>> +	add_branch_description(&sb, branch_name);
>> +	fprintf(rev->diffopt.file, "%s", sb.buf);
>> +	fprintf(rev->diffopt.file, "---\n", sb.buf);
>>  	strbuf_release(&sb);
> I would have expected that this feature would not change anything
> other than replacing the constant string *body we unconditionally
> print with the log message of the empty commit at the tip, so from
> that expectation, I was hoping that a patch looked nothing more than
> this:
>
>  builtin/log.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 6c1fa896ad..0af19d5b36 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -986,6 +986,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
> +			      struct commit *cover,
>  			      int quiet)
>  {
>  	const char *committer;
> @@ -1021,7 +1022,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	if (!branch_name)
>  		branch_name = find_branch_name(rev);
>  
> -	msg = body;
> +	if (cover)
> +		msg = get_cover_from_commit(cover);
> +	else
> +		msg = body;
>  	pp.fmt = CMIT_FMT_EMAIL;
>  	pp.date_mode.type = DATE_RFC2822;
>  	pp.rev = rev;
>
>
> plus a newly written function get_cover_from_commit().  Why does
> this patch need to change a lot more than that, I have to wonder.

The added code is to avoid the get_cover_from_commit generating a single strbuf that needs to be reparse/resplit by pp_user_info/pp_title_line/pp_remainder.
There was a helper that did the right job in my case so I took it ;)

The triple dash is so that the diffstat/shortlog as not seen as part of the cover letter.
As said in the cover letter for this series, it kinda breaks legacy behaviour right now.
It should either be printed only for cover-at-tip, or a new separator should be added.

>
> This is totally unrelated, but I wonder if it makes sense to do
> something similar for branch.description, too.  If the user has a
> meaningful description prepared with "git branch --edit-desc", it is
> somewhat insulting to the user to still add "*** BLURB HERE ***".

I guess so.
And the branch description should probably not be added when using cover-at-tip either.

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

> The triple dash is so that the diffstat/shortlog as not seen as
> part of the cover letter.  As said in the cover letter for this
> series, it kinda breaks legacy behaviour right now.  It should
> either be printed only for cover-at-tip, or a new separator should
> be added.

This reminds me of a couple of random thoughts I had, so before I
disconnect from my terminal and forget about them...

[1] format-patch and am must round-trip.

I mentioned four uses cases around the "cover letter at the
tip" in my earlier message

    https://public-inbox.org/git/xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com/

Specifically, (2) we should be able to run "format-patch" and record
the log message of the empty commit at the tip to the cover letter,
and (4) we should be able to accept such output with "am" and end up
with the same sequence of commits as the original (modulo committer
identity and timestamps).  So from the output we produce with this
step, "am" should be able to split the material that came from the
original empty commit from the surrounding cruft like shortlog and
diffstat.  The output format of this step needs to be designed with
that in mind.

[2] reusing cover letter material in merge may not be ideal.

When people write a cover letter, they write different things in it.
What they wanted to achieve, why they chose the approach they took,
how the series is organized, which part of the series they find iffy
and/orneeds special attention from the reviewers, where to find the
previous iteration, what changed since the previous iterations, etc.

All of them are to help the reviewers, many of who have already
looked at the previous rounds, to understand and judge this round of
the submission.

The message in a merge commit as a part of the final history,
however, cannot refer to anything from "previous rounds", as the
previous attempts are not part of the final history readers of "git
log" can refer to whey they are trying to understand the merge.
What exactly goes in a merge commit and how the messages are phrased
may be different from project to project, but for this project, I've
been trying to write them in an end-user facing terms, i.e. they are
designed in such a way that "git log --first-parent --merges" can be
read as if they were entries in the release notes, summarizing fixes
and features by describing their user-visible effects.  This is only
one part of what people write in their cover letters (i.e. "what
they wanted to achive").

So there probably needs a convention meant to be followed by human
users when writing cover letters, so a mechanical process can tell
which part of the text is to be made into the merge commit without
understanding human languages.

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 14:05, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>
>> The triple dash is so that the diffstat/shortlog as not seen as
>> part of the cover letter.  As said in the cover letter for this
>> series, it kinda breaks legacy behaviour right now.  It should
>> either be printed only for cover-at-tip, or a new separator should
>> be added.
> This reminds me of a couple of random thoughts I had, so before I
> disconnect from my terminal and forget about them...
>
> [1] format-patch and am must round-trip.
>
> I mentioned four uses cases around the "cover letter at the
> tip" in my earlier message
>
>     https://public-inbox.org/git/xmqqbmk68o9d.fsf@gitster.mtv.corp.google.com/
>
> Specifically, (2) we should be able to run "format-patch" and record
> the log message of the empty commit at the tip to the cover letter,
> and (4) we should be able to accept such output with "am" and end up
> with the same sequence of commits as the original (modulo committer
> identity and timestamps).  So from the output we produce with this
> step, "am" should be able to split the material that came from the
> original empty commit from the surrounding cruft like shortlog and
> diffstat.  The output format of this step needs to be designed with
> that in mind.

This should be the case with the current RFC (apart from the branch description which is kept at the moment).
Things like git am --signoff will break this of course.

>
> [2] reusing cover letter material in merge may not be ideal.
>
> When people write a cover letter, they write different things in it.
> What they wanted to achieve, why they chose the approach they took,
> how the series is organized, which part of the series they find iffy
> and/orneeds special attention from the reviewers, where to find the
> previous iteration, what changed since the previous iterations, etc.
>
> All of them are to help the reviewers, many of who have already
> looked at the previous rounds, to understand and judge this round of
> the submission.
>
> The message in a merge commit as a part of the final history,
> however, cannot refer to anything from "previous rounds", as the
> previous attempts are not part of the final history readers of "git
> log" can refer to whey they are trying to understand the merge.
> What exactly goes in a merge commit and how the messages are phrased
> may be different from project to project, but for this project, I've
> been trying to write them in an end-user facing terms, i.e. they are
> designed in such a way that "git log --first-parent --merges" can be
> read as if they were entries in the release notes, summarizing fixes
> and features by describing their user-visible effects.  This is only
> one part of what people write in their cover letters (i.e. "what
> they wanted to achive").
>
> So there probably needs a convention meant to be followed by human
> users when writing cover letters, so a mechanical process can tell
> which part of the text is to be made into the merge commit without
> understanding human languages.

In the long term, I agree this would be nice.
As a first step, could we force the --edit option when using --cover-at-tip ?
The basic merge message would come from the cover letter but can/should be edited to clear the extra stuff out.

Auto-stripping those extra infos, may also conflicts with the point (3) from your previous mail.
If git merge is to keep only the relevant infos, git format-patch from this merge will not be able to access those.

The advantage of a manual edit is that it's the commiter's choice. Keep the info if the branch is to be resubmitted. Drop them once it's merged for good.

Nicolas

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

>> So there probably needs a convention meant to be followed by human
>> users when writing cover letters, so a mechanical process can tell
>> which part of the text is to be made into the merge commit without
>> understanding human languages.
>
> In the long term, I agree this would be nice.  As a first step,
> could we force the --edit option when using --cover-at-tip ?  The
> basic merge message would come from the cover letter but
> can/should be edited to clear the extra stuff out.

Ah, "git merge" by default opens the editor these days, so there is
no need to do a special "force"-ing only when we are taking the
initial log message material from the empty commit at the tip.  So
that plan would work rather well, I would imagine.

Good thinking.  Thanks.

Re: [RFC 2/3] am: semi working --cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 10:17, Nicolas Morey-Chaisemartin a écrit :
>
> Le 14/11/2017 à 07:00, Junio C Hamano a écrit :
>> Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:
>>
>> By the way, don't we want to sanity check state->last (which we
>> learn by running "git mailsplit" that splits the incoming mbox into
>> pieces and counts the number of messages) against state->series_len?
>> Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover
>> letter, and then follow-up with [PATCH 7/6].  For somebody like me,
>> it would be more convenient if the above code (more-or-less) ignored
>> series_len and called do_apply_cover() after applying the last patch
>> (which would be [PATCH 7/6]) based on what state->last says.
> I thought about that.
> Is there a use case for cover after the last patch works and removes the need to touch am_next (can be done out of the loop in am_run).

Do you have an opinion on that ? It has quite a big impact on how things are done !
Single series only would mean a simple flush at the end.
Multiple series makes things a whole lot complex.
We do not know the series_id of the next patch until it's parsed by parse_mail.
Which would mean interrupting parse_mail when detecting a new series to call parse_mail on the cover_id plus an extra detection at the end of the loop.

Nicolas

Re: [RFC 2/3] am: semi working --cover-at-tip

[Junio C Hamano]

Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.de> writes:

>> I thought about that.

>> Is there a use case for cover after the last patch works and
>> removes the need to touch am_next (can be done out of the loop in
>> am_run).
>
> Do you have an opinion on that ? It has quite a big impact on how things are done !
> Single series only would mean a simple flush at the end.
> Multiple series makes things a whole lot complex.

I am not sure what you even mean.  Are you wondering what "am"
should do to a mbox with, say, these messages?

	1: [PATCH 0/2] Cover for series A
	2: [PATCH 1/2] patch 1 of series A
	3: [PATCH 2/2] patch 2 of series A
	4: [PATCH 0/3] Cover for series B
	5: [PATCH 1/3] patch 1 of series B
	6: [PATCH 2/3] patch 2 of series B
	7: [PATCH 2/3] patch 3 of series B

Running "am" on the whole thing and expecting covers to become the
capping empty commit at the tip is crazy, I would think, for such a
mbox, as there is no way to tell the command (after it processes 1,
2 and 3, to create commits out of 1, 2 and then an empty one out of
0 to finish one topic off) that it must create a new branch to store
the next series, and building the second series on top of the
capping empty commit at the tip of first series would not make any
sense---the "tip empty commit" for the first series will no longer
be at the "tip".

What I was alluding to is a different case, in which additional
patches are sent as a follow-up later, ending up in a mbox like
this:

	1: [PATCH 0/2] Cover for series A
	2: [PATCH 1/2] patch 1 of series A
	3: [PATCH 2/2] patch 2 of series A
	4: [PATCH 3/2] patch 3 of series A

Naturally, the cover letter may not list 3/2 in its short-log
section, but the description for the overall goal and approach
of the series in it should still be valid even with patch 3/2.
The total number of messages mailsplit gives us would be 4, your
subject parser would read "2" as the number of patches, which would
make the number of messages for the series to be expected "3"
(i.e. "2" plus cover), but "am" would want to create commits for
patches 1, 2, and 3, and then cap it with the cover material.