[RFC/PATCH 0/2] place cherry pick line below commit title

[RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

This is somewhat of a follow-up to my previous e-mail with subject
"[PATCH] sequencer: support folding in rfc2822 footer" [1], in which I
proposed relaxing the definition of a commit message footer to allow
multiple-line field bodies (as described in RFC2822), but its strictness
was deemed deliberate.

Below is a patch set that allows placing the "cherry picked from" line
without taking into account the definition of a commit message footer.
For example, "git cherry-pick -x" (with the appropriate configuration
variable or argument) would, to this commit message:

  commit title

  This is an explanatory paragraph.

  Footer: foo

place the "(cherry picked from ...)" line below "commit title".

Would this be better?

[1] <1472846322-5592-1-git-send-email-jonathantanmy@google.com>

Jonathan Tan (2):
  sequencer: refactor message and origin appending
  sequencer: allow origin line below commit title

 Documentation/config.txt          |  4 +++
 Documentation/git-cherry-pick.txt | 15 ++++++++-
 builtin/revert.c                  | 38 ++++++++++++++++++++-
 sequencer.c                       | 69 +++++++++++++++++++++++++++++----------
 sequencer.h                       |  7 ++++
 t/t3511-cherry-pick-x.sh          | 59 +++++++++++++++++++++++++++++++++
 6 files changed, 172 insertions(+), 20 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[RFC/PATCH 1/2] sequencer: refactor message and origin appending

[Jonathan Tan]

Move the appending of the commit message and origin information into its
own function, in preparation for a subsequent patch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 sequencer.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3804fa9..b29c9ca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -443,6 +443,33 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
 		return 1;
 }
 
+/*
+ * Appends the commit log message, including the cherry picked notification if
+ * record_origin is nonzero.
+ */
+static void append_message(struct strbuf *msgbuf,
+			   const struct commit_message *msg,
+			   int record_origin,
+			   const struct commit *commit)
+{
+	/*
+	 * Append the commit log message to msgbuf; it starts
+	 * after the tree, parent, author, committer
+	 * information followed by "\n\n".
+	 */
+	const char *p = strstr(msg->message, "\n\n");
+	if (p)
+		strbuf_addstr(msgbuf, skip_blank_lines(p + 2));
+
+	if (record_origin) {
+		if (!has_conforming_footer(msgbuf, NULL, 0))
+			strbuf_addch(msgbuf, '\n');
+		strbuf_addstr(msgbuf, cherry_picked_prefix);
+		strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
+		strbuf_addstr(msgbuf, ")\n");
+	}
+}
+
 static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
@@ -534,29 +561,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {
-		const char *p;
-
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
 
-		/*
-		 * Append the commit log message to msgbuf; it starts
-		 * after the tree, parent, author, committer
-		 * information followed by "\n\n".
-		 */
-		p = strstr(msg.message, "\n\n");
-		if (p)
-			strbuf_addstr(&msgbuf, skip_blank_lines(p + 2));
-
-		if (opts->record_origin) {
-			if (!has_conforming_footer(&msgbuf, NULL, 0))
-				strbuf_addch(&msgbuf, '\n');
-			strbuf_addstr(&msgbuf, cherry_picked_prefix);
-			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-			strbuf_addstr(&msgbuf, ")\n");
-		}
+		append_message(&msgbuf, &msg, opts->record_origin, commit);
 	}
 
 	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
-- 
2.8.0.rc3.226.g39d4020

[RFC/PATCH 2/2] sequencer: allow origin line below commit title

[Jonathan Tan]

When git cherry-pick -x is invoked, a "(cherry picked from commit ...)"
line is appended to the footer of a commit message that Git interprets
to contain a footer; otherwise it is appended at the end as a new
paragraph, preceded by a blank line. This behavior may appear
inconsistent, especially to users who differ from Git in their
interpretation of what constitutes a footer.

Provide the user, through a configuration option and command-line flag,
the option of placing the "cherry picked" line below the commit title
instead of the current behavior.  This allows the "cherry picked" line
to be placed in a consistent manner, independent of the nature of the
footer of the existing commit message.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/config.txt          |  4 +++
 Documentation/git-cherry-pick.txt | 15 +++++++++-
 builtin/revert.c                  | 38 ++++++++++++++++++++++++-
 sequencer.c                       | 39 ++++++++++++++++++++------
 sequencer.h                       |  7 +++++
 t/t3511-cherry-pick-x.sh          | 59 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..fb1990f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -945,6 +945,10 @@ browser.<tool>.path::
 	browse HTML help (see `-w` option in linkgit:git-help[1]) or a
 	working repository in gitweb (see linkgit:git-instaweb[1]).
 
+cherrypick.originLineLocation::
+	Default for the `--origin-line-location` option in git-cherry-pick.
+	Defaults to `bottom`.
+
 clean.requireForce::
 	A boolean to make git-clean do nothing unless given -f,
 	-i or -n.   Defaults to true.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771..5a8359f 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -58,7 +58,7 @@ OPTIONS
 	message prior to committing.
 
 -x::
-	When recording the commit, append a line that says
+	When recording the commit, add a line that says
 	"(cherry picked from commit ...)" to the original commit
 	message in order to indicate which commit this change was
 	cherry-picked from.  This is done only for cherry
@@ -71,6 +71,14 @@ OPTIONS
 	development branch), adding this information can be
 	useful.
 
+--origin-line-location::
+	Where to put the "(cherry picked from commit ...)" line when requested
+	with the `-x` option.  May be `top`, meaning at the top of the commit
+	message body, immediately below the commit title (see the DISCUSSION
+	section of linkgit:git-commit[1]), or `bottom`, meaning at the end of
+	the commit message.  The default is controlled by the
+	`cherrypick.originLineLocation` configuration variable.
+
 -r::
 	It used to be that the command defaulted to do `-x`
 	described above, and `-r` was to disable it.  Now the
@@ -224,6 +232,11 @@ the working tree.
 spending extra time to avoid mistakes based on incorrectly matching
 context lines.
 
