# [PATCH 1/4] Improve message-id generation flow control for format-patch 4 messages from 2008-02-17 to 2008-02-18. Participants: Daniel Barkalow, Johannes Schindelin. Thread: https://gitlist.dev/t/12150 ## Daniel Barkalow, 2008-02-17 18:35 Subject: [PATCH 1/4] Improve message-id generation flow control for format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802171335240.5816%40iabervon.org ``` Signed-off-by: Daniel Barkalow --- builtin-log.c | 29 ++++++++++++++--------------- 1 files changed, 14 insertions(+), 15 deletions(-) diff --git a/builtin-log.c b/builtin-log.c index 99d69f0..867cc13 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -575,16 +575,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha o2->flags = flags2; } -static void gen_message_id(char *dest, unsigned int length, char *base) +static void gen_message_id(struct rev_info *info, char *base) { const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME); const char *email_start = strrchr(committer, '<'); const char *email_end = strrchr(committer, '>'); - if(!email_start || !email_end || email_start > email_end - 1) + struct strbuf buf; + if (!email_start || !email_end || email_start > email_end - 1) die("Could not extract email from committer identity."); - snprintf(dest, length, "%s.%lu.git.%.*s", base, - (unsigned long) time(NULL), - (int)(email_end - email_start - 1), email_start + 1); + strbuf_init(&buf, 0); + strbuf_addf(&buf, "%s.%lu.git.%.*s", base, + (unsigned long) time(NULL), + (int)(email_end - email_start - 1), email_start + 1); + info->message_id = strbuf_detach(&buf, NULL); } static const char *clean_message_id(const char *msg_id) @@ -625,8 +628,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) const char *in_reply_to = NULL; struct patch_ids ids; char *add_signoff = NULL; - char message_id[1024]; - char ref_message_id[1024]; git_config(git_format_config); init_revisions(&rev, prefix); @@ -809,15 +810,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.nr = total - nr + (start_number - 1); /* Make the second and subsequent mails replies to the first */ if (thread) { - if (nr == (total - 2)) { - strncpy(ref_message_id, message_id, - sizeof(ref_message_id)); - ref_message_id[sizeof(ref_message_id)-1]='\0'; - rev.ref_message_id = ref_message_id; + if (rev.message_id) { + if (rev.ref_message_id) + free((char *) rev.message_id); + else + rev.ref_message_id = rev.message_id; } - gen_message_id(message_id, sizeof(message_id), - sha1_to_hex(commit->object.sha1)); - rev.message_id = message_id; + gen_message_id(&rev, sha1_to_hex(commit->object.sha1)); } if (!use_stdout) if (reopen_stdout(commit, rev.nr, keep_subject, -- 1.5.4.1.1350.g2b9ee ``` ## Johannes Schindelin, 2008-02-18 12:41 Subject: Re: [PATCH 1/4] Improve message-id generation flow control for format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802181238280.30505%40racer.site In-Reply-To: ``` Hi, On Sun, 17 Feb 2008, Daniel Barkalow wrote: > diff --git a/builtin-log.c b/builtin-log.c > index 99d69f0..867cc13 100644 > --- a/builtin-log.c > +++ b/builtin-log.c > @@ -575,16 +575,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha > o2->flags = flags2; > } > > -static void gen_message_id(char *dest, unsigned int length, char *base) > +static void gen_message_id(struct rev_info *info, char *base) > { > const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME); > const char *email_start = strrchr(committer, '<'); > const char *email_end = strrchr(committer, '>'); > - if(!email_start || !email_end || email_start > email_end - 1) > + struct strbuf buf; > + if (!email_start || !email_end || email_start > email_end - 1) > die("Could not extract email from committer identity."); > - snprintf(dest, length, "%s.%lu.git.%.*s", base, > - (unsigned long) time(NULL), > - (int)(email_end - email_start - 1), email_start + 1); > + strbuf_init(&buf, 0); > + strbuf_addf(&buf, "%s.%lu.git.%.*s", base, > + (unsigned long) time(NULL), > + (int)(email_end - email_start - 1), email_start + 1); > + info->message_id = strbuf_detach(&buf, NULL); With this last line, and... > @@ -809,15 +810,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > rev.nr = total - nr + (start_number - 1); > /* Make the second and subsequent mails replies to the first */ > if (thread) { > - if (nr == (total - 2)) { > - strncpy(ref_message_id, message_id, > - sizeof(ref_message_id)); > - ref_message_id[sizeof(ref_message_id)-1]='\0'; > - rev.ref_message_id = ref_message_id; > + if (rev.message_id) { > + if (rev.ref_message_id) > + free((char *) rev.message_id); ... this one, you should make the message_id member of struct rev_info a "char *". At least for this developer, "const char *" is a sign that the caller should clean up, and that the pointer _might_ point to a constant. Ciao, Dscho ``` ## Daniel Barkalow, 2008-02-18 17:43 Subject: Re: [PATCH 1/4] Improve message-id generation flow control for format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802181229340.5816%40iabervon.org In-Reply-To: ``` On Mon, 18 Feb 2008, Johannes Schindelin wrote: > Hi, > > On Sun, 17 Feb 2008, Daniel Barkalow wrote: > > > diff --git a/builtin-log.c b/builtin-log.c > > index 99d69f0..867cc13 100644 > > --- a/builtin-log.c > > +++ b/builtin-log.c > > @@ -575,16 +575,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha > > o2->flags = flags2; > > } > > > > -static void gen_message_id(char *dest, unsigned int length, char *base) > > +static void gen_message_id(struct rev_info *info, char *base) > > { > > const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME); > > const char *email_start = strrchr(committer, '<'); > > const char *email_end = strrchr(committer, '>'); > > - if(!email_start || !email_end || email_start > email_end - 1) > > + struct strbuf buf; > > + if (!email_start || !email_end || email_start > email_end - 1) > > die("Could not extract email from committer identity."); > > - snprintf(dest, length, "%s.%lu.git.%.*s", base, > > - (unsigned long) time(NULL), > > - (int)(email_end - email_start - 1), email_start + 1); > > + strbuf_init(&buf, 0); > > + strbuf_addf(&buf, "%s.%lu.git.%.*s", base, > > + (unsigned long) time(NULL), > > + (int)(email_end - email_start - 1), email_start + 1); > > + info->message_id = strbuf_detach(&buf, NULL); > > With this last line, and... > > > @@ -809,15 +810,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > rev.nr = total - nr + (start_number - 1); > > /* Make the second and subsequent mails replies to the first */ > > if (thread) { > > - if (nr == (total - 2)) { > > - strncpy(ref_message_id, message_id, > > - sizeof(ref_message_id)); > > - ref_message_id[sizeof(ref_message_id)-1]='\0'; > > - rev.ref_message_id = ref_message_id; > > + if (rev.message_id) { > > + if (rev.ref_message_id) > > + free((char *) rev.message_id); > > ... this one, you should make the message_id member of struct rev_info a > "char *". At least for this developer, "const char *" is a sign that the > caller should clean up, and that the pointer _might_ point to a constant. It's sort of like that, in that this *is* the caller, and it's using gen_message_id to set it and cleaning it up, and it could put in a constant (in which case it would have to know this and not free it), but I agree that it's more suggestive of the right things as a "char *". -Daniel *This .sig left intentionally blank* ``` ## Johannes Schindelin, 2008-02-18 18:00 Subject: Re: [PATCH 1/4] Improve message-id generation flow control for format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802181759280.30505%40racer.site In-Reply-To: ``` Hi, On Mon, 18 Feb 2008, Daniel Barkalow wrote: > It's sort of like that, in that this *is* the caller, Sorry, I should have been clearer. It is all too easy to forget that reasoning and use rev_info->message_id in a different way, and then mix the code paths. Ciao, Dscho ```