[PATCH 0/6] Bring "format-patch --notes" closer to a real feature

[PATCH 0/6] Bring "format-patch --notes" closer to a real feature

[Junio C Hamano]

This replaces the earlier "wip" with a real thing.

We never advertised the "--notes" option to format-patch (or
anything related to the pretty format options for that matter)
because the behaviour of these options was whatever they happened to
do, not what they were designed to do.

It had a few obvious glitches:

 * The notes section was appended immediately after the log message,
   and then the three-dash line was added.  Such a supplimental
   material should come after the three-dash line.

 * The logic to append a new sign-off with "format-patch --signoff"
   worked on the message after the note was added, which made the
   detection of existing sign-off lines incorrect.

This updates the handling of "--notes" option to correct these, in
an attempt to bring it closer to a real feature.

Junio C Hamano (6):
  pretty: remove reencode_commit_message()
  format_note(): simplify API
  pretty: prepare notes message at a centralized place
  pretty_print_commit(): do not append notes message
  format-patch: append --signature after notes
  format-patch --notes: show notes after three-dashes

 builtin/blame.c         |  5 +++--
 commit.h                |  4 +---
 log-tree.c              | 32 ++++++++++++++++++++++++++++----
 notes.c                 | 13 +++++++------
 notes.h                 |  6 +-----
 pretty.c                | 22 ++++------------------
 revision.c              |  2 +-
 revision.h              |  1 +
 t/t4014-format-patch.sh | 15 +++++++++++++--
 9 files changed, 59 insertions(+), 41 deletions(-)

-- 
1.8.0.rc3.112.gdb88a5e

[PATCH 1/6] pretty: remove reencode_commit_message()

[Junio C Hamano]

This function has only two callsites, and is a thin wrapper whose
usefulness is dubious.  When the caller needs to learn the log
output encoding, it should be able to do so by directly calling
get_log_output_encoding() and calling the underlying
logmsg_reencode() with it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/blame.c |  5 +++--
 commit.h        |  2 --
 pretty.c        | 13 ++-----------
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index c27ef21..cfae569 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1425,7 +1425,7 @@ static void get_commit_info(struct commit *commit,
 			    int detailed)
 {
 	int len;
-	const char *subject;
+	const char *subject, *encoding;
 	char *reencoded, *message;
 	static char author_name[1024];
 	static char author_mail[1024];
@@ -1446,7 +1446,8 @@ static void get_commit_info(struct commit *commit,
 			die("Cannot read commit %s",
 			    sha1_to_hex(commit->object.sha1));
 	}
-	reencoded = reencode_commit_message(commit, NULL);
+	encoding = get_log_output_encoding();
+	reencoded = logmsg_reencode(commit, encoding);
 	message   = reencoded ? reencoded : commit->buffer;
 	ret->author = author_name;
 	ret->author_mail = author_mail;
diff --git a/commit.h b/commit.h
index 9f21313..a822af8 100644
--- a/commit.h
+++ b/commit.h
@@ -99,8 +99,6 @@ extern int has_non_ascii(const char *text);
 struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 extern char *logmsg_reencode(const struct commit *commit,
 			     const char *output_encoding);
-extern char *reencode_commit_message(const struct commit *commit,
-				     const char **encoding_p);
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
diff --git a/pretty.c b/pretty.c
index 8b1ea9f..c311a68 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1341,16 +1341,6 @@ void pp_remainder(const struct pretty_print_context *pp,
 	}
 }
 
-char *reencode_commit_message(const struct commit *commit, const char **encoding_p)
-{
-	const char *encoding;
-
-	encoding = get_log_output_encoding();
-	if (encoding_p)
-		*encoding_p = encoding;
-	return logmsg_reencode(commit, encoding);
-}
-
 void pretty_print_commit(const struct pretty_print_context *pp,
 			 const struct commit *commit,
 			 struct strbuf *sb)
@@ -1367,7 +1357,8 @@ void pretty_print_commit(const struct pretty_print_context *pp,
 		return;
 	}
 
-	reencoded = reencode_commit_message(commit, &encoding);
+	encoding = get_log_output_encoding();
+	reencoded = logmsg_reencode(commit, encoding);
 	if (reencoded) {
 		msg = reencoded;
 	}
-- 
1.8.0.rc3.112.gdb88a5e

[PATCH 2/6] format_note(): simplify API

[Junio C Hamano]

We either stuff the notes message without modification for %N
userformat, or format it for human consumption.  Using two bits
is an overkill that does not benefit anybody.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 notes.c    | 13 +++++++------
 notes.h    |  6 +-----
 pretty.c   |  5 ++---
 revision.c |  2 +-
 4 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/notes.c b/notes.c