+CONFIGURATION
+-------------
+cherrypick.originLineLocation::
+	Default for the `--origin-line-location` option.  Defaults to `bottom`.
+
 SEE ALSO
 --------
 linkgit:git-revert[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 4e69380..a5459a0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -71,11 +71,25 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
+static int set_origin_line(enum origin_line *line, const char *str)
+{
+	if (!strcmp(str, "bottom")) {
+		*line = ORIGIN_LINE_BOTTOM;
+		return 1;
+	}
+	if (!strcmp(str, "top")) {
+		*line = ORIGIN_LINE_TOP;
+		return 1;
+	}
+	return 0;
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
 	int cmd = 0;
+	const char *origin_str = NULL;
 	struct option base_options[] = {
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
@@ -98,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOL('x', NULL, &opts->record_origin, N_("append commit name")),
+			OPT_STRING(0, "origin-line-location", &origin_str, N_("origin-line-location"), N_("location of appended commit name")),
 			OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
@@ -125,6 +140,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	else
 		opts->subcommand = REPLAY_NONE;
 
+	/* Set the origin line location */
+	if (origin_str)
+		if (!set_origin_line(&opts->origin_line, origin_str))
+			die(_("%s: --origin-line-location must be top or bottom"),
+			    me);
+
 	/* Check for incompatible command line arguments */
 	if (opts->subcommand != REPLAY_NONE) {
 		char *this_operation;
@@ -176,6 +197,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		usage_with_options(usage_str, options);
 }
 
+static int git_cherry_pick_config(const char *var, const char *value,
+				  void *opts_)
+{
+	struct replay_opts *opts = opts_;
+
+	if (!strcmp(var, "cherrypick.originlinelocation")) {
+		if (!value)
+			return config_error_nonbool(var);
+		set_origin_line(&opts->origin_line, value);
+		return 0;
+	}
+
+	return git_default_config(var, value, opts_);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -200,7 +236,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(opts));
 	opts.action = REPLAY_PICK;
-	git_config(git_default_config, NULL);
+	git_config(git_cherry_pick_config, &opts);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
diff --git a/sequencer.c b/sequencer.c
index b29c9ca..ef9e5bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -450,23 +450,45 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
 static void append_message(struct strbuf *msgbuf,
 			   const struct commit_message *msg,
 			   int record_origin,
+			   enum origin_line origin_line,
 			   const struct commit *commit)
 {
 	/*
-	 * Append the commit log message to msgbuf; it starts
+	 * The commit log message starts
 	 * after the tree, parent, author, committer
 	 * information followed by "\n\n".
 	 */
 	const char *p = strstr(msg->message, "\n\n");
-	if (p)
-		strbuf_addstr(msgbuf, skip_blank_lines(p + 2));
+	p = skip_blank_lines(p + 2);
+	if (!record_origin) {
+		strbuf_addstr(msgbuf, p);
+		return;
+	}
 
-	if (record_origin) {
+	switch (origin_line) {
+	case ORIGIN_LINE_TOP:
+		/* First, add only the subject. */
+		p = format_subject(msgbuf, p, "\n");
+		strbuf_addstr(msgbuf, "\n\n");
+		break;
+	case ORIGIN_LINE_BOTTOM:
+		strbuf_addstr(msgbuf, p);
 		if (!has_conforming_footer(msgbuf, NULL, 0))
 			strbuf_addch(msgbuf, '\n');
-		strbuf_addstr(msgbuf, cherry_picked_prefix);
-		strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
-		strbuf_addstr(msgbuf, ")\n");
+		break;
+	}
+
+	strbuf_addstr(msgbuf, cherry_picked_prefix);
+	strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
+	strbuf_addstr(msgbuf, ")\n");
+
+	if (origin_line == ORIGIN_LINE_TOP) {
+		/* Add the rest of the commit message. */
+		p = skip_blank_lines(p);
+		if (*p) {
+			strbuf_addch(msgbuf, '\n');
+			strbuf_addstr(msgbuf, p);
+		}
 	}
 }
 
@@ -566,7 +588,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		next = commit;
 		next_label = msg.label;
 
-		append_message(&msgbuf, &msg, opts->record_origin, commit);
+		append_message(&msgbuf, &msg, opts->record_origin,
+			       opts->origin_line, commit);
 	}
 
 	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..7cf381c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -20,6 +20,11 @@ enum replay_subcommand {
 	REPLAY_ROLLBACK
 };
 
+enum origin_line {
+	ORIGIN_LINE_BOTTOM,
+	ORIGIN_LINE_TOP
+};
+
 struct replay_opts {
 	enum replay_action action;
 	enum replay_subcommand subcommand;
@@ -46,6 +51,8 @@ struct replay_opts {
 
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
+
+	enum origin_line origin_line;
 };
 
 int sequencer_pick_revisions(struct replay_opts *opts);
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..57e3861 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -244,4 +244,63 @@ test_expect_success 'cherry-pick preserves commit message' '
 	test_cmp expect actual
 '
 
+mesg_one_para="This is a commit message
+in one paragraph"
+
+test_expect_success 'cherry-pick -x (top location) one-paragraph commit message' '
+	pristine_detach initial &&
+	test_config cherrypick.originLineLocation top &&
+	test_commit "$mesg_one_para" foo b mesg-one-para &&
+	git reset --hard initial &&
+	sha1=$(git rev-parse mesg-one-para^0) &&
+	git cherry-pick -x mesg-one-para &&
+	cat <<-EOF >expect &&
+		$mesg_one_para
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+space=" "
+mesg_multi_para="$mesg_one_para
+$space
+
+$mesg_one_para"
+
+test_expect_success 'cherry-pick -x (top location) multi-paragraph commit message' '
+	pristine_detach initial &&
+	test_config cherrypick.originLineLocation top &&
+	test_commit "$mesg_multi_para" foo b mesg-multi-para &&
+	git reset --hard initial &&
+	sha1=$(git rev-parse mesg-multi-para^0) &&
+	git cherry-pick -x mesg-multi-para &&
+	cat <<-EOF >expect &&
+		$mesg_one_para
+
+		(cherry picked from commit $sha1)
+
+		$mesg_one_para
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -x location argument overrides config' '
+	test_config cherrypick.originLineLocation top &&
+	git reset --hard initial &&
+	sha1=$(git rev-parse mesg-multi-para^0) &&
+	git cherry-pick -x --origin-line-location=bottom mesg-multi-para &&
+	cat <<-EOF >expect &&
+		$mesg_one_para
+
+		$mesg_one_para
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> This is somewhat of a follow-up to my previous e-mail with subject
> "[PATCH] sequencer: support folding in rfc2822 footer" [1], in which I
> proposed relaxing the definition of a commit message footer to allow
> multiple-line field bodies (as described in RFC2822), but its strictness
> was deemed deliberate.

It does not necessarily mean we can never change it when we did
something deliberately, though.  With a good enough justification,
and with a transitition plan if the backward incompatibility is
severe enough to warrant one, we can change things.

I vaguely recall that there were some discussion on the definition
of "what's a trailer line" with folks from the kernel land, perhaps
while discussing the interpret-trailers topic.  IIRC, when somebody
passes an improved version along, the resulting message's trailer
block may look like this:

    Signed-off-by: Original Author <original@author.xz>
    [fixed typo in the variable names]
    Signed-off-by: Somebhody Else <somebody@else.xz>

and an obvious "wish" of theirs was to treat not just RFC2822-like
"a line that begins with token followed by a colon" but also these
short comments as part of the trailer block.  Your original wish in
[*1*] is to also treat "a line that begin with a whitespace that
follows a line that begins with token followed by a colon" as part
of the trailer block and I personally think that is a reasonable
thing to wish for, too.

I recall that I was somewhat surprised and dissapointed to see no
change to interpret-trailers when you tried [*1*], which was really
about improving the definition of what the trailer block is, by the
way.

In any case, if we want to improve what the trailer block is, we
would certainly need to make sure what is inserted by "cherry-pick -x"
is also considered as part of the trailer block, so it may be necessary
to change it to "Cherry-picked-from: ..." while doing so.  I dunno.

> Below is a patch set that allows placing the "cherry picked from" line
> without taking into account the definition of a commit message footer.
> For example, "git cherry-pick -x" (with the appropriate configuration
> variable or argument) would, to this commit message:
>
>   commit title
>
>   This is an explanatory paragraph.
>
>   Footer: foo
>
> place the "(cherry picked from ...)" line below "commit title".
>
> Would this be better?

It is not immediately obvious what such a change buys us.  Wouldn't
the current code place that line below "Footer: foo"?  I cannot
think of any reason why anybody would want to place "cherry-picked
from" immediately below the title and before the first line of the
body.


[Footnotes]

*1* http://public-inbox.org/git/1472846322-5592-1-git-send-email-jonathantanmy@google.com/

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 09/29/2016 02:56 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> This is somewhat of a follow-up to my previous e-mail with subject
>> "[PATCH] sequencer: support folding in rfc2822 footer" [1], in which I
>> proposed relaxing the definition of a commit message footer to allow
>> multiple-line field bodies (as described in RFC2822), but its strictness
>> was deemed deliberate.
>
> It does not necessarily mean we can never change it when we did
> something deliberately, though.  With a good enough justification,
> and with a transitition plan if the backward incompatibility is
> severe enough to warrant one, we can change things.
>
> I vaguely recall that there were some discussion on the definition
> of "what's a trailer line" with folks from the kernel land, perhaps
> while discussing the interpret-trailers topic.  IIRC, when somebody
> passes an improved version along, the resulting message's trailer
> block may look like this:
>
>     Signed-off-by: Original Author <original@author.xz>
>     [fixed typo in the variable names]
>     Signed-off-by: Somebhody Else <somebody@else.xz>
>
> and an obvious "wish" of theirs was to treat not just RFC2822-like
> "a line that begins with token followed by a colon" but also these
> short comments as part of the trailer block.  Your original wish in
> [*1*] is to also treat "a line that begin with a whitespace that
> follows a line that begins with token followed by a colon" as part
> of the trailer block and I personally think that is a reasonable
> thing to wish for, too.

If we allowed arbitrary lines in the trailer block, this would solve my 
original problem, yes.

> I recall that I was somewhat surprised and dissapointed to see no
> change to interpret-trailers when you tried [*1*], which was really
> about improving the definition of what the trailer block is, by the
> way.

Sorry, I had missed that.

Looking at that, it seems that sequencer.c started interpreting the last 
paragraph of the commit message as a footer and adding an exception for 
"cherry picked from" in commit b971e04 ("sequencer.c: always separate 
"(cherry picked from" from commit body", 2013-02-12). So the 
interpretations of sequencer.c and interpret-trailers were already 
divergent, but I should have probably at least discussed that.

>> Below is a patch set that allows placing the "cherry picked from" line
>> without taking into account the definition of a commit message footer.
>> For example, "git cherry-pick -x" (with the appropriate configuration
>> variable or argument) would, to this commit message:
>>
>>   commit title
>>
>>   This is an explanatory paragraph.
>>
>>   Footer: foo
>>
>> place the "(cherry picked from ...)" line below "commit title".
>>
>> Would this be better?
>
> It is not immediately obvious what such a change buys us.  Wouldn't
> the current code place that line below "Footer: foo"?  I cannot
> think of any reason why anybody would want to place "cherry-picked
> from" immediately below the title and before the first line of the
> body.

Yes, the current code would place it below "Footer: foo" without a blank 
line before it, but if it thinks that the so-called footer is not 
actually a footer, it would insert a blank line before that line.

As for a reason:

1) I do not have a specific reason for placing it in that exact 
position, but I would like to be able to place the "cherry picked from" 
line without affecting the last paragraph (specifically, without making 
the "cherry picked from" line the only line in the last paragraph). It 
seems to me that placing it below the title was the most straightforward 
way to do that - this way, Git can have its own idea of what a footer 
constitutes, and the user can treat it as completely separate from the 
"cherry picked from" line mechanism.

1a) (Avoiding the footer might also be a good way of more clearly 
defining what the footer is. For example, currently, "cherry picked 
from" is treated as a special case in sequencer.c but not in trailer.c, 
as far as I can tell. If we consistently avoided the footer, we wouldn't 
need such a special case anywhere.)

2) The Linux kernel's repository has some "commit ... upstream." lines 
in this position (below the commit title) - for example, in commit 
dacc0987fd2e.

[1] 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/dacc0987fd2e25a8b4b8c19778862ba12ce76d0a

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> I vaguely recall that there were some discussion on the definition
>> of "what's a trailer line" with folks from the kernel land, perhaps
>> while discussing the interpret-trailers topic.  IIRC, when somebody
>> passes an improved version along, the resulting message's trailer
>> block may look like this:
>>
>>     Signed-off-by: Original Author <original@author.xz>
>>     [fixed typo in the variable names]
>>     Signed-off-by: Somebhody Else <somebody@else.xz>
>>
>> and an obvious "wish" of theirs was to treat not just RFC2822-like
>> "a line that begins with token followed by a colon" but also these
>> short comments as part of the trailer block.  Your original wish in
>> [*1*] is to also treat "a line that begin with a whitespace that
>> follows a line that begins with token followed by a colon" as part
>> of the trailer block and I personally think that is a reasonable
>> thing to wish for, too.
>
> If we allowed arbitrary lines in the trailer block, this would solve
> my original problem, yes.

OK.

> Looking at that, it seems that sequencer.c started interpreting the
> last paragraph of the commit message as a footer and adding an
> exception for "cherry picked from" in commit b971e04 ("sequencer.c:
> always separate "(cherry picked from" from commit body",
> 2013-02-12). So the interpretations of sequencer.c and
> interpret-trailers were already divergent, but I should have probably
> at least discussed that.

It is not too late to discuss it.  I still think it is a good longer
term plan to try to unify the definition of what a trailer block is
and the implementation of the code that determines the boundary
between the log message proper and the trailer block and that allows
us to manipulate the trailer block, that currently is scattered
across multiple places into one.  Historically, "commit -s" had one
(because it needed to decide if it needs to see if the last sign-off
is already the one it is adding, and to decide if a blank line is
needed before the sing-off being added), "am -s" had another, and
"cherry-pick" probably had one, too.  "interpret-trailers" was, at
least originally, envisioned as an effort to develop a unified
machinery that can be called from these codepaths, and to aid the
development and encourage its use, it also had its own end-user
facing command.  Your interest in the "trailer" topic may be a good
trigger for us to further that original vision.

> As for a reason:
>
> 1) I do not have a specific reason for placing it in that exact
> position, but I would like to be able to place the "cherry picked
> from" line without affecting the last paragraph (specifically, without
> making the "cherry picked from" line the only line in the last
> paragraph).
> ...
> 1a) (Avoiding the footer might also be a good way of more clearly
> defining what the footer is. For example, currently, "cherry picked
> from" is treated as a special case in sequencer.c but not in
> trailer.c, as far as I can tell. If we consistently avoided the
> footer, we wouldn't need such a special case anywhere.)

That is one of the numerous shortcomings of the "interpret-trailers"
that is still not finished, I would say.

> 2) The Linux kernel's repository has some "commit ... upstream." lines
> in this position (below the commit title) - for example, in commit
> dacc0987fd2e.

