{"thread":{"id":"11917","subject":"[PATCH 1/4] Improve message-id generation flow control for format-patch","startedAt":"2008-02-06T16:43:18Z","lastAt":"2008-02-06T23:01:09Z","messageCount":7,"participants":["Daniel Barkalow","Junio C Hamano","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"67672","messageId":"alpine.LNX.1.00.0802061140360.13593@iabervon.org","threadId":"11917","inReplyTo":null,"subject":"[PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-06T16:43:18Z","receivedAt":"2008-02-06T16:43:18Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Signed-off-by: Daniel Barkalow \n---\n builtin-log.c | 29 ++++++++++++++---------------\n 1 files changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex dcc9f81..1f74d66 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -576,16 +576,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n \to2->flags = flags2;\n }\n \n-static void gen_message_id(char *dest, unsigned int length, char *base)\n+static void gen_message_id(struct rev_info *info, char *base)\n {\n \tconst char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);\n \tconst char *email_start = strrchr(committer, '<');\n \tconst char *email_end = strrchr(committer, '>');\n-\tif(!email_start || !email_end || email_start > email_end - 1)\n+\tstruct strbuf buf;\n+\tif (!email_start || !email_end || email_start > email_end - 1)\n \t\tdie(\"Could not extract email from committer identity.\");\n-\tsnprintf(dest, length, \"%s.%lu.git.%.*s\", base,\n-\t\t (unsigned long) time(NULL),\n-\t\t (int)(email_end - email_start - 1), email_start + 1);\n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addf(&buf, \"%s.%lu.git.%.*s\", base,\n+\t\t (unsigned long) time(NULL),\n+\t\t (int)(email_end - email_start - 1), email_start + 1);\n+\tinfo->message_id = buf.buf;\n }\n \n static const char *clean_message_id(const char *msg_id)\n@@ -626,8 +629,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tconst char *in_reply_to = NULL;\n \tstruct patch_ids ids;\n \tchar *add_signoff = NULL;\n-\tchar message_id[1024];\n-\tchar ref_message_id[1024];\n \n \tgit_config(git_format_config);\n \tinit_revisions(&rev, prefix);\n@@ -810,15 +811,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\trev.nr = total - nr + (start_number - 1);\n \t\t/* Make the second and subsequent mails replies to the first */\n \t\tif (thread) {\n-\t\t\tif (nr == (total - 2)) {\n-\t\t\t\tstrncpy(ref_message_id, message_id,\n-\t\t\t\t\tsizeof(ref_message_id));\n-\t\t\t\tref_message_id[sizeof(ref_message_id)-1]='\\0';\n-\t\t\t\trev.ref_message_id = ref_message_id;\n+\t\t\tif (rev.message_id) {\n+\t\t\t\tif (rev.ref_message_id)\n+\t\t\t\t\tfree((char *) rev.message_id);\n+\t\t\t\telse\n+\t\t\t\t\trev.ref_message_id = rev.message_id;\n \t\t\t}\n-\t\t\tgen_message_id(message_id, sizeof(message_id),\n-\t\t\t\t sha1_to_hex(commit->object.sha1));\n-\t\t\trev.message_id = message_id;\n+\t\t\tgen_message_id(&rev, sha1_to_hex(commit->object.sha1));\n \t\t}\n \t\tif (!use_stdout)\n \t\t\tif (reopen_stdout(commit, rev.nr, keep_subject,\n-- \n1.5.4.27.gf6864\n"},{"id":"67703","messageId":"7vsl05vncj.fsf@gitster.siamese.dyndns.org","threadId":"11917","inReplyTo":"alpine.LNX.1.00.0802061140360.13593@iabervon.org","subject":"Re: [PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-06T20:31:08Z","receivedAt":"2008-02-06T20:31:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> Signed-off-by: Daniel Barkalow \n> ---\n> builtin-log.c | 29 ++++++++++++++---------------\n> 1 files changed, 14 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin-log.c b/builtin-log.c\n> index dcc9f81..1f74d66 100644\n> --- a/builtin-log.c\n> +++ b/builtin-log.c\n> @@ -576,16 +576,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n> \to2->flags = flags2;\n> }\n> \n> -static void gen_message_id(char *dest, unsigned int length, char *base)\n> +static void gen_message_id(struct rev_info *info, char *base)\n> {\n> \tconst char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);\n> \tconst char *email_start = strrchr(committer, '<');\n> \tconst char *email_end = strrchr(committer, '>');\n> -\tif(!email_start || !email_end || email_start > email_end - 1)\n> +\tstruct strbuf buf;\n> +\tif (!email_start || !email_end || email_start > email_end - 1)\n> \t\tdie(\"Could not extract email from committer identity.\");\n> -\tsnprintf(dest, length, \"%s.%lu.git.%.*s\", base,\n> -\t\t (unsigned long) time(NULL),\n> -\t\t (int)(email_end - email_start - 1), email_start + 1);\n> +\tstrbuf_init(&buf, 0);\n> +\tstrbuf_addf(&buf, \"%s.%lu.git.%.*s\", base,\n> +\t\t (unsigned long) time(NULL),\n> +\t\t (int)(email_end - email_start - 1), email_start + 1);\n> +\tinfo->message_id = buf.buf;\n\nI wonder how the rule established by b315c5c (strbuf change: be\nsure ->buf is never ever NULL) and at the beginning of strbuf.h\napplies here. I think the current implementation of strbuf\nhappens to allow this, and it is very handy. Perhaps the rule\nstated there should be loosened and allow copying the buf away\nwhen you know you have stuff in there (i.e. ->buf != slopbuf).\nPierre, what do you think?\n\nWhat the patch does itself is much nicer than the original.\n"},{"id":"67713","messageId":"20080206211326.GA9441@artemis.madism.org","threadId":"11917","inReplyTo":"7vsl05vncj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-02-06T21:13:26Z","receivedAt":"2008-02-06T21:13:26Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Wed, Feb 06, 2008 at 08:31:08PM +0000, Junio C Hamano wrote:\n> Daniel Barkalow writes:\n> \n> > Signed-off-by: Daniel Barkalow \n> > ---\n> > builtin-log.c | 29 ++++++++++++++---------------\n> > 1 files changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/builtin-log.c b/builtin-log.c\n> > index dcc9f81..1f74d66 100644\n> > --- a/builtin-log.c\n> > +++ b/builtin-log.c\n> > @@ -576,16 +576,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n> > \to2->flags = flags2;\n> > }\n> > \n> > -static void gen_message_id(char *dest, unsigned int length, char *base)\n> > +static void gen_message_id(struct rev_info *info, char *base)\n> > {\n> > \tconst char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);\n> > \tconst char *email_start = strrchr(committer, '<');\n> > \tconst char *email_end = strrchr(committer, '>');\n> > -\tif(!email_start || !email_end || email_start > email_end - 1)\n> > +\tstruct strbuf buf;\n> > +\tif (!email_start || !email_end || email_start > email_end - 1)\n> > \t\tdie(\"Could not extract email from committer identity.\");\n> > -\tsnprintf(dest, length, \"%s.%lu.git.%.*s\", base,\n> > -\t\t (unsigned long) time(NULL),\n> > -\t\t (int)(email_end - email_start - 1), email_start + 1);\n> > +\tstrbuf_init(&buf, 0);\n> > +\tstrbuf_addf(&buf, \"%s.%lu.git.%.*s\", base,\n> > +\t\t (unsigned long) time(NULL),\n> > +\t\t (int)(email_end - email_start - 1), email_start + 1);\n> > +\tinfo->message_id = buf.buf;\n> \n> I wonder how the rule established by b315c5c (strbuf change: be\n> sure ->buf is never ever NULL) and at the beginning of strbuf.h\n> applies here. I think the current implementation of strbuf\n> happens to allow this, and it is very handy. Perhaps the rule\n> stated there should be loosened and allow copying the buf away\n> when you know you have stuff in there (i.e. ->buf != slopbuf).\n> Pierre, what do you think?\n> \n> What the patch does itself is much nicer than the original.\n\n Why wouldn't you just use strbuf_detach ? I mean replacing:\n\n+\tinfo->message_id = buf.buf;\n\nwith:\n\n+\tinfo->message_id = strbuf_detach(&buf, NULL);\n\n isn't really hard to read, and has the nice side effect to prevent\nerrors that could happen in the future (like reusing buf and screwing\nwith info->message_id without noticing it). I'd rather stand on the safe\nside here, it's more forward-compatible and idiot-proof[0].\n\n\n [0] Not that I believe git contributors are idiots, but I firmly\n believe in defensive programming when it doesn't impact\n performances. And I don't believe it would here.\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"67716","messageId":"alpine.LNX.1.00.0802061703230.13593@iabervon.org","threadId":"11917","inReplyTo":"20080206211326.GA9441@artemis.madism.org","subject":"Re: [PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-06T22:10:30Z","receivedAt":"2008-02-06T22:10:30Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 6 Feb 2008, Pierre Habouzit wrote:\n\n> On Wed, Feb 06, 2008 at 08:31:08PM +0000, Junio C Hamano wrote:\n> > Daniel Barkalow writes:\n> > \n> > > Signed-off-by: Daniel Barkalow \n> > > ---\n> > > builtin-log.c | 29 ++++++++++++++---------------\n> > > 1 files changed, 14 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/builtin-log.c b/builtin-log.c\n> > > index dcc9f81..1f74d66 100644\n> > > --- a/builtin-log.c\n> > > +++ b/builtin-log.c\n> > > @@ -576,16 +576,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n> > > \to2->flags = flags2;\n> > > }\n> > > \n> > > -static void gen_message_id(char *dest, unsigned int length, char *base)\n> > > +static void gen_message_id(struct rev_info *info, char *base)\n> > > {\n> > > \tconst char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);\n> > > \tconst char *email_start = strrchr(committer, '<');\n> > > \tconst char *email_end = strrchr(committer, '>');\n> > > -\tif(!email_start || !email_end || email_start > email_end - 1)\n> > > +\tstruct strbuf buf;\n> > > +\tif (!email_start || !email_end || email_start > email_end - 1)\n> > > \t\tdie(\"Could not extract email from committer identity.\");\n> > > -\tsnprintf(dest, length, \"%s.%lu.git.%.*s\", base,\n> > > -\t\t (unsigned long) time(NULL),\n> > > -\t\t (int)(email_end - email_start - 1), email_start + 1);\n> > > +\tstrbuf_init(&buf, 0);\n> > > +\tstrbuf_addf(&buf, \"%s.%lu.git.%.*s\", base,\n> > > +\t\t (unsigned long) time(NULL),\n> > > +\t\t (int)(email_end - email_start - 1), email_start + 1);\n> > > +\tinfo->message_id = buf.buf;\n> > \n> > I wonder how the rule established by b315c5c (strbuf change: be\n> > sure ->buf is never ever NULL) and at the beginning of strbuf.h\n> > applies here. I think the current implementation of strbuf\n> > happens to allow this, and it is very handy. Perhaps the rule\n> > stated there should be loosened and allow copying the buf away\n> > when you know you have stuff in there (i.e. ->buf != slopbuf).\n> > Pierre, what do you think?\n> > \n> > What the patch does itself is much nicer than the original.\n> \n> Why wouldn't you just use strbuf_detach ? I mean replacing:\n> \n> +\tinfo->message_id = buf.buf;\n> \n> with:\n> \n> +\tinfo->message_id = strbuf_detach(&buf, NULL);\n> \n> isn't really hard to read, and has the nice side effect to prevent\n> errors that could happen in the future (like reusing buf and screwing\n> with info->message_id without noticing it). I'd rather stand on the safe\n> side here, it's more forward-compatible and idiot-proof[0].\n\nIs it actually right to have buf go out of scope right after \nstrbuf_detach()? It sort of looks like it would leak memory from buf.buf. \nI'm happy to do whatever the API wants there, and I didn't see anything to \nleave the struct as if strbuf_release were called, but with the string \nextracted for the caller.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"67725","messageId":"20080206225335.GA23510@artemis.madism.org","threadId":"11917","inReplyTo":"alpine.LNX.1.00.0802061703230.13593@iabervon.org","subject":"Re: [PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-02-06T22:53:35Z","receivedAt":"2008-02-06T22:53:35Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Wed, Feb 06, 2008 at 10:10:30PM +0000, Daniel Barkalow wrote:\n> On Wed, 6 Feb 2008, Pierre Habouzit wrote:\n> \n> > On Wed, Feb 06, 2008 at 08:31:08PM +0000, Junio C Hamano wrote:\n> > > I wonder how the rule established by b315c5c (strbuf change: be\n> > > sure ->buf is never ever NULL) and at the beginning of strbuf.h\n> > > applies here. I think the current implementation of strbuf\n> > > happens to allow this, and it is very handy. Perhaps the rule\n> > > stated there should be loosened and allow copying the buf away\n> > > when you know you have stuff in there (i.e. ->buf != slopbuf).\n> > > Pierre, what do you think?\n> > > \n> > > What the patch does itself is much nicer than the original.\n> > \n> > Why wouldn't you just use strbuf_detach ? I mean replacing:\n> > \n> > +\tinfo->message_id = buf.buf;\n> > \n> > with:\n> > \n> > +\tinfo->message_id = strbuf_detach(&buf, NULL);\n> > \n> > isn't really hard to read, and has the nice side effect to prevent\n> > errors that could happen in the future (like reusing buf and screwing\n> > with info->message_id without noticing it). I'd rather stand on the safe\n> > side here, it's more forward-compatible and idiot-proof[0].\n> \n> Is it actually right to have buf go out of scope right after \n> strbuf_detach()? It sort of looks like it would leak memory from buf.buf. \n> I'm happy to do whatever the API wants there, and I didn't see anything to \n> leave the struct as if strbuf_release were called, but with the string \n> extracted for the caller.\n\n err no, strbuf_detach gives you a pointer you are supposed to free()\nlater, and inits the strbuf passed as its argument to be used again,\nthough if you don't, you leak nothing.\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"67727","messageId":"20080206225648.GB23510@artemis.madism.org","threadId":"11917","inReplyTo":"20080206225335.GA23510@artemis.madism.org","subject":"Re: [PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-02-06T22:56:48Z","receivedAt":"2008-02-06T22:56:48Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Wed, Feb 06, 2008 at 10:53:35PM +0000, Pierre Habouzit wrote:\n> On Wed, Feb 06, 2008 at 10:10:30PM +0000, Daniel Barkalow wrote:\n> > On Wed, 6 Feb 2008, Pierre Habouzit wrote:\n> > \n> > > On Wed, Feb 06, 2008 at 08:31:08PM +0000, Junio C Hamano wrote:\n> > > > I wonder how the rule established by b315c5c (strbuf change: be\n> > > > sure ->buf is never ever NULL) and at the beginning of strbuf.h\n> > > > applies here. I think the current implementation of strbuf\n> > > > happens to allow this, and it is very handy. Perhaps the rule\n> > > > stated there should be loosened and allow copying the buf away\n> > > > when you know you have stuff in there (i.e. ->buf != slopbuf).\n> > > > Pierre, what do you think?\n> > > > \n> > > > What the patch does itself is much nicer than the original.\n> > > \n> > > Why wouldn't you just use strbuf_detach ? I mean replacing:\n> > > \n> > > +\tinfo->message_id = buf.buf;\n> > > \n> > > with:\n> > > \n> > > +\tinfo->message_id = strbuf_detach(&buf, NULL);\n> > > \n> > > isn't really hard to read, and has the nice side effect to prevent\n> > > errors that could happen in the future (like reusing buf and screwing\n> > > with info->message_id without noticing it). I'd rather stand on the safe\n> > > side here, it's more forward-compatible and idiot-proof[0].\n> > \n> > Is it actually right to have buf go out of scope right after \n> > strbuf_detach()? It sort of looks like it would leak memory from buf.buf. \n> > I'm happy to do whatever the API wants there, and I didn't see anything to \n> > leave the struct as if strbuf_release were called, but with the string \n> > extracted for the caller.\n> \n> err no, strbuf_detach gives you a pointer you are supposed to free()\n> later, and inits the strbuf passed as its argument to be used again,\n> though if you don't, you leak nothing.\n\n In fact, strbuf_detach is the rough equivalent of doing that:\n\n info->message_id = buf.buf;\n buf.buf = NULL;\n\n\n Except that it sets buf.buf to a magic place so that it's never NULL,\nand that it also keeps the internal invariants in place. But after a\nstrbuf_detach, a strbuf doesn't holds any allocated memory anymore.\nYou'll see in many places in the code that\n`return strbuf_detach(&sb, NULL)` is quite idiomatic, and the function\ndoes exactly what it means \"Please detach the memory allocated in that\nbuffer and give it to me\".\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"67729","messageId":"alpine.LNX.1.00.0802061800490.13593@iabervon.org","threadId":"11917","inReplyTo":"20080206225648.GB23510@artemis.madism.org","subject":"Re: [PATCH 1/4] Improve message-id generation flow control for format-patch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-06T23:01:09Z","receivedAt":"2008-02-06T23:01:09Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 6 Feb 2008, Pierre Habouzit wrote:\n\n> On Wed, Feb 06, 2008 at 10:53:35PM +0000, Pierre Habouzit wrote:\n> > On Wed, Feb 06, 2008 at 10:10:30PM +0000, Daniel Barkalow wrote:\n> > > On Wed, 6 Feb 2008, Pierre Habouzit wrote:\n> > > \n> > > > On Wed, Feb 06, 2008 at 08:31:08PM +0000, Junio C Hamano wrote:\n> > > > > I wonder how the rule established by b315c5c (strbuf change: be\n> > > > > sure ->buf is never ever NULL) and at the beginning of strbuf.h\n> > > > > applies here. I think the current implementation of strbuf\n> > > > > happens to allow this, and it is very handy. Perhaps the rule\n> > > > > stated there should be loosened and allow copying the buf away\n> > > > > when you know you have stuff in there (i.e. ->buf != slopbuf).\n> > > > > Pierre, what do you think?\n> > > > > \n> > > > > What the patch does itself is much nicer than the original.\n> > > > \n> > > > Why wouldn't you just use strbuf_detach ? I mean replacing:\n> > > > \n> > > > +\tinfo->message_id = buf.buf;\n> > > > \n> > > > with:\n> > > > \n> > > > +\tinfo->message_id = strbuf_detach(&buf, NULL);\n> > > > \n> > > > isn't really hard to read, and has the nice side effect to prevent\n> > > > errors that could happen in the future (like reusing buf and screwing\n> > > > with info->message_id without noticing it). I'd rather stand on the safe\n> > > > side here, it's more forward-compatible and idiot-proof[0].\n> > > \n> > > Is it actually right to have buf go out of scope right after \n> > > strbuf_detach()? It sort of looks like it would leak memory from buf.buf. \n> > > I'm happy to do whatever the API wants there, and I didn't see anything to \n> > > leave the struct as if strbuf_release were called, but with the string \n> > > extracted for the caller.\n> > \n> > err no, strbuf_detach gives you a pointer you are supposed to free()\n> > later, and inits the strbuf passed as its argument to be used again,\n> > though if you don't, you leak nothing.\n> \n> In fact, strbuf_detach is the rough equivalent of doing that:\n> \n> info->message_id = buf.buf;\n> buf.buf = NULL;\n> \n> \n> Except that it sets buf.buf to a magic place so that it's never NULL,\n> and that it also keeps the internal invariants in place. But after a\n> strbuf_detach, a strbuf doesn't holds any allocated memory anymore.\n> You'll see in many places in the code that\n> `return strbuf_detach(&sb, NULL)` is quite idiomatic, and the function\n> does exactly what it means \"Please detach the memory allocated in that\n> buffer and give it to me\".\n\nAh, good. That's what I'll use, then.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}