index bc454e1..97097db 100644
--- a/notes.c
+++ b/notes.c
@@ -1204,10 +1204,11 @@ void free_notes(struct notes_tree *t)
  * If the given notes_tree is NULL, the internal/default notes_tree will be
  * used instead.
  *
- * 'flags' is a bitwise combination of the flags for format_display_notes.
+ * (raw != 0) gives the %N userformat; otherwise, the note message is given
+ * for human consumption.
  */
 static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
-			struct strbuf *sb, const char *output_encoding, int flags)
+			struct strbuf *sb, const char *output_encoding, int raw)
 {
 	static const char utf8[] = "utf-8";
 	const unsigned char *sha1;
@@ -1244,7 +1245,7 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 	if (msglen && msg[msglen - 1] == '\n')
 		msglen--;
 
-	if (flags & NOTES_SHOW_HEADER) {
+	if (!raw) {
 		const char *ref = t->ref;
 		if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
 			strbuf_addstr(sb, "\nNotes:\n");
@@ -1260,7 +1261,7 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 	for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {
 		linelen = strchrnul(msg_p, '\n') - msg_p;
 
-		if (flags & NOTES_INDENT)
+		if (!raw)
 			strbuf_addstr(sb, "    ");
 		strbuf_add(sb, msg_p, linelen);
 		strbuf_addch(sb, '\n');
@@ -1270,13 +1271,13 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 }
 
 void format_display_notes(const unsigned char *object_sha1,
-			  struct strbuf *sb, const char *output_encoding, int flags)
+			  struct strbuf *sb, const char *output_encoding, int raw)
 {
 	int i;
 	assert(display_notes_trees);
 	for (i = 0; display_notes_trees[i]; i++)
 		format_note(display_notes_trees[i], object_sha1, sb,
-			    output_encoding, flags);
+			    output_encoding, raw);
 }
 
 int copy_note(struct notes_tree *t,
diff --git a/notes.h b/notes.h
index 3592b19..3324c48 100644
--- a/notes.h
+++ b/notes.h
@@ -237,10 +237,6 @@ void prune_notes(struct notes_tree *t, int flags);
  */
 void free_notes(struct notes_tree *t);
 
-/* Flags controlling how notes are formatted */
-#define NOTES_SHOW_HEADER 1
-#define NOTES_INDENT 2
-
 struct string_list;
 
 struct display_notes_opt {
@@ -274,7 +270,7 @@ void init_display_notes(struct display_notes_opt *opt);
  * You *must* call init_display_notes() before using this function.
  */
 void format_display_notes(const unsigned char *object_sha1,
-			  struct strbuf *sb, const char *output_encoding, int flags);
+			  struct strbuf *sb, const char *output_encoding, int raw);
 
 /*
  * Load the notes tree from each ref listed in 'refs'.  The output is
diff --git a/pretty.c b/pretty.c
index c311a68..735cf0f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1035,7 +1035,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 	case 'N':
 		if (c->pretty_ctx->show_notes) {
 			format_display_notes(commit->object.sha1, sb,
-				    get_log_output_encoding(), 0);
+					     get_log_output_encoding(), 1);
 			return 1;
 		}
 		return 0;
@@ -1419,8 +1419,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,
 		strbuf_addch(sb, '\n');
 
 	if (pp->show_notes)
-		format_display_notes(commit->object.sha1, sb, encoding,
-				     NOTES_SHOW_HEADER | NOTES_INDENT);
+		format_display_notes(commit->object.sha1, sb, encoding, 0);
 
 	free(reencoded);
 }
diff --git a/revision.c b/revision.c
index a09e60b..ddfba11 100644
--- a/revision.c
+++ b/revision.c
@@ -2236,7 +2236,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 		if (!buf.len)
 			strbuf_addstr(&buf, commit->buffer);
 		format_display_notes(commit->object.sha1, &buf,
-				     get_log_output_encoding(), 0);
+				     get_log_output_encoding(), 1);
 	}
 
 	/* Find either in the commit object, or in the temporary */
-- 
1.8.0.rc3.112.gdb88a5e

[PATCH 3/6] pretty: prepare notes message at a centralized place

[Junio C Hamano]

Instead of passing a boolean show_notes around, pass an optional
string that is to be inserted after the log message proper is shown.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.h   |  2 +-
 log-tree.c | 14 +++++++++++++-
 pretty.c   |  9 ++++-----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/commit.h b/commit.h
index a822af8..7b43e45 100644
--- a/commit.h
+++ b/commit.h
@@ -86,7 +86,7 @@ struct pretty_print_context {
 	enum date_mode date_mode;
 	unsigned date_mode_explicit:1;
 	int need_8bit_cte;
-	int show_notes;
+	char *notes_message;
 	struct reflog_walk_info *reflog_info;
 	const char *output_encoding;
 };
diff --git a/log-tree.c b/log-tree.c
index c894930..84e9f5b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -540,7 +540,6 @@ void show_log(struct rev_info *opt)
 	struct pretty_print_context ctx = {0};
 
 	opt->loginfo = NULL;
-	ctx.show_notes = opt->show_notes;
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
 
@@ -648,6 +647,18 @@ void show_log(struct rev_info *opt)
 	if (!commit->buffer)
 		return;
 
+	if (opt->show_notes) {
+		int raw;
+		struct strbuf notebuf = STRBUF_INIT;
+
+		raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
+		format_display_notes(commit->object.sha1, &notebuf,
+				     get_log_output_encoding(), raw);
+		ctx.notes_message = notebuf.len
+			? strbuf_detach(&notebuf, NULL)
+			: xcalloc(1, 1);
+	}
+
 	/*
 	 * And then the pretty-printed message itself
 	 */
@@ -689,6 +700,7 @@ void show_log(struct rev_info *opt)
 	}
 
 	strbuf_release(&msgbuf);
+	free(ctx.notes_message);
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
diff --git a/pretty.c b/pretty.c
index 735cf0f..a53eb53 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1033,9 +1033,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		if (c->pretty_ctx->show_notes) {
-			format_display_notes(commit->object.sha1, sb,
-					     get_log_output_encoding(), 1);
+		if (c->pretty_ctx->notes_message) {
+			strbuf_addstr(sb, c->pretty_ctx->notes_message);
 			return 1;
 		}
 		return 0;
@@ -1418,8 +1417,8 @@ void pretty_print_commit(const struct pretty_print_context *pp,
 	if (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (pp->show_notes)
-		format_display_notes(commit->object.sha1, sb, encoding, 0);
+	if (pp->notes_message && *pp->notes_message)
+		strbuf_addstr(sb, pp->notes_message);
 
 	free(reencoded);
 }
-- 
1.8.0.rc3.112.gdb88a5e

Re: [PATCH 3/6] pretty: prepare notes message at a centralized place

[Jeff King]

On Wed, Oct 17, 2012 at 10:45:25PM -0700, Junio C Hamano wrote:

> +	if (opt->show_notes) {
> +		int raw;
> +		struct strbuf notebuf = STRBUF_INIT;
> +
> +		raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
> +		format_display_notes(commit->object.sha1, &notebuf,
> +				     get_log_output_encoding(), raw);
> +		ctx.notes_message = notebuf.len
> +			? strbuf_detach(&notebuf, NULL)
> +			: xcalloc(1, 1);
> +	}

This last line seems like it is caused by a bug in the strbuf API.
Detaching an empty string will sometimes get you NULL and sometimes not.
For example, this:

  struct strbuf foo = STRBUF_INIT;
  strbuf_detach(&foo, NULL);

will return NULL. But this:

  struct strbuf foo = STRBUF_INIT;
  strbuf_addstr(&foo, "bar");
  strbuf_reset(&foo);
  strbuf_detach(&foo, NULL);

will get you a zero-length string. Which just seems insane to me. The
whole point of strbuf_detach is that you do not have to care about the
internal representation. It should probably always return a newly
allocated zero-length string.

Looking through a few uses of strbuf_detach, it looks like callers
assume they will always get a pointer from strbuf_detach, and we are
saved by implementation details. For example, sha1_file_to_archive might
have an empty file, but the fact that strbuf_attach always allocates a
byte means that the detach will never return NULL. Similarly,
argv_array_pushf would never want to turn an empty string into an
accidental NULL; it is saved by the fact that strbuf_vaddf will always
preemptively allocate 64 bytes.

It's possible that switching it would create bugs elsewhere (there are
over 100 uses of strbuf_detach, so maybe somebody really does want this
NULL behavior), but I tend to think it is just as likely to be fixing
undiscovered bugs.

-Peff

Re: [PATCH 3/6] pretty: prepare notes message at a centralized place

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's possible that switching it would create bugs elsewhere (there are
> over 100 uses of strbuf_detach, so maybe somebody really does want this
> NULL behavior), but I tend to think it is just as likely to be fixing
> undiscovered bugs.

Yeah, I tend to agree.

This "format-patch --notes" is obviously a post 1.8.0 topic, and so
is the strbuf_detach() clean-up.  Let me bookmark this thread in
case it hasn't been resolved when I came back from my vacation, so
that I won't forget ;-).

Re: [PATCH 3/6] pretty: prepare notes message at a centralized place

[Jeff King]

On Thu, Oct 18, 2012 at 02:17:01AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It's possible that switching it would create bugs elsewhere (there are
> > over 100 uses of strbuf_detach, so maybe somebody really does want this
> > NULL behavior), but I tend to think it is just as likely to be fixing
> > undiscovered bugs.
> 
> Yeah, I tend to agree.
> 
> This "format-patch --notes" is obviously a post 1.8.0 topic, and so
> is the strbuf_detach() clean-up.  Let me bookmark this thread in
> case it hasn't been resolved when I came back from my vacation, so
> that I won't forget ;-).