"A group of people seem to prefer it there" does not lead to
"therefore let's move it there for everybody".  It does open a
possibility that we may want to add a new option to put it there,
but does not justify changing what existing "-x" option does.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 09/30/2016 12:34 PM, Junio C Hamano wrote:
>> 2) The Linux kernel's repository has some "commit ... upstream." lines
>> in this position (below the commit title) - for example, in commit
>> dacc0987fd2e.
>
> "A group of people seem to prefer it there" does not lead to
> "therefore let's move it there for everybody".  It does open a
> possibility that we may want to add a new option to put it there,
> but does not justify changing what existing "-x" option does.

To clarify, my patch adds the new option you described (to place it 
below the title instead of at the bottom of the commit message). The 
default is still the current behavior.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> On 09/30/2016 12:34 PM, Junio C Hamano wrote:
>>> 2) The Linux kernel's repository has some "commit ... upstream." lines
>>> in this position (below the commit title) - for example, in commit
>>> dacc0987fd2e.
>>
>> "A group of people seem to prefer it there" does not lead to
>> "therefore let's move it there for everybody".  It does open a
>> possibility that we may want to add a new option to put it there,
>> but does not justify changing what existing "-x" option does.
>
> To clarify, my patch adds the new option you described (to place it
> below the title instead of at the bottom of the commit message). The
> default is still the current behavior.

