# [PATCH] builtin-commit: fix --signoff 5 messages from 2007-11-10 to 2007-11-10. Participants: Johannes Schindelin, Junio C Hamano, Pierre Habouzit. Thread: https://gitlist.dev/t/10781 ## Johannes Schindelin, 2007-11-10 05:49 Subject: [PATCH] builtin-commit: fix --signoff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711100548071.4362%40racer.site ``` The Signed-off-by: line contained a spurious timestamp. Signed-off-by: Johannes Schindelin --- builtin-commit.c | 31 ++++++++++++++++++++----------- 1 files changed, 20 insertions(+), 11 deletions(-) diff --git a/builtin-commit.c b/builtin-commit.c index e8bc4c4..f79ad48 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -181,21 +181,30 @@ static int prepare_log_message(const char *index_file, const char *prefix) die("could not open %s\n", git_path(commit_editmsg)); stripspace(&sb, 0); - if (fwrite(sb.buf, 1, sb.len, fp) < sb.len) - die("could not write commit template: %s\n", - strerror(errno)); if (signoff) { - const char *info, *bol; - - info = git_committer_info(1); - strbuf_addch(&sb, '\0'); - bol = strrchr(sb.buf + sb.len - 1, '\n'); - if (!bol || prefixcmp(bol, sign_off_header)) - fprintf(fp, "\n"); - fprintf(fp, "%s%s\n", sign_off_header, git_committer_info(1)); + struct strbuf sob; + const char *p; + int i; + + strbuf_init(&sob, 0); + strbuf_addstr(&sob, sign_off_header); + strbuf_addstr(&sob, git_committer_info(1)); + p = strrchr(sob.buf, '>'); + if (p) + strbuf_setlen(&sob, p + 1 - sob.buf); + strbuf_addch(&sob, '\n'); + + for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--) + ; /* do nothing */ + if (prefixcmp(sb.buf + i, sob.buf)) + strbuf_addbuf(&sb, &sob); } + if (fwrite(sb.buf, 1, sb.len, fp) < sb.len) + die("could not write commit template: %s\n", + strerror(errno)); + strbuf_release(&sb); if (in_merge && !no_edit) -- 1.5.3.5.1674.g6e7f7 ``` ## Junio C Hamano, 2007-11-10 09:06 Subject: Re: [PATCH] builtin-commit: fix --signoff Message-ID: <7vr6iyo4ff.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr6iyo4ff.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > if (signoff) { > ... > + strbuf_init(&sob, 0); > + strbuf_addstr(&sob, sign_off_header); > + strbuf_addstr(&sob, git_committer_info(1)); > + p = strrchr(sob.buf, '>'); > + if (p) > + strbuf_setlen(&sob, p + 1 - sob.buf); > + strbuf_addch(&sob, '\n'); > + > + for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--) > + ; /* do nothing */ > + if (prefixcmp(sb.buf + i, sob.buf)) > + strbuf_addbuf(&sb, &sob); > } At this point doesn't this leak sob.buf? ``` ## Pierre Habouzit, 2007-11-10 13:16 Subject: Re: [PATCH] builtin-commit: fix --signoff Message-ID: <20071110131656.GD25204@artemis.corp> URL: https://gitlist.dev/e/20071110131656.GD25204%40artemis.corp In-Reply-To: <7vr6iyo4ff.fsf@gitster.siamese.dyndns.org> ``` On Sat, Nov 10, 2007 at 09:06:28AM +0000, Junio C Hamano wrote: > Johannes Schindelin writes: > > > if (signoff) { > > ... > > + strbuf_init(&sob, 0); > > + strbuf_addstr(&sob, sign_off_header); > > + strbuf_addstr(&sob, git_committer_info(1)); > > + p = strrchr(sob.buf, '>'); > > + if (p) > > + strbuf_setlen(&sob, p + 1 - sob.buf); > > + strbuf_addch(&sob, '\n'); > > + > > + for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--) > > + ; /* do nothing */ > > + if (prefixcmp(sb.buf + i, sob.buf)) > > + strbuf_addbuf(&sb, &sob); > > } > > At this point doesn't this leak sob.buf? It does, strbuf_addbuf copies `sob` but doesn't release resources. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Johannes Schindelin, 2007-11-10 13:47 Subject: Re: [PATCH] builtin-commit: fix --signoff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711101346120.4362%40racer.site In-Reply-To: <7vr6iyo4ff.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sat, 10 Nov 2007, Junio C Hamano wrote: > Johannes Schindelin writes: > > > if (signoff) { > > ... > > + strbuf_init(&sob, 0); > > + strbuf_addstr(&sob, sign_off_header); > > + strbuf_addstr(&sob, git_committer_info(1)); > > + p = strrchr(sob.buf, '>'); > > + if (p) > > + strbuf_setlen(&sob, p + 1 - sob.buf); > > + strbuf_addch(&sob, '\n'); > > + > > + for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--) > > + ; /* do nothing */ > > + if (prefixcmp(sb.buf + i, sob.buf)) > > + strbuf_addbuf(&sb, &sob); > > } > > At this point doesn't this leak sob.buf? Darn. I had a "strbuf_release(&sob);" there, but at some stage removed it by mistake. But there is more to be fixed. Rather than let git_committer_info() add the timestamp, only to strip it away, is a waste. Will redo the patch. Ciao, Dscho ``` ## Johannes Schindelin, 2007-11-10 15:58 Subject: [PATCH] builtin-commit: fix --signoff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711101557520.4362%40racer.site In-Reply-To: ``` The Signed-off-by: line contained a spurious timestamp. The reason was a call to git_committer_info(1), which automatically added the timestamp. Instead, fmt_ident() was taught to interpret an empty string for the date (as opposed to NULL, which still triggers the default behavior) as "do not bother with the timestamp", and builtin-commit.c uses it. Signed-off-by: Johannes Schindelin --- On Sat, 10 Nov 2007, Johannes Schindelin wrote: > Will redo the patch. And here it is. builtin-commit.c | 29 ++++++++++++++++++----------- ident.c | 10 +++++++--- t/t7500-commit.sh | 13 +++++++++++++ 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/builtin-commit.c b/builtin-commit.c index e8bc4c4..a4d69ad 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -181,21 +181,28 @@ static int prepare_log_message(const char *index_file, const char *prefix) die("could not open %s\n", git_path(commit_editmsg)); stripspace(&sb, 0); - if (fwrite(sb.buf, 1, sb.len, fp) < sb.len) - die("could not write commit template: %s\n", - strerror(errno)); if (signoff) { - const char *info, *bol; - - info = git_committer_info(1); - strbuf_addch(&sb, '\0'); - bol = strrchr(sb.buf + sb.len - 1, '\n'); - if (!bol || prefixcmp(bol, sign_off_header)) - fprintf(fp, "\n"); - fprintf(fp, "%s%s\n", sign_off_header, git_committer_info(1)); + struct strbuf sob; + int i; + + strbuf_init(&sob, 0); + strbuf_addstr(&sob, sign_off_header); + strbuf_addstr(&sob, fmt_ident(getenv("GIT_COMMITTER_NAME"), + getenv("GIT_COMMITTER_EMAIL"), "", 1)); + strbuf_addch(&sob, '\n'); + + for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--) + ; /* do nothing */ + if (prefixcmp(sb.buf + i, sob.buf)) + strbuf_addbuf(&sb, &sob); + strbuf_release(&sob); } + if (fwrite(sb.buf, 1, sb.len, fp) < sb.len) + die("could not write commit template: %s\n", + strerror(errno)); + strbuf_release(&sb); if (in_merge && !no_edit) diff --git a/ident.c b/ident.c index 9b2a852..5be7533 100644 --- a/ident.c +++ b/ident.c @@ -224,13 +224,17 @@ const char *fmt_ident(const char *name, const char *email, } strcpy(date, git_default_date); - if (date_str) - parse_date(date_str, date, sizeof(date)); + if (date_str) { + if (*date_str) + parse_date(date_str, date, sizeof(date)); + else + date[0] = '\0'; + } i = copy(buffer, sizeof(buffer), 0, name); i = add_raw(buffer, sizeof(buffer), i, " <"); i = copy(buffer, sizeof(buffer), i, email); - i = add_raw(buffer, sizeof(buffer), i, "> "); + i = add_raw(buffer, sizeof(buffer), i, date[0] ? "> " : ">"); i = copy(buffer, sizeof(buffer), i, date); if (i >= sizeof(buffer)) die("Impossibly long personal identifier"); diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh index abbf54b..13d5a0c 100755 --- a/t/t7500-commit.sh +++ b/t/t7500-commit.sh @@ -93,4 +93,17 @@ test_expect_success 'commit message from file should override template' ' commit_msg_is "standard input msg" ' +cat > expect << EOF +zort +Signed-off-by: C O Mitter +EOF + +test_expect_success '--signoff' ' + echo "yet another content *narf*" >> foo && + echo "zort" | + GIT_EDITOR=../t7500/add-content git commit -s -F - foo && + git cat-file commit HEAD | sed "1,/^$/d" > output && + git diff expect output +' + test_done -- 1.5.3.5.1674.g6e7f7 ```