Actually, I have found a few segfaults, one of them remotely triggerable
in http-backend. I think it can probably wait until post-1.8.0 as it
does not have any security implications, though.

Details in a moment.

-Peff

[PATCH] strbuf: always return a non-NULL value from strbuf_detach

[Jeff King]

On Thu, Oct 18, 2012 at 03:49:21AM -0400, Jeff King wrote:

> This last line seems like it is caused by a bug in the strbuf API.
> Detaching an empty string will sometimes get you NULL and sometimes not.
> For example, this:
> 
>   struct strbuf foo = STRBUF_INIT;
>   strbuf_detach(&foo, NULL);
> 
> will return NULL. But this:
> 
>   struct strbuf foo = STRBUF_INIT;
>   strbuf_addstr(&foo, "bar");
>   strbuf_reset(&foo);
>   strbuf_detach(&foo, NULL);
> 
> will get you a zero-length string. Which just seems insane to me. The
> whole point of strbuf_detach is that you do not have to care about the
> internal representation. It should probably always return a newly
> allocated zero-length string.
> [...]
> It's possible that switching it would create bugs elsewhere (there are
> over 100 uses of strbuf_detach, so maybe somebody really does want this
> NULL behavior), but I tend to think it is just as likely to be fixing
> undiscovered bugs.