Ah, sorry, I missed that.  No objection from me on this point then.

Thanks.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

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

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>> I vaguely recall that there were some discussion on the definition
>>> of "what's a trailer line" with folks from the kernel land, perhaps
>>> while discussing the interpret-trailers topic.  IIRC, when somebody
>>> passes an improved version along, the resulting message's trailer
>>> block may look like this:
>>>
>>>     Signed-off-by: Original Author <original@author.xz>
>>>     [fixed typo in the variable names]
>>>     Signed-off-by: Somebhody Else <somebody@else.xz>
>>>
>>> and an obvious "wish" of theirs was to treat not just RFC2822-like
>>> "a line that begins with token followed by a colon" but also these
>>> short comments as part of the trailer block.  Your original wish in
>>> [*1*] is to also treat "a line that begin with a whitespace that
>>> follows a line that begins with token followed by a colon" as part
>>> of the trailer block and I personally think that is a reasonable
>>> thing to wish for, too.
>>
>> If we allowed arbitrary lines in the trailer block, this would solve
>> my original problem, yes.

Here is an experiment I ran during my lunch break.  The script
(attached) is meant to run in the kernel repository and
for each log messages of each non-merge commit:

 * find its last paragraph, where the definition of paragraph is
   simply "a blank/empty line";

 * inspect if there is at least one RFC2822-header-looking line, or
   a line that begins with "(cherry picked from";

 * dump the ones that do not pass the above criteria.

My cursory look of the output did not spot a legitimate trailer
block that we should have identified.  The output lines shown were
ones that are not signed off at all (e.g. af8c34ce6ae32add that says
"Linux 4.7-rc2"), ones that has three-dash line "---" in them
(e.g. 133d558216d9), ones that has diffstat that should have been
after "---" (e.g. 259307074bfcf1f).

