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

2012-10-18 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-10-18 Thread Junio C Hamano 
Jeff King p...@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 ;-).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

 Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-10-17 Thread 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 gits...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html