So I just read through all 108 grep hits for strbuf_detach. In almost
every case, the NULL return is not triggerable, because we do _some_
strbuf operation. And even if it is empty, like strbuf_read from an
empty source, or strbuf_addstr on an empty string, we still end up
calling `strbuf_grow(sb, 0)`, which will allocate.

There are a few cases where there are code paths where we really might
not add anything to the strbuf, and changing strbuf_detach would have an
impact:

  In log.c:cmd_format_patch, creating the rev.extra_headers string from
  the individual headers currently ends up NULL when you have no such
  headers. But it ends up not mattering if we have NULL or an empty
  string, since all code paths just end up appending it to our headers
  anyway, and the empty string is a noop.

  In commit.c:read_commit_extra_header_lines, a commit without a value
  will get a NULL value instead of the empty string. But we end up not
  dereferencing the NULL, because it just gets fed to add_extra_header
  later, which will only look at the value if its length parameter is
  non-zero. So it is built to expect the current behavior, but would be
  fine with a switch.

  In http-push.c:xml_entities, we will return NULL if fed an empty
  string. You can dereference NULL by doing "git http-push ''", although
  on glibc systems it will not segfault, because we feed the NULL to
  printf, which converts it to "(null)".

  In http-backend.c:get_parameters, we call url_decode_parameter_* to
  look at the contents of $QUERY_STRING.  These functions can return
  NULL via strbuf_detach if fed an empty string. We are ready to handle
  a NULL value like "empty=&other=value". But not an empty name, like
  "one=1&&two=2" (note the bogus double-& which yields an empty
  parameter). You can get a segfault by sending that to a smart-http
  server.

Probably more detail than you wanted, but I'm pretty confident now that
we should switch it, and that it won't cause any regressions. Patch is
below.

-- >8 --
Subject: strbuf: always return a non-NULL value from strbuf_detach

The current behavior is to return NULL when strbuf did not
actually allocate a string. This can be quite surprising to
callers, though, who may feed the strbuf from arbitrary data
and expect to always get a valid value.

In most cases, it does not make a difference because calling
any strbuf function will cause an allocation (even if the
function ends up not inserting any data). But if the code is
structured like:

  struct strbuf buf = STRBUF_INIT;
  if (some_condition)
	  strbuf_addstr(&buf, some_string);
  return strbuf_detach(&buf, NULL);

then you may or may not return NULL, depending on the
condition. This can cause us to segfault in http-push
(when fed an empty URL) and in http-backend (when an empty
parameter like "foo=bar&&" is in the $QUERY_STRING).

This patch forces strbuf_detach to allocate an empty
NUL-terminated string when it is called on a strbuf that has
not been allocated.