The story is the same if you run it in git.git; the "do we have at
least one rfc2822-header-looking line or '(cherry picked from' line
in the last paragraph? if so, then that is an existing trailer
block" seems to be a good heuristics to cover many cases like
these:

    d0196c8d5d3057c5c21a82f3d0113ca8e501033b
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    [tomi.valkeinen@ti.com: resolved conflicts]
    Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

    59f0aa9480cfef9173a648cec4537addc5f3ad94
    Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916
            http://bugzilla.kernel.org/show_bug.cgi?id=10100
            https://lkml.org/lkml/2008/2/25/282
    Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399
            https://bugzilla.kernel.org/show_bug.cgi?id=12461
            https://bugzilla.kernel.org/show_bug.cgi?id=11880
    Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884
            https://bugzilla.kernel.org/show_bug.cgi?id=14081
            https://bugzilla.kernel.org/show_bug.cgi?id=14086
            https://bugzilla.kernel.org/show_bug.cgi?id=14446
    Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911
    Signed-off-by: Lv Zheng <lv.zheng@intel.com>
    Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

-- >8 --
#!/bin/sh

git log --no-merges |
perl -e '
	sub flush {
		my ($commit, @lines) = @_;
		my $seen_good = 0;
		for (@lines) {
			if (/^[-A-Za-z0-9]+: / ||
			    /^\(cherry picked from/) {
				$seen_good = 1;
				last;
			}
		}
		if (!$seen_good) {
			print "\n$commit\n";
			for (@lines) {
				print;
			}
		}
	}

	my (@lines, $this);
	while (<>) {
		if (/^commit (.*)$/) {
			my $next = $1;
			flush($this, @lines);
			@lines = ();
			$this = $next;
		}
		if (s/^    //) {
			if (/^\s*$/) {
				@lines = ();
			} else {
				push @lines, $_;
			}
		}
	}
	if (@lines && $this) {
		flush($this, @lines);
	}
'

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 09/30/2016 01:49 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>>> I vaguely recall that there were some discussion on the definition
>>>> of "what's a trailer line" with folks from the kernel land, perhaps
>>>> while discussing the interpret-trailers topic.  IIRC, when somebody
>>>> passes an improved version along, the resulting message's trailer
>>>> block may look like this:
>>>>
>>>>     Signed-off-by: Original Author <original@author.xz>
>>>>     [fixed typo in the variable names]
>>>>     Signed-off-by: Somebhody Else <somebody@else.xz>
>>>>
>>>> and an obvious "wish" of theirs was to treat not just RFC2822-like
>>>> "a line that begins with token followed by a colon" but also these
>>>> short comments as part of the trailer block.  Your original wish in
>>>> [*1*] is to also treat "a line that begin with a whitespace that
>>>> follows a line that begins with token followed by a colon" as part
>>>> of the trailer block and I personally think that is a reasonable
>>>> thing to wish for, too.
>>>
>>> If we allowed arbitrary lines in the trailer block, this would solve
>>> my original problem, yes.
>
> Here is an experiment I ran during my lunch break.  The script
> (attached) is meant to run in the kernel repository and
> for each log messages of each non-merge commit:
>
>  * find its last paragraph, where the definition of paragraph is
>    simply "a blank/empty line";
>
>  * inspect if there is at least one RFC2822-header-looking line, or
>    a line that begins with "(cherry picked from";
>
>  * dump the ones that do not pass the above criteria.
>
> My cursory look of the output did not spot a legitimate trailer
> block that we should have identified.  The output lines shown were
> ones that are not signed off at all (e.g. af8c34ce6ae32add that says
> "Linux 4.7-rc2"), ones that has three-dash line "---" in them
> (e.g. 133d558216d9), ones that has diffstat that should have been
> after "---" (e.g. 259307074bfcf1f).
>
> The story is the same if you run it in git.git; the "do we have at
> least one rfc2822-header-looking line or '(cherry picked from' line
> in the last paragraph? if so, then that is an existing trailer
> block" seems to be a good heuristics to cover many cases like
> these:
>
>     d0196c8d5d3057c5c21a82f3d0113ca8e501033b
>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>     [tomi.valkeinen@ti.com: resolved conflicts]
>     Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
>     59f0aa9480cfef9173a648cec4537addc5f3ad94
>     Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916
>             http://bugzilla.kernel.org/show_bug.cgi?id=10100
>             https://lkml.org/lkml/2008/2/25/282
>     Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399
>             https://bugzilla.kernel.org/show_bug.cgi?id=12461
>             https://bugzilla.kernel.org/show_bug.cgi?id=11880
>     Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884
>             https://bugzilla.kernel.org/show_bug.cgi?id=14081
>             https://bugzilla.kernel.org/show_bug.cgi?id=14086
>             https://bugzilla.kernel.org/show_bug.cgi?id=14446
>     Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911
>     Signed-off-by: Lv Zheng <lv.zheng@intel.com>
>     Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

That sounds reasonable to me. Would a patch set to implement this new 
trailer block heuristic (in both sequencer and trailer) be reasonable? 
And if yes, should trailer know about the "(cherry picked from" prefix? 
(I can see it both ways - knowing about the "(cherry picked from" prefix 
would make it consistent with sequencer, but it seems like a detail that 
it shouldn't know about. Writing "Cherry-picked-from:" instead probably 
wouldn't solve our problem because, for backwards compatibility, we 
would still need to support reading the old format.)

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> That sounds reasonable to me. Would a patch set to implement this new
> trailer block heuristic (in both sequencer and trailer) be reasonable?
> And if yes, should trailer know about the "(cherry picked from"
> prefix? (I can see it both ways - knowing about the "(cherry picked
> from" prefix would make it consistent with sequencer, but it seems
> like a detail that it shouldn't know about. Writing
> "Cherry-picked-from:" instead probably wouldn't solve our problem
> because, for backwards compatibility, we would still need to support
> reading the old format.)

If we were to go that route, I'd suggest keeping the historical
practice supported, exactly because you would need to be prepared to
cherry-pick an old commit.

It may be necessary for the code to analize the lines in a block
identified as "likely to be a trailing block" more carefully,
though.  The example 59f0aa94 in the message you are responding to
has "Link 1:" that consists of 3 physical lines.  An instruction to
interpret-trailers to add a new one _after_ "Link-$n:" may have to
treat these as a single logical line and do the addition after them,
i.e. before "Link 2:" line, for example.

I also saw

	Signed-off-by: Some body <some@body.xz> (some comment
        that extends to the next line without being indented)
	Sined-off-by: Some body Else <some.body@else.xz>

where the only clue that the second line is logically a part of the
first one was the balancing of parentheses (or [brakets]).  To
accomodate real-world use cases, you may have to take into account a
lot more than the strict rfc-822 style "line folding".

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 10/03/2016 12:17 PM, Junio C Hamano wrote:
> It may be necessary for the code to analize the lines in a block
> identified as "likely to be a trailing block" more carefully,
> though.  The example 59f0aa94 in the message you are responding to
> has "Link 1:" that consists of 3 physical lines.  An instruction to
> interpret-trailers to add a new one _after_ "Link-$n:" may have to
> treat these as a single logical line and do the addition after them,
> i.e. before "Link 2:" line, for example.
>
> I also saw
>
> 	Signed-off-by: Some body <some@body.xz> (some comment
>         that extends to the next line without being indented)
> 	Sined-off-by: Some body Else <some.body@else.xz>
>
> where the only clue that the second line is logically a part of the
> first one was the balancing of parentheses (or [brakets]).  To
> accomodate real-world use cases, you may have to take into account a
> lot more than the strict rfc-822 style "line folding".

If we define a logical trailer line as the set of physical lines until 
the next trailer line...it is true that this has some precedence in that 
such lines can be written (although this might not be intentional):

   $ git interpret-trailers --trailer "a=foo
   bar" </dev/null

   a: foo
   bar

This solves the "Line 1:" case above, but raises other issues:

1. Checking for existence (trailer.ifexists) is now conceptually more 
difficult. For example, handling of inner whitespace in between lines 
might be confusing (currently, there is only one line, and whitespace 
handling is clearly described).
2. Replacement (trailer.ifexists=replace) might replace more than expected.
3. There is a corner case - the first line might not be a trailer line. 
Even if interpret-trailers knew about "(cherry picked from", a user 
might enter something else there. (We could just declare such blocks as 
non-trailers, but if we are already loosening the definition of a 
trailer, this might be something that we should allow.)

My initial thought was to think of a trailer as a block of trailer lines 
possibly interspersed with other lines. This leads to interpret-trailers 
placing the trailer in the wrong place if trailer.where=after, as you 
describe, but this might not be a big problem if trailer.where=after is 
not widely used (and it is not the default). (The other options behave 
as expected.)

There are other options like checking for indentation or checking for 
balanced parentheses/brackets, but I think that these would lead to 
surprising behavior for the user (this would mean that whitespace or 
certain characters could turn a valid trailer into an invalid one or 
vice versa, or change the behavior of trailer.ifexists, especially 
"replace").

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> There are other options like checking for indentation or checking for
> balanced parentheses/brackets, but I think that these would lead to
> surprising behavior for the user (this would mean that whitespace or
> certain characters could turn a valid trailer into an invalid one or
> vice versa, or change the behavior of trailer.ifexists, especially
> "replace").

Yes, that is exactly why I said that it may be necessary for the
code to analize the lines in a block identified as "likely to be a
trailing block" more carefully.  We can afford to be loose as long
as the only allowed operation is to append one at the end, but once
we start removing/replacing an existing entry, etc., the definition
of what an entry is becomes very much relevant.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 10/03/2016 03:13 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> There are other options like checking for indentation or checking for
>> balanced parentheses/brackets, but I think that these would lead to
>> surprising behavior for the user (this would mean that whitespace or
>> certain characters could turn a valid trailer into an invalid one or
>> vice versa, or change the behavior of trailer.ifexists, especially
>> "replace").
>
> Yes, that is exactly why I said that it may be necessary for the
> code to analize the lines in a block identified as "likely to be a
> trailing block" more carefully.  We can afford to be loose as long
> as the only allowed operation is to append one at the end, but once
> we start removing/replacing an existing entry, etc., the definition
> of what an entry is becomes very much relevant.

I agree, and I was trying to discuss the possible alternatives for the 
definition of what an entry is in my previous e-mail.

If you think that the alternatives are still too loose, I'm not sure if 
we can make it any tighter. As far as I know, we're dealing with 
trailers like the following:

   Signed-off-by: A <author@example.com>
   [This has nothing to do with the above line]
   Signed-off-by: B <buthor@example.com>

and:

   Link 1: a link
     a continuation of the above

and:

   Signed-off-by: Some body <some@body.xz> (comment
   on two lines)

As I stated in the quoted paragraph, one possibility is to use 
indentation and/or balanced parentheses/brackets to determine if a 
trailer line continues onto the next line, and this would handle all the 
above cases, but I still think that these would lead to surprising 
behavior. Hence my suggestion to just simply define it as a single 
physical line. But if you think that the pros (of the more complicated 
approach) outweigh the cons, I'm OK with that.

One alternative is to postpone this decision by changing sequencer only 
(and not trailer) to tolerate other lines in the trailer. This would 
make them even more divergent (sequencer supports arbitrary lines while 
trailer doesn't), but they were divergent already (sequencer supports 
"(cherry picked by" but trailer doesn't).

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> One alternative is to postpone this decision by changing sequencer
> only (and not trailer) to tolerate other lines in the trailer. This
> would make them even more divergent (sequencer supports arbitrary
> lines while trailer doesn't), but they were divergent already
> (sequencer supports "(cherry picked by" but trailer doesn't).

Given that we internally do not use the "trailers" for anything
real, anything you decide to do here would be an improvement ;-)
Before, users couldn't even get any of the examples (below, from
your message) recognized as trailer blocks.

>   Signed-off-by: A <author@example.com>
>   [This has nothing to do with the above line]
>   Signed-off-by: B <buthor@example.com>
>
> and:
>
>   Link 1: a link
>     a continuation of the above
>
> and:
>
>   Signed-off-by: Some body <some@body.xz> (comment
>   on two lines)

So I would say it is perfectly OK if your update works only for
cases we can clearly define the semantics for.  For example, we can
even start with something simple like:

 * A RFC822-header like line, together with any number of whitespace
   indented lines that immediately follow it, will be taken as a
   single logical trailer element (with embedded LF in it if it uses
   the "line folding").  For the purpose of "replace", the entire
   single logical trailer element is replaced.

 * A line that begins with "(cherry picked from" and "[" becomes a
   single logical trailer element.  No continuation of anything
   fancy.

 * A line with any other shape is a garbage line in a trailer
   block.  It is kept in its place, but because it does not even
   have <token> part, it will not participate in locating with
   "trailer.where", "trailer.ifexists", etc.

A block of lines that appear as the last paragraph in a commit
message is a trailer block if and only if certain number or
percentage of lines are non-garbage lines according to the above
definition.

The operations done by the codepaths in the core part of the system
are much simpler subset of what "interpret-trailers" wants to
support, namely, 

 - append "(cherry picked from X)" at the end.

 - append "S-o-b:" at the end.

 - append "S-o-b:" unless the same line appears as the last line in
   the existing trailer block.

and these are quite compatible with a simplified definition of what
a logical line is illustrated in the above example, I would think.

I wonder if we can share a new helper function to do the detection
(and classification) of a trailer block and parsing the logical
lines out of a commit log message.  The function signature could be
as simple as taking a single <const char *> (or a strbuf) that holds
a commit log message, and splitting it out into something like:

    struct {
	const char *whole;
	const char *end_of_message_proper;
	struct {
		const char *token;
		const char *contents;
	} *trailer;
	int alloc_trailers, nr_trailers;
    };

where 

 - whole points at the first byte of the input, i.e. the beginning
   of the commit message buffer.

 - end-of-message-proper points at the first byte of the trailer
   block into the buffer at "whole".

 - token is a canonical header name for easy comparison for
   interpret-trailers (you can use NULL for garbage lines, and made
   up token like "[bracket]" and "(cherrypick)" that would not clash
   with real tokens like "Signed-off-by").

 - contents is the bytes on the logical line, including the header
   part

E.g. an element in trailer[] array may say

    {
	.token = "Signed-off-by",
        .contents = "Signed-Off-By: Some Body <some@body.xz>\n",
    }

With something like that, you can manipulate the "insert at ...",
"replace", etc. in the trailer[] array and then produce an updated
commit message fairly easily (i.e. copy out the bytes beginning at
"whole" up to "end_of_message_proper", then iterate over trailer[]
array and show their contents field).  The codepaths in the core
part only need to know 

 - how to check the last item in trailer[] array to see if it ends
   with the same sign-off as they are trying to add.

 - how to append one new element to the trailer[] array.

 - reproduce an updated commit log message after the above.

Hmm?

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

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

> A block of lines that appear as the last paragraph in a commit
> message is a trailer block if and only if certain number or
> percentage of lines are non-garbage lines according to the above
> definition.
> ...
> I wonder if we can share a new helper function to do the detection
> (and classification) of a trailer block and parsing the logical
> lines out of a commit log message.  The function signature could be
> as simple as taking a single <const char *> (or a strbuf) that holds
> a commit log message, and splitting it out into something like:
>
>     struct {
> 	const char *whole;
> 	const char *end_of_message_proper;
> 	struct {
> 		const char *token;
> 		const char *contents;
> 	} *trailer;
> 	int alloc_trailers, nr_trailers;
>     };
>
> where 
> ...

An addendum.  We may also want to be prepared to accept an input
that has some garbage lines _after_ the trailer block, if we can
clearly identify them as such.  For example, we could change the
definition of "the last paragraph" as "the block of lines that
do not have any empty (or blank) line, that appear either at the end
of the input, or immediately before three-dash lines", to allow

    commit title
    
    explanation of the change

    Signed-off-by: Some Body <some@body.xz>
    [some other things]
    Acked-by: Some Other Person <some@other.xz>

    ---
     additional comment

which (unfortunately) is a rather common pattern for people who plan
to send the commit over e-mail.

If we add a new field "const char *beginning_of_tail_garbage" next
to "end_of_message_proper" that points at the blank line before the
three-dash line in the above example, the parser should be able to
break such an input into a parsed form, allow the trailer[] array to
be manipulated and reproduce a commit log message.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 10/04/2016 11:28 AM, Junio C Hamano wrote:
> An addendum.  We may also want to be prepared to accept an input
> that has some garbage lines _after_ the trailer block, if we can
> clearly identify them as such.  For example, we could change the
> definition of "the last paragraph" as "the block of lines that
> do not have any empty (or blank) line, that appear either at the end
> of the input, or immediately before three-dash lines", to allow
>
>     commit title
>
>     explanation of the change
>
>     Signed-off-by: Some Body <some@body.xz>
>     [some other things]
>     Acked-by: Some Other Person <some@other.xz>
>
>     ---
>      additional comment
>
> which (unfortunately) is a rather common pattern for people who plan
> to send the commit over e-mail.
>
> If we add a new field "const char *beginning_of_tail_garbage" next
> to "end_of_message_proper" that points at the blank line before the
> three-dash line in the above example, the parser should be able to
> break such an input into a parsed form, allow the trailer[] array to
> be manipulated and reproduce a commit log message.

How important is this feature? It doesn't seem too difficult to add, 
although it does break compatibility (in particular, "--signoff" must 
now be documented as "after the last trailer" instead of "at the end of 
the commit message").

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> How important is this feature? It doesn't seem too difficult to add,
> although it does break compatibility (in particular, "--signoff" must
> now be documented as "after the last trailer" instead of "at the end
> of the commit message").

The sign-off has always been meant to be appended at the end of the
trailer block, so there is no "compatibility" issue.  It's just how
you phrase the explanation of the behaviour, I would think.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Jonathan Tan]

On 10/04/2016 10:25 AM, Junio C Hamano wrote:
> So I would say it is perfectly OK if your update works only for
> cases we can clearly define the semantics for.  For example, we can
> even start with something simple like:
>
>  * A RFC822-header like line, together with any number of whitespace
>    indented lines that immediately follow it, will be taken as a
>    single logical trailer element (with embedded LF in it if it uses
>    the "line folding").  For the purpose of "replace", the entire
>    single logical trailer element is replaced.
>
>  * A line that begins with "(cherry picked from" and "[" becomes a
>    single logical trailer element.  No continuation of anything
>    fancy.
>
>  * A line with any other shape is a garbage line in a trailer
>    block.  It is kept in its place, but because it does not even
>    have <token> part, it will not participate in locating with
>    "trailer.where", "trailer.ifexists", etc.

Sounds reasonable to me. Would the "[" be a bit of overspecification, 
though, since Git doesn't produce it? Also, identifying it as a garbage 
line probably wouldn't change any behavior - in the Linux kernel 
examples, it is used to show what happened in between sign-offs, so 
there will always be one "Signed-off-by:" at the top.  (But I do not 
feel strongly about this.)

> A block of lines that appear as the last paragraph in a commit
> message is a trailer block if and only if certain number or
> percentage of lines are non-garbage lines according to the above
> definition.

I think the number should be 1 - that seems like the easiest to explain. 
But I'm OK with other suggestions.

> I wonder if we can share a new helper function to do the detection
> (and classification) of a trailer block and parsing the logical
> lines out of a commit log message.  The function signature could be
> as simple as taking a single <const char *> (or a strbuf) that holds
> a commit log message, and splitting it out into something like:
>
>     struct {
> 	const char *whole;
> 	const char *end_of_message_proper;
> 	struct {
> 		const char *token;
> 		const char *contents;
> 	} *trailer;
> 	int alloc_trailers, nr_trailers;
>     };
>
> where
>
>  - whole points at the first byte of the input, i.e. the beginning
>    of the commit message buffer.
>
>  - end-of-message-proper points at the first byte of the trailer
>    block into the buffer at "whole".
>
>  - token is a canonical header name for easy comparison for
>    interpret-trailers (you can use NULL for garbage lines, and made
>    up token like "[bracket]" and "(cherrypick)" that would not clash
>    with real tokens like "Signed-off-by").
>
>  - contents is the bytes on the logical line, including the header
>    part
>
> E.g. an element in trailer[] array may say
>
>     {
> 	.token = "Signed-off-by",
>         .contents = "Signed-Off-By: Some Body <some@body.xz>\n",
>     }

I get the impression from the rest of your e-mail that no strings are 
meant to be copied - is that true? (That sounds like a good idea to me.) 
In which case this might be better:

   struct {
     const char *first_trailer; /* = end_of_message_proper */
     struct {
       const char *start;
       const char *value;
       const char *end;
     } *trailers;
     int trailers_nr, trailers_alloc;
   };

start = value for "[", "(cherry picked from" and garbage lines. We also 
need end because there is no \0 there (we didn't copy any strings).

The existing code (in trailer.c) uses a linked list to store trailers, 
but an array (as written in your e-mail) is probably better for us since 
clients would want to access the last element (as also written in your 
e-mail).

> With something like that, you can manipulate the "insert at ...",
> "replace", etc. in the trailer[] array and then produce an updated
> commit message fairly easily (i.e. copy out the bytes beginning at
> "whole" up to "end_of_message_proper", then iterate over trailer[]
> array and show their contents field).  The codepaths in the core
> part only need to know
>
>  - how to check the last item in trailer[] array to see if it ends
>    with the same sign-off as they are trying to add.
>
>  - how to append one new element to the trailer[] array.
>
>  - reproduce an updated commit log message after the above.

I don't think we need trailer block struct -> commit message conversion 
- when adding a new trailer or replacing an existing trailer, the client 
code can just remember the index and then modify its behavior 
accordingly when iterating through all trailers. But this conversion can 
be easily added if/when we need it.

 > Hmm?

Overall, this seems like a good idea - I'll go ahead and do this if 
there are no other objections.

It just occurred to me that there could be some corner cases when the 
trailer separator is configured to not include ":" - I'll make sure to 
include tests that check those corner cases.

Re: [RFC/PATCH 0/2] place cherry pick line below commit title

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Sounds reasonable to me. Would the "[" be a bit of overspecification,
> though, since Git doesn't produce it? Also, identifying it as a
> garbage line probably wouldn't change any behavior - in the Linux
> kernel examples, it is used to show what happened in between
> sign-offs, so there will always be one "Signed-off-by:" at the top.

Good thinking.  As "interpret trailers" cannot locate such a line to
manipulate (as it lacks <token>), we can simply treat it as a
garbage line.

>>     struct {
>> 	const char *whole;
>> 	const char *end_of_message_proper;
>> 	struct {
>> 		const char *token;
>> 		const char *contents;
>> 	} *trailer;
>> 	int alloc_trailers, nr_trailers;
>>     };
>>
>> where
>>
>>  - whole points at the first byte of the input, i.e. the beginning
>>    of the commit message buffer.
>>
>>  - end-of-message-proper points at the first byte of the trailer
>>    block into the buffer at "whole".
>>
>>  - token is a canonical header name for easy comparison for
>>    interpret-trailers (you can use NULL for garbage lines, and made
>>    up token like "[bracket]" and "(cherrypick)" that would not clash
>>    with real tokens like "Signed-off-by").
>>
>>  - contents is the bytes on the logical line, including the header
>>    part
>>
>> E.g. an element in trailer[] array may say
>>
>>     {
>> 	.token = "Signed-off-by",
>>         .contents = "Signed-Off-By: Some Body <some@body.xz>\n",
>>     }
>
> I get the impression from the rest of your e-mail that no strings are
> meant to be copied - is that true? (That sounds like a good idea to
> me.)

I was envisioning that "whole", "end-of-message" can point into the
input buffer, while trailer[].contents may have to be copied, if
only to make it to a NUL-terminated string.  But I am fine with
<ptr,len> or <begin,end> pair to avoid copying .contents if that is
desired.  You'd need to worry about differentiating .contents that
need to be freed after "interpret trailers" inserted a new entry or
replaced the contents, though.