I investigated all call-sites of strbuf_detach. The majority
are either not affected by the change (because they call a
strbuf_* function unconditionally), or can handle the empty
string just as easily as NULL.

Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index 0510f76..4b9e30c 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -44,7 +44,9 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
 
 char *strbuf_detach(struct strbuf *sb, size_t *sz)
 {
-	char *res = sb->alloc ? sb->buf : NULL;
+	char *res;
+	strbuf_grow(sb, 0);
+	res = sb->buf;
 	if (sz)
 		*sz = sb->len;
 	strbuf_init(sb, 0);
-- 
1.8.0.rc3.3.gba630e1

[PATCH 4/6] pretty_print_commit(): do not append notes message

[Junio C Hamano]

The only case pretty_print_commit() appends notes message to the log
message taken from the commit is when show_log() calls it with the
notes_message field set, and the output format is not the userformat
(i.e. when substituting "%N").  No other users of this function sets
this field in the pretty_print_context, as can be easily verified in
the previous step.

Hoist the code to append the notes message to the caller.

Up to this point, no functionality change is intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 log-tree.c | 3 +++
 pretty.c   | 3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 84e9f5b..e7e08f4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -672,6 +672,9 @@ void show_log(struct rev_info *opt)
 	ctx.reflog_info = opt->reflog_info;
 	ctx.fmt = opt->commit_format;
 	pretty_print_commit(&ctx, commit, &msgbuf);
+	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
+	    ctx.notes_message && *ctx.notes_message)
+		strbuf_addstr(&msgbuf, ctx.notes_message);
 
 	if (opt->add_signoff)
 		append_signoff(&msgbuf, opt->add_signoff);
diff --git a/pretty.c b/pretty.c
index a53eb53..1925e9c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1417,9 +1417,6 @@ void pretty_print_commit(const struct pretty_print_context *pp,
 	if (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (pp->notes_message && *pp->notes_message)
-		strbuf_addstr(sb, pp->notes_message);
-
 	free(reencoded);
 }
 
-- 
1.8.0.rc3.112.gdb88a5e

[PATCH 5/6] format-patch: append --signature after notes

[Junio C Hamano]

When appending a new signature with "format-patch --signature", if
the "--notes" option is also in effect, the location of the new
signature (and if the signature should be added in the first place)
should be decided using the contents of the original commit log
message, before the message from the notes is added.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 log-tree.c              |  6 ++++--
 t/t4014-format-patch.sh | 12 ++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index e7e08f4..4390b11 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -672,12 +672,14 @@ void show_log(struct rev_info *opt)
 	ctx.reflog_info = opt->reflog_info;
 	ctx.fmt = opt->commit_format;
 	pretty_print_commit(&ctx, commit, &msgbuf);
+
+	if (opt->add_signoff)
+		append_signoff(&msgbuf, opt->add_signoff);
+
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
 	    ctx.notes_message && *ctx.notes_message)
 		strbuf_addstr(&msgbuf, ctx.notes_message);
 
-	if (opt->add_signoff)
-		append_signoff(&msgbuf, opt->add_signoff);
 	if (opt->show_log_size) {
 		printf("log size %i\n", (int)msgbuf.len);
 		graph_show_oneline(opt->graph);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 959aa26..bea6381 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -616,8 +616,16 @@ test_expect_success 'format-patch --in-reply-to' '
 '
 
 test_expect_success 'format-patch --signoff' '
-	git format-patch -1 --signoff --stdout |
-	grep "^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	git format-patch -1 --signoff --stdout >out &&
+	grep "^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" out
+'
+
+test_expect_success 'format-patch --notes --signoff' '
+	git notes --ref test add -m "test message" HEAD &&
+	git format-patch -1 --signoff --stdout --notes=test >out &&
+	# Notes message must come after S-o-b
+	! sed "/^Signed-off-by: /q" out | grep "test message" &&
+	sed "1,/^Signed-off-by: /d" out | grep "test message"
 '
 
 echo "fatal: --name-only does not make sense" > expect.name-only
-- 
1.8.0.rc3.112.gdb88a5e

[PATCH 6/6] format-patch --notes: show notes after three-dashes

[Junio C Hamano]

When inserting the note after the commit log message to format-patch
output, add three dashes before the note.  Record the fact that we
did so in the rev_info and omit showing duplicated three dashes in
the usual codepath that is used when notes are not being shown.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 log-tree.c              | 15 +++++++++++----
 revision.h              |  1 +
 t/t4014-format-patch.sh |  7 +++++--
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 4390b11..712a22b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -677,8 +677,13 @@ void show_log(struct rev_info *opt)
 		append_signoff(&msgbuf, opt->add_signoff);
 
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
-	    ctx.notes_message && *ctx.notes_message)
+	    ctx.notes_message && *ctx.notes_message) {
+		if (ctx.fmt == CMIT_FMT_EMAIL) {
+			strbuf_addstr(&msgbuf, "---\n");
+			opt->shown_dashes = 1;
+		}
 		strbuf_addstr(&msgbuf, ctx.notes_message);
+	}
 
 	if (opt->show_log_size) {
 		printf("log size %i\n", (int)msgbuf.len);
@@ -710,6 +715,7 @@ void show_log(struct rev_info *opt)
 
 int log_tree_diff_flush(struct rev_info *opt)
 {
+	opt->shown_dashes = 0;
 	diffcore_std(&opt->diffopt);
 
 	if (diff_queue_is_empty()) {
@@ -737,10 +743,11 @@ int log_tree_diff_flush(struct rev_info *opt)
 					opt->diffopt.output_prefix_data);
 				fwrite(msg->buf, msg->len, 1, stdout);
 			}
-			if ((pch & opt->diffopt.output_format) == pch) {
-				printf("---");
+			if (!opt->shown_dashes) {
+				if ((pch & opt->diffopt.output_format) == pch)
+					printf("---");
+				putchar('\n');
 			}
-			putchar('\n');
 		}
 	}
 	diff_flush(&opt->diffopt);
diff --git a/revision.h b/revision.h
index a95bd0b..059bfff 100644
--- a/revision.h
+++ b/revision.h
@@ -111,6 +111,7 @@ struct rev_info {
 
 	/* Format info */
 	unsigned int	shown_one:1,
+			shown_dashes:1,
 			show_merge:1,
 			show_notes:1,
 			show_notes_given:1,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index bea6381..9750ba6 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -623,9 +623,12 @@ test_expect_success 'format-patch --signoff' '
 test_expect_success 'format-patch --notes --signoff' '
 	git notes --ref test add -m "test message" HEAD &&
 	git format-patch -1 --signoff --stdout --notes=test >out &&
-	# Notes message must come after S-o-b
+	# Three dashes must come after S-o-b
 	! sed "/^Signed-off-by: /q" out | grep "test message" &&
-	sed "1,/^Signed-off-by: /d" out | grep "test message"
+	sed "1,/^Signed-off-by: /d" out | grep "test message" &&
+	# Notes message must come after three dashes
+	! sed "/^---$/q" out | grep "test message" &&
+	sed "1,/^---$/d" out | grep "test message"
 '
 
 echo "fatal: --name-only does not make sense" > expect.name-only
-- 
1.8.0.rc3.112.gdb88a5e

Re: [PATCH 6/6] format-patch --notes: show notes after three-dashes

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> When inserting the note after the commit log message to format-patch
> output, add three dashes before the note.  Record the fact that we
> did so in the rev_info and omit showing duplicated three dashes in
> the usual codepath that is used when notes are not being shown.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Should this also include a documentation update to make this substantive 
benefit visible, whether that be in the format-patch man pages, the 
SubmittingPatches guide, in the git-notes description of 'A typical 
use...', or even in the user-manual?

Do you have a preference?

> ---
> log-tree.c              | 15 +++++++++++----
> revision.h              |  1 +
> t/t4014-format-patch.sh |  7 +++++--
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 4390b11..712a22b 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -677,8 +677,13 @@ void show_log(struct rev_info *opt)
>  append_signoff(&msgbuf, opt->add_signoff);
>
>  if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
> -     ctx.notes_message && *ctx.notes_message)
> +     ctx.notes_message && *ctx.notes_message) {
> + if (ctx.fmt == CMIT_FMT_EMAIL) {
> + strbuf_addstr(&msgbuf, "---\n");
> + opt->shown_dashes = 1;
> + }
>  strbuf_addstr(&msgbuf, ctx.notes_message);
> + }
>
>  if (opt->show_log_size) {
>  printf("log size %i\n", (int)msgbuf.len);
> @@ -710,6 +715,7 @@ void show_log(struct rev_info *opt)
>
> int log_tree_diff_flush(struct rev_info *opt)
> {
> + opt->shown_dashes = 0;
>  diffcore_std(&opt->diffopt);
>
>  if (diff_queue_is_empty()) {
> @@ -737,10 +743,11 @@ int log_tree_diff_flush(struct rev_info *opt)
>  opt->diffopt.output_prefix_data);
>  fwrite(msg->buf, msg->len, 1, stdout);
>  }
> - if ((pch & opt->diffopt.output_format) == pch) {
> - printf("---");
> + if (!opt->shown_dashes) {
> + if ((pch & opt->diffopt.output_format) == pch)
> + printf("---");
> + putchar('\n');
>  }
> - putchar('\n');
>  }
>  }
>  diff_flush(&opt->diffopt);
> diff --git a/revision.h b/revision.h
> index a95bd0b..059bfff 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -111,6 +111,7 @@ struct rev_info {
>
>  /* Format info */
>  unsigned int shown_one:1,
> + shown_dashes:1,
>  show_merge:1,
>  show_notes:1,
>  show_notes_given:1,
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index bea6381..9750ba6 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -623,9 +623,12 @@ test_expect_success 'format-patch --signoff' '
> test_expect_success 'format-patch --notes --signoff' '
>  git notes --ref test add -m "test message" HEAD &&
>  git format-patch -1 --signoff --stdout --notes=test >out &&
> - # Notes message must come after S-o-b
> + # Three dashes must come after S-o-b
>  ! sed "/^Signed-off-by: /q" out | grep "test message" &&
> - sed "1,/^Signed-off-by: /d" out | grep "test message"
> + sed "1,/^Signed-off-by: /d" out | grep "test message" &&
> + # Notes message must come after three dashes
> + ! sed "/^---$/q" out | grep "test message" &&
> + sed "1,/^---$/d" out | grep "test message"
> '
>
> echo "fatal: --name-only does not make sense" > expect.name-only
> -- 
> 1.8.0.rc3.112.gdb88a5e
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2013.0.2741 / Virus Database: 2614/5837 - Release Date: 
> 10/17/12
> 

Re: [PATCH 6/6] format-patch --notes: show notes after three-dashes

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> From: "Junio C Hamano" <gitster@pobox.com>
>> When inserting the note after the commit log message to format-patch
>> output, add three dashes before the note.  Record the fact that we
>> did so in the rev_info and omit showing duplicated three dashes in
>> the usual codepath that is used when notes are not being shown.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Should this also include a documentation update to make this
> substantive benefit visible, whether that be in the format-patch man
> pages, the SubmittingPatches guide, in the git-notes description of 'A
> typical use...', or even in the user-manual?

Eric Blake (http://mid.gmane.org/507EB310.8020904@redhat.com) was
already working on a documentation updates already, I thought.

As long as what it does is explained in format-patch, that is fine.

I do not think this deserves to be in the SubmittingPatches.  We do
tell people to hide "here is the context of the change" additional
explanation after three dashes, but how the submitters prepare that
text is entirely up to them (and I personally do not think notes is
not necessarily the right tool to do so).

Re: [PATCH 6/6] format-patch --notes: show notes after three-dashes

[Junio C Hamano]

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

> As long as what it does is explained in format-patch, that is fine.
>
> I do not think this deserves to be in the SubmittingPatches.  We do
> tell people to hide "here is the context of the change" additional
> explanation after three dashes, but how the submitters prepare that
> text is entirely up to them (and I personally do not think notes is
> not necessarily the right tool to do so).

Just in case, here is what I'll queue as a placeholder on 'pu'.

-- >8 --
Subject: [PATCH] Documentation: decribe format-patch --notes

Even though I coded this, I am not sure what use scenarios would benefit
from this option, so the description is unnecessarily negative at this
moment. People who do want to use this feature need to come up with a
more plausible use case and replace it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6d43f56..066dc8b 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 		   [--ignore-if-in-upstream]
 		   [--subject-prefix=Subject-Prefix]
 		   [--to=<email>] [--cc=<email>]
-		   [--cover-letter] [--quiet]
+		   [--cover-letter] [--quiet] [--notes[=<ref>]]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -191,6 +191,19 @@ will want to ensure that threading is disabled for `git send-email`.
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--notes[=<ref>]::
+	Append the notes (see linkgit:git-notes[1]) for the commit
+	after the three-dash line.
++
+The expected use case of this is to write supporting explanation for
+the commit that does not belong to the commit log message proper
+when (or after) you create the commit, and include it in your patch
+submission.  But if you can plan ahead and write it down, there may
+not be a good reason not to write it in your commit message, and if
+you can't, you can always edit the output of format-patch before
+sending it out, so the practical value of this option is somewhat
+dubious, unless your workflow is broken.
+
 --[no]-signature=<signature>::
 	Add a signature to each message produced. Per RFC 3676 the signature
 	is separated from the body by a line with '-- ' on it. If the
-- 
1.8.0.rc3.162.g1f53438

Re: [PATCH 6/6] format-patch --notes: show notes after three-dashes

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> From: "Junio C Hamano" <gitster@pobox.com>
>>> When inserting the note after the commit log message to format-patch
>>> output, add three dashes before the note.  Record the fact that we
>>> did so in the rev_info and omit showing duplicated three dashes in
>>> the usual codepath that is used when notes are not being shown.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> Should this also include a documentation update to make this
>> substantive benefit visible, whether that be in the format-patch man
>> pages, the SubmittingPatches guide, in the git-notes description of 
>> 'A
>> typical use...', or even in the user-manual?
>
> Eric Blake (http://mid.gmane.org/507EB310.8020904@redhat.com) was
Added to distribution.

> already working on a documentation updates already, I thought.
>
> As long as what it does is explained in format-patch, that is fine.
>
> I do not think this deserves to be in the SubmittingPatches.  We do
> tell people to hide "here is the context of the change" additional
> explanation after three dashes, but how the submitters prepare that
> text is entirely up to them (and I personally do not think notes is
> not necessarily the right tool to do so).
>
I've prepared a short set of patches for the documenation I mentioned.

I, like Eric, feel some of the methods available are a bit of a Catch 
22.
We do need to at least hint about capabilities as a way of helping new
users who aren't already experts (the inverse Kruger Dunning problem).

I have tried hard to make the patches concise so hopefully they will be
acceptable.

Philip 

Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature

[Jeff King]

On Wed, Oct 17, 2012 at 10:45:22PM -0700, Junio C Hamano wrote:

> We never advertised the "--notes" option to format-patch (or
> anything related to the pretty format options for that matter)
> because the behaviour of these options was whatever they happened to
> do, not what they were designed to do.
> 
> It had a few obvious glitches:
> 
>  * The notes section was appended immediately after the log message,
>    and then the three-dash line was added.  Such a supplimental
>    material should come after the three-dash line.
> 
>  * The logic to append a new sign-off with "format-patch --signoff"
>    worked on the message after the note was added, which made the
>    detection of existing sign-off lines incorrect.
> 
> This updates the handling of "--notes" option to correct these, in
> an attempt to bring it closer to a real feature.

I just read through the whole series. Aside from the tangent about
strbuf_detach, I didn't see anything wrong. Thanks for moving this
forward.

-Peff

Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature

[Nguyen Thai Ngoc Duy]

On Thu, Oct 18, 2012 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This replaces the earlier "wip" with a real thing.
>
> We never advertised the "--notes" option to format-patch (or
> anything related to the pretty format options for that matter)
> because the behaviour of these options was whatever they happened to
> do, not what they were designed to do.

Stupid question: does "git am" recreate notes from "format-patch
--notes" output? If it does not, should it? I think it could be a nice
way of copying notes from one machine to another, but not enabled by
default (iow "am --notes").
-- 
Duy

Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature

[Michael J Gruber]

Nguyen Thai Ngoc Duy venit, vidit, dixit 18.10.2012 13:06:
> On Thu, Oct 18, 2012 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This replaces the earlier "wip" with a real thing.
>>
>> We never advertised the "--notes" option to format-patch (or
>> anything related to the pretty format options for that matter)
>> because the behaviour of these options was whatever they happened to
>> do, not what they were designed to do.
> 
> Stupid question: does "git am" recreate notes from "format-patch
> --notes" output? If it does not, should it? I think it could be a nice
> way of copying notes from one machine to another, but not enabled by
> default (iow "am --notes").
> 

Not yet, but you can already start basing your am patches on top of
Junio's series ;)

am should allow a notes ref (--notes=hereyougo) like other similar
instances.

Michael

Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Oct 18, 2012 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This replaces the earlier "wip" with a real thing.
>>
>> We never advertised the "--notes" option to format-patch (or
>> anything related to the pretty format options for that matter)
>> because the behaviour of these options was whatever they happened to
>> do, not what they were designed to do.
>
> Stupid question: does "git am" recreate notes from "format-patch
> --notes" output? If it does not, should it? I think it could be a nice
> way of copying notes from one machine to another, but not enabled by
> default (iow "am --notes").

Thinking about what the "notes" are, I do not think it should, at
least by default; the model is broken at the conceptual level.

The notes are comments on a commit you make after the fact, because
you do not want to (or cannot afford to) amend the commit to include
the comment.  When you are sending it out over e-mailto be applied
with "am", as opposed to asking the other to pull, you are by
definition willing to see the commit replayed with modification.

I think it is sensible for format-patch/am pipeline when asked to
use --notes to add the notes section after "---" as additional
material to help the recipient understand the context of the patch
better, which is done with this series.  If the submitter (or the
recipient) wants to incorporate the description from the notes to
update the proposed log message, it can easily be done by editing
the output of "format-patch --notes" before sending it out (or
before applying it with "am").

That does not mean that the recipient should not use notes for his
own purpose on the resulting commit, by the way.  It would be a
convenient feature to prime the contents of such a new note the
recipient creates on the resulting commit from the comment after
"---" before the diffstat or "diff --git" line.  Note that (no pun
intended) that additional comment does not have to originate from
any notes in the submitter's repository.  If saving the additional
comments the submitter attached from the notes to the patch is
useful, it would equally be useful to save typed-in comments on the
patch that came from the submitter's fingers, not from the notes.

It is something you can do by inspecting $dotest/patch file in your
post-applypatch hook with today's git.  If many people use and find
such a feature desirable, we could add inbuilt support for it, but I
do not think we are there yet.